inline note interface wip #2
ClosedPublic

Authored by brauch on Aug 14 2018, 10:32 AM.

Details

Summary

This is a slightly modified version of the InlineNote interface written by Michal in D12662.

Changes:

  • move the interface to the view instead of the document (rather trivial)
  • rework how notes are queried (see below)

I'm not sure about which API is better, the old suggestion or this one. The problem I see with the old one is that you need to return pointers to instances of the notes, which you need to memory-manage yourself. This means you either allocate all of them immediately, or you need some rather complex tracking of what instances you created and when to free them again.

Effectively, I tried to implement inline error display (see attachments below) for KDevelop, once with the API from the other patch and once with this one. The amount of lines is approximately the same, but with this API, you can implement it only as a view on top of what's already there, i.e. you only add three member functions which return things based on the existing data. With the other API, you need a vector as member which contains all the notes instances and you need to manage that.

What do you think? I'd especially be interested in Michal's thoughts :)

Diff Detail

Repository
R39 KTextEditor
Branch
inlinenotes
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1932
Build 1950: arc lint + arc unit
brauch created this revision.Aug 14 2018, 10:32 AM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptAug 14 2018, 10:32 AM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
brauch requested review of this revision.Aug 14 2018, 10:32 AM
anthonyfieroni added inline comments.
src/render/katerenderer.cpp
766

const

767
for (const auto& inlineNote : inlineNotes) {
1049–1050

same as above

src/view/kateview.cpp
3654–3655

Use new syntax connect

3667
provider->disconnect(this);
brauch edited the summary of this revision. (Show Details)Aug 14 2018, 11:39 AM

Sample patch for KDevelop's problem highlighter plus screenshot:


brauch updated this revision to Diff 39692.Aug 14 2018, 1:08 PM

add missing files

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?

src/include/ktexteditor/inlinenoteinterface.h
146

Perhaps this could be QVarLengthArray of some size too?

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

brauch updated this revision to Diff 39721.Aug 14 2018, 3:23 PM

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.

brauch updated this revision to Diff 39802.Aug 15 2018, 6:26 PM

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.

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.

I think this goes into the right direction :-)

src/include/ktexteditor/inlinenoteinterface.h
3

author missing

74

@since 5.50

101

Please add a d-pointer as placeholder.

private:

/**
 * private d-pointer
 */
class InlineNoteInterfacePrivate *const d = nullptr;
118
  • move to InlineNote
  • turn into enum class
174–177
  • API documentation is wrong
  • please use @param instead of \param everywhere, i.e. we prefer @xyz over \xyz
196

noteActivated -> inlineNoteActivated()

296

can you try making all variables private?

326

please add d-pointer:

private:
    class InlineNotePrivate * const d = nullptr;
brauch updated this revision to Diff 39815.Aug 15 2018, 9:55 PM

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

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

src/include/CMakeLists.txt
5

Could we have also InlineNoteProvider and InlineNote?

src/include/ktexteditor/inlinenoteinterface.h
5

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

72

@author Sven Brauch, Michal Srb

146

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

148–149

There is only one @param note

200

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);
216

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

321

please initialize all members in-class.

src/utils/ktexteditor.cpp
240

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

InlineNote::InlineNote() = default;
brauch updated this revision to Diff 39888.Aug 16 2018, 9:49 PM
brauch marked 21 inline comments as done.

Implement Dominik's suggestions

brauch updated this revision to Diff 39889.Aug 16 2018, 9:53 PM

update license text

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

cullmann accepted this revision.Aug 16 2018, 10:14 PM

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.

This revision is now accepted and ready to land.Aug 16 2018, 10:14 PM
brauch closed this revision.Aug 16 2018, 11:15 PM

commit 4ea5fee0afe5c76bbee07563c23ede808aa059de
Author: Sven Brauch <mail@svenbrauch.de>
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