Having the annotation tools on the main toolbar makes it overflow for all but the largest window sizes. How about putting them in a secondary toolbar below the main one that's hidden until the user shows it by clicking on a "Show annotation tools" item on the main toolbar and/or menubar?

The Line width dropdown menu button should make the whole button clickable to show the drop-down rather than only the space under the arrow on the right

The entries in the Line width dropdown should visually reflect the width of the line. Could these graphics be generated programmatically rather than using icons?

Squiggle and Arrow need new icons; please file a bug on Breeze | Icons and request them

The draw-polygon icon might make it seem like that tool can only draw pentagons, consider using draw_polyline or draw-polygon-star instead

The Color button should actually show the current color rather than a generic icon

The Inner color button needs something to show that it exists; right now it just looks like whitespace in the toolbar. It should show its color like the other button, and for "no color", maybe an empty transparent square?

Straight Line is mis-named, it's for drawing anything but a straight line! :) Should be something like "Freehand line"

I can't figure out what Pin Annotation actually does

It's not clear to me how to select existing annotations once an annotation tool has been activated; consider maybe adding a "select annotations" tool or mode under the Selection dropdown menu.

There is a TODO list of the working and planned feature on task T8076. Some of your suggestions are already there and I added the missing one. I have updated the description of this review to point to that TODO for better clarity.

Having the annotation tools on the main toolbar makes it overflow for all but the largest window sizes. How about putting them in a secondary toolbar below the main one that's hidden until the user shows it by clicking on a "Show annotation tools" item on the main toolbar and/or menubar?

Already in TODO

The Line width dropdown menu button should make the whole button clickable to show the drop-down rather than only the space under the arrow on the right

Added to TODO

The entries in the Line width dropdown should visually reflect the width of the line. Could these graphics be generated programmatically rather than using icons?

Added to TODO

Squiggle and Arrow need new icons; please file a bug on Breeze | Icons and request them

Already in TODO. (I was waiting before requesting icons to see if I managed to get something working :-) )

The draw-polygon icon might make it seem like that tool can only draw pentagons, consider using draw_polyline or draw-polygon-star instead

I'll set it to draw_polyline

The Color button should actually show the current color rather than a generic icon

This should already work.

The Inner color button needs something to show that it exists; right now it just looks like whitespace in the toolbar. It should show its color like the other button, and for "no color", maybe an empty transparent square?

Also the icons of color and inner color need to be done from scratch. I was thinking at a circle fillable with the current color and a border to show it exists.

Straight Line is mis-named, it's for drawing anything but a straight line! :) Should be something like "Freehand line"

Have you removed .config/okularpartrc ? Otherwise the toolbar picks up the annotation in you custom orders and the buttons are mismatched.

I can't figure out what Pin Annotation actually does

If checked the current annotation tool is kept selected after use (as double-click does in the current Okular). Needs a better name/tooltip. Added to TODO.

It's not clear to me how to select existing annotations once an annotation tool has been activated; consider maybe adding a "select annotations" tool or mode under the Selection dropdown menu.

Currently you need to click Esc to deselect the annotation, then you can select the annotations (standard Browse mode). If instead 'pin annotation' is unchecked, the annotation is deselected automatically and you can select annotation. Beside the fact that clicking on an annotation does not select it (added to TODO) and that selecting and annotation does not switch to Browse mode (added to TODO), it works as the current version of Okular. I think we do not need a dedicated Selection tool.

Having the annotation tools on the main toolbar makes it overflow for all but the largest window sizes. How about putting them in a secondary toolbar below the main one that's hidden until the user shows it by clicking on a "Show annotation tools" item on the main toolbar and/or menubar?

The Line width dropdown menu button should make the whole button clickable to show the drop-down rather than only the space under the arrow on the right

The entries in the Line width dropdown should visually reflect the width of the line. Could these graphics be generated programmatically rather than using icons?

Squiggle and Arrow need new icons; please file a bug on Breeze | Icons and request them

The draw-polygon icon might make it seem like that tool can only draw pentagons, consider using draw_polyline or draw-polygon-star instead

The Color button should actually show the current color rather than a generic icon

The Inner color button needs something to show that it exists; right now it just looks like whitespace in the toolbar. It should show its color like the other button, and for "no color", maybe an empty transparent square?

Straight Line is mis-named, it's for drawing anything but a straight line! :) Should be something like "Freehand line"

I can't figure out what Pin Annotation actually does

It's not clear to me how to select existing annotations once an annotation tool has been activated; consider maybe adding a "select annotations" tool or mode under the Selection dropdown menu.

The Color button should actually show the current color rather than a generic icon

This should already work.

My mistake, it totally does work!

The Inner color button needs something to show that it exists; right now it just looks like whitespace in the toolbar. It should show its color like the other button, and for "no color", maybe an empty transparent square?

Also the icons of color and inner color need to be done from scratch. I was thinking at a circle fillable with the current color and a border to show it exists.

Yep, that sounds good. Also I found out why it looked invisible: its icon disappears when using the Line tool, or any other tool that doesn't have an inner color, and does not restore its icon when using a tool that does have an inner color. It should probably become conditionally and temporarily disabled instead.

Straight Line is mis-named, it's for drawing anything but a straight line! :) Should be something like "Freehand line"

Have you removed .config/okularpartrc ? Otherwise the toolbar picks up the annotation in you custom orders and the buttons are mismatched.

Yep, I did. Did it again for good measure. It still says, "Straight Line":

I can't figure out what Pin Annotation actually does

If checked the current annotation tool is kept selected after use (as double-click does in the current Okular). Needs a better name/tooltip. Added to TODO.

Can't we just keep the old double-click behavior? I think that's good. Various other similar tools use a double-click to mean "activate this tool and then keep it active after you've used it once" so it's not a totally alien UI. Then we could keep the pin icon as an additional visual status indicator of whether the current tool is "sticky" or not.

It's not clear to me how to select existing annotations once an annotation tool has been activated; consider maybe adding a "select annotations" tool or mode under the Selection dropdown menu.

Currently you need to click Esc to deselect the annotation, then you can select the annotations (standard Browse mode). If instead 'pin annotation' is unchecked, the annotation is deselected automatically and you can select annotation. Beside the fact that clicking on an annotation does not select it (added to TODO) and that selecting and annotation does not switch to Browse mode (added to TODO), it works as the current version of Okular. I think we do not need a dedicated Selection tool.

Got it. My first impulse to deselect the currently-active annotation tool was to to click on it again. Currently this does nothing. Maybe it should de-select it.

If checked the current annotation tool is kept selected after use (as double-click does in the current Okular). Needs a better name/tooltip. Added to TODO.

Can't we just keep the old double-click behavior? I think that's good. Various other similar tools use a double-click to mean "activate this tool and then keep it active after you've used it once" so it's not a totally alien UI. Then we could keep the pin icon as an additional visual status indicator of whether the current tool is "sticky" or not.

Using double-click is probably used to some [how many?] Okular users, but others asked for something like a sticky button. An option to make selected actions sticky by default, without sticky button, would be perfect for me.
From other applications I know left-click: use the tool, right-click: stop using the tool. If there is no fallback tool (see below), the tool will be remembered for the next left click.

Actually, I don’t know any application where a tool automatically deselects. Do you have an example?

It's not clear to me how to select existing annotations once an annotation tool has been activated; consider maybe adding a "select annotations" tool or mode under the Selection dropdown menu.

Currently you need to click Esc to deselect the annotation, then you can select the annotations (standard Browse mode). If instead 'pin annotation' is unchecked, the annotation is deselected automatically and you can select annotation. Beside the fact that clicking on an annotation does not select it (added to TODO) and that selecting and annotation does not switch to Browse mode (added to TODO), it works as the current version of Okular. I think we do not need a dedicated Selection tool.

Got it. My first impulse to deselect the currently-active annotation tool was to to click on it again. Currently this does nothing. Maybe it should de-select it.

Clicking a selected tool to deselect sounds ok to me, although they are probably an exclusive action group. That would allow to use exactly one shortcut to enable and disable a single annotation tool.

Does right-click still work to deselect a tool? That is how I know it from many other applications (not all).

Using double-click is probably used to some [how many?] Okular users, but others asked for something like a sticky button. An option to make selected actions sticky by default, without sticky button, would be perfect for me.
From other applications I know left-click: use the tool, right-click: stop using the tool. If there is no fallback tool (see below), the tool will be remembered for the next left click.

Actually, I don’t know any application where a tool automatically deselects. Do you have an example?

It's very common in the Mac world, which is what I'm most familiar with outside of the Linux world.

Sticky-by-default would probably be okay as long as we can make it very clear how to un-select the tool. Probably implementing multiple methods would be good (hit esc key, left-click again on the tool, right-click anywhere, etc).

set sticky by default, we will make some users that use the non-sticky mode unhappy

set non-sticky by default and use double click to stick, we go back to the current situation (bug 358057)

I we keep the sticky button, the double click to stick becomes pretty irrelevant.

To sum up, I would:

keep the sticky button to make the feature clearly visible to the user (see bug 358057). As it is now this feature is hard to discover, took me years to figure it exists.

make the tool sticky by default on first installation, then save the state of the sticky button (if a user prefer non-sticky annotation, after unchecking the sticky button he will have it unchecked when he relaunch Okular)

Sticky-by-default would probably be okay as long as we can make it very clear how to un-select the tool. Probably implementing multiple methods would be good (hit esc key, left-click again on the tool, right-click anywhere, etc).

keep the sticky button to make the feature clearly visible to the user (see bug 358057). As it is now this feature is hard to discover, took me years to figure it exists.

make the tool sticky by default on first installation, then save the state of the sticky button (if a user prefer non-sticky annotation, after unchecking the sticky button he will have it unchecked when he relaunch Okular)

Or: Why is this still PageViewToolBar? It is not anymore in the PageView?

I'll move it, added to TODO

Actually the code of the other toolbar (Browse, Zoom, Selection) is managed in the file pageview.cpp. In which file would you move the code of PageViewToolBar?

Because it’s now a normal toolbar (defined in part.rc), the actions just need to be constructed and connected at some time. And we need some code to show/hide that toolbar when annotations are activated/deactivated, just like the PageViewToolBar is shown/hidden now.

So why do we need this class (PageViewToolBar) at all? It is just convenient, because it bundles all the annotation actions (ac->addAction(annArrow) and so on) and their slots and some more. But it is not a toolbar itself. Maybe AnnotationToolBarController is a more accurate name. And AnnotationToolBarController would be at the same place as PageViewToolBar was. Or do I understand something wrong?

So I think the new class PageViewToolBar is fine (I have looked at the code now), just the name is misleading now.

Maybe the 4 left buttons should indicate that they require further action (drawing). Currently they look like buttons in a word processor, where you have to select the text first. *1) Using the existing dynamic annotation icons might look better, as soon as someone made them more low-resolution friently.

The first two separators look a bit like only the first four buttons are annotation tools, and the next 3+2 buttons are just options. I. e. there is not a mutually exclusion between the 4+3+2 buttons, but only between the 4, 3, and 2 individually. Maybe it’s better to group them not with separators, but with ToolActions/ToggleActionMenus. Could be tested later by customizing the toolbar via Configure Toolbars.

*1) Maybe an interesting idea: Select some text in Text Selection mode, then just click the annotation button.

Maybe the 4 left buttons should indicate that they require further action (drawing). Currently they look like buttons in a word processor, where you have to select the text first. *1) Using the existing dynamic annotation icons might look better, as soon as someone made them more low-resolution friently.

I am more in favor of using standard breeze icons for the annotation tools. I opened a bug to ask for the icons (https://bugs.kde.org/show_bug.cgi?id=408283), so we can ask for a specific design that make them understandable.

The first two separators look a bit like only the first four buttons are annotation tools, and the next 3+2 buttons are just options. I. e. there is not a mutually exclusion between the 4+3+2 buttons, but only between the 4, 3, and 2 individually. Maybe it’s better to group them not with separators, but with ToolActions/ToggleActionMenus. Could be tested later by customizing the toolbar via Configure Toolbars.

I agree in removing the separators.

*1) Maybe an interesting idea: Select some text in Text Selection mode, then just click the annotation button.

I have managed to re-enable the action Tools > Review to show/hide the annotation toolbar.

This action needs to be defined in shell.cpp, because to use KToggleToolBarAnnotation we need to pass a reference of the KToolbar to its constructor, and the toolbar does not yet exists when we setup the actions in pageview.cpp (line 633).
This action needs to be created for each new part (e.g. when multiple tabs are created). This seems to work properly.

Another weird thing I have noticed is that KToggleToolBarAnnotation is only able to toggle the annotation toolbar if this is also defined in shell.rc, while it is unable to toggle it when defined only in part.rc (even tough I am able to retrieve the toolbar instance correctly from shell.cpp, e.g. using toolBars(), also when the toolbar is only defined in part.rc). For this reason I had to define an empty annotation toolbar in shell.rc with the same name as the one in part.rc so that the two are merged. Is this the expected behavior of KToggleToolBarAnnotation? ( I include @cfeck in this discussion given that we were discussing KToggleToolBarAnnotation functionalities)

I messed up something... I see many files I have not modified that are marked as modified here. Probably because I changed the origin remote and my master was not in sync with upstream. I'll fix this with the next arc diff, now it does not let me arc diff again because it says that the diff is empty.

I messed up something... I see many files I have not modified that are marked as modified here. Probably because I changed the origin remote and my master was not in sync with upstream. I'll fix this with the next arc diff, now it does not let me arc diff again because it says that the diff is empty.

Did you try git pull in the master branch, then git merge master and arc diff --base git:master or so on your branch? I think you had the wrong base.

With the latest push all the features of the toolbar are implemented. There are some horrible things in the code, but I will clean and refactor it.

Now it is time for you to test the toolbar and provide me feedback on the UI / UX please. (No need to comment on the code for now).

Few notes:

See the section Discuss in T8076 for a list of things where I need input

See the section Test Plan in T8076 for a partial list of things to test

I am experimentally using D21971 for the geometrical annotation menu and the stamp menu

As usual remove ~/.config/okularpartrc (and possibly also ~/.config/okularrc if you want to test the defaults of the toolbar). ~/.config/okularpartrc should be auto-updated by kconf_update now, once you install okular system-wide.

For my own UI review, I must continue to push for putting the annotation tools on a second toolbar that appears below the main one when needed. Adding them to the main toolbar is only feasible when the window is really really wide; otherwise most or all of the buttons get elided. Once they're in their own toolbar, there should be enough horizontal space to give most or all of the buttons text on the side, which should resolve the confusion regarding what they are and which ones are actions vs settings for other tools.

For my own UI review, I must continue to push for putting the annotation tools on a second toolbar that appears below the main one when needed.

The tools are already on a toolbar of their own (called annotationToolbar). If you unlock the toolbars you should be able to move it around.

By issuing rm ~/.config/okularrc && rm ~/.config/okularpartrc before launching Okular you should obtain the defaut behavior that currently is to put the annotation toolbar below the main toolbar (see shell.rc:26) and display only the icons (screenshot 1). If we change the default to text alongside icons the toolbar overflow even on a widescreen (mine is 1920x1080) (screenshot 2 and 3). In this second case I think we have the following two solutions for me:

further group some tools in a sub-menu (as for the geometrical tools). A possibility is to group underline, squiggle, strike-through (screenshot 4). Still the toolbar is pretty full on a widescreen.

Put the configuration actions on a toolbar of their own a set icons only as a default for this toolbar (screenshot 5)

(Keep everything in the same annotationToolbar and set the annotation tools to text alongside icon and the configuration actions to icons only one by one. It is possible to do this from the UI, but I do not know if it is possible to set as a default from part.rc (screenshot 5) )

Screenshot 1

Screenshot 2

Screenshot 3 (toolbar expanded)

Screenshot 4 (underline is a placeholder for the menu that will contain underline, squiggle and strikethrough)

Thanks, I was indeed not seeing the latest version. After deleting the necessary config files, I see the latest version in its own toolbar below the main one. This is a lot better than the current one already, but I think we can polish it even more. I have a lot of thoughts percolating and it may take me a few days to get them all written down. But I will do so soon!

Instead of using a horizontal toolbar below the main toolbar, instead I might experiment with a vertical toolbar like the one shown in Gwenview's View mode. We could make the toolbar live inside the Reviews tab, which already shows a list of all annotations (and then we should unify the terminology vis-a-vis "reviews" vs "annotations." REASON: cramming everything into a horizontal toolbar seems impossible for a feature this rich; using a vertical toolbar provides us enough horizontal space to show labels for everything, and enough vertical space to easily hold everything. Also we re-use an existing UI element that's currently rather bare.

Show a button on the toolbar by default that holds the list of favorite annotations, and pre-populate it with the current set of default annotations. Label the button "Quick annotations". At the bottom of the list, add an entry that says something like, "Show advanced annotation settings" that will display the full vertical toolbar in the Reviews tab. REASON: This will make the whole annotations UI much more discoverable.

Remove "Favorites" button from the annotation toolbar (since it'll be on the main toolbar instead)

Rename "Add to favorites" to say "Add to quick annotations"

Given Okular's conservative Frameworks dependency policy, I need to marshall VDG resources ASAP for the icons. Do you have a full list of the icons we need?

Show a button on the toolbar by default that holds the list of favorite annotations, [...] "Quick annotations". At the bottom of the list, add an entry that says something like, "Show advanced annotation settings" that will display the full vertical toolbar in the Reviews tab.

You mean a menu button in the main toolbar, with an entry that simply opens the Reviews tab? Or can the annotation toolbar itself be shown/hidden in the Reviews tab?

Show a button on the toolbar by default that holds the list of favorite annotations, [...] "Quick annotations". At the bottom of the list, add an entry that says something like, "Show advanced annotation settings" that will display the full vertical toolbar in the Reviews tab.

You mean a menu button in the main toolbar, with an entry that simply opens the Reviews tab? Or can the annotation toolbar itself be shown/hidden in the Reviews tab?

Sort of. The proposed button in the toolbar would show a menu of pre-configured favorite annotations when clicked on, with a menu item at the bottom that opens the Reviews tab, which under my proposal also holds the full annotations toolbar.

Instead of using a horizontal toolbar below the main toolbar, instead I might experiment with a vertical toolbar like the one shown in Gwenview's View mode. We could make the toolbar live inside the Reviews tab, which already shows a list of all annotations (and then we should unify the terminology vis-a-vis "reviews" vs "annotations." REASON: cramming everything into a horizontal toolbar seems impossible for a feature this rich; using a vertical toolbar provides us enough horizontal space to show labels for everything, and enough vertical space to easily hold everything. Also we re-use an existing UI element that's currently rather bare.

How would you fit the annotation actions in the Reviews tab?

Would you create a sub-tab in it (as in Gwenview where the tabs are at the bottom)? -

Can you provide a minimal mockup of this?

Show a button on the toolbar by default that holds the list of favorite annotations, and pre-populate it with the current set of default annotations. Label the button "Quick annotations". At the bottom of the list, add an entry that says something like, "Show advanced annotation settings" that will display the full vertical toolbar in the Reviews tab. REASON: This will make the whole annotations UI much more discoverable.

I would rather not cram the "Quick annotations" by populating it with all the basic annotations. The user will end up with the same set of tools in two different places, which is not the point of "Quick annotations". I would instead populate it with 3-4 example custom annotations to illustrate the purpose of that list, putting the likely most used annotations. For example: yellow and green highlighter (to demonstrate we can have two highlighter colors), inline note, popup note, and typewriter.

Remove "Favorites" button from the annotation toolbar (since it'll be on the main toolbar instead)

Rename "Add to favorites" to say "Add to quick annotations"

Ok.

Given Okular's conservative Frameworks dependency policy, I need to marshall VDG resources ASAP for the icons. Do you have a full list of the icons we need?

I would rather not cram the "Quick annotations" by populating it with all the basic annotations. The user will end up with the same set of tools in two different places, which is not the point of "Quick annotations". I would instead populate it with 3-4 example custom annotations to illustrate the purpose of that list, putting the likely most used annotations. For example: yellow and green highlighter (to demonstrate we can have two highlighter colors), inline note, popup note, and typewriter.

Would you create a sub-tab in it (as in Gwenview where the tabs are at the bottom)? -

Can you provide a minimal mockup of this?

Having it tabbed like Gwenview was what I was envisioning, yeah. Basically copy the UX of Gwenview's sidebar, but inside Okular's Reviews tab.

I’m not sure whether I understand you. This is a screenshot of the sidebar in Gwenview “Operations”. Additionally to the sidebar tab Reviews, you want a tab “Add Annotations”, looking like this?

Or you want to add a tab bar to the bottom of the Reviews tab, containing Annotations and Add Annotations?
I would simply put the annotation toolbar on top of the Reviews tab, on top of the search box. Probably it would cover multiple lines, if that is possible with Qt.

Just to be complete: the other tabs in Gwenview are

Folders, a file system view, which might make sense in Okular too.

Information, some meta information like Okular’s dialogs Properties and Embedded Files. IMHO these dialogs can stay in the File menu, instead of adding controls to the sidebar.

Would you create a sub-tab in it (as in Gwenview where the tabs are at the bottom)? -

Can you provide a minimal mockup of this?

Having it tabbed like Gwenview was what I was envisioning, yeah. Basically copy the UX of Gwenview's sidebar, but inside Okular's Reviews tab.

I’m not sure whether I understand you. This is a screenshot of the sidebar in Gwenview “Operations”. Additionally to the sidebar tab Reviews, you want a tab “Add Annotations”, looking like this?

Or you want to add a tab bar to the bottom of the Reviews tab, containing Annotations and Add Annotations?
I would simply put the annotation toolbar on top of the Reviews tab, on top of the search box. Probably it would cover multiple lines, if that is possible with Qt.

Yeah, that might make more sense that having a tabbed view inside the review tab. But yes, I was envisioning a vertical toolbar like the above screenshot.