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 <jfireba...@kde.org>
> + Copyright (C) 2001 Anders Lund <and...@alweb.dk>
> + Copyright (C) 2001 Christoph Cullmann <cullm...@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
