D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

Sun Sep 16 22:19:46 BST 2018

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 <http://doc.qt.io/qt-5/qstyleoptionviewitem.html#decorationSize-var>. 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(...) <http://doc.qt.io/qt-5/qabstractitemdelegate.html#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 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.

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwrite-devel/attachments/20180916/58d8c693/attachment-0001.html>