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

Authored by simgunz on Feb 24 2018, 4:17 PM.

Details

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

Diff Detail

Repository
R223 Okular
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
simgunz created this revision.Feb 24 2018, 4:17 PM
Restricted Application added a project: Okular. · View Herald TranscriptFeb 24 2018, 4:17 PM
simgunz requested review of this revision.Feb 24 2018, 4:17 PM

Doesn't an asterisk after a title typically denote an unsaved file? Maybe we should come up with a different UI metaphor here.

I agree. Any idea?

Put the annotations containing a popup note in italic? Not my favorite choice though.

I would like something subtle.

aacid added a subscriber: aacid.Feb 26 2018, 10:20 PM
aacid added inline comments.
ui/annotationmodel.cpp
346 ↗(On Diff #27929)

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

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?

aacid requested changes to this revision.Mar 13 2018, 10:54 PM
This revision now requires changes to proceed.Mar 13 2018, 10:54 PM

In Foxit reader the comments to the annotations are explicitly shown in the comments bar:

Evince does not have this feature

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.

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

For me seems a good short term solution.

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.

aacid added a comment.Fri, Apr 6, 9:24 PM

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?

simgunz updated this revision to Diff 31567.Sat, Apr 7, 8:00 AM

Add suffix "with comment" instead of asterisk for more clarity

simgunz updated this revision to Diff 31569.Sat, Apr 7, 8:07 AM
  • Remove extra space

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.

aacid added a comment.EditedSat, Apr 7, 10:22 AM

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 &topLeft, const QModelIndex &bottomRight, const QVector<int> &roles)
{
    emit dataChanged(mapFromSource(topLeft), mapFromSource(bottomRight), roles);
}

Can you give it a try?

rkflx added a subscriber: rkflx.Sat, Apr 7, 12:23 PM

(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:

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.

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.

rkflx added a comment.EditedSat, Apr 7, 1:17 PM

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 D10797, 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 ContentsCommits is only visible on Phab.

To investigate what the author changed, the reviewer could compare both local arcpatch-* branches, but I guess most people just use the options for comparing revisions under Revision ContentsHistory, or click on the email link leading to the same comparison URL.

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 recorded commit sha on your local branch is missing (which should not happen if you just add a reverting commit), then you'd have to manually provide the ID of the Diff.

Edit: Or did you mean with "reverting" that you did a git reset --hard <sha>? In that case Arcanist obviously has no way of knowing the relationship with any of the 10,000 patches on Phab and your local checkout (short of comparing the title, or the diff itself, but this is not reliable…).

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.)

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.

simgunz updated this revision to Diff 32535.Thu, Apr 19, 6:17 AM
  • Propagate dataChanged in Author and Page proxy models
aacid accepted this revision.Fri, Apr 20, 5:37 PM

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

This revision is now accepted and ready to land.Fri, Apr 20, 5:37 PM
This revision was automatically updated to reflect the committed changes.