D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

[Dominik Haumann]

dhaumann accepted this revision.
dhaumann added a comment.
This revision is now accepted and ready to land.


  Feel free to push this for 5.52 (you'd have to adapt the @since 5.53 again).
  
  I only have minor comments that should not hold back this patch. One thing that pops into my eyes is that there are still some "todo" comments, which even talk about cornercases / bugs. Since I think you know what you are doing, I leave it up to you to decide wither it's ok as is or whether there need to be more fixes.
  
  In any case, it was never my (and I think I can also safely say) nor any other's intention to hold this patch back. So yes, pinging more often is ok imo. Luckily, you are not a first-time contributor (who would be very much discouraged by our review delays), so I am sure / hope we'll see more patches from you in the future as well! :-)

INLINE COMMENTS

> abstractannotationitemdelegate.h:66
> +    /**
> +     * The view where the annotation is shown
> +     */

Is the view pointer always valid when the style option is passed as argument? If so, I suggest to add this as comment to avoid unnecessary if() calls / error handling etc...

> kateannotationitemdelegate.h:20
> +
> +#ifndef __KATE_ANNOTATIONITEMDELEGATE_H__
> +#define __KATE_ANNOTATIONITEMDELEGATE_H__

All keywords starting with __ are reserved, I suggest to remove __ at the beginning and at the end.

REPOSITORY
  R39 KTextEditor

BRANCH
  addAnnotationItemDelegate

REVISION DETAIL
  https://phabricator.kde.org/D8708

To: kossebau, #kate, #kdevelop, dhaumann
Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns, demsking, sars
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwrite-devel/attachments/20181017/60957bff/attachment-0001.html>