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

[Albert Astals Cid]

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=32535=32662#toc

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10797?vs=32535=32662

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

AFFECTED FILES
  ui/annotationproxymodels.cpp
  ui/annotationproxymodels.h
  ui/guiutils.cpp

To: simgunz, #okular, aacid
Cc: rkflx, aacid, ngraham

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

[Albert Astals Cid]

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-with-note

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

To: simgunz, #okular, aacid
Cc: rkflx, aacid, ngraham

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

[Simone Gaiarin]

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=32535

BRANCH
  annotations-with-note

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

AFFECTED FILES
  ui/annotationproxymodels.cpp
  ui/annotationproxymodels.h
  ui/guiutils.cpp

To: simgunz, #okular, aacid
Cc: rkflx, aacid, ngraham, michaelweghorn

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

[Henrik Fehlauer]

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 `arc diff` Arcanist 
will just add the "Differential Revision" line with the Revision ID obtained 
from Phab to the local commit message.
  
  This means that indeed somehow you must have been removing the commit 
containing that message from the commit range Arcanist looked at.

REPOSITORY
  R223 Okular

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

To: simgunz, #okular, aacid
Cc: rkflx, aacid, ngraham, michaelweghorn

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

[Henrik Fehlauer]

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 command)?
  
  
  Anyone wanting to review or simply try out a patch usually just issues `arc 
patch D10797`. This will make sure the repo is up-to-date to be able to find 
the commit you attached your revision to, create a new local branch named 
`arcpatch-D10797`, download all dependent revisions and finally apply the 
patch. In case the author updates the patch, the reviewer will re-issue `arc 
patch`, get a new branch called `arcpatch-D10797_1` and so on. Note that on 
these branches there is only a single squashed commit per Diff, i.e. (at least 
currently, might change in the future) what you see in Revision Contents > 
Commits is only visible on Phab.
  
  To investigate what the author changed, you could compare both local 
branches, but I guess most people just just the options for comparing revisions 
under Revision Contents > History.
  
  > If I overwrite some commits, as I did today, does it create any problem to 
the reviewer that already "pulled" from arc the review once? (in git this will 
create diverging branches between local and remote) Or does arc redownload the 
diff completely each time, so avoiding problems?
  
  Phabricator remembers all Diffs you uploaded. `arc patch` will simply 
re-download the complete patch, using the latest revision unless you specify an 
older revision.
  
  > This was not my fear, but I feared to mess up things to the reviewer that 
already "pulled" the review before.
  
  Nothing to fear. If it looks good in the Diff viewer on Phabricator, it will 
look good for your reviewer.
  
  > It was complaining about the fact that the commit with the revert did not 
contain a valid "Differential Revision" ID
  
  Ah, my comment might have been misleading. This line will only get added once 
you do `arc patch` or `arc amend`. However, you won't need to issue those 
commands or even add the line manually, because Arcanist will do it for you 
once you `arc land`.
  
  There is a second way `arc diff` can recognize the revision, which works by 
comparing your local commit sha with what has been recorded on Phabricator 
(IIRC – if not, `arc which` will tell you the real reason). If you change 
things in such a way that the commit sha on your local branch changed (which 
should not happen if you just add a reverting commit), then you'd have to 
manually provide the revision ID.
  
  > I'll also have a look at  T7116  and 
maybe I can give some feedback from a point of view of a newcomer.
  
  That would be appreciated ;) (Or just update the Wiki yourself.)

REPOSITORY
  R223 Okular

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

To: simgunz, #okular, aacid
Cc: rkflx, aacid, ngraham, michaelweghorn

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

[Simone Gaiarin]

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 and apply it manually to your code 
(probably arc does this with some command)?
  If I overwrite some commits, as I did today, does it create any problem to 
the reviewer that already "pulled" from arc the review once? (in git this will 
create diverging branches between local and remote) Or does arc redownload the 
diff completely each time, so avoiding problems?
  
  > IOW, you should be able to do pretty much want you want on your local 
branch without having to fear to break something in the upstream Git repo 
(unless you `git push` or `arc land`).
  
  This was not my fear, but I feared to mess up things to the reviewer that 
already "pulled" the review before.
  
  > Also, in general I'd expect you could just `arc diff` over a commit range 
containing a local revert. I cannot say too much without knowing details about 
"complaining and didn't let me update the revision", but given your accidental 
new Diff it seems to me that you got in a situation where `arc` got the commit 
range wrong, i.e. it could not find a commit containing the "Differential 
Revision" line, so it could not attach to that.
  
  It was complaining about the fact that the commit with the revert did not 
contain a valid "Differential Revision" ID
  
  >   a commit containing  the "Differential Revision" line,
  
  Can you explain this better? I've never written any "Differential Revision" 
line in my commit messages explicitly. Does arc produces new commits with this 
line?
  
  > Use `arc which` before `arc diff` to get more information about what 
Arcanist will do.
  
  Thanks. That is something I was looking for. I should re-read the phabricator 
guide more often.
  
  I'll also have a look at  T7116  and maybe 
I can give some feedback from a point of view of a newcomer.

REPOSITORY
  R223 Okular

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

To: simgunz, #okular, aacid
Cc: rkflx, aacid, ngraham, michaelweghorn

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

[Henrik Fehlauer]

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, so allow me to add some 
WoT:
  
  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, because 
when I just tried to revert it phabricator was complaining and didn't let me 
update the revision. Is pushing a rebased branch to the phabricator diff a bad 
practice (usually it is in git)?
  
  
  You're not really pushing a "branch" with `arc`, you can think of it more as 
a diff describing a range of commits. By default Arcanist takes all commits 
between HEAD and the merge-base of your branch and its upstream tracking 
branch, i.e. likely `master`, but this can be changed, of course. See 
https://secure.phabricator.com/book/phabricator/article/arcanist_commit_ranges/ 
for more.
  
  IOW, you should be able to do pretty much want you want on your local branch 
without having to fear to break something in the upstream Git repo (unless you 
`git push` or `arc land`). Also, in general I'd expect you could just `arc 
diff` over a commit range containing a local revert. I cannot say too much 
without knowing details about "complaining and didn't let me update the 
revision", but given your accidental new Diff it seems to me that you got in a 
situation where `arc` got the commit range wrong, i.e. it could not find a 
commit containing the "Differential Revision" line, so it could not attach to 
that. Use `arc which` before `arc diff` to get more information about what 
Arcanist will do.

REPOSITORY
  R223 Okular

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

To: simgunz, #okular, aacid
Cc: rkflx, aacid, ngraham, michaelweghorn

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

[Albert Astals Cid]

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, because 
when I just tried to revert it phabricator was complaining and didn't let me 
update the revision. Is pushing a rebased branch to the phabricator diff a bad 
practice (usually it is in git)?
  
  
  Not really an experct in phabricator use myself, so no idea :D
  
  Ok, so we now have to fix the issue with updating of the model. The problem 
is not where you higlighted it, the problem is in the proxy models uses, since 
if you do this
  
  - m_view->setModel( m_authorProxy );
  
  +m_view->setModel( m_model );
  
  it works. Those proxy models could be definitely better coded, i'm almost 
sure the brokenness is in the two QAbstractProxyModel, my hunch is that on both 
their ::setSourceModel we need to connect to the dataChanged signal of the 
source model to a slot that does something similar to do this
  
  void OurProxyModel::sourceDataChanged(const QModelIndex , const 
QModelIndex , const QVector )
  {
  
emit dataChanged(mapFromSource(topLeft), mapFromSource(bottomRight), roles);
  
  }
  
  Can you give it a try?

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

[Simone Gaiarin]

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 
the revision. Is pushing a rebased branch to the phabricator diff a bad 
practice (usually it is in git)?
  
  > Pop-up note can have empty contents, but i guess since contents it's its 
main focus we can leave it out?
  
  Yes. I also didn't add "with comment" to "Caret" for the same reason.

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

[Simone Gaiarin]

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=31569

BRANCH
  annotations-with-note

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

AFFECTED FILES
  ui/guiutils.cpp

To: simgunz, #okular, aacid
Cc: aacid, ngraham, michaelweghorn

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

[Simone Gaiarin]

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=31567

BRANCH
  annotations-with-note

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

AFFECTED FILES
  ui/guiutils.cpp

To: simgunz, #okular, aacid
Cc: aacid, ngraham, michaelweghorn

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

[Albert Astals Cid]

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 hasComment = !ann->contents().isEmpty();
  >   ret =  hasComment ? i18n( "Highlight with Comment" ) : i18n( "Highlight 
Note" );
  >
  
  
  Yeah, that's it. Make the bool const for extra points :)
  
  > I guess I do not need to add "with Comment" to Pop-up note and Inline note 
given that they always have a comment.
  
  Pop-up note can have empty contents, but i guess since contents it's its main 
focus we can leave it out?

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

[Simone Gaiarin]

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 Comment" ) : i18n( "Highlight 
Note" );
  
  I guess I do not need to add "with Comment" to Pop-up note and Inline note 
given that they always have a comment.

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

[Albert Astals Cid]

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

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

[Simone Gaiarin]

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 we can add a small icon with a pencil (similar to the one in the evince 
screenshot) next to the annotation somewhere to mark that the annotation has a 
text note associated.
  
  The approach of foxit reader is best for me: one have a full view of the 
annotations in that way and can interact with the annotations in the side pane 
directly. The current approach of Okular does provide very little info.

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

[Albert Astals Cid]

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

[Albert Astals Cid]

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("This annotation contains text, %1 is the caption of the annotation", "%1 
*", caption);

or similar

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: aacid, ngraham, michaelweghorn

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

[Albert Astals Cid]

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 this functionality? Can we get inspiration from 
them?

REPOSITORY
  R223 Okular

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

To: simgunz, #okular
Cc: aacid, ngraham, michaelweghorn

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

[Simone Gaiarin]

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, michaelweghorn, aacid

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

[Nathaniel Graham]

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

[Simone Gaiarin]

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 contain an annotation as shapes or 
highlights.
  
  Problem: The side review is updated only when the mouse is hovered over it. 
It should be updated as soon as the content of the popup is changed. To do this 
the method notifyAnnotationChanges of DocumentPrivate in document.cpp should be 
made public so that it can be connected to the signal TextChanged of the 
textEdit in annotwindow.cpp.
  
  BUG: 389836

REPOSITORY
  R223 Okular

BRANCH
  annotations-with-note

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

AFFECTED FILES
  ui/annotationmodel.cpp

To: simgunz, #okular
Cc: michaelweghorn, ngraham, aacid