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 <jfirebaugh at kde.org>
> +   Copyright (C) 2001 Anders Lund <anders at alweb.dk>
> +   Copyright (C) 2001 Christoph Cullmann <cullmann at kde.org>

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 this, since this is your work. Even if it's based on others, the work of others is in the other files.

> kateviewhelpers.cpp:2626
> +
> +        // TODO: catch delegate being destroyed
> +        m_annotationItemDelegate = delegate;

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

> kateviewhelpers.cpp:2636
> +
> +        QTimer::singleShot(0, this, SLOT(update()));
>      }

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

> kateviewhelpers.cpp:2646
> +    styleOption->contentFontMetrics = m_view->renderer()->config()->fontMetrics();
> +    // TODO have delegate paint all background or not?
> +//     styleOption->backgroundBrush = m_view->renderer()->config()->iconBarColor();

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

> kateviewhelpers.cpp:2664
>  {
> -    if (annotationLineWidth(line) > m_annotationBorderWidth) {
> -        m_annotationBorderWidth = annotationLineWidth(line);
> +    // TODO: why has the default value been 8, where is that magic number from?
> +    int width = 8;

Maybe 4+4 margin or so?

> kateviewhelpers.cpp:2706
> +
> +    QTimer::singleShot(0, this, SLOT(update()));
> +}

Again, single shot timer needed?

> kateviewhelpers.h:356-357
> +    KTextEditor::AbstractAnnotationItemDelegate *m_annotationItemDelegate;
> +    bool m_hasUniformAnnotationItemSizes;
> +    bool m_isDefaultAnnotationItemDelegate;
> +

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

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/20180915/83bd6c6a/attachment-0001.html>