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

To test this revision
Delete or Temporary move aside the following files:

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

What is working and what is planned
See T8076

Fixes:

BUG: 386578
BUG: 374728
BUG: 352310
BUG: 330518
BUG: 341914
BUG: 157289
BUG: 358057

Diff Detail

Repository
R223 Okular
Branch
new-annotation-toolbar
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13632
Build 13650: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
  • 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.

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

anthonyfieroni added inline comments.
part.rc
2

Increase version.

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

If we remove the sticky button, we have two options:

  • 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)
  • disable annotation on right click anywhere (as current Okular)
  • disable annotation on left click on annotation (as current Okular) [ I have sent a patch to Qt to modify QActionGroup, https://codereview.qt-project.org/c/qt/qtbase/+/255770)
  • disable annotation on Esc (as current Okular)

Foxit reader has a "Pin" button. How is it implemented in other readers?

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

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)
  • disable annotation on right click anywhere (as current Okular)
  • disable annotation on left click on [activated] annotation [button] (as current Okular) [ I have sent a patch to Qt to modify QActionGroup, https://codereview.qt-project.org/c/qt/qtbase/+/255770)
  • disable annotation on Esc (as current Okular)

Seems both compatible and is what I consider optimal.

If someone does (not) want sticky, the pin button can be set to the desired state, and then removed together with the shortcut, because it is a normal toolbar now, right?

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

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

I'll move it, added to TODO

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?

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.

simgunz updated this revision to Diff 59208.Jun 5 2019, 4:02 PM
  • Use draw-polyline icon for Polygon
  • Increase version of part.rc
  • Change icon of strikethrough (remove symbolic suffix)
  • Add TODO in code
  • Rename PageViewToolBar to AnnotationActionHandler
  • No need to forceHide mechanism
  • Use KToggleAnnotationToolBar to show/hide toolbar (not working)

I renamed PageViewToolBar and moved to a file by itself. I took inspiration from Dolphin for the name.

There is still code to be cleaned though.

ui/pageview.cpp
661–662

Anyone knows how KToggleToolBarAction is supposed to work?

Currently it does nothing.

Reading the code, it seems that the constructor where the toolbar is specified by name does nothing, unless there is some other magic going on.
https://api.kde.org/frameworks/kxmlgui/html/ktoggletoolbaraction_8cpp_source.html#l00061

aacid added a subscriber: aacid.Jun 11 2019, 10:40 PM
aacid added inline comments.
ui/pageview.cpp
661–662

You're right, it seems it never has. Can you use the other one?

simgunz added inline comments.Jun 12 2019, 6:34 AM
ui/pageview.cpp
661–662

Is it there a clean way to obtain a reference to the toolbar from within this class?
I guess that KMainWindow::toolBar(name) is a possibility, but how do I get a reference to the main window?

aacid added inline comments.Jun 13 2019, 9:38 PM
ui/annotationactionhandler.cpp
132

no i18n?

ui/pageview.cpp
661–662

right, that's actually a tricky question.

I'll try to figure it out, but you may want to play with stuff and not wait for me, a close friend of mine is getting married so don't expect anything from me until monday/tuesday

simgunz added inline comments.Jun 14 2019, 5:49 AM
ui/pageview.cpp
661–662

Your help is appreciated. No rush, I can work on other things in the meanwhile.

What I tried so far:

Calling this in setupActions of PageViewAnnotator:

qDebug() << KMainWindow::memberList();
qDebug() << KMainWindow::memberList().count();
qDebug() << KMainWindow::memberList()[0]->toolBars().count();

I get

(Shell(0x55c3e0f80fc0, name="okular::Shell#"))
1
0

So it seems that the toolbars have not been created yet at this point. I'll keep experimenting.

simgunz updated this revision to Diff 59781.EditedJun 14 2019, 10:22 AM

Changes:

  • Added opacity action
  • Cleaned width action
  • Added action to access advanced settings for all annotations
  • Big code refactor:
    • m_toolDefinition is now a QDomDocument, so that the toolElement are not randomly deleted when the associated QDomDocument goes out of scope.
    • AnnotationActionHandler acts directly on PageViewAnnotator instead of emitting signals
  • Further code cleaning in pageview.cpp
  • Annotations work only in Normal mouse mode, and switch to it when they are selected

Commits:

  • Remove unused variable
  • Remove tooltips that are never shown in any case
  • Add width action icons
  • Use KToggleAction
  • Make m_toolsDefinition a QDomDocument
  • Remove unused method
  • Refactor code and remove obsolete code
  • Detach annotation when mouse mode is not Normal mode
  • Use Qt5 connect syntax where possible
  • Add Advanced Settings dialog
  • Set better action names
  • Add opacity selection action
  • Fix width text

Code is still messy in some parts, I'll clean it at the end.

davidhurka added inline comments.Jun 16 2019, 5:02 PM
ui/toolaction.cpp
71

Why remove this tooltip?

To make ToolAction (or my inferior ToggleActionMenu) more universal to use it for a Change Colors feature, it would be nice if tooltips are not needed anymore.

simgunz added inline comments.Jun 16 2019, 6:37 PM
ui/toolaction.cpp
71

That tooltip does not appear anywhere in the UI in practice, at least I have not manage to make it appear. Have you?

Now I am using ToolAction to show other things, so the tooltips, if needed, need to be set outside. Still a TODO.

Where is ToggleActionMenu defined?

simgunz updated this revision to Diff 59954.Jun 16 2019, 7:27 PM

Added the favorite tools.

Now only the stamp tool is missing.

PS: As usual, the code is still messy, I'll refactor it. For now I am more interested in feedback on the UI/UX

Commits:

  • Rename XML actions
  • Keep a state of the selected tool
  • Add favorite annotation tools
  • Correctly enable annotations
  • Fix advanced settings not saved

For now I am more interested in feedback on the UI/UX

Do you have screenshots?

ui/toolaction.cpp
71

The tooltip should become visible when you hover the toolbar button with the three selection tools. It’s in the main toolbar by default. See also D21622. On my Okular it worked, on yours not?

ToggleActionMenu is still a TODO in D21755. ;)

What will you use ToolAction for? In that case I probably shouldn’t break it.

simgunz added a comment.EditedJun 17 2019, 2:58 PM

Favorite annotations:
The Star adds the currently selected tool to the favorite list, the bookmark symbol displays the list of the favorite tools:

Once the star is clicked a custom annotation name is asked to the user. Clicking cancel aborts the action:

Customize the favorite tools:

Opacity selector:
Activates on click and displays a list of possible opacities with an icon:

simgunz updated this revision to Diff 60004.Jun 17 2019, 6:51 PM
  • Fix and simplify width action
  • Fix and simplify opacity action
  • Fix annotation tools actions
  • Rename color action
  • Formatting

Don’t have UI feedback that asks for action already in this patch. :)

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

Don’t have UI feedback that asks for action already in this patch. :)

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

simgunz updated this revision to Diff 60168.Jun 20 2019, 7:07 PM
  • Remove unuseful annotation separators
  • Correctly set favorites action enabled
  • Properly trigger favorite action
  • Allow unchecking annotation actions
  • Minor refactor
  • Save state of continuous mode
simgunz retitled this revision from New annotation toolbar to [WIP] New annotation toolbar.Jun 28 2019, 6:09 AM
simgunz updated this revision to Diff 61188.Jul 5 2019, 5:05 AM
  • Clean parseTool
  • Initialize private members
  • Add action to toggle annotation toolbar, set default position below
simgunz marked 6 inline comments as done.Jul 5 2019, 5:35 AM
simgunz added a subscriber: cfeck.

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.EditedWed, Sep 18, 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.