Details

Reviewers

michalsrb
dhaumann
cullmann

Commits

R39:4ea5fee0afe5: Add inline note interface

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 1837
Build 1855: 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

Harbormaster completed remote builds in B1819: Diff 39682.Aug 14 2018, 10:32 AM

brauch added reviewers: michalsrb, dhaumann, cullmann.Aug 14 2018, 10:33 AM

anthonyfieroni added a subscriber: anthonyfieroni.Aug 14 2018, 10:39 AM

anthonyfieroni added inline comments.

src/render/katerenderer.cpp
765	const
766	for (const auto& inlineNote : inlineNotes) {
1048–1049	same as above
src/view/kateview.cpp
3653–3654	Use new syntax connect
3666	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:

inline-problems.diff6 KBDownload

add missing files

Harbormaster completed remote builds in B1825: Diff 39692.Aug 14 2018, 1:08 PM

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

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.

Harbormaster completed remote builds in B1837: Diff 39721.Aug 14 2018, 3:23 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.

Harbormaster completed remote builds in B1878: Diff 39802.Aug 15 2018, 6:26 PM

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;

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

Harbormaster completed remote builds in B1887: Diff 39815.Aug 15 2018, 9:55 PM

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

src/include/CMakeLists.txt
5 ↗	(On Diff #39815)	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
238	When members are initialized in the header file, you can write here: InlineNote::InlineNote() = default;

cullmann mentioned this in D12662: Add InlineNoteInterface.Aug 16 2018, 9:49 PM

Implement Dominik's suggestions

Harbormaster completed remote builds in B1931: Diff 39888.Aug 16 2018, 9:49 PM

update license text

Harbormaster completed remote builds in B1932: Diff 39889.Aug 16 2018, 9:53 PM

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

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

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

		Path
M		CMakeLists.txt (4 lines)
M		src/document/katedocument.h (1 line)
A	M	src/include/ktexteditor/inlinenoteinterface.h (272 lines)
M		src/render/katerenderer.h (1 line)
M		src/render/katerenderer.cpp (60 lines)
M		src/utils/ktexteditor.cpp (13 lines)
M		src/view/kateview.h (19 lines)
M		src/view/kateview.cpp (64 lines)
M		src/view/kateviewinternal.h (5 lines)
M		src/view/kateviewinternal.cpp (61 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	39682	5499a0d		Aug 14 2018, 10:32 AM	★	★
Diff 2	39692	5499a0d	add missing files	Aug 14 2018, 1:08 PM	★	★
Diff 3	39721	5499a0d	add noteActivated notifier function	Aug 14 2018, 3:23 PM	★	★
Diff 4	39802	5499a0d	I added the rest of the interaction interface (click, mouseover)	Aug 15 2018, 6:26 PM	★	★
Diff 5	39815	5499a0d	address Dominik's suggestion and split focus handling and click handling	Aug 15 2018, 9:55 PM	★	★
Diff 6	39888	5499a0d	Implement Dominik's suggestions	Aug 16 2018, 9:49 PM	★	★
Diff 7	39889	5499a0d	update license text	Aug 16 2018, 9:53 PM	★	★

inline note interface wip #2
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 39721

CMakeLists.txt

src/document/katedocument.h

src/include/ktexteditor/inlinenoteinterface.h

src/render/katerenderer.h

src/render/katerenderer.cpp

src/utils/ktexteditor.cpp

src/view/kateview.h

src/view/kateview.cpp

src/view/kateviewinternal.h

src/view/kateviewinternal.cpp

inline note interface wip #2ClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 39721

CMakeLists.txt

src/document/katedocument.h

src/include/ktexteditor/inlinenoteinterface.h

src/render/katerenderer.h

src/render/katerenderer.cpp

src/utils/ktexteditor.cpp

src/view/kateview.h

src/view/kateview.cpp

src/view/kateviewinternal.h

src/view/kateviewinternal.cpp

inline note interface wip #2
ClosedPublic
Actions

Revision Contents
Changeset List