D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  In D15580#554104 , @davidhurka 
wrote:
  
  > In D15580#554079 , @simgunz 
wrote:
  >
  > > Should I configure the toolbar so that if the user changes to "text 
alongside icons" the toolbar appears like this?
  > >  (the user can then expand the remaining icon-only buttons one by one by 
right clicking on them if he needs to visualize the text)
  >
  >
  > I think that will be easier to tell as soon as a user decides to show the 
text, and complains about it.
  
  
  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  Is the icon fine for the toolbar hide/close button? Do you have a better 
suggestion?
  
  F7674605: Screenshot_20191027_092856.png 

  *Hide toolbar button*

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 68813.
simgunz marked 4 inline comments as done.
simgunz added a comment.


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

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=68768=68813

BRANCH
  new-annotation-toolbar_ToggleActionMenu

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added inline comments.

INLINE COMMENTS

> davidhurka wrote in shell.cpp:370
> >> @simgunz just wrote:
> > 
> > https://phabricator.kde.org/D15580#544534
> 
> That’s what I thought about. :)

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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  In D15580#554079 , @simgunz wrote:
  
  > 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 
.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[David Hurka]

davidhurka added inline comments.

INLINE COMMENTS

> davidhurka wrote in 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?

>> @simgunz just wrote:
> 
> https://phabricator.kde.org/D15580#544534

That’s what I thought about. :)

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[David Hurka]

davidhurka added a comment.


  In D15580#554079 , @simgunz wrote:
  
  > Should I configure the toolbar so that if the user changes to "text 
alongside icons" the toolbar appears like this?
  >  (the user can then expand the remaining icon-only buttons one by one by 
right clicking on them if he needs to visualize the text)
  
  
  I think that will be easier to tell as soon as a user decides to show the 
text, and complains about it.
  
  ---
  
  I 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.

INLINE COMMENTS

> editannottooldialog.cpp:40
>  {
> +m_builtinTool = builtinTool;
> +

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
> +
> +# remove user-defined annotation tools to make space to new builtin 
> annotation tools
> +Id=annotation-toolbar

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

> shell.cpp:370
> +  toolBar( "mainToolBar" );
> +  KToggleToolBarAction * aToggleAnnotator = new KToggleToolBarAction( 
> toolBar( "annotationToolBar" ), i18n(""), this );
> +  aToggleAnnotator->setIcon( QIcon::fromTheme( 
> QStringLiteral("draw-freehand") ) );

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?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  In D15580#553421 , @ngraham wrote:
  
  > All right, that works!
  >
  > I think there's only one remaining thing I noticed, then I'm ready to give 
it a UI stamp of approval: the toolbar should have a close button on the 
far-right side (you can use an expanding spacer to position it there) so people 
don't have to go up to the menubar to close the toolbar once they're done using 
it.
  
  
  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.
  
  F7669753: Screenshot_20191025_210846.png 

  
  //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.//
  
  F7669750: Screenshot_20191025_211307.png 

  
  //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)
  
  F7670023: Screenshot_20191025_222139.png 

  //Non-obvious actions expanded, obvious one compressed.//

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 68768.
simgunz added a comment.


  - Warn user that stamps in PDF are an experimental feature

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=68650=68768

BRANCH
  new-annotation-toolbar_ToggleActionMenu

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 68650.
simgunz added a comment.


  - Show annotation toolbar when quick annotation is selected

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=68649=68650

BRANCH
  new-annotation-toolbar_ToggleActionMenu

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  In D15580#552773 , @ngraham wrote:
  
  > In D15580#552455 , @simgunz 
wrote:
  >
  > > When an annotation is selected its annotation toolbar button is checked 
and Browse Mode is checked, exactly as in the current Okular version. 
  > >  When you select a quick annotation tool what happens is that the 
corresponding annotation action gets checked. In a sense a quick annotation is 
just an alias for an effective annotation with a specified set of settings 
(color, etc.). For this reason the quick annotation cannot have a checked 
state. 
  > >  This however has a quirkiness in the case the annotation toolbar is 
hidden. In that case we are in Browse Mode and an annotation tool is active but 
we do not see the corresponding checked action in the hidden toolbar. This can 
be "solved" by showing the annotation toolbar when a quick annotation is 
selected, but I think it is going to become frustrating very soon.
  >
  >
  >
  
  
  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 Tools > Annotations and Settings > Toolbars Shown > Annotation 
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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 68649.
simgunz added a comment.


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

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=68493=68649

BRANCH
  new-annotation-toolbar_ToggleActionMenu

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  In D15580#552455 , @simgunz wrote:
  
  > When an annotation is selected its annotation toolbar button is checked and 
Browse Mode is checked, exactly as in the current Okular version. 
  >  When you select a quick annotation tool what happens is that the 
corresponding annotation action gets checked. In a sense a quick annotation is 
just an alias for an effective annotation with a specified set of settings 
(color, etc.). For this reason the quick annotation cannot have a checked 
state. 
  >  This however has a quirkiness in the case the annotation toolbar is 
hidden. In that case we are in Browse Mode and an annotation tool is active but 
we do not see the corresponding checked action in the hidden toolbar. This can 
be "solved" by showing the annotation toolbar when a quick annotation is 
selected, but I think it is going to become frustrating very soon.
  
  
  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. :-)

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  In D15580#552362 , @ngraham wrote:
  
  > This is super fantastic.
  >
  > I have a few more observations from use:
  >
  > - When using one of the quick annotations, the Quick Annotations button 
should have a checked state so you can tell that one of its tools is active
  
  
  When an annotation is selected its annotation toolbar button is checked and 
Browse Mode is checked, exactly as in the current Okular version. 
  When you select a quick annotation tool what happens is that the 
corresponding annotation action gets checked. In a sense a quick annotation is 
just an alias for an effective annotation with a specified set of settings 
(color, etc.). For this reason the quick annotation cannot have a checked 
state. 
  This however has a quirkiness in the case the annotation toolbar is hidden. 
In that case we are in Browse Mode and an annotation tool is active but we do 
not see the corresponding checked action in the hidden toolbar. This can be 
"solved" by showing the annotation toolbar when a quick annotation is selected, 
but I think it is going to become frustrating very soon.
  
  > (otherwise the previously-active toolbar button still looks checked despite 
not being active)
  
  This should not happen. Probably you are referring to Browse Mode that 
doesn't get unchecked, but that is the intended behavior and match that of the 
current Okular version.
  What happens should be:
  
  Example 1:
  
  - Text Selction is checked
  - Select Quick Annotations > Green Highlighter
  
  Results:
  
  - Browse Mode action gets checked
  - Highlighter action gets checked
  
  Example 1:
  
  - Browse Mode is checked
  - Ellipse is checked
  - Select Quick Annotations > Green Highlighter
  
  Results:
  
  - Browse Mode remains checked
  - Highlighter action gets checked
  
  If we make the quick annotation checkable we have the following problem:
  
  - The Quick Annotation won't display a `edit-draw` icon with the text "Quick 
Annotation" but it will display a specific action, i.e. "Green Highlighter" 
which might be confusing for the user, given that Quick Annotation would be 
never displayed. Exactly what happens now for the Stamp annotation (in this 
case you see "Approved" by default).
  
  Bottom line: unless there is a clever way to make the actions checkable but 
display the Quick Annotation button, I think that the current implementation is 
the the less worse situation. Regarding browse mode, we can also uncheck it 
when an annotation is selected, if there are no side effects.
  
  > - Maybe add a new menu item at the bottom of the Quick Annotations menu 
saying "Show all" that will open the full toolbar That's a good idea. It would 
be: `[ ] Show Annotation Toolbar` (menu items need to start with action verbs). 
And then the menu item in the Tools menu would be the same. probably the same 
QAction would just used for both; then they would both keep track of state 
properly.
  
  Yes, but I do not know how to do it. See 
https://phabricator.kde.org/D15580#544534 for the details of the problem.
  
  Actually this exact action already exists in Settings > Toolbars Shown > 
Annotations Toolbar but I do not know of to access it and if it available when 
we set up the actions.
  
  > - The menu item that says, "Configure annotations..." should probably say 
"Configure quick annotations..." since it applies to the annotations in the 
quick annotations list, not all annotations more generally. Or even just 
"Configure..."
  
  I thought about this, but I decided to leave "Configure Annotations" given 
that in the same KCM module the user can configure the "annotation identity", 
which is not related to "Quick Annotations" but to "Annotations" in general. 
  Changing it to "Configure" won't work either. This is a standard action that 
will appear in the "Configure Toolbar" and "Configure Shortcuts" dialog. If you 
call it "Configure" it will become ambiguous for the user.
  
  > - I would add text to the Keep the active annotation button active after 
use toolbutton, since otherwise it's a bit hard to tell what it does. Maybe 
"Keep active" or "Keep active after use" (maybe that's too long)?
  
  Currently the action name and thus the text is "Pin". You mean I should 
change "Pin" to "Keep Active"?
  
  > - Maybe don't have the Keep Active button checked by default. In testing, 
it feels more natural to have to click on an annotation's button after each use.
  
  I agree with @davidhurka here regarding having it on by default. I cannot 
figure out a use case where a user wants to add a single annotation or keep 
reselecting it each time.
  My typical use case is: I am reading a paper/book and I check the highlighter 
to highlight the text multiple times while I read, so I want it always active. 
The highlighter is a common use case, think at e-readers which have only 
highlighters and notes.
  Another one is to fill in multiple 

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  In D15580#552381 , @davidhurka 
wrote:
  
  > Yeah, but [_] Annotation Toolbar would be more natural, I think. At least 
for users who figured out the meaning of “toolbar”.
  
  
  That's a good idea. It would be: `[ ] Show Annotation Toolbar` (menu items 
need to start with action verbs). And then the menu item in the Tools menu 
would be the same. probably the same QAction would just used for both; then 
they would both keep track of state properly.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[David Hurka]

davidhurka added a comment.


  Didn’t yet test this, but I like some of Nate’s ideas.
  
  In D15580#552362 , @ngraham wrote:
  
  > This is super fantastic.
  >
  > I have a few more observations from use:
  >
  > - Maybe add a new menu item at the bottom of the Quick Annotations menu 
saying "Show all" that will open the full toolbar
  
  
  Yeah, but [_] Annotation Toolbar would be more natural, I think. At least for 
users who figured out the meaning of “toolbar”.
  
  > - I would add text to the Keep the active annotation button active after 
use toolbutton, since otherwise it's a bit hard to tell what it does. Maybe 
"Keep active" or "Keep active after use" (maybe that's too long)?
  
  Maybe “Keep selected”? I prefer “Keep active”.
  
  > - Maybe don't have the Keep Active button checked by default. In testing, 
it feels more natural to have to click on an annotation's button after each use.
  
  I think on by default makes sense. When the annotation tools don’t behave as 
expected (i. e. stay active), it will be easy to uncheck something.
  
  > - It might be nice if hitting the [9] key multiple times cycled through the 
items in the shape annotation menu
  > - The [0] key could do the same for the stamp annotation
  
  +1

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 68493.
simgunz added a comment.


  - Set toolbar default to icononly

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=68372=68493

BRANCH
  new-annotation-toolbar_ToggleActionMenu

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added dependencies: D21755: [RFC] Replace ToolAction by a more 
universal “ToggleActionMenu”, D21971: [DEMO] Enhance ToggleActionMenu with 
ImplicitDefaultAction mode..

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a subscriber: trickyricky26.
simgunz added a comment.


  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:
  F7637125: Screenshot_20191020_173447.png 

  
  New color pickers:
  F7637115: Screenshot_20191020_173322.png 

  
  F7637120: Screenshot_20191020_173405.png 


REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 68372.
simgunz added a comment.


  - 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

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=67832=68372

BRANCH
  new-annotation-toolbar_ToggleActionMenu

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  > 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)
  
  F7592924: Screenshot_20191014_095818.png 

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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[David Hurka]

davidhurka added a comment.


  In D15580#546492 , @ngraham wrote:
  
  > Typically the color chooser includes a "transparent" item.
  
  
  Also a bit complicated for me as user. :)
  
  In D15580#546395 , @simgunz wrote:
  
  > [...]
  >  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 F7585426: 
image.png 
  
  
  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  In D15580#546492 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  Typically the color chooser includes a "transparent" item.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  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 
ToggleActionMenu 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?
  
  F7585426: image.png 
  Just to give you an idea

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 67832.
simgunz added a comment.


  - 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

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=67744=67832

BRANCH
  new-annotation-toolbar_ToggleActionMenu

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz edited the summary of this revision.
simgunz edited the test plan for this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 67744.
simgunz added a comment.


  - Use new highlighter icon

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=67589=67744

BRANCH
  annotation-toolbar-refactor

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz marked 2 inline comments as done.
simgunz added a comment.


  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?
  
  2. 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::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 :-)

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz marked 4 inline comments as done.
simgunz added inline comments.

INLINE COMMENTS

> davidhurka wrote in 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.

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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 67589.
simgunz added a comment.


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

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=67517=67589

BRANCH
  annotation-toolbar-refactor

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  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.
  
  F7549017: Screenshot_20191008_182346.png 

  Screenshot with the new squiggle and line width icons.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 67517.
simgunz added a comment.


  - 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

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=67036=67517

BRANCH
  annotation-toolbar-refactor

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.h
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 67036.
simgunz added a comment.


  - 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

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=64017=67036

BRANCH
  new-annotation-toolbar_ToggleActionMenu

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.h
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz edited the summary of this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

Re: D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

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.

On Tue, Aug 20, 2019 at 6:26 PM Nathaniel Graham <
nore...@phabricator.kde.org> wrote:

> ngraham added a comment. View Revision
> 
>
> In D15580#515250 , @davidhurka
>  wrote:
>
> In D15580#514707 , @ngraham
>  wrote:
>
> In D15580#513826 , @simgunz
>  wrote:
>
>- 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?
> F7264576: image.png 
> 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.
>
> *REPOSITORY*
> R223 Okular
>
> *REVISION DETAIL*
> https://phabricator.kde.org/D15580
>
> *To: *simgunz, Okular, VDG
> *Cc: *ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham,
> tobiasdeiminger, okular-devel, maguirre, fbampaloukas, joaonetto, kezik,
> tfella, darcyshen
>

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, andisa, siddharthmanthan, maguirre, 
fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  In D15580#515250 , @davidhurka 
wrote:
  
  > In D15580#514707 , @ngraham 
wrote:
  >
  > > In D15580#513826 , @simgunz 
wrote:
  > >
  > > > - 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?
  >  F7264576: image.png 
  >  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[David Hurka]

davidhurka added a comment.


  In D15580#514707 , @ngraham wrote:
  
  > In D15580#513826 , @simgunz 
wrote:
  >
  > > - 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?
  F7264576: image.png 
  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  In D15580#513826 , @simgunz wrote:
  
  > - 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.
  
  In D15580#513826 , @simgunz wrote:
  
  > 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 64017.
simgunz added a comment.


  - Change typewriter and pin icons

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=62679=64017

BRANCH
  new-annotation-toolbar_ToggleActionMenu

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.rc
  shell/shell.cpp
  shell/shell.h
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/tools.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  In D15580#513253 , @ngraham wrote:
  
  > Some thoughts:
  >
  > - Instead of using a horizontal toolbar below the main toolbar, instead I 
might experiment with a vertical toolbar like the one shown in Gwenview's View 
mode. We could make the toolbar live inside the Reviews tab, which already 
shows a list of all annotations (and then we should unify the terminology 
vis-a-vis "reviews" vs "annotations." REASON: cramming everything into a 
horizontal toolbar seems impossible for a feature this rich; using a vertical 
toolbar provides us enough horizontal space to show labels for everything, and 
enough vertical space to easily hold everything. Also we re-use an existing UI 
element that's currently rather bare.
  
  
  
  
  - 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz edited the summary of this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  In D15580#513395 , @davidhurka 
wrote:
  
  > In D15580#513253 , @ngraham 
wrote:
  >
  > > 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[David Hurka]

davidhurka added a comment.


  In D15580#513253 , @ngraham wrote:
  
  > 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?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  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 appears to 
be somewhat mis-named and already shows a list of all 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?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  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!

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  In D15580#506692 , @ngraham wrote:
  
  > For my own UI review, I must continue to push for putting the annotation 
tools on a second toolbar that appears below the main one when needed.
  
  
  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) )
  
  F7165410: Screenshot_20190805_122801.png 

  Screenshot 1
  
  F7165400: Screenshot_20190805_122640.png 

  Screenshot 2
  
  F7165405: Screenshot_20190805_122727.png 

  Screenshot 3 (toolbar expanded)
  
  F7165456: Screenshot_20190805_124108.png 

  Screenshot 4 (underline is a placeholder for the menu that will contain 
underline, squiggle and strikethrough)
  
  F7165489: Screenshot_20190805_124817.png 

  Screenshot 5 (config actions are `icons only`)

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a reviewer: VDG.
ngraham added a comment.


  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular, #vdg
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 62679.
simgunz added a comment.


  - Fix detach annotation when mouse mode unchecked

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=62678=62679

BRANCH
  new-annotation-toolbar_ToggleActionMenu

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.rc
  shell/shell.cpp
  shell/shell.h
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/tools.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 62678.
simgunz added a comment.


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

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=62478=62678

BRANCH
  new-annotation-toolbar_ToggleActionMenu

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.rc
  shell/shell.cpp
  shell/shell.h
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/tools.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 62478.
simgunz added a comment.


  - 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

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=62320=62478

BRANCH
  new-annotation-toolbar

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.rc
  shell/shell.cpp
  shell/shell.h
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/tools.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/toolaction.cpp
  ui/toolaction.h

To: simgunz, #okular
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Luigi Toscano]

ltoscano added a comment.


  In D15580#500275 , @davidhurka 
wrote:
  
  > In D15580#500264 , @simgunz 
wrote:
  >
  > > @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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: ltoscano, cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[David Hurka]

davidhurka added a comment.


  In D15580#500264 , @simgunz wrote:
  
  > @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?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, okular-devel, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


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

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, okular-devel, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz edited the summary of this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, okular-devel, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz edited the summary of this revision.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, okular-devel, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 62320.
simgunz added a comment.


  - Check if chosen color is valid before storing it
  - Notify PageViewAnnotator when the color has changed
  - Add missing annotation tools actions
  - Add XML annotation tools and connect corresponding actions
  - Add typewriter annotation tool
  - Set actions groups for all tools and text tools
  - Reimplement setToolsEnabled and setTextToolsEnabled
  - Reimplement setToolsEnabled and setTextToolsEnabled (correct way)
  - Disable code previously used to define annotation toolbar elements
  - Remove unused static constant
  - Revert "New annotation toolbar"
  - Add method to set color picker colorized icon
  - Remove method annotationColor
  - Set color of annotations tools when changed in color picker
  - Read color from settings when annotation tool selected
  - Save annotation tool settings to file on exit
  - Rename and simplify method to deselect all annotation actions
  - Refactor colorChanged slot
  - Add font selector
  - Add methods to access annotation and engine elements
  - Add basic implementation of width selector
  - Minor refactor
  - Remove unused code
  - Store font and width actions
  - Enable config actions based on selected annotation
  - Re-sort code
  - Add arrow annotation
  - Rename variables
  - Add inner color selector
  - Clean setEnabled method
  - Set inner color icon to transparent if tool does not support it
  - Allow setting font color
  - Fix widthAction wrongly enabled for inline-note
  - Fix crash on application exit
  - Improve annotations default colors
  - Fix arrow annotation tool XML
  - Use draw-polyline icon for Polygon
  - 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)
  - 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
  - Rename XML actions
  - Keep a state of the selected tool
  - Add favorite annotation tools
  - Correctly enable annotations
  - Fix advanced settings not saved
  - Fix and simplify width action
  - Fix and simplify opacity action
  - Fix annotation tools actions
  - Rename color action
  - Formatting
  - Remove unuseful annotation separators
  - Correctly set favorites action enabled
  - Properly trigger favorite action
  - Allow unchecking annotation actions
  - Minor refactor
  - Save state of continuous mode
  - Clean parseTool
  - Initialize private members
  - Add action to toggle annotation toolbar, set default position below
  - Add i18n
  - Move default value to declaration
  - Add tooltips
  - Fix error in rebase
  - Disable name and type edit for builtin tools
  - 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

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=61742=62320

BRANCH
  new-annotation-toolbar

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  part.rc
  shell/shell.cpp
  shell/shell.h
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/tools.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/toolaction.cpp

To: simgunz, #okular
Cc: cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, okular-devel, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[David Hurka]

davidhurka added a comment.


  You could consider to add these bugs as BUG keywords to this patch: 386578, 
374728, 352310, 330518, 341914, 157289
  
  - https://bugs.kde.org/show_bug.cgi?id=386578 Allow annotation mode to 
"stick" for better use with a stylus
  - https://bugs.kde.org/show_bug.cgi?id=374728 Changing annotation colours
  - https://bugs.kde.org/show_bug.cgi?id=352310 to organize better the 
annotation tools, i want to add separators between them
  - https://bugs.kde.org/show_bug.cgi?id=330518 Add configure icon to the 
annotation toolbar
  - https://bugs.kde.org/show_bug.cgi?id=341914 Allow Annotation Tools in 
Toolbar
  - https://bugs.kde.org/show_bug.cgi?id=157289 Make the review bar a standard 
toolbar
  
  In D15580#495344 , @simgunz wrote:
  
  > 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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: cfeck, aacid, davidhurka, knambiar, ngraham, tobiasdeiminger, okular-devel, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: cfeck, aacid, anthonyfieroni, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, maguirre, fbampaloukas, joaonetto, kezik, 
tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 61742.
simgunz added a comment.


  - 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

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=61190=61742

BRANCH
  new-annotation-toolbar

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  .gitlab-ci.yml
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  core/document.cpp
  core/signatureutils.h
  core/textdocumentgenerator.cpp
  generators/dvi/special.cpp
  generators/poppler/pdfsignatureutils.h
  generators/xps/generator_xps.cpp
  part.rc
  shell/shell.cpp
  shell/shell.h
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/tools.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toolaction.cpp

To: simgunz, #okular
Cc: cfeck, aacid, anthonyfieroni, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, maguirre, fbampaloukas, joaonetto, kezik, 
tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 61190.
simgunz added a comment.


  - Add i18n
  - Move default value to declaration
  - Add tooltips

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=61188=61190

BRANCH
  new-annotation-toolbar

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/okular.kcfg
  part.rc
  shell/shell.cpp
  shell/shell.h
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/data/tools.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/toolaction.cpp

To: simgunz, #okular
Cc: cfeck, aacid, anthonyfieroni, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, maguirre, fbampaloukas, joaonetto, tfella, 
darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz marked 6 inline comments as done.
simgunz added a subscriber: cfeck.
simgunz added a comment.


  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)

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: cfeck, aacid, anthonyfieroni, davidhurka, knambiar, ngraham, 
tobiasdeiminger, okular-devel, maguirre, fbampaloukas, joaonetto, tfella, 
darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 61188.
simgunz added a comment.


  - Clean parseTool
  - Initialize private members
  - Add action to toggle annotation toolbar, set default position below

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=60168=61188

BRANCH
  new-annotation-toolbar

REVISION DETAIL
  https://phabricator.kde.org/D15580

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/okular.kcfg
  part.rc
  shell/shell.cpp
  shell/shell.h
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/data/tools.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/toolaction.cpp

To: simgunz, #okular
Cc: aacid, anthonyfieroni, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, maguirre, fbampaloukas, joaonetto, tfella, darcyshen

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz retitled this revision from "New annotation toolbar" to "[WIP] New 
annotation toolbar".

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D15580

To: simgunz, #okular
Cc: aacid, anthonyfieroni, davidhurka, knambiar, ngraham, tobiasdeiminger, 
okular-devel, fbampaloukas, joaonetto, tfella, darcyshen