Add InlineNoteInterface
AbandonedPublic

Authored by cullmann on May 2 2018, 1:27 PM.

Details

Reviewers
michalsrb
Group Reviewers
KTextEditor
Summary

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.

Diff Detail

Repository
R39 KTextEditor
Lint
No Linters Available
Unit
No Unit Test Coverage
michalsrb created this revision.May 2 2018, 1:27 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMay 2 2018, 1:27 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
michalsrb requested review of this revision.May 2 2018, 1:27 PM

This is my first attempt to propose such API, it may not be ideal.

If the inline note is placed into text, the space for it is created using QTextCharFormat's absolute spacing between the two letters. I admit it is quite a misuse, but it was the least intrusive way to create the space. Everything else seems to work with it out of the box without further modifications.

A kate plugin that demonstrates how to use it is here:
https://github.com/michalsrb/sampleinlinenotesplugin

You can find screenshots in the "samples" directory.

brauch added a subscriber: brauch.May 2 2018, 4:10 PM

Awesome idea! Do you have a screenshot of how it looks?

Here are screenshots from the sample plugin:

The plugin provides notes that are just hardcoded for those specific files. The style of the notes is completely up to the user of the interface.

brauch added a comment.May 2 2018, 6:36 PM

Looks good from the implementation too so far. One thing I do not see is any changes to the cursorToX / xToCursor functions, is there really no change required there?

Some things which come to my mind for testing would be:

  • is selection rendered correctly if it includes notes, at the end, beginning, or middle of lines, also mult-line selections?
  • what happens when clicking or dragging from or into the notes?
  • does it still work properly with dynamic word-wrap on?
  • does it work properly with code-folding? what happens if a note is at the border of a folding region?
src/document/katedocument.cpp
5295

Can you use new-style connect here, i.e. the function-pointer syntax
connect(provider, &NoteProvider::reset, this, &DocumentPrivate::inlineNotesReset)?
This gives compile-time argument type checking.

src/render/katerenderer.cpp
771

indent

777

indent

Looks good from the implementation too so far. One thing I do not see is any changes to the cursorToX / xToCursor functions, is there really no change required there?

No change was really needed. As the spaces are created when laying out the line, either by offsetting the start of the line or by formatting the text, it gets stored in the layout and the cursorToX / xToCursor work as expected. Even moving the cursor around with arrow keys works nicely (the cursor moves to the closest position above/below even if one of the lines is shifted).

Some things which come to my mind for testing would be:

  • is selection rendered correctly if it includes notes, at the end, beginning, or middle of lines, also mult-line selections?

I thought regular selection works fine, but actually I just found a glitch; if there is a note at the beginning of the line, it does not render the selection background under it:

A selection in block mode behaves like if the notes are not there:

I can't tell whether it is good or bad behavior. Do you think it should stay this way - selecting the block of the real text, or should it instead select a "visual" block?

I was thinking it would be best to avoid this problem and hide the notes when doing block mode selection, but should that be done by ktexteditor directly, or by the user of the interface? I also imagine that anything that uses it would provide some quick on/off toggle for cases like this.

  • what happens when clicking or dragging from or into the notes?

Clicking into the note always places the text cursor on the right side of the note - in other words, the note acts as extension of the letter on its left. Dragging makes normal selection from that position.

  • does it still work properly with dynamic word-wrap on?

Hmm, looks like notes at the beginning of the text ruin the breaking of the line:

And it also does not place the notes well if the wrapped line keeps keeps indentation, like in this case:

  • does it work properly with code-folding? what happens if a note is at the border of a folding region?

It works fine. When a line is hidden, so are the notes in it.

One more thing that needs to be decided: At this moment if a note is placed past the end of the text too far to the right and word-wrap is on, it does not wrap, but renders partially or completely out of view. (This happens kinda by default as the layout of the line is not modified for notes out of the text.) My thinking was that it does not matter as one would either place notes in the text, or outside at a column that will be visible without scrolling/wrapping. But if you think it should wrap, I can try to find a way to make that happen.

Thank you for the review, I'll fix these problems and make the changes you noted.

ngraham added a subscriber: ngraham.May 3 2018, 1:37 PM
brauch added a comment.May 3 2018, 6:16 PM

I think fixing the selection rendering issue would be nice.

Regarding blockmode behaviour, I think it will be a bit strange either way. One thing to compare is the behaviour with non-fixed width fonts: there block mode also selects constant columns (i.e. it will look ragged). This is probably closest to what you want when you use block mode, so I think it should stay the way you have it now.

Regarding linewrap, I think what at least must always be the case is that with dynwrap on, all text is visible. That notes cannot be placed nicely is probably unavoidable in extreme cases. I guess the best behaviour would be to just wrap the notes as if they were part of the text, with the restriction that you cannot split them up ...

I would not rely on the user of the interface placing his notes in a visible column. If e.g. KDevelop would place some notes, and then the user resizes his window, and then they need to be re-placed by the interface user? That seems strange. Consider also that you can have a split view with two different widths for the same file and stuff like that.

Oh, good point by the way: Are you sure this should be an interface to a *view*? Maybe it should instead be attached to the document? I'm not sure, I just want to bring the discussion up.

Oh, good point by the way: Are you sure this should be an interface to a *view*? Maybe it should instead be attached to the document? I'm not sure, I just want to bring the discussion up.

Actually, I think view might make sense, as you only mess with the layouts and renderer.
But the implementation ATM makes it a interface of the document, or I am mistaken?

brauch added a comment.EditedMay 3 2018, 10:28 PM

Oh, heh, yes it does. It's just the docstring which says otherwise ("is an interface for the View"), and that's what I looked at at that time. That should be changed. ;)

Hm, maybe the question is less what is needed and more what it is supposed to do semantically. Does one add annotations to a *document*, or does one add them to a specific *view* of that document (considering that there can be more than one view of a document, even visible at once, e.g. splitview)? I think you can find arguments for both, but I feel like the document variant (as it is here) will be easier to handle.

This is already an excellent patch. The API documentation is already really nice.

I have some minor suggestions (see inline comments in patch). Besides that, I wonder whether this should really be a View extension interaface. Currently, this interface is implemented by the KTextEditor::Document, while the API documentation mentions this interafce is a View extension interface. I would prefer a View extension interface.

Could you provide an updated revision of this patch?

And also: I think we have to discuss unit tests as well - since such kind of code will break otherwise when we have to change related code parts. Would be nice to have a discussion about this already now.

src/document/katedocument.cpp
5316–5320

This look like premature optimization. I would prefer to delete this "common cases first" block. Except if this turned out to be a bottleneck already?

src/include/ktexteditor/inlinenoteinterface.h
49

It would be nice to maybe include a picture here: Put a png into ktexteditor/docs/pics/ and use it like this:

  • \image html inlinenotes.png "Inline note showing a CSS color"
53

Could you add this note:

\note A InlineNoteProvider instance can be reused/shared by multipe views, just make sure to unregister the provider from \e all views before deleting the provider.
117

I would prefer

virtual KTextEditor::Cursor position() const = 0;

This way, a InlineNote contains not only the column, but ALL information (line + column) that defines the position of the note.

155

InlineNoteProvider is _a_ object that can be ... (here is 'a' missing)

src/render/katerenderer.cpp
773

Please use const for variables that do not change anymore:

const int textLength = ...
dhaumann added inline comments.May 6 2018, 6:53 PM
src/document/katedocument.cpp
5316–5320

Btw, may I am wrong here, since this is really called often when painting lines... So this may be good to have.

brauch added inline comments.May 7 2018, 7:12 AM
src/document/katedocument.cpp
5316–5320

I also stumbled upon it but it avoids an alloc in a very deep loop, so it might be worth it here.

Hey there, what's happening to this? I think this is a really nice feature and it would be very sad if it would bitrot :(
Anything anyone can assist with? We are currently at Akademy, KDE's annual conference, so we would have time to discuss any issues right now.

Restricted Application edited subscribers, added: kde-frameworks-devel, kwrite-devel; removed: Frameworks. · View Herald TranscriptAug 12 2018, 9:54 AM

Sorry, I had almost no time to work on it. I got stuck trying to figure out how to properly handle the notes at the beginning of the lines.

Then I created KDevelop plugin as experiment to try if the interface is usable:
https://github.com/michalsrb/kdev-sourceinfo

With that experience I think two things should be done differently:

  • The notes should use something like MovingCursor instead of fixed line + column, so they move when the text is edited, instead of waiting for refresh from the provider.
  • Currently the notes are "glued" to the character on their left side - it is not possible to place cursor in between that letter and the note. It feels unnatural when editing. In other editors (e.g. IntelliJ) the note behaves as separate character/object and the cursor can be placed on both sides. The note is still not editable, pressing backspace or delete just skips over the note. Any advice on how to achieve this with ktexteditor would be helpful.

Wow, that looks amazing! Really impressive.

Using moving cursors for this sounds good to me. Other things which cannot be updated in real-time do that as well (highlighting, error underlines, ...)

Regarding the cursor issue: I think you would need a state variable whether the cursor is left or right of the note. You could then patch KateViewInternal::placeCursor to set this correctly and also the cursor move functions to take this into account. That's not ideal since it requires to modify a few places, but probably actually adding a character would require more modifications...

By the way, other people around here are very impressed by this patch as well, and we'd really like to get this merged :)
Moving e.g. KDevelop's warning markers into an inline note instead of the annoying popup would make a real difference for usability ...

I'd like to play with this a bit wrt what can be done in KDevelop with it (I want the problem popups gone). Would you mind if I do some changes along the way? I would post an updated patch here, in case I actually come up with useful changes ...

I'd like to play with this a bit wrt what can be done in KDevelop with it (I want the problem popups gone). Would you mind if I do some changes along the way? I would post an updated patch here, in case I actually come up with useful changes ...

No problem, go ahead.

cullmann commandeered this revision.Aug 16 2018, 9:49 PM
cullmann added a reviewer: michalsrb.

Hi,
we will process further things of this request in Sven's copy D14826: inline note interface wip #2.

cullmann abandoned this revision.Aug 16 2018, 9:49 PM

Head over to D14826: inline note interface wip #2. for the current state.