dhaumann added a comment.
This is already an excellent patch. The API documentation is already really
nice.
I have some minor suggestions (see inline comments in patch). Besides that, I
wonder whether this should really be a View extension interaface. Currently,
this interface is implemented by the KTextEditor::Document, while the API
documentation mentions this interafce is a View extension interface. I would
prefer a View extension interface.
Could you provide an updated revision of this patch?
And also: I think we have to discuss unit tests as well - since such kind of
code will break otherwise when we have to change related code parts. Would be
nice to have a discussion about this already now.
INLINE COMMENTS
> katedocument.cpp:5316-5320
> + // Common cases first
> + switch (m_inlineNoteProviders.size()) {
> + case 0: return {};
> + case 1: return m_inlineNoteProviders.first()->inlineNotes(line);
> + }
This look like premature optimization. I would prefer to delete this "common
cases first" block. Except if this turned out to be a bottleneck already?
> inlinenoteinterface.h:49
> + * property.
> + *
> + * To register as inline note provider, call registerInlineNoteProvider()
> with
It would be nice to maybe include a picture here: Put a png into
ktexteditor/docs/pics/ and use it like this:
- \image html inlinenotes.png "Inline note showing a CSS color"
> inlinenoteinterface.h:53
> + * your inline note provider by calling unregisterInlineNoteProvider().
> + *
> + * \section inlinenote_access Accessing the InlineNoteInterface
Could you add this note:
\note A InlineNoteProvider instance can be reused/shared by multipe views,
just make sure to unregister the provider from \e all views before deleting the
provider.
> inlinenoteinterface.h:117
> + */
> + virtual int column() const = 0;
> +
I would prefer
virtual KTextEditor::Cursor position() const = 0;
This way, a InlineNote contains not only the column, but ALL information (line
+ column) that defines the position of the note.
> inlinenoteinterface.h:155
> + *
> + * InlineNoteProvider is object that can be queried for inline notes in the
> + * document. It emits signals when the notes change and should be queried
> again.
InlineNoteProvider is _a_ object that can be ... (here is 'a' missing)
> katerenderer.cpp:773
> + qreal x =
> range->viewLine(viewLine).lineLayout().cursorToX(column) - xStart;
> + int textLength = range->length();
> + if (column == 0 || column < textLength) {
Please use const for variables that do not change anymore:
const int textLength = ...
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D12662
To: michalsrb, #ktexteditor
Cc: dhaumann, cullmann, ngraham, brauch, #frameworks, michaelh, kevinapavew,
bruns, demsking, sars
