D12662: Add InlineNoteInterface

[Dominik Haumann]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180506/6ad4e13b/attachment-0001.html>