D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:9a0505af2dbf: Introduce AbstractAnnotationItemDelegate for 
more control by consumer (authored by kossebau).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D8708?vs=43868=44806#toc

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=43868=44806

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop, dhaumann
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 43868.
kossebau marked an inline comment as done.
kossebau added a comment.


  update to Dominik's last review:
  
  - remove __ from include guard
  - add comment that Option::view is always set

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=43516=43868

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop, dhaumann
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau marked 2 inline comments as done.
kossebau added a comment.


  In D8708#344872 , @dhaumann wrote:
  
  > 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.

INLINE COMMENTS

> dhaumann wrote in kateannotationitemdelegate.h:20
> All keywords starting with __ are reserved, I suggest to remove __ at the 
> beginning and at the end.

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.

REPOSITORY
  R39 KTextEditor

BRANCH
  addAnnotationItemDelegate

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

To: kossebau, #kate, #kdevelop, dhaumann
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Dominik Haumann]

dhaumann accepted this revision.
dhaumann added a comment.
This revision is now accepted and ready to land.


  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! :-)

INLINE COMMENTS

> abstractannotationitemdelegate.h:66
> +/**
> + * The view where the annotation is shown
> + */

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...

> kateannotationitemdelegate.h:20
> +
> +#ifndef __KATE_ANNOTATIONITEMDELEGATE_H__
> +#define __KATE_ANNOTATIONITEMDELEGATE_H__

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

REPOSITORY
  R39 KTextEditor

BRANCH
  addAnnotationItemDelegate

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

To: kossebau, #kate, #kdevelop, dhaumann
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 43516.
kossebau added a comment.


  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.

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=42903=43516

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Christoph Cullmann]

cullmann added a comment.


  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.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau added a comment.


  @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?

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D8708#337408 , @dhaumann wrote:
  
  > 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)

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Dominik Haumann]

dhaumann added a comment.


  If Christoph accepts, I am fine with this.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Dominik Haumann]

dhaumann added a comment.


  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.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Christoph Cullmann]

cullmann added a comment.


  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.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 42903.
kossebau added a comment.


  Improve rendering in scaled mode

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=41828=42903

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns, 
demsking, cullmann, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns, 
demsking, cullmann, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D8708#326590 , @dhaumann wrote:
  
  > 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 ;)

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 41828.
kossebau added a comment.


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

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=41778=41828

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 41778.
kossebau marked 3 inline comments as done.
kossebau added a comment.


  Update to Dominik's first review

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=41694=41778

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/utils/ktexteditor.cpp
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, 
ngraham, bruns, demsking, cullmann, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau marked 15 inline comments as done.
kossebau added inline comments.

INLINE COMMENTS

> dhaumann wrote in abstractannotationitemdelegate.h:52
> I am asking myself: Is wrappedLineCount >= 1 always true? If so, can you add 
> this as documentation? wrappedLineCount == 1 means no wrapping line?

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.

> dhaumann wrote in abstractannotationitemdelegate.h:65
> Is this to be set by the user, and if so, is there any special meaning to a 
> default-constructed QSize()?

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?

> dhaumann wrote in abstractannotationitemdelegate.h:80
> Is this a bitmask? The << 0, <<1, <<2 notation implies that it is.

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.

> dhaumann wrote in abstractannotationitemdelegate.h:95
> 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.

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.

> dhaumann wrote in kateannotationitemdelegate.cpp:52-53
> Given you check for a valid pointers here, would it also be an option to pass 
> references? Or would that violate Qt style API?

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

> dhaumann wrote in kateannotationitemdelegate.h:2-4
> Is this copyright really correct?

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

> dhaumann wrote in kateannotationitemdelegate.h:9
> 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.

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).

> dhaumann wrote in kateviewhelpers.cpp:2636
> Do you really need the timer here? I thought update() goes through the event 
> queue anyways?

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

> dhaumann wrote in kateviewhelpers.cpp:2646
> 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 :-)

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 

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Dominik Haumann]

dhaumann added a comment.


  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.

INLINE COMMENTS

> abstractannotationitemdelegate.h:39
> + * \brief The style option set for an annotation item, as painted by 
> AbstractAnnotationItemDelegate
> + */
> +class KTEXTEDITOR_EXPORT StyleOptionAnnotationItem : public QStyleOption

@since 5.52 :-)

> abstractannotationitemdelegate.h:52
> +/**
> + * Number of wrapped lines for the given real line
> + */

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

> abstractannotationitemdelegate.h:65
> +/**
> + * Recommended size for icons or other symbols
> + */

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

> abstractannotationitemdelegate.h:80
> + */
> +enum AnnotationItemGroupPosition {
> +InvalidGroupPosition = 0,  ///< Position not specified or not 
> belonging to a group

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

> abstractannotationitemdelegate.h:95
> +public:
> +StyleOptionAnnotationItem()
> +: contentFontMetrics(QFont())

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.

> abstractannotationitemdelegate.h:114
> +/**
> + * \brief An delegate for rendering line annotation information and handling 
> events for them
> + *

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

> abstractannotationitemdelegate.h:136
> +protected:
> +explicit AbstractAnnotationItemDelegate(QObject *parent = nullptr)
> +: QObject(parent)

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

> abstractannotationitemdelegate.h:141
> +public:
> +~AbstractAnnotationItemDelegate() override = default;
> +

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

> abstractannotationitemdelegate.h:179
> + * Whenever a help event occurs, this function is called with the event 
> view option
> + * and @p model and @p line specifiying the item where the event occurs.
> + * This pure abstract function must be reimplemented to provide custom 
> tooltips.

typo: specifying

> abstractannotationitemdelegate.h:183
> + * @param view the view for which the help event is requested
> + * @param option the style option object with the info needed for 
> styling, incl. the rect of the annotation
> + * @param model the annotation model providing the annotation information

inlc. -> including. No need for abbreviations

> annotationinterface.h:285
> + * and additionally
> + * - (1) set a custom AbstractAnnotationItemDelegate for the View:
> + *

Change trailing ':' to '.'

> annotationinterface.h:288
> + * For a more detailed explanation about whether you want to set a custom
> + * delegate fpr rendering the annotations, read the detailed documentation 
> about the
> + * AbstractAnnotationItemDelegate.

typo: fpr

> annotationinterface.h:328
> +/**
> + * Returns the currently used \ref AbstractAnnotationItemDelegate
> + *

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

> annotationinterface.h:330
> + *
> + * @returns the current \ref AbstractAnnotationItemDelegate
> + */

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

> kateannotationitemdelegate.cpp:52-53
> +{
> +Q_ASSERT(painter);
> +Q_ASSERT(model);
> +if (!painter || !model) {

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

> kateannotationitemdelegate.cpp:57
> +}
> +// TODO: also test line for validness for sake of completeness?
> +

validness --> validity :-)

> kateannotationitemdelegate.h:2-4
> +   Copyright (C) 2002 John Firebaugh 
> +   Copyright (C) 2001 Anders Lund 
> +   Copyright (C) 2001 Christoph Cullmann 

Is this copyright really correct?

> kateannotationitemdelegate.h:9
> +   modify it under the terms of the GNU Library General Public
> +   License version 2 as published by the Free Software Foundation.
> +

This is v2 only, I think this should be avoided at all costs... We try to get 
to v2+... I think you can change 

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau updated this revision to Diff 41694.
kossebau added a comment.


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

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8708?vs=41225=41694

BRANCH
  addAnnotationItemDelegate

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

AFFECTED FILES
  src/CMakeLists.txt
  src/include/CMakeLists.txt
  src/include/ktexteditor/abstractannotationitemdelegate.h
  src/include/ktexteditor/annotationinterface.h
  src/view/kateannotationitemdelegate.cpp
  src/view/kateannotationitemdelegate.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewhelpers.cpp
  src/view/kateviewhelpers.h
  src/view/kateviewinternal.h

To: kossebau, #kate, #kdevelop
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Friedrich W. H. Kossebau]

kossebau retitled this revision from "[WIP] Introduce 
AbstractAnnotationItemDelegate for more control by consumer" to "Introduce 
AbstractAnnotationItemDelegate for more control by consumer".
kossebau edited the summary of this revision.
kossebau added reviewers: Kate, KDevelop.

REPOSITORY
  R39 KTextEditor

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

To: kossebau, #kate, #kdevelop
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars, dhaumann