D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

This revision was automatically updated to reflect the committed changes. Closed by commit R39:9a0505af2dbf: Introduce AbstractAnnotationItemDelegate for more control by consumer (authored by kossebau). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D8708?vs=43868=44806#toc REPOSITORY

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau updated this revision to Diff 43868. kossebau marked an inline comment as done. kossebau added a comment. update to Dominik's last review: - remove __ from include guard - add comment that Option::view is always set REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau marked 2 inline comments as done. kossebau added a comment. In D8708#344872 , @dhaumann wrote: > Feel free to push this for 5.52 (you'd have to adapt the @since 5.53 again). Thanks for review again :) Would prefer waiting for

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

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

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau updated this revision to Diff 43516. kossebau added a comment. Updating: - bump since to 5.53 - remove todo question about background paiting, assumed not to be required task of the delegate given the current code - implement todo about catching destroyed user delegate (not

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

cullmann added a comment. Hi, not that I am a expert in delegates but from a glance the interface part looks ok. I would have no issues with merging this. It is an optional feature, we have one user. We can still evolve this interface with some Vx version if bad issues are

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau added a comment. @cullmann @dhaumann So, what to do? :) Do you think if we delay one more month you will find time to give this the wanted deeper review? Or will this continue to (understandable) lack your motivation given you are so far not a consumer of this new API?

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau added a comment. In D8708#337408 , @dhaumann wrote: > Hm, given the size of the patch, and given it introduces new public API we cannot easily change, I think a silent +1 since noone reacts is not good enough. I agree that the

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

dhaumann added a comment. If Christoph accepts, I am fine with this. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8708 To: kossebau, #kate, #kdevelop Cc: cullmann, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns, demsking, sars

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

dhaumann added a comment. Hm, given the size of the patch, and given it introduces new public API we cannot easily change, I think a silent +1 since noone reacts is not good enough. That said, I will not have time for another review until Oct 14th - so I would appreciate another review

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

cullmann added a comment. As we have a usecase for this (the extension in KDevelop) I am ok if that goes in, if the extension in KDevelop is going in, too, as consumer. We can still fix minor issues in the KF6 iteration, they will never be found if it is not there and used. REPOSITORY

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau updated this revision to Diff 42903. kossebau added a comment. Improve rendering in scaled mode REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8708?vs=41828=42903 BRANCH addAnnotationItemDelegate REVISION DETAIL

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau added a comment. Thanks everyone who so far looked at this, especially @dhaumann for detailed comments. I am tempted to interpret the lack of further comments, especially the lack of principal objections as an implicit +1 on this patch as it is now :) While I still am

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau added a comment. In D8708#326590 , @dhaumann wrote: > What I would like to see is a comment // KF6: Merge KTextEditor::AnnotationViewInterfaceV2 into KTextEditor::AnnotationViewInterface (kossebau). > For me, this comment is really

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau updated this revision to Diff 41828. kossebau added a comment. also add todo about merging KTextEditor::AnnotationViewInterfaceV2 into KTextEditor::AnnotationViewInterface REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8708?vs=41778=41828

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau updated this revision to Diff 41778. kossebau marked 3 inline comments as done. kossebau added a comment. Update to Dominik's first review REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8708?vs=41694=41778 BRANCH addAnnotationItemDelegate

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau marked 15 inline comments as done. kossebau added inline comments. INLINE COMMENTS > dhaumann wrote in abstractannotationitemdelegate.h:52 > I am asking myself: Is wrappedLineCount >= 1 always true? If so, can you add > this as documentation? wrappedLineCount == 1 means no wrapping

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

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

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau updated this revision to Diff 41694. kossebau added a comment. fix crash due to (no longer needed) circular dependencies on view object constrctions REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8708?vs=41225=41694 BRANCH

D8708: Introduce AbstractAnnotationItemDelegate for more control by consumer

kossebau retitled this revision from "[WIP] Introduce AbstractAnnotationItemDelegate for more control by consumer" to "Introduce AbstractAnnotationItemDelegate for more control by consumer". kossebau edited the summary of this revision. kossebau added reviewers: Kate, KDevelop. REPOSITORY R39