D14826: inline note interface wip #2

2018-08-16 Thread Sven Brauch 
brauch closed this revision.
brauch added a comment.


  commit 4ea5fee0afe5c76bbee07563c23ede808aa059de
  Author: Sven Brauch 
  Date:   Tue Aug 14 12:31:31 2018 +0200
  
Add inline note interface

Original patch by Michal Srb.

The inline note interface provides a way to render arbitrary things in
the text. The layout of the line is adapted to create space for the note.

Possible applications include showing a name of a function parameter on call
side or rendering square with color preview next to CSS color property.

Subscribers: kwrite-devel, kde-frameworks-devel

Tags: #kate, #frameworks

Differential Revision: https://phabricator.kde.org/D14826

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-16 Thread Christoph Cullmann 
cullmann accepted this revision.
cullmann added a comment.
This revision is now accepted and ready to land.


  Avoiding the QObject inheritance requires some ugly QObject casts, therefore 
we keep that ATM.
  We merge it that way and talk tomorrow morning once more about that detail.

REPOSITORY
  R39 KTextEditor

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-16 Thread Christoph Cullmann 
cullmann added a comment.


  I think the Provider needs not to be an QObject, just an interface, IMHO.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-16 Thread Sven Brauch 
brauch updated this revision to Diff 39889.
brauch added a comment.


  update license text

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39888=39889

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/include/ktexteditor/inlinenote.h
  src/include/ktexteditor/inlinenoteinterface.h
  src/include/ktexteditor/inlinenoteprovider.h
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-16 Thread Sven Brauch 
brauch updated this revision to Diff 39888.
brauch marked 21 inline comments as done.
brauch added a comment.


  Implement Dominik's suggestions

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39815=39888

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/include/ktexteditor/inlinenote.h
  src/include/ktexteditor/inlinenoteinterface.h
  src/include/ktexteditor/inlinenoteprovider.h
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-16 Thread Dominik Haumann 
dhaumann added a comment.


  I think one more iteration, and this can be merged. Can you look into this 
again?

INLINE COMMENTS

> CMakeLists.txt:5
>AnnotationInterface CodeCompletionModelControllerInterface MovingCursor 
> Range TextHintInterface
> -  Cursor MarkInterface MovingInterface
> +  Cursor MarkInterface MovingInterface InlineNoteInterface
>Document  MovingRange View

Could we have also InlineNoteProvider and InlineNote?

> inlinenoteinterface.h:4
> +   This library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Library General Public
> +   License as published by the Free Software Foundation; either

Btw, as license I would previer LGPLv2+ (as it currently is stated) +e.V. 
option to relicense.

> inlinenoteinterface.h:71
> + * @endcode
> + *
> + * @see InlineNoteProvider

@author Sven Brauch, Michal Srb

> inlinenoteinterface.h:147-148
> + *
> + * @param height the height of the line in pixels
> + * @param font the font used by the editor
> + *

There is only one @param note

> inlinenoteinterface.h:199
> + */
> +virtual void inlineNoteFocusChanged(const InlineNote& note, bool 
> hasFocus, const QPoint& pos);
> +

I would prefer two functions:

  virtual void inlineNoteFocusInEvent(const InlineNote& note, const QPoint& 
pos);
  virtual void inlineNoteMoveEvent(const InlineNote& note, const QPoint& pos);
  virtual void inlineNoteFocusOutEvent(const InlineNote& note);

> inlinenoteinterface.h:215
> +
> +class KTEXTEDITOR_EXPORT InlineNote
> +{

API docs missing for InlineNote, but we can add that later.

> inlinenoteinterface.h:320
> +private:
> +InlineNoteProvider* m_provider;
> +const KTextEditor::View* m_view;

please initialize all members in-class.

> michalsrb wrote in inlinenoteinterface.h:145
> Perhaps this could be `QVarLengthArray` of some size too?

We discussed that here, and let's use QVector for now. If this turns out to be 
an issue, we will change for KF6.

> ktexteditor.cpp:238
> +
> +InlineNote::InlineNote()
> +: m_provider(nullptr)

When members are initialized in the header file, you can write here:

  InlineNote::InlineNote() = default;

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-15 Thread Sven Brauch 
brauch updated this revision to Diff 39815.
brauch added a comment.


  address Dominik's suggestion and split focus handling and click handling

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39802=39815

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  CMakeLists.txt
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/include/ktexteditor/inlinenoteinterface.h
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-15 Thread Dominik Haumann 
dhaumann added a comment.


  I think this goes into the right direction :-)

INLINE COMMENTS

> inlinenoteinterface.h:2
> +/* This file is part of the KDE libraries
> +
> +   This library is free software; you can redistribute it and/or

author missing

> inlinenoteinterface.h:73
> + * \see InlineNoteProvider
> + */
> +class KTEXTEDITOR_EXPORT InlineNoteInterface

@since 5.50

> inlinenoteinterface.h:100
> +virtual void 
> unregisterInlineNoteProvider(KTextEditor::InlineNoteProvider *provider) = 0;
> +};
> +

Please add a d-pointer as placeholder.

private:

  /**
   * private d-pointer
   */
  class InlineNoteInterfacePrivate *const d = nullptr;

> inlinenoteinterface.h:117
> +public:
> +enum ActivationType {
> +Inactive, ///< The note is inactive (the default)

- move to InlineNote
- turn into enum class

> inlinenoteinterface.h:173-176
> + * \param note note to paint, containing location and index
> + * \param height the height of the line in pixels
> + * \param font the QFont used in the editor
> + * \param painter painter prepared for rendering the note

- API documentation is wrong
- please use @param instead of \param everywhere, i.e. we prefer @xyz over \xyz

> inlinenoteinterface.h:195
> + */
> +virtual void noteActivated(const InlineNote& note, ActivationType type, 
> QPoint pos) {
> +Q_UNUSED(note);

noteActivated -> inlineNoteActivated()

> inlinenoteinterface.h:295
> +}
> +
> +/**

can you try making all variables private?

> inlinenoteinterface.h:325
> +int lineHeight;
> +};
> +

please add d-pointer:

  private:
  class InlineNotePrivate * const d = nullptr;

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-15 Thread Christoph Cullmann 
cullmann added a comment.


  I would not implement any function inline and just hide their implementation 
in the .cpp to be able to alter them later (for InlineNote).
  For the activation, I would like to be able to differentiate between the 
different mouse buttons like the qt mousepressed stuff would do.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-15 Thread Sven Brauch 
brauch updated this revision to Diff 39802.
brauch added a comment.


  I added the rest of the interaction interface (click, mouseover)
  and reduced the API a bit by moving a few hints into the InlineNote
  object.
  
  Only thing I still intend to change about the API would be that
  we use the Qt mouse button enum to provide details about mouse events,
  otherwise nothing comes to mind.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39721=39802

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  CMakeLists.txt
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/include/ktexteditor/inlinenoteinterface.h
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-14 Thread Sven Brauch 
brauch updated this revision to Diff 39721.
brauch added a comment.


  add noteActivated notifier function
  
  When a note is mouse-overed or clicked, a function on the note
  provider is called, giving the point it was hovered or clicked
  in note coordinates, the type of event, and the note the event
  occured on.
  
  This can be used to e.g. expand notes on mouse-over, show a tooltip,
  or execute actions when clicking. You should even be able to e.g.
  highlight pseudo-buttons mouseover.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39692=39721

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  CMakeLists.txt
  src/document/katedocument.h
  src/include/ktexteditor/inlinenoteinterface.h
  src/render/katerenderer.cpp
  src/render/katerenderer.h
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp
  src/view/kateviewinternal.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-14 Thread Sven Brauch 
brauch added a comment.


  Thanks for the feedback! I will try doing a few more things with this 
interace and then maybe discuss again with the other kate people here at 
Akademy about which one they like better.
  
  About the tracking, I don't think anything is needed on the side of the 
interface. You can see how you can potentially do it in the KDevelop patch I 
attached: create a moving cursor for the location you want the note to track, 
and then compute the note's position from the moving cursor's position as 
needed in the getter each time.
  I think this is even better than doing it in the interface itself, since it 
is more flexible with regards to how exactly the moving cursor behaves.
  
  Regarding the QVarLengthArray, performance-wise it would be better, but it 
makes the public API ugly (QVarLengthArray is a relatively internal, hacky 
class which is supposed to be only used in special cases), so I think we should 
first profile whether this is a bottleneck in any possible use case (it 
probably isn't).
  
  Currently, I'm trying out whether we can add some simple interaction ("note 
clicked") as well already, since I think that would be nice long-term. If the 
API is nice, we can fix small uglyness like the cursor navigation around notes 
at any later time IMHO.
  
  Best,
  Sven

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-14 Thread Michal Srb 
michalsrb added a comment.


  Thank you for working on this. This interface would work for the 
kdev-sourceinfo use case just as well as the old one.
  
  I think the problem with the old interface that you described is valid. This 
version gives more flexibility to the `InlineNotesProvider` with regards to how 
it stores the notes internally.
  
  Do you have plan how to track the movement of the notes during edits? Maybe 
the InlineNote could also hold revision number for which it was created?

INLINE COMMENTS

> inlinenoteinterface.h:145
> + */
> +virtual QVector inlineNotes(int line) const = 0;
> +

Perhaps this could be `QVarLengthArray` of some size too?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-14 Thread Sven Brauch 
brauch updated this revision to Diff 39692.
brauch added a comment.


  add missing files

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14826?vs=39682=39692

BRANCH
  inlinenotes

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/include/ktexteditor/inlinenoteinterface.h
  src/render/katerenderer.cpp
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-14 Thread Sven Brauch 
brauch added a comment.


  Sample patch for KDevelop's problem highlighter plus screenshot:
  F6192637: hl.png 
  F6192639: inline-problems.diff 

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-14 Thread Sven Brauch 
brauch edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-14 Thread Anthony Fieroni 
anthonyfieroni added inline comments.

INLINE COMMENTS

> katerenderer.cpp:765
> +// Draw inline notes
> +auto inlineNotes = m_view->inlineNotes(range->line());
> +foreach (const KTextEditor::InlineNote& inlineNote, inlineNotes) {

const

> katerenderer.cpp:766
> +auto inlineNotes = m_view->inlineNotes(range->line());
> +foreach (const KTextEditor::InlineNote& inlineNote, inlineNotes) {
> +int column = inlineNote.column();

for (const auto& inlineNote : inlineNotes) {

> katerenderer.cpp:1048-1049
> +
> +auto inlineNotes = m_view->inlineNotes(lineLayout->line());
> +foreach (const KTextEditor::InlineNote& inlineNote, inlineNotes) {
> +int column = inlineNote.column();

same as above

> kateview.cpp:3653-3654
> +
> +connect(provider, SIGNAL(reset()), this, SLOT(inlineNotesReset()));
> +connect(provider, SIGNAL(lineChanged(int)), this, 
> SLOT(inlineNotesLineChanged(int)));
> +

Use new syntax connect

> kateview.cpp:3666
> +
> +disconnect(provider, nullptr, this, nullptr);
> +

provider->disconnect(this);

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: anthonyfieroni, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-14 Thread Sven Brauch 
brauch added reviewers: michalsrb, dhaumann, cullmann.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D14826

To: brauch, michalsrb, dhaumann, cullmann
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D14826: inline note interface wip #2

2018-08-14 Thread Sven Brauch 
brauch created this revision.
Herald added projects: Kate, Frameworks.
Herald added subscribers: kde-frameworks-devel, kwrite-devel.
brauch requested review of this revision.

REPOSITORY
  R39 KTextEditor

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D14826

AFFECTED FILES
  src/document/katedocument.h
  src/include/CMakeLists.txt
  src/render/katerenderer.cpp
  src/utils/ktexteditor.cpp
  src/view/kateview.cpp
  src/view/kateview.h

To: brauch
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann