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

Moved to KDE invent.

Moved to KDE invent.

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?

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.

I compiled and it works. Looks good to me.

You need to merge master into this review or rebase because there is a merge conflict. I suggested you the modifications that were introduced in master (qAsConst) and few other changes.

It does not compile indeed. My fault in the review process, I missed the last commit. I have added the inline comments to fix the bug.

The class has the features needed for both D15580 and D21195 and I do not see evident bugs. For me, this revision is good enough for now, and in case we can fix it later (it used in few places so it should not be hard to adapt the code if needed).

In D21971#560810, @davidhurka wrote:

• Restrict toolbar button popup mode to DelayedPopup and MenuButtonPopup using a custom enum
  
  Honestly, I’m not sure whether this is good. We say no one wants to use InstantPopup, but as well we could say no one wants DelayedPopup. @ngraham?

In D21971#560495, @davidhurka wrote:

In D21971#559485, @simgunz wrote:

To sum it up, if I understand correctly:

• we can remove InstantPopup given that we can use KActionMenu or KSelectAction to provide that use case (so no need to raise exceptions)

Yes, but how? It can’t simply be forbidden in the constructor argument list. Remove it from the body of the constructor, so KActionMenu is set to MenuButtonPopup instead?

• Apply KDE Frameworks astyle

• Minor refactor

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.

• Merge remote-tracking branch 'origin/master' into new-annotation-toolbar_ToggleActionMenu

• Add setToolBarVisible method
• Fix annotation toolbar only hidden in first main window

To sum it up, if I understand correctly:

• you agree in removing DefaultLogic
• we can remove InstantPopup given that we can use KActionMenu or KSelectAction to provide that use case (so no need to raise exceptions)

I also would like to get rid of DefaultLogic and just use ImplicitDefaultAction, but I am not sure if this creates problem when ToggleActionMenu is placed in a menu. Should it behave as a standard KActionMenu when in a menu? I think that when in a menu there is no meaning of "default action", because when clicked, the ToggleActionMenu should just open the submenu, right?

I have tried to analyze all the possible cases regarding ToggleActionMenu placed in a toolbar.

• Use full ellipsis
• Ignore locale when saving numbers in config files

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.

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?

Thanks for testing this.

In D15580#554972, @aacid wrote:

Doesn't need this a docbook update?

• Merge remote-tracking branch 'origin/master' into new-annotation-toolbar_ToggleActionMenu

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.

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.

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

• Add action to hide the annotation toolbar
• Preserve user-defined annotation tools
• Remove unsueful initialization

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.

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.

• Warn user that stamps in PDF are an experimental feature

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.

• Show annotation toolbar when quick annotation is selected

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.

• Rename action: pin > keep active
• Add action to quick annotation menu to show annotation toolbar

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#552362, @ngraham wrote:

This is super fantastic.

I have a few more observations from use:

• When using one of the quick annotations, the Quick Annotations button should have a checked state so you can tell that one of its tools is active

• Set toolbar default to icononly

The icon is now rendered correctly in Okular and cuttlefish.

The toolbar is almost 100% ready from my side. What is left to do:

• The opacity icon is rendered incorrectly but it is going to be fixed soon by @trickyricky26.
• There are two TODO not yet fixed in T8076, but I do not know how to solve them (see my comment on this above)
• We need to finish and merge D21755 and D21971 (I am currently use the code in D21971).

• Use new draw-arrow icon
• Move bookmark and pin to the right of the toolbar
• Avoid foreach
• Fix reference arguments
• Use new edit-opacity icon
• Remove support for inline note text color (too messy for now)
• Fix code style
• Add list of predefined colors in color picker
• Remove 0% opacity
• Add color pickers action name
• Rename slot
• Add i18n context

First, thanks for the excellent work on the new icons for Okular. I really like them.

I think this is ok. Two wishes:

• The toolbar button shows a checked state, when the fill color is se

As it is now that button already displays a rectangle with the currect color (or nothing if no fill color). That should be enough to show the user the current state. Having the button as it is now (InstantPopup) seems more usable to me.
Isn't this enough? (Maybe I never posted a screenshot with this displayed)

In D15580#546492, @ngraham wrote:

Typically the color chooser includes a "transparent" item.

Edit: changed ToggleActionMenu to KSelectAction

• Fix stamp not deselected with Pin unchecked
• Move weird stamps to the bottom of the list
• Fix cannot change text color of typewriter and inline note
• Make stamp tool Id a static const
• Make default stamps a static const
• Document properties of engines and annotations
• Revert allow setting inline note text color
• Do not allow setting the text color for inline note

• Use new highlighter icon

I need some input on two things left to be done:

• Make ktoggleannotationaction to be owned by shell.cpp
• Add hack to allow respecting the default toolbars layout

I have refactored and cleaned the code. There are two tricky TODO missing. It seems that also the icons are coming. I think we are almost there.

• Rename favorite > quick in settings
• Refactor
• Clean pageviewutils
• Increase part.rc version
• Improve tools defaults
• Clean code
• Remove unused method
• Rename methods
• Rename, move and comment methods
• Fix quickTools
• Move methods
• Move / rename methods
• Make methods private
• Update description
• Move methods to original position
• Improve checks
• Minor style change
• Rename methods favorite > quick
• Reorder methods
• Move methods to private class
• Correctly disable all actions when annotations not allowed
• Rename variables
• Indent
• Clean parseTool
• Code style fixes
• Clean constructor
• Remove unuseful signal
• Simplify code to set picker color
• Clean tooltips method
• Clean populate quick annotation
• Clean icons generator method
• Reorder methods
• Rename method
• Simplify deselecting tool code
• Put similar methods close together
• Add method description
• Remove unuseful connection
• Remove comment
• Add comments regarding action group workaround
• Refactor stamp tool code
• Comments
• Remove unused code
• Drop unuseful slots
• Simplify updateConfigActions method
• Use new edit-line-width icon
• Refactor method to set color picker icon
• Fix stamp tool not properly selected
• Refactor parseTool
• Refactor updateConfigActions: remove none, no annotation tooltips
• Use isValid instead of setting Qt::transparent
• Spaces
• Move instruction at the end
• Refactor insertion of custom action (width and opacity)
• Move code to select stamp action in a method
• Style

• Merge remote-tracking branch 'origin/master' into new-annotation-toolbar_ToggleActionMenu
• Fix annotation toolbar name
• Merge remote-tracking branch 'origin/master' into new-annotation-toolbar_ToggleActionMenu
• Rename Reviews to Annotations in UI
• Move favorite annotations to main toolbar
• Add format-text-underline-squiggle icon
• Make annotation config actions icon-only by default
• Change typewriter icon to tool-text
• Rename favorite to quick annotation in all places
• Add colored rectangle to color icons
• Change color icon based on selected tool (text/stroke)
• Set correct icon for advanced settings
• Fix inline-note inner color management
• Remove redundant instructions
• Default style of annotation action to system default
• Remove deprecated shortcut tag
• Annotation toolbar hidden by default
• Populate quick annotations
• Add link to "configure annotation" settings in quick annotations

Here's another idea: would we make the buttons lose their text labels when there's not enough horizontal space to show everything. This would be more user-friendly than moving them onto an overflow menu. Kirigami offers this feature natively but we might need to hack it into Okular or KXMLGui.

I have tried with new layout of the annotation toolbar:

• Annotation tools actions, add to quick annotation action, and pin action are be defaults text-alongside-icon (to make their meaning more explicit)
• Annotation config actions are be defaults icon-only (their meaning is quite obvious and common to many other applications)
• Moved "Quick Annotation" to main toolbar
• Moved "Pin" and "Add to Quick Annotations" to the left of the annotation config actions

EDIT: text layout

• Change typewriter and pin icons

In D15580#513253, @ngraham wrote:

Some thoughts:

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

In D15580#506692, @ngraham wrote:

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.

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.

• Fix detach annotation when mouse mode unchecked

• Use ToggleActionMenu
• Add stamp annotation
• Fix swap straight line and arrow
• Use Title style capitalization
• Show real stamp symbols if squared
• Better stamp icon

I did test this today and it seems to be working correctly, at least in the use cases of Okular, i.e. for select annotation tools and for the annotation toolbar. Actually I am currently using in D15580 now (will push soon), given that ToolAction was not enough for what I needed ( I'll update my code accordingly if this revision is modified).

Final version of the new UI components:

• Fix messages and i18nc

In D22064#501557, @ngraham wrote:

Exactly what I was imagining!I might also add at the beginning something like: "Warning: feature is considered experimental."

• Add geometrical annotations to action collection
• Move arrow before straight line
• Allow setting action text
• Set default shortcuts
• Set favorite tools default shortcuts
• Fix i18n argument
• Auto-update conf file
• Add semantic information to i18n
• Refactor width an opacity
• Remove space before percentage sign
• Insert custom width and opacity in action list