New annotation toolbar
AcceptedPublic

Authored by simgunz on Sep 17 2018, 3:52 PM.

Details

Summary

As discussed in the task T8076 I am proposing a new annotation toolbar to replace the current one.

The current implementation is very rough, but can be used as a first point to discuss how to proceed in the implementation.

The toolbar uses the current PageViewAnnotator engine based on XML files (with few modifications)

What is working and what is planned
See T8076

Fixes:

BUG: 386578
BUG: 374728
BUG: 352310
BUG: 330518
BUG: 341914
BUG: 157289
BUG: 358057
BUG: 412767
BUG: 413595
FIXED-IN: 1.9.0

Test Plan

Before testing this revision
Delete or Temporary move aside the following files:

  • ~/.config/okularpartrc
  • ~/.config/okularrc
  • ~/.local/share/kxmlgui5/okular/part.rc
  • ~/.local/share/kxmlgui5/okular/shell.rc

Nomenclature
Actions in the main toolbar:

  • Quick annotations

Actions in the annotation toolbar:

  • Annotation actions Highlighter, Underline, Squiggle, Strike out, Typewriter, Inline note, Popup note, Freehand line, Arrow, Straight line, Rectangle, Ellipse, Polygon, Stamp
  • Annotation config actions Line width, Color, Inner color, Opacity, Font, Annotation settings
  • Other actions Add to Quick Annotations, Pin

Tests

  • First run: annotation toolbar not visible
  • First run: selecting Tools > Annotations shows the annotation toolbar below the main toolbar
  • No annotation action selected: Quick Annotations is enabled, Add to quick annotations is disabled, Annotation config actions are disabled, Pin is enabled
  • Click an annotation action when none selected: browse mode is selected, it is possible to use the annotation tool, the Annotation config actions are enabled and their values set to the ones for the current annotation (taken from okularpartrc)
  • Click the currently selected annotation action: the action is unchecked and the tool disabled (back to browse mode)
  • Click ESC: the currently selected annotation action is unchecked
  • The current document is an image: Highlighter, Underline, Squiggle, Strike out are disabled (also in Quick annotations)
  • The current document is protected: All actions are disabled
  • The annotation systems works when multiple Okular tabs are open (the selected annotation is per-tab)
  • The state of Pin action is remembered across Okular launches
  • In Configure Okular > Annotations it is possible to only configure the quick annotations. MOdifying them here updates the Quick annotations list after clicking Apply
  • Keys 1-9,0 select the (builtin) Annotation actions
  • Keys Alt+1-9,0 select the quick annotation actions
  • If a custom Line Width or Opacity is set through the Annotation Settings dialog, its value appears as a new checked action in the Line width or Opacity menu
  • If a custom stamp is selected through the Annotation Settings dialog, its name or filename (without path) appears as a new checked action in the Stamp menu
  • Selecting a quick action selects the corresponding action and loads its config values (color, line width, ...)
  • All actions have tooltips (some change based on the fact that the icon is enabled or not)
  • Color icon is a format-text-color (if inline note or typewriter) or format-stroke-color for all other annotations
  • Setting the color and fill color works for all annotations (to be tested carefully, can be problematic for typewriter and inline note given the different internal mechanism to store the color in the settings)
  • If Pin unchecked the selected annotation is unchecked after it has be used once and we are back to Browse mode (tested separately for stamp annotation)

Diff Detail

Repository
R223 Okular
Branch
new-annotation-toolbar_ToggleActionMenu
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15283
Build 15301: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
ngraham edited the summary of this revision. (Show Details)Oct 16 2019, 10:22 PM
simgunz updated this revision to Diff 68372.Oct 20 2019, 3:31 PM
  • 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

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

New layout of the toolbar:

New color pickers:

Those color pickers are great. Can we get the tool buttons moved back to being icons-only so they'll fit in one row for an un-maximized window?

simgunz updated this revision to Diff 68493.Oct 21 2019, 10:58 PM
  • Set toolbar default to icononly

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 (otherwise the previously-active toolbar button still looks checked despite not being active)
  • Maybe add a new menu item at the bottom of the Quick Annotations menu saying "Show all" that will open the full toolbar
  • The menu item that says, "Configure annotations..." should probably say "Configure quick annotations..." since it applies to the annotations in the quick annotations list, not all annotations more generally. Or even just "Configure..."
  • I would add text to the Keep the active annotation button active after use toolbutton, since otherwise it's a bit hard to tell what it does. Maybe "Keep active" or "Keep active after use" (maybe that's too long)?
  • Maybe don't have the Keep Active button checked by default. In testing, it feels more natural to have to click on an annotation's button after each use.
  • When an existing annotation is selected, it would be really nice if the controls for choosing the color, line thickness, opacity, font details etc. became active again and allowed you to change that annotation's appearance after the fact. You can already edit it by right-clicking and going to Properties, but it would be even nicer to be able to do this in a more direct manner IMO
  • It would be nice if highlight, underline, squiggle, and strikethrough annotations were mouse-selectable while the annotations toolbar is open. That way it would be more obvious how to delete them, and you could change their properties using the above method
  • It might be nice if hitting the 9 key multiple times cycled through the items in the shape annotation menu
  • The 0 key could do the same for the stamp annotation
  • Now that we have a way to add stamp annotations using this new method, we need to show the message that stamp annotations are an experimental feature so that users know that they can't necessarily rely on it. Currently they only see this in the annotation settings window

Overall this is feeling really good and I think it's quite close to being ready for prime time.

Didn’t yet test this, but I like some of Nate’s ideas.

This is super fantastic.

I have a few more observations from use:

  • Maybe add a new menu item at the bottom of the Quick Annotations menu saying "Show all" that will open the full toolbar

Yeah, but [_] Annotation Toolbar would be more natural, I think. At least for users who figured out the meaning of “toolbar”.

  • I would add text to the Keep the active annotation button active after use toolbutton, since otherwise it's a bit hard to tell what it does. Maybe "Keep active" or "Keep active after use" (maybe that's too long)?

Maybe “Keep selected”? I prefer “Keep active”.

  • Maybe don't have the Keep Active button checked by default. In testing, it feels more natural to have to click on an annotation's button after each use.

I think on by default makes sense. When the annotation tools don’t behave as expected (i. e. stay active), it will be easy to uncheck something.

  • It might be nice if hitting the 9 key multiple times cycled through the items in the shape annotation menu
  • The 0 key could do the same for the stamp annotation

+1

Yeah, but [_] Annotation Toolbar would be more natural, I think. At least for users who figured out the meaning of “toolbar”.

That's a good idea. It would be: [ ] Show Annotation Toolbar (menu items need to start with action verbs). And then the menu item in the Tools menu would be the same. probably the same QAction would just used for both; then they would both keep track of state properly.

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

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.

(otherwise the previously-active toolbar button still looks checked despite not being active)

This should not happen. Probably you are referring to Browse Mode that doesn't get unchecked, but that is the intended behavior and match that of the current Okular version.
What happens should be:

Example 1:

  • Text Selction is checked
  • Select Quick AnnotationsGreen Highlighter

Results:

  • Browse Mode action gets checked
  • Highlighter action gets checked

Example 1:

  • Browse Mode is checked
  • Ellipse is checked
  • Select Quick AnnotationsGreen Highlighter

Results:

  • Browse Mode remains checked
  • Highlighter action gets checked

If we make the quick annotation checkable we have the following problem:

  • The Quick Annotation won't display a edit-draw icon with the text "Quick Annotation" but it will display a specific action, i.e. "Green Highlighter" which might be confusing for the user, given that Quick Annotation would be never displayed. Exactly what happens now for the Stamp annotation (in this case you see "Approved" by default).

Bottom line: unless there is a clever way to make the actions checkable but display the Quick Annotation button, I think that the current implementation is the the less worse situation. Regarding browse mode, we can also uncheck it when an annotation is selected, if there are no side effects.

  • Maybe add a new menu item at the bottom of the Quick Annotations menu saying "Show all" that will open the full toolbar That's a good idea. It would be: [ ] Show Annotation Toolbar (menu items need to start with action verbs). And then the menu item in the Tools menu would be the same. probably the same QAction would just used for both; then they would both keep track of state properly.

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

Actually this exact action already exists in SettingsToolbars ShownAnnotations Toolbar but I do not know of to access it and if it available when we set up the actions.

  • The menu item that says, "Configure annotations..." should probably say "Configure quick annotations..." since it applies to the annotations in the quick annotations list, not all annotations more generally. Or even just "Configure..."

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.

  • I would add text to the Keep the active annotation button active after use toolbutton, since otherwise it's a bit hard to tell what it does. Maybe "Keep active" or "Keep active after use" (maybe that's too long)?

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

  • Maybe don't have the Keep Active button checked by default. In testing, it feels more natural to have to click on an annotation's button after each use.

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?

  • Now that we have a way to add stamp annotations using this new method, we need to show the message that stamp annotations are an experimental feature so that users know that they can't necessarily rely on it. Currently they only see this in the annotation settings window

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


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.

  • When an existing annotation is selected, it would be really nice if the controls for choosing the color, line thickness, opacity, font details etc. became active again and allowed you to change that annotation's appearance after the fact. You can already edit it by right-clicking and going to Properties, but it would be even nicer to be able to do this in a more direct manner IMO

I agree

  • It would be nice if highlight, underline, squiggle, and strikethrough annotations were mouse-selectable while the annotations toolbar is open. That way it would be more obvious how to delete them, and you could change their properties using the above method

Well at this point even the other annotations, so you can change their properties.

  • It might be nice if hitting the 9 key multiple times cycled through the items in the shape annotation menu
  • The 0 key could do the same for the stamp annotation

Rather difficult to do. It means that everytime the shortcut is triggered we need to reset the shortcuts of the actions in that list. This should be implemented upstream in KSelectAction, so that a shortcut could be assigned to it possibly.

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?

simgunz updated this revision to Diff 68649.Thu, Oct 24, 7:27 AM
  • Rename action: pin > keep active
  • Add action to quick annotation menu to show annotation toolbar

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 ToolsAnnotations and SettingsToolbars ShownAnnotation 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.

simgunz updated this revision to Diff 68650.Thu, Oct 24, 8:12 AM
  • Show annotation toolbar when quick annotation is selected

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.

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)


Non-obvious actions expanded, obvious one compressed.

simgunz updated this revision to Diff 68768.Fri, Oct 25, 8:24 PM
  • Warn user that stamps in PDF are an experimental feature

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.

conf/editannottooldialog.cpp
40

Why initialize m_builtinTool to false in the initializer list? There aren’t superclass constructors or so which need it, or do I miss something?

okular.upd
27

Is there a way to keep the user-defined annotations?

shell/shell.cpp
370

I remember you had struggle to find the annotation toolbar when you tried to show and hide it. Is this code related to the problem and did you solve the problem?

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

davidhurka added inline comments.Fri, Oct 25, 10:07 PM
shell/shell.cpp
370

@simgunz just wrote:

https://phabricator.kde.org/D15580#544534

That’s what I thought about. :)

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.

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.

simgunz updated this revision to Diff 68813.Sun, Oct 27, 8:27 AM
simgunz marked 4 inline comments as done.
  • Add action to hide the annotation toolbar
  • Preserve user-defined annotation tools
  • Remove unsueful initialization
simgunz added inline comments.Sun, Oct 27, 8:28 AM
shell/shell.cpp
370

Initially I put that action in part.rc, while now I moved it to shell.rc. This makes more sense given that the toolbar belongs to the main window somehow owns the toolbars and I create this action in Shell::setupActions() that makes more sense. The problem is that setupActions is called before setupGUI and createGUI so the toolbar does not exist yet. To solve this I have to call toolBar( "mainToolBar" ); to force the creation of the toolbar and so be able to "bind" the KToggleToolBarAction to an existing toolbar.

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


*Hide toolbar button*

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.

simgunz retitled this revision from [WIP] New annotation toolbar to New annotation toolbar.Sun, Oct 27, 8:51 AM

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.

simgunz updated this revision to Diff 68843.Sun, Oct 27, 5:44 PM
  • Merge remote-tracking branch 'origin/master' into new-annotation-toolbar_ToggleActionMenu
ngraham accepted this revision.Sun, Oct 27, 9:20 PM

I'm happy with the UI now. :) Subsequent improvements and polish can be delivered in follow-up patches.

This revision is now accepted and ready to land.Sun, Oct 27, 9:20 PM

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.

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

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

andreashurka added a comment.EditedThu, Oct 31, 9:40 PM

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.

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

yurchor added inline comments.Sat, Nov 2, 5:52 PM
ui/annotationactionhandler.cpp
388

May it be a complete ellipsis, for consistency (.. -> ...)?

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?

shell/shell.cpp
370

This (maybe) causes one problem now. The default is to open new documents in new windows.

Open a new document with the Open action of an existing shell, so a new Okular window appears. Open the annotation toolbar in both windows. Click Hide in the second window. -> Only the annotation toolbar in the first window is closed.

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.

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.

simgunz added inline comments.Sun, Nov 3, 8:22 PM
shell/shell.cpp
370

Good that you spotted this bug, though it is unrelated to this change (I think). If you use ToolsAnnotations or the shortcut F6 everything is working correctly. The problem is only related to the Hide buttton in the toolbar. I indeed expected some weird behavior somewhere.

ui/annotationactionhandler.cpp
493

This implementation is buggy. How to detect in which main window are we? (Even if we move aToggleAnnotator to part.rc I think we still need to figure out in which part are we, so should not give any benefit)

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 SettingsLocaleDetailed SettingsNumbers? May be related to that.

simgunz updated this revision to Diff 69254.Mon, Nov 4, 10:05 AM
  • Use full ellipsis
  • Ignore locale when saving numbers in config files
davidhurka added inline comments.Mon, Nov 4, 8:09 PM
shell/shell.cpp
370

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.

ngraham edited the summary of this revision. (Show Details)Tue, Nov 5, 3:14 PM
simgunz updated this revision to Diff 69379.Thu, Nov 7, 10:22 AM
  • Add setToolBarVisible method
  • Fix annotation toolbar only hidden in first main window
Restricted Application added a project: Documentation. · View Herald TranscriptThu, Nov 7, 10:22 AM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript
simgunz updated this revision to Diff 69380.Thu, Nov 7, 10:24 AM
  • Merge remote-tracking branch 'origin/master' into new-annotation-toolbar_ToggleActionMenu
simgunz marked 2 inline comments as done.Thu, Nov 7, 6:05 PM

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.

simgunz updated this revision to Diff 69405.Thu, Nov 7, 6:21 PM
  • Minor refactor
simgunz updated this revision to Diff 69406.Thu, Nov 7, 6:51 PM
  • Apply KDE Frameworks astyle

Still looks great to me in testing.