In D15580#552455, @simgunz wrote:

When an annotation is selected its annotation toolbar button is checked and Browse Mode is checked, exactly as in the current Okular version.
When you select a quick annotation tool what happens is that the corresponding annotation action gets checked. In a sense a quick annotation is just an alias for an effective annotation with a specified set of settings (color, etc.). For this reason the quick annotation cannot have a checked state.
This however has a quirkiness in the case the annotation toolbar is hidden. In that case we are in Browse Mode and an annotation tool is active but we do not see the corresponding checked action in the hidden toolbar. This can be "solved" by showing the annotation toolbar when a quick annotation is selected, but I think it is going to become frustrating very soon.

Yeah, I don't think that's ideal.

Regarding browse mode, we can also uncheck it when an annotation is selected, if there are no side effects.

That's mostly what I think should happen. Not just browse mode though, but rather any tool that's currently active should become unchecked when you're using an annotation. Okular has a set of mutually-exclusive tools; annotations are just additional tools added to this list. For example you can't be both using an annotation and also in Browse mode. Rather, Okular would go back to Browse mode after you're done using an annotation tool.

Yes, but I do not know how to do it. See https://phabricator.kde.org/D15580#544534 for the details of the problem.

For now maybe we should just have an action rather than a toggle that's in sync with the toggle action in the menubar?

I thought about this, but I decided to leave "Configure Annotations" given that in the same KCM module the user can configure the "annotation identity", which is not related to "Quick Annotations" but to "Annotations" in general.
Changing it to "Configure" won't work either. This is a standard action that will appear in the "Configure Toolbar" and "Configure Shortcuts" dialog. If you call it "Configure" it will become ambiguous for the user.

Okay, let's drop it.

Currently the action name and thus the text is "Pin". You mean I should change "Pin" to "Keep Active"?

Yeah.

I agree with @davidhurka here regarding having it on by default. I cannot figure out a use case where a user wants to add a single annotation or keep reselecting it each time.
My typical use case is: I am reading a paper/book and I check the highlighter to highlight the text multiple times while I read, so I want it always active. The highlighter is a common use case, think at e-readers which have only highlighters and notes.
Another one is to fill in multiple forms with the typewriter.
What real-world use cases do you have in mind where you do not need it always active?

All right, you've convinced me! Never mind on this.

Where would you show it? In the action tooltip? e.g "Approved - Stamps are an experimental feature"

Maybe in a KMessageWidget that goes across the top of the view, like other notes that are presented like this.

---

The things from here and below are very interesting but also quite complicated to implement and time-consuming. I would implement them in following revisions, or we won't ever terminate this one while trying to make the toolbar perfect. Release early, release often.

OK, fair enough. :-)

Maybe in a KMessageWidget that goes across the top of the view, like other notes that are presented like this.

KMessageWidget does not have a mechanism to dismiss it forever. I think that showing it each time a user uses the stamp annotation is going to become annoying pretty soon. Maybe a message box that can be dismissed forever?

In D15580#552773, @ngraham wrote:

In D15580#552455, @simgunz wrote:

When an annotation is selected its annotation toolbar button is checked and Browse Mode is checked, exactly as in the current Okular version.
When you select a quick annotation tool what happens is that the corresponding annotation action gets checked. In a sense a quick annotation is just an alias for an effective annotation with a specified set of settings (color, etc.). For this reason the quick annotation cannot have a checked state.
This however has a quirkiness in the case the annotation toolbar is hidden. In that case we are in Browse Mode and an annotation tool is active but we do not see the corresponding checked action in the hidden toolbar. This can be "solved" by showing the annotation toolbar when a quick annotation is selected, but I think it is going to become frustrating very soon.

I propose the following solution:

• Clicking a quick annotation shows the annotation toolbar (in addition to what already happen as checking the corresponding action)

This has the following benefits:

• DIscoverability. Once a user selects an annotation from the quick annotation list, he discovers the existence of the annotation toolbar
• We do not need an extra action Show Annotation Toolbar which would be the third copy of Tools → Annotations and Settings → Toolbars Shown → Annotation Toolbar and desynced from them. Moreover when the toolbar is visible clicking on Show Annotation Toolbar does nothing.
• Making the quick annotations checkable is a duplication of functions from me. Copies of the same annotation can be checkable in two different toolbars. Leaving as it is now the quick annotations are just a proxy to the annotation toolbar, not a tool on its own.

Bottom line: to use the annotations you have to have the annotation toolbar open, which makes sense and it is in agreement with the current behavior of Okular.

See the current implementation.

All right, that works!

I think there's only one remaining thing I noticed, then I'm ready to give it a UI stamp of approval: the toolbar should have a close button on the far-right side (you can use an expanding spacer to position it there) so people don't have to go up to the menubar to close the toolbar once they're done using it.

In D15580#553421, @ngraham wrote:

All right, that works!

I think there's only one remaining thing I noticed, then I'm ready to give it a UI stamp of approval: the toolbar should have a close button on the far-right side (you can use an expanding spacer to position it there) so people don't have to go up to the menubar to close the toolbar once they're done using it.

I have never seen a close button on a standard toolbar. Are you sure it is a correct design pattern?

There are cases where the close button might be misleading given the configurability of the toolbars.

Annotation toolbar in the same row as the main toolbar. There is not distinction between the two, so it is not clear what the close button applies to.

Having the button right aligned may put it in a weird position when the window is maximized.

Regarding the HIG guidelines. We are currently violating the following two rules

1. Don’t change the button style from the default, which is text beside icons.
2. Don’t hide toolbars by default. If a toolbar can be hidden, users should easily be able to make the toolbar viewable again.

Is this fine?

My opinion:

1. Don't care much. "Icon-only" is fine for me.
2. I think it makes sense to hide it by default. It is consistent with the current Okular behavior, so users that are used to the current toolbar will find no difference.

Now the default is "icon-only". If a user changes to "text alongside icons" the toolbar will overflow even on a large monitor. Should I configure the toolbar so that if the user changes to "text alongside icons" the toolbar appears like this?
(the user can then expand the remaining icon-only buttons one by one by right clicking on them if he needs to visualize the text)

In D15580#554079, @simgunz wrote:

Should I configure the toolbar so that if the user changes to "text alongside icons" the toolbar appears like this?
(the user can then expand the remaining icon-only buttons one by one by right clicking on them if he needs to visualize the text)

I think that will be easier to tell as soon as a user decides to show the text, and complains about it.

I looked at your code, and I it looks fine, as far as I am common with the Okular code. But I didn’t compile and test yet.

I am wondering about my code. You merged both my pathes (D21755 and D21971) into your development branch, right? Maybe you should rebase on D21971, so Phabricator (or gitlab) doesn’t amalganate unrelated changes.

Besides that: How (well) does ToggleActionMenu work for you? I see that you don’t use suggestDefaultAction(). But if I got it right, Okular does not remember annotation tool selection across sessions, so it’s useless.

I think there's only one remaining thing I noticed, then I'm ready to give it a UI stamp of approval: the toolbar should have a close button on the far-right side (you can use an expanding spacer to position it there) so people don't have to go up to the menubar to close the toolbar once they're done using it.

I have never seen a close button on a standard toolbar. Are you sure it is a correct design pattern?

One more thing regarding this. kpartgui.dtd does not have a "spacer" item. So that and the close action should be inserted programmatically with some non-so-nice code to retrieve the annotation toolbar (see https://phabricator.kde.org/D15580#544534).

@davidhurka I am going to reply to your comments Sunday

In D15580#554079, @simgunz wrote:

I have never seen a close button on a standard toolbar. Are you sure it is a correct design pattern?

This isn't a standard toolbar; it's a toolbar that can be shown and hidden. Once it's shown, it's not obvious how to hide it--especially now that it shows itself after using a quick annotation tool. That'll make it appear, but it won't be obvious how to make it go away again if it doesn't have an integrated close button.

Even though the HIG recommends against it, I think it's fine to change the button style here since we're quite space-constrained.

Expanding spacers were added in 4357ef235ecb8b8b71ca0867d6cfc02acf292fae.

In D15580#554104, @davidhurka wrote:

In D15580#554079, @simgunz wrote:

Should I configure the toolbar so that if the user changes to "text alongside icons" the toolbar appears like this?
(the user can then expand the remaining icon-only buttons one by one by right clicking on them if he needs to visualize the text)

I think that will be easier to tell as soon as a user decides to show the text, and complains about it.

Seems a good strategy.

---

I looked at your code, and I it looks fine, as far as I am common with the Okular code. But I didn’t compile and test yet.

I am wondering about my code. You merged both my pathes (D21755 and D21971) into your development branch, right? Maybe you should rebase on D21971, so Phabricator (or gitlab) doesn’t amalganate unrelated changes.

I will dedicate sometime to those reviews this week and see if you can finish them and merge them. Then I'll rebase.

Besides that: How (well) does ToggleActionMenu work for you? I see that you don’t use suggestDefaultAction(). But if I got it right, Okular does not remember annotation tool selection across sessions, so it’s useless.

I'll report on this soon, I still need to figure out different combinations of the possible configurations of ToggleActionMenu.

This isn't a standard toolbar; it's a toolbar that can be shown and hidden. Once it's shown, it's not obvious how to hide it--especially now that it shows itself after using a quick annotation tool. That'll make it appear, but it won't be obvious how to make it go away again if it doesn't have an integrated close button.

Ok

Even though the HIG recommends against it, I think it's fine to change the button style here since we're quite space-constrained.

Ok

Expanding spacers were added in 4357ef235ecb8b8b71ca0867d6cfc02acf292fae.

Great, that makes things much easier to implement.

Is the icon fine for the toolbar hide/close button? Do you have a better suggestion?

Regarding unchecking the mouse mode actions when an annotation is selected. As for now I think this is extremely complex to implement. I suggest leaving it as is for now, and once Qt 5.14 is out we can use the new option that allows unchecking all actions in a QActionGroup. In this way we can keep the mouse mode action group and the annotations action group separated and still be able to uncheck all the actions in one or the other group.

In D15580#554598, @simgunz wrote:

Is the icon fine for the toolbar hide/close button? Do you have a better suggestion?

*Hide toolbar button*

Semantically, the icon you used (something like tab-close) would be correct, but I think it looks too much like a destructive action, with it being red in a circle. Considering this only closes the toolbar, which you can open again at any time, I think this icon draws too much attention to itself.
Something like paint-none or a monochrome version of tab-close or paint-none, or even a icon like hide_table_row (we would have to make some symlinks for more semantically correct icon names) might be better suited for this.

Semantically, the icon you used (something like tab-close) would be correct, but I think it looks too much like a destructive action, with it being red in a circle. Considering this only closes the toolbar, which you can open again at any time, I think this icon draws too much attention to itself.
Something like paint-none or a monochrome version of tab-close or paint-none, or even a icon like hide_table_row (we would have to make some symlinks for more semantically correct icon names) might be better suited for this.

Initially I was looking for a grey X like the X on the top right of a window but I could not find it. I agree that tab-close draws too much attention to itself. I have no preferences, so I leave the decision on what icon to use to you.

window-close-symbolic is black instead of red FWIW. Maybe we should just use that for now

@trickyricky26 and VDG, I do agree that our red close buttons draw too much attention to themselves for non-destructive close actions. We might want to consider adding a version that's gray.

Can the patch be rebased against the current git/master for testing and preliminary documentation rewrite?

Thanks in advance for your work.

Doesn't need this a docbook update?

I agree, please don't commit such a big change without updating the documentation at the same time.

In D15580#554972, @aacid wrote:

Doesn't need this a docbook update?

I think that @yurchor is taking care of the documentation.

We also need to complete the other reviews on which this one depends, so I am not going to commit until we finish these two things.

I've just tested this on manjaro-kde

Operating System: Manjaro Linux
KDE Plasma Version: 5.17.1
KDE Frameworks Version: 5.63.0
Qt Version: 5.13.1

git clone ...
git apply D15580.diff
mkdir build && cd build
cmake ..
make
sudo make install

and removed all config files.
I've found five issues:

• stamps won't get printed (both to pdf and paper)
• when setting the opacity to something below 100% through the toolbar button Opacity it gets set to 0%
• when creating lines ( arrow, straight line, polygon) painting artifacts appear. They disappear when confirming with left click but stay when aborting with right click.
• very short and thick arrows look weirder than expected (same for short lines with opened arrow ends)

• Zooming out while drawing a line makes the preview disappear but in exchange we get more artifacts.

In D15580#557314, @andreashurka wrote:

• stamps won't get printed (both to pdf and paper)

This is a pre-existing issue: https://bugs.kde.org/show_bug.cgi?id=383651

Definitely needs to be fixed eventually of course. :) From what I understand it's rather challenging.

Thanks for testing this.

In D15580#557314, @andreashurka wrote:

• when setting the opacity to something below 100% through the toolbar button Opacity it gets set to 0%

I cannot reproduce this. Does this happen for all the annotations? Can you set the opacity of a tool to 90%, close Okular and then post the content of ./config/okularpartrc? Have you tried to reproduce this in the current version of Okular?

• when creating lines ( arrow, straight line, polygon) painting artifacts appear. They disappear when confirming with left click but stay when aborting with right click.
• very short and thick arrows look weirder than expected (same for short lines with opened arrow ends)
• Zooming out while drawing a line makes the preview disappear but in exchange we get more artifacts.

These three issues are unrelated to this review. I can reproduce them in the current okular version. You can open a bug report. Regarding the short arrows, it seems to me an artificial use case, so I do not regard it as a problem.

In D15580#557433, @simgunz wrote:

I cannot reproduce this. Does this happen for all the annotations? Can you set the opacity of a tool to 90%, close Okular and then post the content of ./config/okularpartrc? Have you tried to reproduce this in the current version of Okular?

Haven't tried it in the current version, gonna do that tomorrow.
The other issues are kind of unlikely to happen in real world use cases so I won't hurry do test them in normal version and report them. But I'll probably do it eventually.

In D15580#557463, @andreashurka wrote:

In D15580#557433, @simgunz wrote:

I cannot reproduce this. Does this happen for all the annotations? Can you set the opacity of a tool to 90%, close Okular and then post the content of ./config/okularpartrc? Have you tried to reproduce this in the current version of Okular?

Haven't tried it in the current version, gonna do that tomorrow.

Correct me if I'm wrong, but the current version (repos) doesn't have the annotation toolbar so I cannot use it to tweak settings.
Or am I missing something?
Changing it through the settings menu works just fine in both, with and without annotation toolbar. It is just very slow...

I managed to test your patch now.

Nice to see the ToggleActionMenu working, this is what I had in mind once. Now I see that it can behave somehow weird: When e. g. Ellipsis is selected, and you select again Ellipsis from the Geometric Shape button menu, it gets deselected, like if you simply clicked the toolbar button.

The reason is that you connect to the action group. Then you don’t know whether the menu entry or the toolbar button was clicked.

Do you think this is weird?

Correct me if I'm wrong, but the current version (repos) doesn't have the annotation toolbar so I cannot use it to tweak settings.
Or am I missing something?

Right. You have to go through the settings to change anything in the current version.

Changing it through the settings menu works just fine in both, with and without annotation toolbar. It is just very slow...

Ok. So it is strange that the opacity does not work when you change it through the annotation toolbar. You should check/post your ~/.config/okularpartrc because there you can see the actual value of the opacity. Once you change the opacity and close Okular the value gets written in the file. Looking at that file may help in figuring out why the opacity is not set correctly for you.

In D15580#557968, @davidhurka wrote:

I managed to test your patch now.

Nice to see the ToggleActionMenu working, this is what I had in mind once. Now I see that it can behave somehow weird: When e. g. Ellipsis is selected, and you select again Ellipsis from the Geometric Shape button menu, it gets deselected, like if you simply clicked the toolbar button.

The reason is that you connect to the action group. Then you don’t know whether the menu entry or the toolbar button was clicked.

Do you think this is weird?

Given that QActionGroup (Qt < 5.14 ) does not support to uncheck all actions I had to implement that workaround. Maybe it is a bit weird but as for now that is the intended behavior and I think it is now a big issue (likely the user will click the toolbutton instead of opening the menu and clicking on the currently selected action). I think it is better to wait Qt 5.14 and then rethink this given that we can use the new ExclusiveOptional mode of QActionGroup.

In D15580#558239, @simgunz wrote:

Ok. So it is strange that the opacity does not work when you change it through the annotation toolbar. You should check/post your ~/.config/okularpartrc because there you can see the actual value of the opacity. Once you change the opacity and close Okular the value gets written in the file. Looking at that file may help in figuring out why the opacity is not set correctly for you.

I did that for a few minutes now and figured out that the same happens with line width (arrow) as well, as long as you choose x.5 . Looks like the toolbar menus are saving double/float/long (whatever) in a different format from how the menu does:
Toolbar ="0\\,4"
Menu ="0.4"
The variant with the dot is working while double backslash comma is not (at least over here)
I hope this is helping.

I did that for a few minutes now and figured out that the same happens with line width (arrow) as well, as long as you choose x.5 . Looks like the toolbar menus are saving double/float/long (whatever) in a different format from how the menu does:
Toolbar ="0\\,4"
Menu ="0.4"
The variant with the dot is working while double backslash comma is not (at least over here)
I hope this is helping.

What is your locale for numbers in System Settings → Locale → Detailed Settings → Numbers? May be related to that.

Might it be a hint that there are too many toolbars? I think only the part should have the annotation toolbar, so it is clear which is affected by the hide action.

Not defining the annotation toolbar also in shell.rc makes the implementation of the action to toggle the toolbar more difficult (if not impossible) given the tricky way in which the toolbars are created.
Currently I create the action showAnnotationToolBar when the part is created, but at this point the UI of the part does not exist yet, so the annotation toolbar must already exist (and it is the one in shell.rc). Creating the action later in the code creates other problems, so the current implementation is sanest I could think of.

Now the annotation toolbar is toggled in the correct window.

I should have fixed all the bugs that were found by the testers.

Still looks great to me in testing.

Looks like the dependent patch has landed, so we can more forward with this.

Still LGTM.

aacid requested to write some autotest for the ToggleActionMenu before merging this. I'll merge master in this review and work on the autotests soon.

Thanks!

This is huge, i would really welcome if it could be moved over to invent.kde.org since i'm 92% sure some of the clazy/clang-tidy checks will fail.

In D15580#605611, @simgunz wrote:

aacid requested to write some autotest for the ToggleActionMenu before merging this. I'll merge master in this review and work on the autotests soon.

It’s okay to me that you write the autotests. If you wish, I could do that instead.

I am new to autotests, so I will have to learn how to do that. I am also unsure about what should be tested. The ToggleActionMenu itself, or its behaviour in Okular?

In D15580#605913, @davidhurka wrote:

In D15580#605611, @simgunz wrote:

aacid requested to write some autotest for the ToggleActionMenu before merging this. I'll merge master in this review and work on the autotests soon.

It’s okay to me that you write the autotests. If you wish, I could do that instead.

I am new to autotests, so I will have to learn how to do that. I am also unsure about what should be tested. The ToggleActionMenu itself, or its behaviour in Okular?

I'm more than happy if you do that :-)

I am also not an expert on UI autotests, my suggestion is to have a look at the other Okular autotests and take inspiration from there. Probably it is good to test the component on its own (all the possible combination of parameters) and also the behavior in Okular.

I'll try to move this to invent soon.

In D15580#605913, @davidhurka wrote:

In D15580#605611, @simgunz wrote:

aacid requested to write some autotest for the ToggleActionMenu before merging this. I'll merge master in this review and work on the autotests soon.

It’s okay to me that you write the autotests. If you wish, I could do that instead.

I am new to autotests, so I will have to learn how to do that. I am also unsure about what should be tested. The ToggleActionMenu itself, or its behaviour in Okular?

The functionality in Okular, so that if in the future we replace ToggleActionMenu with EvenBetterToggleActionMenu we can spot regressions

It would be nice if someone can test compile and run Kile with the new toolbar.

Kile cannot be stared (immediately crashes) here. I have Kile and Okular from git/master.

However, Kile works fine with Okular from release/19.12 branch.

Thanks in advance for your testing.

In D15580#606046, @yurchor wrote:

It would be nice if someone can test compile and run Kile with the new toolbar.

Kile cannot be stared (immediately crashes) here. I have Kile and Okular from git/master.

However, Kile works fine with Okular from release/19.12 branch.

Thanks in advance for your testing.

But the new toolbar hasn't landed yet (it's this patch).

Maybe it's D21755 that creates problems for you?

Can you locally revert that and see if it fixes the issue?

@simgunz now that we have https://invent.kde.org/kde/okular/merge_requests/105 can you discard this one?

In D15580#606022, @aacid wrote:

In D15580#605913, @davidhurka wrote:

In D15580#605611, @simgunz wrote:

aacid requested to write some autotest for the ToggleActionMenu before merging this. I'll merge master in this review and work on the autotests soon.

It’s okay to me that you write the autotests. If you wish, I could do that instead.

I am new to autotests, so I will have to learn how to do that. I am also unsure about what should be tested. The ToggleActionMenu itself, or its behaviour in Okular?

davidhurka (David Hurka)

User

The functionality in Okular, so that if in the future we replace ToggleActionMenu with EvenBetterToggleActionMenu we can spot regressions

I’m working on the autotests. But I had no backup, and so need to start over again. :(

The functionality in Okular means for me that I trigger a mouse mode action, and then test the default action of the toolbar button. Is that what you mean?

Can I add the ToggleActionMenu test to parttest, or do I need a new test?

And someone needs to explain how to create merge requests on invent.kde.org. I tried three ways and failed, and unfortunately I can only find instructions for Phabricator.

And someone needs to explain how to create merge requests on invent.kde.org. I tried three ways and failed, and unfortunately I can only find instructions for Phabricator.

1. Fork the repo with the "Fork" button here: https://invent.kde.org/kde/okular
2. Clone it: git@invent.kde.org:dhurka/okular.git
3. Create branch: git checkout -b ann-toolbar
4. Do some changes (or just apply your commit from the Phabricator repo (git format-patch -1) with git am <patch_file>).
5. Push the branch: git push -u origin ann-toolbar
6. Click on the link that is proposed by gitlab in your working console to create MR (you should be logged in in your browser).

When you create the merge request:

• set the title to WIP: title of merge request to mark it as work in progress
• push the commits as you work so you have a backup, can see the results of the CI tests and possibly receive feedbacks

See also https://community.kde.org/Infrastructure/GitLab