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

Reply via email to