Introduce AbstractAnnotationItemDelegate for more control by consumer
Needs ReviewPublic

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

Details

Reviewers
None
Group Reviewers
Kate
KDevelop
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 2957
Build 2975: arc lint + arc unit
kossebau created this revision.Nov 8 2017, 12:36 AM
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.Tue, Sep 4, 3:44 PM

update to latest master

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

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

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

fix not listening to sizeHintChanged signal

kossebau updated this revision to Diff 41225.EditedSat, Sep 8, 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.Sat, Sep 8, 8:56 PM
kossebau edited the summary of this revision. (Show Details)
kossebau added reviewers: Kate, KDevelop.
kossebau updated this revision to Diff 41694.Sat, Sep 15, 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?

2644

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

2662–2674

Maybe 4+4 margin or so?

2704

Again, single shot timer needed?

src/view/kateviewhelpers.h
356–357

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

kossebau marked 15 inline comments as done.Sun, Sep 16, 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.

2644

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.Sun, Sep 16, 9:20 PM
kossebau marked 3 inline comments as done.

Update to Dominik's first review

kossebau updated this revision to Diff 41828.Mon, Sep 17, 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 ;)