Introduce AbstractAnnotationItemDelegate for more control by consumer
ClosedPublic

Authored by kossebau on Nov 8 2017, 12:36 AM.

Details

Summary

The AbstractAnnotationItemDelegate is modelled after QAbstractItemDelegate
and should allow consumers of the annotation interfaces to customize the
rendering and the tooltip per annotation as wanted.

Diff Detail

Repository
R39 KTextEditor
Branch
addAnnotationItemDelegate
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4027
Build 4045: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptNov 8 2017, 12:36 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript

Example for custom rendering in KDevelop as currently proposed with the matching KDevelop patch D8709:

kossebau updated this revision to Diff 22229.Nov 12 2017, 2:00 PM

Update: add some API dox, more API fine-tuning

kossebau updated this revision to Diff 40990.Sep 4 2018, 3:44 PM

update to latest master

Restricted Application edited subscribers, added: kde-frameworks-devel, kwrite-devel; removed: Frameworks. · View Herald TranscriptSep 4 2018, 3:44 PM
kossebau updated this revision to Diff 41078.Sep 6 2018, 12:42 AM

Dump update with some more changes (still TODOs left to handle)

kossebau updated this revision to Diff 41112.Sep 6 2018, 3:13 PM

fix not listening to sizeHintChanged signal

kossebau updated this revision to Diff 41225.EditedSep 8 2018, 8:54 PM

Reaching review candidate state, please start reviewing the code.

Target is Frameworks 5.52, so the one after the upcoming Frameworks version.
The current planned consumer of the new API is KDevelop 5.4, which may be
branched end of year or next year, so nothing urgent to push and giving this
another month of API reconsiderations is fine.

For some more verbose presentation of this patch see https://frinring.wordpress.com/2018/09/09/from-code-to-related-bug-report-or-review-in-just-a-hover-click/

kossebau retitled this revision from [WIP] Introduce AbstractAnnotationItemDelegate for more control by consumer to Introduce AbstractAnnotationItemDelegate for more control by consumer.Sep 8 2018, 8:56 PM
kossebau edited the summary of this revision. (Show Details)
kossebau added reviewers: Kate, KDevelop.
kossebau updated this revision to Diff 41694.Sep 15 2018, 12:04 PM

fix crash due to (no longer needed) circular dependencies on view object
constrctions

I commented on some things - I did not try this, though.

What I would like to see is a comment // KF6: Merge KTextEditor::AnnotationViewInterfaceV2 into KTextEditor::AnnotationViewInterface (kossebau).
For me, this comment is really important, since this tells you that you will in 2-3 years (when Qt6 arrives) work on this and merge it down: Since there is only you (KDevelop) who is using this interface, so you have to maintain it ;)

Would you go over my comments and provide an updated version? Not all comments are real issues.

src/include/ktexteditor/abstractannotationitemdelegate.h
40

@since 5.52 :-)

53

I am asking myself: Is wrappedLineCount >= 1 always true? If so, can you add this as documentation? wrappedLineCount == 1 means no wrapping line?

66

Is this to be set by the user, and if so, is there any special meaning to a default-constructed QSize()?

81

Is this a bitmask? The << 0, <<1, <<2 notation implies that it is.

96

You export the class, but then the constructors are inlined. Could you please move the implementation to the cpp file? That leaves us more room to fixes by shipping an updated version of the library.

115

Typ: A delegate ...
And: I suggest to remove "for them".

137

Same here: Could you move the implementation to ktexteditor.cpp?

142

Same here: Please move the destructor to ktexteditor.cpp. You can keep = default there, but not here.

180

typo: specifying

184

inlc. -> including. No need for abbreviations

src/include/ktexteditor/annotationinterface.h
285

Change trailing ':' to '.'

288

typo: fpr

328

I think you don't need to type "\ref". doxygen will create the link for you anyways. Same below.

330

... or a nullptr, if no delegate was set. maybe? I am asking, since it could also return some default implementation.

src/view/kateannotationitemdelegate.cpp
53–54

Given you check for a valid pointers here, would it also be an option to pass references? Or would that violate Qt style API?

58

validness --> validity :-)

src/view/kateannotationitemdelegate.h
3–5

Is this copyright really correct?

10

This is v2 only, I think this should be avoided at all costs... We try to get to v2+... I think you can change this, since this is your work. Even if it's based on others, the work of others is in the other files.

src/view/kateviewhelpers.cpp
2624

You could do that via QPointer, if the delegate is QObject base. It's a poor man's solution which has many issues on its own, though. Maybe it's better to simply assume the user uses the interface correctly...

2634

Do you really need the timer here? I thought update() goes through the event queue anyways?

2647

2nd time this comment pops up. Do you have an answer? In Qt, this would be an extra setAutoFillBackgroundEnabled(bool) , or something similar... In any case - this needs to be decided before the interface goes live :-)

2679–2691

Maybe 4+4 margin or so?

2721

Again, single shot timer needed?

src/view/kateviewhelpers.h
362–363

I would prefer in-class member initialization here for the bools.

kossebau marked 15 inline comments as done.Sep 16 2018, 9:19 PM
kossebau added inline comments.
src/include/ktexteditor/abstractannotationitemdelegate.h
53

Yes. I see now that this can be confusing and semantically strange, if wrappedLineCount == 1 means actually no wrapping here. Unsure what ould be a better way to express this. One ould use the same name and define 0 to mean that no wrapping is done. And 1 would just be a bogus value? Proposals very welcome.

66

This parameter was inspired by QStyleOptionViewItem::decorationSize. I imagined it could be e.g. used if one ever starts to show avatar icons for commit authors in the annotations, or for other icon-based indicators which could be shown (bug fix, reviewed, etc).

There is no specification on the meaning of an invalid QSize with QStyleOptionViewItem::decorationSize. I would tend to leave this here unspecified as well for now, until this gets in real use or the QStyleOptionViewItem one gets specified?

81

Yes, as a line could be both begin and end of an annotation grouping (if both neighbour lines belong to different groups). Of course InGroup is mutually exclusive to GroupBegin and GroupEnd. IIRC (been some time since Nob 2017 :) ) I had used normal enum values, with another value GroupBeginAndEnd. But the resulting code using the enum was more complicated.
Not sure these days, perhaps I should retry. Need to recheck what similar enumas there are in the Qt world, so at least things are consistent.

96

Okay. I had looked at the other interface classes, those I looked at, even if QObject-based, are header-only, that's why it was done like this. Will change, as I remember from other projets e,g. Windows has issues with this.

src/view/kateannotationitemdelegate.cpp
53–54

It is following QAbstractItemDelegate ::paint(...) signature here. So I would lean to stay with the current code. But as you prefer.

src/view/kateannotationitemdelegate.h
3–5

Indeed for the headerthere is no code copied over, well, besides the API being inspired by QAbstractItemDelegate. Reduced to mine.

10

Changed this for the header and also for the source. While the initial code was copied over from kateviewhelpers.cpp (thus the complete license header), comparing it now to the original, I would think it has been that much rewritten to match the delegate API, that it safely can be called a copyrightable product of its own, with no substantial parts of the original left (where not required by the general Qt API).

src/view/kateviewhelpers.cpp
2634

I copied existing code here, for consistency. No really sure when to call it directly and when to go via event loop.

2647

Yes, this is a TODO question to you Kate developers :)
Currently KateIconBorder::paintBorder() paints the background per line with

p.fillRect(0, y, w - 5, h, m_view->renderer()->config()->iconBarColor());

before starting to draw the annotation stuff and icons, which also cares for the displayed lines with no content.
I am unsure whether the delgate should be asked to take responsibility to care for at least blanking the view, or if it should assume some default background has been already painted.
QStyleOptionViewItem has a property backgroundBrush which seems to be expeted to be used by QAbstractItemDelegate, so if following that API completley, we would also do this here.
Just not sure if you prefer/like that. IMHO the delegate should care for the background, even if it means the KateIconBorder::paintBorder needs to be changed to no longer prefill the whole baclkground. Or it could still do it, for simplicity of code, but officially the delegate should do it.

kossebau updated this revision to Diff 41778.Sep 16 2018, 9:20 PM
kossebau marked 3 inline comments as done.

Update to Dominik's first review

kossebau updated this revision to Diff 41828.Sep 17 2018, 1:13 PM

also add todo about merging KTextEditor::AnnotationViewInterfaceV2 into KTextEditor::AnnotationViewInterface

What I would like to see is a comment // KF6: Merge KTextEditor::AnnotationViewInterfaceV2 into KTextEditor::AnnotationViewInterface (kossebau).
For me, this comment is really important, since this tells you that you will in 2-3 years (when Qt6 arrives) work on this and merge it down: Since there is only you (KDevelop) who is using this interface, so you have to maintain it ;)

Forgot that in the previous update, now added. Curious though why you do not expect Kate to offer users some similar annotation display experience one day if working on versioned text documents ;)

Thanks everyone who so far looked at this, especially @dhaumann for detailed comments.

I am tempted to interpret the lack of further comments, especially the lack of principal objections as an implicit +1 on this patch as it is now :)

While I still am slightly unsure about parts of the API, given no better ideas there I would leave it now to real life experience gathering. And then polishing things as found needed in any next ABI/API version.

To keep things moving, I will go and merge this patch, once KF 5.51 has been tagged the upcoming WE, so there are 4 weeks to give the new API and also the default implementation some testing and considerations before getting published with 5.52. Of course unless there are objections raised now.

kossebau updated this revision to Diff 42903.Oct 5 2018, 12:27 AM

Improve rendering in scaled mode

As we have a usecase for this (the extension in KDevelop) I am ok if that goes in, if the extension in KDevelop is going in, too, as consumer.
We can still fix minor issues in the KF6 iteration, they will never be found if it is not there and used.

Hm, given the size of the patch, and given it introduces new public API we cannot easily change, I think a silent +1 since noone reacts is not good enough.

That said, I will not have time for another review until Oct 14th - so I would appreciate another review by someone else, if possible.

If Christoph accepts, I am fine with this.

Hm, given the size of the patch, and given it introduces new public API we cannot easily change, I think a silent +1 since noone reacts is not good enough.

I agree that the amount of review done is not good enough. I would have liked some more feedback on both the patches, but most people sadly never got further than "nice feature, looking forward". I was very glad when you did your initial review :)

On the other hand I have reiterated these patches a few times while they waited on phab for review, also tried multiple approaches last year before uploading the delegate one. So while my perfectionism-self still sees some issues in the new API, my pragmatism-self is actually pretty okay with it.
The additional things I would have liked in the API actually would mean some rework of the currently line-based rendering approach inside KateIconBorder, which due to review needs might have worsened the chance of getting at least the basic feature in at all. So I dumped that work and reduced the patch to what we have now.

That said, I will not have time for another review until Oct 14th - so I would appreciate another review by someone else, if possible.

Given this is API only needed for KDevelop 5.4, which might happen only begin of next year, I am okay with delaying another KF5 cycle/month, and schedule now to target 5.23 (merge after 5.22 has been tagged). If this actually means getting deep embracing reviews :) I was only going for action seeing that no-one seems to (have time to) care, before this feature is reaching it's first anniversary here on phabricator in November :( Not sure if I should have nagged more to get you to review this? But then I dislike being nagged myself for things which are pretty visible on phabricator, unless it's quick simple patches.

So, anyone thinking they will have time & interest to do a full review in the October weeks?

And I still think the annotation border feature would be a nice tool for people using kate/kwrite on versioned documents as well (given there is some VCS support to hook in). Is this really no use case Kate users have as well? -> https://frinring.wordpress.com/2018/09/09/from-code-to-related-bug-report-or-review-in-just-a-hover-click/ Or do people not use Kate to work on old codebases? :P
(a future planned addition will be integration to addressbooks/gravatar, so you could see the faces of the authors and/or have the option to directly contact them in case of need. All that could be done as one finds fancy thanks to the new delegate API introduced here, without having to twist something in KTextEditor)

@cullmann @dhaumann So, what to do? :) Do you think if we delay one more month you will find time to give this the wanted deeper review?

Or will this continue to (understandable) lack your motivation given you are so far not a consumer of this new API?

Hi,

not that I am a expert in delegates but from a glance the interface part looks ok.
I would have no issues with merging this.
It is an optional feature, we have one user.
We can still evolve this interface with some Vx version if bad issues are discovered after some usage in KDevelop and do a cleanup for KF6.

But we can wait for Dominik's opinion.

kossebau updated this revision to Diff 43516.Oct 13 2018, 2:17 AM

Updating:

  • bump since to 5.53
  • remove todo question about background paiting, assumed not to be required task of the delegate given the current code
  • implement todo about catching destroyed user delegate (not needed by kdevelop, done for completeness)
  • implement setting wrappedline info in styleoption for helpevent (not needed by kdevelop, done for completeness)

Given we are almost one week into 5.52 code development, and given that
Dominik might find time to do some more review, I propose to delay now
to the next release cycle, to hopefully have no objections left/raised to push
then right after 5.52 tagging.

dhaumann accepted this revision.Oct 17 2018, 6:31 PM

Feel free to push this for 5.52 (you'd have to adapt the @since 5.53 again).

I only have minor comments that should not hold back this patch. One thing that pops into my eyes is that there are still some "todo" comments, which even talk about cornercases / bugs. Since I think you know what you are doing, I leave it up to you to decide wither it's ok as is or whether there need to be more fixes.

In any case, it was never my (and I think I can also safely say) nor any other's intention to hold this patch back. So yes, pinging more often is ok imo. Luckily, you are not a first-time contributor (who would be very much discouraged by our review delays), so I am sure / hope we'll see more patches from you in the future as well! :-)

src/include/ktexteditor/abstractannotationitemdelegate.h
67

Is the view pointer always valid when the style option is passed as argument? If so, I suggest to add this as comment to avoid unnecessary if() calls / error handling etc...

src/view/kateannotationitemdelegate.h
21

All keywords starting with __ are reserved, I suggest to remove __ at the beginning and at the end.

This revision is now accepted and ready to land.Oct 17 2018, 6:31 PM
kossebau marked 2 inline comments as done.Oct 18 2018, 1:41 PM

Feel free to push this for 5.52 (you'd have to adapt the @since 5.53 again).

Thanks for review again :)

Would prefer waiting for 5.53, for some full possible period of testing by people running from master, which should be at least > number of people geting and trying cross-project/repo patches :)

I only have minor comments that should not hold back this patch. One thing that pops into my eyes is that there are still some "todo" comments, which even talk about cornercases / bugs. Since I think you know what you are doing, I leave it up to you to decide wither it's ok as is or whether there need to be more fixes.

Yes, the TODOs left are about non-critical things, like issues already in the old code (those about rendering group delimiters at the lines on the view top/bottom borders), old magic numbers which could see some reasoning, or some check which is practically not needed, but might be expected by some looking at patterns. Left as TODO remarks for my older self or someone else, in case they work on this, so the rough edges are documented and not silently in the code.

In any case, it was never my (and I think I can also safely say) nor any other's intention to hold this patch back. So yes, pinging more often is ok imo. Luckily, you are not a first-time contributor (who would be very much discouraged by our review delays), so I am sure / hope we'll see more patches from you in the future as well! :-)

Okay, white card for more poking about reviews happily received :)
Some other reason might also have been that I left the ""WIP" too long in the title, which might also have sent a wrong signal, when this was rather RFC on the working prototype/pre-production sample.

src/view/kateannotationitemdelegate.h
21

Had those to be in line with other headers in that dir. Will do a separate patch to change that for the existing headers then.

kossebau updated this revision to Diff 43868.Oct 18 2018, 1:42 PM
kossebau marked an inline comment as done.

update to Dominik's last review:

  • remove __ from include guard
  • add comment that Option::view is always set
This revision was automatically updated to reflect the committed changes.