D10797: Add asterisk when an annotation is associated to non-empty popup

This revision was automatically updated to reflect the committed changes. Closed by commit R223:cbc6f671e35c: Change annotation type name when an annotation is associated to non-empty popup (authored by simgunz, committed by aacid). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D10797?vs

D10797: Add asterisk when an annotation is associated to non-empty popup

aacid accepted this revision. aacid added a comment. This revision is now accepted and ready to land. For symmetry i will add the disconnect line in the setSourceModel, i'm not even sure we even use that if codepath but it's nice to be symmetric REPOSITORY R223 Okular BRANCH annotations-

D10797: Add asterisk when an annotation is associated to non-empty popup

simgunz updated this revision to Diff 32535. simgunz added a comment. - Propagate dataChanged in Author and Page proxy models REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10797?vs=31569&id=32535 BRANCH annotations-with-note REVISION DETAIL https://ph

D10797: Add asterisk when an annotation is associated to non-empty popup

rkflx added a comment. In D10797#241905 , @rkflx wrote: > There is a second way `arc diff` can recognize the revision […] (IIRC – if not, `arc which` will tell you the real reason). Hm, I guess that was wishful thinking from my side. Upon

D10797: Add asterisk when an annotation is associated to non-empty popup

rkflx added a comment. In D10797#241886 , @simgunz wrote: > My doubt is: when you want to review a "review" what do you do exactly (I never did it). You download a patch and apply it manually to your code (probably arc does this with some comman

D10797: Add asterisk when an annotation is associated to non-empty popup

simgunz added a comment. > You're not really pushing a "branch" with `arc`, you can think of it more as a diff describing a range of commits. This is indeed what I believed. My doubt is: when you want to review a "review" what do you do exactly (I never did it). You download a patch

D10797: Add asterisk when an annotation is associated to non-empty popup

rkflx added a comment. (OT) Won't comment on the patch, but in the light of T7116: Streamlined onboarding of new contributors it might be worth figuring out if there are any improvements we could do to https://community.kde.org/Infrastructure/Phabricator,

D10797: Add asterisk when an annotation is associated to non-empty popup

aacid added a comment. In D10797#241744 , @simgunz wrote: > I have created a new revision D12013 by mistake. That can be deleted. > > I have removed the commit with the asterisk I have added before, becaus

D10797: Add asterisk when an annotation is associated to non-empty popup

simgunz added a comment. I have created a new revision D12013 by mistake. That can be deleted. I have removed the commit with the asterisk I have added before, because when I just tried to revert it phabricator was complaining and didn't let me update

D10797: Add asterisk when an annotation is associated to non-empty popup

simgunz updated this revision to Diff 31569. simgunz added a comment. - Remove extra space REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10797?vs=31567&id=31569 BRANCH annotations-with-note REVISION DETAIL https://phabricator.kde.org/D10797 AFFECTED

D10797: Add asterisk when an annotation is associated to non-empty popup

simgunz updated this revision to Diff 31567. simgunz added a comment. Add suffix "with comment" instead of asterisk for more clarity REPOSITORY R223 Okular CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10797?vs=27929&id=31567 BRANCH annotations-with-note REVISION DETAIL htt

D10797: Add asterisk when an annotation is associated to non-empty popup

aacid added a comment. In D10797#241432 , @simgunz wrote: > What is the best way to implement this? Given the call to i18n I guess I cannot just append a suffix to ret. > Should I do for each annotation type something like: > > bool hasC

D10797: Add asterisk when an annotation is associated to non-empty popup

simgunz added a comment. What is the best way to implement this? Given the call to i18n I guess I cannot just append a suffix to ret. Should I do for each annotation type something like: bool hasComment = !ann->contents().isEmpty(); ret = hasComment ? i18n( "Highlight with Commen

D10797: Add asterisk when an annotation is associated to non-empty popup

simgunz added a comment. For me seems a good short term solution. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10797 To: simgunz, #okular, aacid Cc: aacid, ngraham, michaelweghorn

D10797: Add asterisk when an annotation is associated to non-empty popup

aacid added a comment. I'm not sure i'm a fan of using an icon for this. What about juts changing the caption? i.e. use ret = i18n( "Freehand Line with Comment" ); instead of ret = i18n( "Freehand Line" ); ? It's very extreme but it's obvious :D REPOSITORY R223 Okular REVI

D10797: Add asterisk when an annotation is associated to non-empty popup

simgunz added a comment. In Foxit reader the comments to the annotations are explicitly shown in the comments bar: F5753503: image.png Evince does not have this feature F5753525: image.png Maybe w

D10797: Add asterisk when an annotation is associated to non-empty popup

aacid requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10797 To: simgunz, #okular, aacid Cc: aacid, ngraham, michaelweghorn

D10797: Add asterisk when an annotation is associated to non-empty popup

aacid added inline comments. INLINE COMMENTS > annotationmodel.cpp:346 > +{ > +caption.append("*"); > +} i'm not sure this is LTR friendly, basically you always put the work on the translators side so it's them that can do it correctly, so you'd do i18nc

D10797: Add asterisk when an annotation is associated to non-empty popup

aacid added a comment. In D10797#214159 , @simgunz wrote: > I agree. Any idea? > > Put the annotations containing a popup note in italic? Not my favorite choice though. > > I would like something subtle. Do any other software has

D10797: Add asterisk when an annotation is associated to non-empty popup

simgunz added a comment. I agree. Any idea? Put the annotations containing a popup note in italic? Not my favorite choice though. I would like something subtle. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10797 To: simgunz, #okular Cc: ngraham, michaelw

D10797: Add asterisk when an annotation is associated to non-empty popup

ngraham added a comment. Doesn't an asterisk after a title typically denote an unsaved file? Maybe we should come up with a different UI metaphor here. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D10797 To: simgunz, #okular Cc: ngraham, michaelweghorn, aacid

D10797: Add asterisk when an annotation is associated to non-empty popup

simgunz created this revision. simgunz added a reviewer: Okular. Restricted Application added a project: Okular. simgunz requested review of this revision. REVISION SUMMARY Mark the annotations that contain a non-empty popup. Especially useful to identify annotations that are not meant to conta