[WIP] New annotation toolbar
Needs ReviewPublic

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

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 18006
Build 18024: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

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)

simgunz updated this revision to Diff 61190.Jul 5 2019, 7:06 AM
  • Add i18n
  • Move default value to declaration
  • Add tooltips
simgunz updated this revision to Diff 61742.Jul 14 2019, 1:54 PM
  • Fix action deselected if favorite tool with the same type is activated
  • Fix geom shapes action disabled when okular reparses config
  • Disable text annotation when file does not support them

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.

You could consider to add these bugs as BUG keywords to this patch: 386578, 374728, 352310, 330518, 341914, 157289

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.

simgunz updated this revision to Diff 62320.EditedJul 22 2019, 4:34 PM

Fix bad rebase

simgunz edited the summary of this revision. (Show Details)Jul 22 2019, 4:38 PM

@davidhurka Thanks for looking up the bug numbers

Do you know if adding the bug numbers separated by a comma without repeating the keyword BUG would work?

simgunz edited the summary of this revision. (Show Details)Jul 22 2019, 4:40 PM

@davidhurka Thanks for looking up the bug numbers

Do you know if adding the bug numbers separated by a comma without repeating the keyword BUG would work?

Probably not: https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords

Do you have to move this to gitlab anyway?

@davidhurka Thanks for looking up the bug numbers

Do you know if adding the bug numbers separated by a comma without repeating the keyword BUG would work?

Probably not: https://community.kde.org/Infrastructure/Phabricator#Add_special_keywords

Do you have to move this to gitlab anyway?

As far as I know, gitlab is not relevant here; the bugzilla integration through BUG works through git hooks.

Do you have to move this to gitlab anyway?

I do not plan to move it given that all the discussion was done here, and that I have not yet seen an official guide regarding the review requests through gitlab.

simgunz updated this revision to Diff 62478.Jul 24 2019, 3:57 PM
  • 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
simgunz updated this revision to Diff 62678.Jul 28 2019, 1:19 PM
  • Use ToggleActionMenu
  • Add stamp annotation
  • Fix swap straight line and arrow
  • Use Title style capitalization
  • Show real stamp symbols if squared
  • Better stamp icon
simgunz updated this revision to Diff 62679.Jul 28 2019, 1:40 PM
  • Fix detach annotation when mouse mode unchecked
simgunz added a comment.EditedJul 28 2019, 1:46 PM

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.

ngraham edited the summary of this revision. (Show Details)Aug 4 2019, 10:08 PM
ngraham edited the summary of this revision. (Show Details)Aug 4 2019, 10:10 PM
ngraham added a reviewer: VDG.Aug 4 2019, 10:55 PM

Thanks @simgunz, this is really excellent work. Adding @VDG for more UI review comments.

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:

  1. 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.
  2. Put the configuration actions on a toolbar of their own a set icons only as a default for this toolbar (screenshot 5)
  3. (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)


Screenshot 5 (config actions are icons only)

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!

ngraham added a comment.EditedAug 17 2019, 3:23 AM

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

Some thoughts:

These thoughts sound good to me.

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

Some thoughts:

These thoughts sound good to me.

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

simgunz edited the summary of this revision. (Show Details)Aug 18 2019, 6:28 AM

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

See bug [[ https://bugs.kde.org/show_bug.cgi?id=408283 | 408283 ] for the rough icon list. Probably we should further discuss the icon design in that bug.

simgunz updated this revision to Diff 64017.Aug 19 2019, 6:38 AM
  • Change typewriter and pin icons
  • 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?

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

Cool, that makes sense to me.

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

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

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.

simgunz added a subscriber: simgunz.EditedSep 18 2019, 4:45 PM

EDIT: text layout

Sorry for the long silence. I have been busy at work and then on vacation.

Personally I do not like the idea of the vertical toolbar specifically for Okular for the following reasons:

  • Okular differently from Gwenview already has a tab bar, which means that further splitting the reviews tab into two pieces add another nesting level.
  • Splitting the Review tab horizontally with the toolbar on top and the reviews below seems quite cluttered to me. Also that review tab could be improved (see T8553) and may require all the vertical space (e.g. show the full text of the notes, allow to reply, etc. See Foxit for example).
  • It is not possible to build the toolbar with KXMLGUI as it is now, but needs to be built programmatically (I think). The Gwenview toolbar is not a standard toolbar, indeed it cannot be edited or moved.

The reasons for which I prefer to make the annotation toolbar a standard toolbar displaying only the icons are:

  • Many other programs have similar icon-only toolbars (e.g. libreoffice, and also other PDF viewers e.g. MacOS viewer and Adobe reader). For the annotation toolbar I would conform with the other PDF viewers.
  • The icon buttons have tooltips and likely the users will quickly learn them
  • The toolbar is standard and can be customized by the user.

If you don't like the proposed vertical toolbar, that's okay, but then we're going to need to sort out a lot of tricky UI issues for the horizontal toolbar and have perfect icons.

simgunz edited the summary of this revision. (Show Details)Sun, Sep 29, 8:28 AM
simgunz updated this revision to Diff 67036.Sun, Sep 29, 4:34 PM
  • 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
simgunz updated this revision to Diff 67517.Tue, Oct 8, 4:22 PM
  • 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
simgunz added a comment.EditedTue, Oct 8, 4:27 PM

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.

Once I fixed the TODO I'll test this further updating the test plan.


Screenshot with the new squiggle and line width icons.

Yeah. draw-highlight is coming too in D24493.

With the better icons, maybe we can move the inviditual tools back to being icons-only to save a ton of space, and then everything will fit into a toolbar when the window isn't super wide.

simgunz updated this revision to Diff 67589.Thu, Oct 10, 6:32 AM
  • Make ktoggleannotationaction to be owned by shell.cpp
  • Add hack to allow respecting the default toolbars layout
simgunz marked 4 inline comments as done.Thu, Oct 10, 6:41 AM
simgunz added inline comments.
ui/toolaction.cpp
71

Currently the tooltip appears only if any of the selection tools is already selected, while it does not appear if another action like "Browse" is selected. Moreover it made sense when one had to click and hold to show the menu, which was quite unintuitive, now that it is enough to click on the arrow, I believe it is redundant.

simgunz marked 2 inline comments as done.Thu, Oct 10, 7:02 AM

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

  1. When a color picker is open and the user presses Esc, the color picker modal dialog is closed but the currently selected annotation action is deselected when it should not. It is not to clear to me how to prevent the propagation of the KeyPressEvent. In PageView::event the event is never stopped, even if it was already accepted and when it reaches PageView::keyReleaseEvent I cannot tell if it was accepted by the color picker or not. Any idea on how to solve this?
  1. I want to add the action mouse_toggle_annotate (to show/hide the annotation toolbar) to the Quick Annotations action in the main toolbar. There are two problems:
    • how to retrieve the mouse_toggle_annotate from within AnnotationActionHandlerPrivate::populateQuickAnnotations. My current solution is the following, but I am not sure it is the best one:
QAction * aToggleAnnotator = qobject_cast<KParts::MainWindow *>(KParts::MainWindow::memberList()[0])->actionCollection()->action( "mouse_toggle_annotate" );
if ( aToggleAnnotator ) {
    aQuickTools->addAction( aToggleAnnotator );
}
  • The first time that AnnotationActionHandlerPrivate::populateQuickAnnotations is reached the mouse_toggle_annotate action does not exist yet because the KPart is created before setupActions is called in shell.cpp. I can manually invoke Part::slotNewConfig() after setupActions has been called, but does not seem a nice solution.

@aacid Your help would be appreciated :-)

simgunz updated this revision to Diff 67744.Fri, Oct 11, 7:48 PM
  • Use new highlighter icon
simgunz edited the summary of this revision. (Show Details)Sun, Oct 13, 9:54 AM
simgunz edited the test plan for this revision. (Show Details)
simgunz updated this revision to Diff 67832.Sun, Oct 13, 9:55 AM
  • 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
simgunz added a comment.EditedSun, Oct 13, 10:14 AM

Edit: changed ToggleActionMenu to KSelectAction

Currently once the inner color of an annotation (e.g. rectangle) has been selected, there is no way to remove it (making the inner part of the annotation transparent) using the toolbar. The only way is through Adavenced Settings.

What do you think is the best way to expose this to the user?

One possibility is to make the Annotation fill color action a KSelectAction in InstantPopup mode with two entries:

  • Select inner color, which opens the color picker
  • Remove inner color which just removes the inner color

What do you think?


Just to give you an idea

Typically the color chooser includes a "transparent" item.

Typically the color chooser includes a "transparent" item.

The things are trickier here. In Okular the opacity of the annotation is common to the whole annotation, i.e. we cannot have an opacity value for the border color and one for the inner color. This means that the opacity needs to be managed by itself (e.g we cannot just show the alpha channel in the two color pickers) using the QColorDialog option ShowAlphaChannel.

If we show the alpha channel only for the inner color picker we still have an undefined situation because the user can either choose a color with opacity 100% or 0% everything in between is invalid. If he chooses 0% we can remove the fill color.

If we do not show the alpha channel I think we cannot display an option to select the transparent color.

That is why I came up with the solution of the submenu in the inner color picker action.

Typically the color chooser includes a "transparent" item.

Also a bit complicated for me as user. :)

[...]
One possibility is to make the Annotation fill color action a KSelectAction in InstantPopup mode with two entries:

  • Select inner color, which opens the color picker
  • Remove inner color which just removes the inner color

I think this is ok. Two wishes:

  • The toolbar button shows a checked state, when the fill color is set.
  • The dropdown menu shows the last ~5 selected colors and/or the colors of the favourite annotations.

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)

  • The dropdown menu shows the last ~5 selected colors and/or the colors of the favourite annotations.

Well actually showing a list of colors is a good idea (I did put this in the future improvements list, but it is actually not that hard to do).

We can have the following action list:

Color picker: color1, color2, ..., colorN, Custom color...
Inner color picker: color1, color2, ..., colorN, Transparent, Custom color...

We should decide which colors to choose.
The basic option is to provide a palette of standard colors, which seems the solution used by most of the other readers if I am not wrong.
The second option is to list the previously selected colors. How would you fill the list by default? With standard colors?
I would discard the third option of having the colors of the favorite annotations.

I am more inclined towards the first option for the following reasons:

  • it is logically simple
  • it is what most of the others readers do (so what users are used to)
  • If a user has to use custom colors a lot, I expect that it saves those as custom tools in the quick annotation list. E.g. I create a yellow, green, blue highlighters and save them to the quick annotations. Then I think an average user will use those three tools/colors most of the time. I think that situations where one has to use different tools with each different colors (so that the quick annotation list explodes) are quite uncommon. (This is also why I created the quick annotation list, to have the same tool with my custom colors ready to use)
ngraham edited the summary of this revision. (Show Details)Wed, Oct 16, 10:22 PM
simgunz updated this revision to Diff 68372.Sun, Oct 20, 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.Mon, Oct 21, 10:58 PM
  • Set toolbar default to icononly