New annotation toolbar
AbandonedPublic

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

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.Oct 24 2019, 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.Oct 24 2019, 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.Oct 25 2019, 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.Oct 25 2019, 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.Oct 27 2019, 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.Oct 27 2019, 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.Oct 27 2019, 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.Oct 27 2019, 5:44 PM
  • Merge remote-tracking branch 'origin/master' into new-annotation-toolbar_ToggleActionMenu
ngraham accepted this revision.Oct 27 2019, 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.Oct 27 2019, 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.EditedOct 31 2019, 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.Nov 2 2019, 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.Nov 3 2019, 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.Nov 4 2019, 10:05 AM
  • Use full ellipsis
  • Ignore locale when saving numbers in config files
davidhurka added inline comments.Nov 4 2019, 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)Nov 5 2019, 3:14 PM
simgunz updated this revision to Diff 69379.Nov 7 2019, 10:22 AM
  • Add setToolBarVisible method
  • Fix annotation toolbar only hidden in first main window
Restricted Application added a project: Documentation. · View Herald TranscriptNov 7 2019, 10:22 AM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript
simgunz updated this revision to Diff 69380.Nov 7 2019, 10:24 AM
  • Merge remote-tracking branch 'origin/master' into new-annotation-toolbar_ToggleActionMenu
simgunz marked 2 inline comments as done.Nov 7 2019, 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.Nov 7 2019, 6:21 PM
  • Minor refactor
simgunz updated this revision to Diff 69406.Nov 7 2019, 6:51 PM
  • Apply KDE Frameworks astyle

Still looks great to me in testing.

Looks like the dependent patch has landed, so we can more forward with this.

Still LGTM.

aacid requested to write some autotest for the ToggleActionMenu before merging this. I'll merge master in this review and work on the autotests soon.

aacid added a comment.Feb 3 2020, 10:48 PM

This is huge, i would really welcome if it could be moved over to invent.kde.org since i'm 92% sure some of the clazy/clang-tidy checks will fail.

aacid requested to write some autotest for the ToggleActionMenu before merging this. I'll merge master in this review and work on the autotests soon.

It’s okay to me that you write the autotests. If you wish, I could do that instead.

I am new to autotests, so I will have to learn how to do that. I am also unsure about what should be tested. The ToggleActionMenu itself, or its behaviour in Okular?

aacid requested to write some autotest for the ToggleActionMenu before merging this. I'll merge master in this review and work on the autotests soon.

It’s okay to me that you write the autotests. If you wish, I could do that instead.

I am new to autotests, so I will have to learn how to do that. I am also unsure about what should be tested. The ToggleActionMenu itself, or its behaviour in Okular?

I'm more than happy if you do that :-)

I am also not an expert on UI autotests, my suggestion is to have a look at the other Okular autotests and take inspiration from there. Probably it is good to test the component on its own (all the possible combination of parameters) and also the behavior in Okular.

I'll try to move this to invent soon.

aacid added a comment.Feb 4 2020, 6:37 PM

aacid requested to write some autotest for the ToggleActionMenu before merging this. I'll merge master in this review and work on the autotests soon.

It’s okay to me that you write the autotests. If you wish, I could do that instead.

I am new to autotests, so I will have to learn how to do that. I am also unsure about what should be tested. The ToggleActionMenu itself, or its behaviour in Okular?

The functionality in Okular, so that if in the future we replace ToggleActionMenu with EvenBetterToggleActionMenu we can spot regressions

It would be nice if someone can test compile and run Kile with the new toolbar.

Kile cannot be stared (immediately crashes) here. I have Kile and Okular from git/master.

However, Kile works fine with Okular from release/19.12 branch.

Thanks in advance for your testing.

aacid added a comment.Feb 4 2020, 10:16 PM

It would be nice if someone can test compile and run Kile with the new toolbar.

Kile cannot be stared (immediately crashes) here. I have Kile and Okular from git/master.

However, Kile works fine with Okular from release/19.12 branch.

Thanks in advance for your testing.

But the new toolbar hasn't landed yet (it's this patch).

Maybe it's D21755 that creates problems for you?

Can you locally revert that and see if it fixes the issue?

aacid requested to write some autotest for the ToggleActionMenu before merging this. I'll merge master in this review and work on the autotests soon.

It’s okay to me that you write the autotests. If you wish, I could do that instead.

I am new to autotests, so I will have to learn how to do that. I am also unsure about what should be tested. The ToggleActionMenu itself, or its behaviour in Okular?

davidhurka (David Hurka)

User

The functionality in Okular, so that if in the future we replace ToggleActionMenu with EvenBetterToggleActionMenu we can spot regressions

I’m working on the autotests. But I had no backup, and so need to start over again. :(

The functionality in Okular means for me that I trigger a mouse mode action, and then test the default action of the toolbar button. Is that what you mean?

Can I add the ToggleActionMenu test to parttest, or do I need a new test?

And someone needs to explain how to create merge requests on invent.kde.org. I tried three ways and failed, and unfortunately I can only find instructions for Phabricator.

yurchor added a comment.EditedFeb 22 2020, 6:40 PM

And someone needs to explain how to create merge requests on invent.kde.org. I tried three ways and failed, and unfortunately I can only find instructions for Phabricator.

  1. Fork the repo with the "Fork" button here: https://invent.kde.org/kde/okular
  2. Clone it: git@invent.kde.org:dhurka/okular.git
  3. Create branch: git checkout -b ann-toolbar
  4. Do some changes (or just apply your commit from the Phabricator repo (git format-patch -1) with git am <patch_file>).
  5. Push the branch: git push -u origin ann-toolbar
  6. Click on the link that is proposed by gitlab in your working console to create MR (you should be logged in in your browser).

When you create the merge request:

  • set the title to WIP: title of merge request to mark it as work in progress
  • push the commits as you work so you have a backup, can see the results of the CI tests and possibly receive feedbacks