D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka abandoned this revision.
davidhurka added a comment.


  Merge request at https://invent.kde.org/kde/okular/-/merge_requests/141

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg, aacid
Cc: simgunz, GB_2, aacid, ngraham, okular-devel, johnzh, andisa, 
siddharthmanthan, maguirre, fbampaloukas, joaonetto, kezik, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[Albert Astals Cid]

aacid requested changes to this revision.
aacid added a comment.
This revision now requires changes to proceed.


  Please move as a Merge Request in https://invent.kde.org/kde/okular
  
  We have pre-commit CI and lots of checks including clazy and clang-tidy there 
so it's a much better place for doing the review/approval/merge of the code.

REPOSITORY
  R223 Okular

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

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

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[Simone Gaiarin]

simgunz added inline comments.

INLINE COMMENTS

> davidhurka wrote in pageview.h:75
> In the meantime I learned more about this.
> 
> The Color Mode menu will stay here in PageView::setupViewerActions(), because 
> it is needed only in viewer modes, and is page view related.
> 
> m_embedMode is not needed, that’s handled by Part.
> 
> But I still don’t like the parameter part, any comments?

You can add an action in `Part::setupActions` similar to 
`options_configure_annotations`. Then in you can access it with something 
similar to

  QAction * aConfigColors = actionCollection()->action( 
QStringLiteral("options_configure_colors") );
  d->aColorModeMenu->addAction( aConfigColors );

REPOSITORY
  R223 Okular

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

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

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka updated this revision to Diff 60144.
davidhurka added a comment.


  - Set popup mode of Color Mode menu, because ToggleActionMenu respects it now.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21195?vs=60109&id=60144

BRANCH
  create-change-colors-menu

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

AFFECTED FILES
  conf/preferencesdialog.cpp
  conf/preferencesdialog.h
  part.cpp
  part.h
  ui/pageview.cpp
  ui/pageview.h

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, fbampaloukas, joaonetto, 
tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka edited the summary of this revision.
davidhurka added a dependency: D21755: [WIP] Replace ToolAction by a more 
universal “ToggleActionMenu”.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, fbampaloukas, joaonetto, 
tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

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

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, fbampaloukas, joaonetto, 
tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka marked an inline comment as done.
davidhurka added a comment.


  In D21195#477747 , @ngraham wrote:
  
  > I would recommend making the toolbar button show the text of the actual 
color change mode that's currently active, much like how the selection tools 
toolbar button currently does. Otherwise you won't know which mode it will 
invoke.
  
  
  Implemented that. How do you like it?

INLINE COMMENTS

> davidhurka wrote in pageview.h:75
> Adding a link to Part is neccessary, because Part controls m_embedMode, which 
> is used to access the configuration dialog.
> 
> However, the whole Change Colors menu is probably better in Part instead of 
> PageView. Currently, every PageView creates a new menu, and with multible 
> tabs their checked states get out of sync.
> 
> m_embedMode gives another question: should I check m_embedMode before 
> creating this menu? In the print preview mode, Change Colors does not make 
> much sense.

In the meantime I learned more about this.

The Color Mode menu will stay here in PageView::setupViewerActions(), because 
it is needed only in viewer modes, and is page view related.

m_embedMode is not needed, that’s handled by Part.

But I still don’t like the parameter part, any comments?

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, fbampaloukas, joaonetto, 
tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka updated this revision to Diff 60109.
davidhurka added a comment.


  - Fix rebase on D21755 

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21195?vs=60108&id=60109

BRANCH
  create-change-colors-menu

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

AFFECTED FILES
  conf/preferencesdialog.cpp
  conf/preferencesdialog.h
  part.cpp
  part.h
  ui/pageview.cpp
  ui/pageview.h

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, fbampaloukas, joaonetto, 
tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka updated this revision to Diff 60108.
davidhurka added a comment.


  - Rebase on D21755 , to use 
ToggleActionMenu.
  - Show the last used color mode on the toolbar button of the Color Mode menu. 
(Normal Colors is a radio option now, like the other color modes.)
  - Add color mode menu to updateActionState().

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21195?vs=58133&id=60108

BRANCH
  create-change-colors-menu

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

AFFECTED FILES
  CMakeLists.txt
  conf/preferencesdialog.cpp
  conf/preferencesdialog.h
  part.cpp
  part.h
  ui/pageview.cpp
  ui/pageview.h
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, fbampaloukas, joaonetto, 
tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka edited the summary of this revision.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, fbampaloukas, joaonetto, 
tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[Nathaniel Graham]

ngraham added a comment.


  I'm not sure about how it should be implemented, I just think the toolbar 
button should show the currently selected color changing option. :)

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka added a comment.


  In D21195#477747 , @ngraham wrote:
  
  > OK, got to test this out. It works well! I would recommend making the 
toolbar button show the text of the actual color change mode that's currently 
active, much like how the selection tools toolbar button currently does. 
Otherwise you won't know which mode it will invoke.
  
  
  Good idea. I’m currently trying to generalize CheckableActionMenu to use the 
same class for Selection tools. It also changes it’s default action when one of 
the actions is triggered.
  
  This implies that the last selected color mode action has to be unchecked. 
And that means that it will not be visible in the Color Mode menu, which color 
mode was used last, because (o) Do not Change Colors will be checked instead. 
In the toolbar button, it would be visible of course.
  
  Or what is your idea?
  
  Does not really work, because CMake thinks it has to rebuild eeeverything 
verytime, which takes about an hour...

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka edited the summary of this revision.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[Nathaniel Graham]

ngraham added a comment.


  OK, got to test this out. It works well! I would recommend making the toolbar 
button show the text of the actual color change mode that's currently active, 
much like how the selection tools toolbar button currently does. Otherwise you 
won't know which mode it will invoke.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka added a comment.


  So I will keep it this way.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka edited the summary of this revision.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[Nathaniel Graham]

ngraham added a comment.


  Their user interfaces are different; the selection button offers three 
different types of selections, but allows a click-and-don't-hold to activate 
the last one. I'm not the hugest fan of this UI; I would prefer three different 
buttons or a dropdown menu that opens on click-and-don't-hold. But there's no 
reason to emulate it since the UI you have here is different.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka added a comment.


  In D21195#468008 , @ngraham wrote:
  
  > +1 for this. I think it makes sense. Trying it out, I like it a lot.
  
  
  Sure?
  
  This isn’t even consistent with the Selection Tools button. They use an 
Okular::ToolButton, with delayed popup. (I don’t like this delay, don’t know 
the intention. Usually I replace the toolbar buttons with more useful ones, so 
I totally forgot about this button.) Should CheckableActionMenu be consistent 
with ToolButton and use delayed popup?
  
  Probably the approach of the Selection Tools button is cleaner, it separates 
menubar submenu and toolbar button, by adding the toolbar button as independent 
action to the KActionCollection. That would make CheckableActionMenu 
superfluous, KActionMenu would just work.
  
  I’ll make a TODO in the description.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[Nathaniel Graham]

ngraham added a comment.


  +1 for this. I think it makes sense. Trying it out, I like it a lot.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka added inline comments.

INLINE COMMENTS

> pageview.cpp:5705
>  
> +void PageView::slotSetChangeColorsMode( QAction * action )
> +{

Just discovered that the existing slots (slotToggleChangeColors(), 
slotSetChangeColors(bool)) are exposed to D-Bus through Okular::Part.

Probably, there should be another slot slotSetChangeColorsMode(QString or 
similar) to set the color mode from D-Bus. Exposing this slot to D-Bus makes 
little sense, because of the QAction* parameter.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka marked an inline comment as done.
davidhurka added a comment.


  I have figured out, what was going on in the QToolButton.
  
  KActionMenu::createWidget() added itself as default action. Then, 
CheckableActionMenu::createWidget() adds another action as default action. 
Then, the QToolButton has two actions, and builds a menu of them, instead of 
using the given menu.
  
  Now I remove the KActionMenu in CheckableActionMenu::createWidget(), so there 
is only my default action left. Then, QToolButton::setMenu() apparently works.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka updated this revision to Diff 58133.
davidhurka marked an inline comment as done.
davidhurka added a comment.


  - Toolbar button finally shows the intended menu
  - Add icon for Configure... action
  - Corrected action-slot connections
  - Named enable/disable action "Change Colors" again, looks better in the 
toolbar

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21195?vs=58129&id=58133

BRANCH
  create-change-colors-menu (branched from master)

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

AFFECTED FILES
  conf/preferencesdialog.cpp
  conf/preferencesdialog.h
  part.cpp
  part.h
  ui/pageview.cpp
  ui/pageview.h

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

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


  I have added CheckableActionMenu, which cuts the default action connection 
between the KActionMenu itself and the toolbar button. Now it is possible to 
make the toolbar button checkable, but not the submenu.
  
  But the toolbar button (QToolButton) pops up a menu which I don’t really 
understand. It is a menu consisting of the default action (ok so far) and the 
KActionMenu itself as submenu (not ok). F6824299: 
Screenshot_2019-05-15_15:03:40.png 
  
  I have one idea how this submenu gets there: KActionMenu::createWidget sets 
the KActionMenu as default action for the QToolButton.
  Now the popup menu works fine. As soon as CheckableActionMenu::createWidget 
sets a new default action, this menu becomes a submenu (?).
  
  QToolButton::setMenu() has no effect, and QToolButton::menu() always returns 
nullptr.
  
  So, default action of the toolbar button works, menu works not.
  
  Someone an idea how I can keep the correct menu?

INLINE COMMENTS

> pageview.cpp:761
> +a->setCheckable( true );
> +a->setData( int( id ) );
> +d->aColorModeMenu->addAction( a );

Can I use Okular::SettingsCore::EnumRenderMode::type somehow for the action 
data, without casting it to int?

> GB_2 wrote in pageview.cpp:672
> No, we always use three dots.
> This item should have the `configure` icon BTW.

Ok I’ll add that.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[Björn Feber]

GB_2 added inline comments.

INLINE COMMENTS

> davidhurka wrote in pageview.cpp:672
> Unicode ellipsis … instead of periods ... ?

No, we always use three dots.
This item should have the `configure` icon BTW.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: GB_2, davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka updated this revision to Diff 58129.
davidhurka added a comment.


  Removed checkbox from submenu, but toolbar button invents other menu structure

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21195?vs=58102&id=58129

BRANCH
  create-change-colors-menu (branched from master)

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

AFFECTED FILES
  conf/preferencesdialog.cpp
  conf/preferencesdialog.h
  part.cpp
  part.h
  ui/pageview.cpp
  ui/pageview.h

To: davidhurka, #okular, #vdg
Cc: davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka marked 4 inline comments as done.
davidhurka added a comment.


  The lambda looks much better, but it uses still `int` as action data, not 
`Okular::SettingsCore::EnumRenderMode::type`, which would make more sense. Is 
it possible to put `Okular::SettingsCore::EnumRenderMode::type` into the 
action’s QVariant? It didn’t work because I couldn’t register it as metatype.
  
  As soon as that works, I will submit lambdas for the other menus (see D21196 
).
  
  I tried to subclass KActionMenu to get the checkbox away. It works almost 
cleanly, but now QToolButton::showMenu() invents some new menu structures. I 
will try to understand what’s going on there.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka updated this revision to Diff 58102.
davidhurka added a comment.


  - Renamed the menu entries to Color Mode and Enable Color Changing
  - Use lambda function instead of function-like macro

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21195?vs=58026&id=58102

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

AFFECTED FILES
  conf/preferencesdialog.cpp
  conf/preferencesdialog.h
  part.cpp
  part.h
  ui/pageview.cpp
  ui/pageview.h

To: davidhurka, #okular, #vdg
Cc: davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> davidhurka wrote in pageview.cpp:637
> That is, I assign a lambda function to a locale `auto` variable, and “call” 
> the variable? (I’m new to lambda, so I will try that.)

Exactly!

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka added a comment.


  In D21195#464938 , @aacid wrote:
  
  > That menu is ultra hard to understand, it's a checked action + menu with an 
action with the same name inside that is also a checked action
  
  
  Maybe the names should be changed. How about:
  
Color Mode ->
[_] Enable Color Modification
(_) Invert Colors
(_) Change Paper Color
(_) Change Dark & Light Colors
(_) Convert to Black & White
  
  The checkbox on the submenu has to be removed, of course. It’s there because 
the KActionMenu is checkable to make the toolbar button toggle. But with a 
subclassed KActionMenu, this should be possible.
  
  > Also, why the double && ?
  
  Whoops, didn’t see that. With `&`, they became accelerators. With `&`, I 
got &.

INLINE COMMENTS

> davidre wrote in pageview.cpp:637
> You could use a lambda instead

That is, I assign a lambda function to a locale `auto` variable, and “call” the 
variable? (I’m new to lambda, so I will try that.)

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Redondo]

davidre added inline comments.

INLINE COMMENTS

> davidhurka wrote in pageview.cpp:637
> I want to avoid function-like macros. Can I just add a private method instead?

You could use a lambda instead

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: davidre, aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[Albert Astals Cid]

aacid added a comment.


  That menu is ultra hard to understand, it's a checked action + menu with an 
action with the same name inside that is also a checked action
  
  Also, why the double && ?

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: aacid, ngraham, okular-devel, joaonetto, tfella, darcyshen

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka added a comment.


  In D21195#464828 , @ngraham wrote:
  
  > Screenshots are appreciated when submitting patches that change or add UI 
elements. :)
  >
  > 
https://community.kde.org/Infrastructure/Phabricator#Include_some_screenshots
  
  
  You were too fast. Can I attach screenshots through `arc diff`?

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka added a comment.


  Okular does not follow the KDELibs coding style, so I tried to follow the 
existing coding style. Maybe I didn’t understand something right, if so, please 
correct me.
  
  Currently, all actions are enabled permanently. Should they be disabled if 
there is no document shown, like the View Mode menu?
  
  Additionally, it’s not possible to assign shortcuts, so this would break Bug 
407217 even more. :(
  When a shortcut is added via Configure Shortcuts, it only works once, then it 
disappears. After a restart of Okular, it works fine. What’s going on? How can 
I fix that?
  
  There are two actions "Change Colors" now in the toolbar configuration 
dialog, is that OK?
  F6821950: after_two_change_colors.png 
  
  If this menu should be inserted to the menubar, I suggest this position. 
Probably it’s bad to make a submenu checkable:
  F6821947: after.png 
  
  For more Request For Comment [RFC], see my inline comments.

INLINE COMMENTS

> pageview.cpp:616
> +// Change Colors action menu
> +d->aChangeColorsMenu = new KActionMenu( QIcon::fromTheme( 
> QStringLiteral("color-management") ), i18n( "Change Colors" ), this );
> +d->aChangeColorsMenu->setDelayed( false );

Which would be a good accelerator for “Change Colors”?

> pageview.cpp:637
> +QActionGroup * cmGroup = new QActionGroup( this );
> +#define ADD_COLORMODE_ACTION( action, name, id ) \
> +do { \

I want to avoid function-like macros. Can I just add a private method instead?

> pageview.cpp:672
> +d->aChangeColorsMenu->addSeparator();
> +QAction * aConfigure = new QAction( i18nc( "@item:inmenu color mode", 
> "C&onfigure..." ), this );
> +d->aChangeColorsMenu->addAction( aConfigure );

Unicode ellipsis … instead of periods ... ?

> pageview.cpp:5585
> +// Assigning a shortcut to a color mode only makes sense if it can 
> toggle the feature on and off.
> +slotToggleChangeColors();
> +}

This toggles Change Colors on and off, if the same shortcut e. g. for Invert 
Colors is triggered twice. But it also toggles if an already selected mode is 
clicked in the menu. Is that OK?

> pageview.h:75
>  void setupBaseActions( KActionCollection * collection );
> -void setupViewerActions( KActionCollection * collection );
> +void setupViewerActions( KActionCollection * collection, 
> Okular::Part * part );
>  void setupActions( KActionCollection * collection );

Adding a link to Part is neccessary, because Part controls m_embedMode, which 
is used to access the configuration dialog.

However, the whole Change Colors menu is probably better in Part instead of 
PageView. Currently, every PageView creates a new menu, and with multible tabs 
their checked states get out of sync.

m_embedMode gives another question: should I check m_embedMode before creating 
this menu? In the print preview mode, Change Colors does not make much sense.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka edited the summary of this revision.

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[Nathaniel Graham]

ngraham added a comment.


  Screenshots are appreciated when submitting patches that change or add UI 
elements. :)
  
  https://community.kde.org/Infrastructure/Phabricator#Include_some_screenshots

REPOSITORY
  R223 Okular

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

To: davidhurka, #okular, #vdg
Cc: ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid

D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka created this revision.
davidhurka added reviewers: Okular, VDG.
Herald added a project: Okular.
Herald added a subscriber: okular-devel.
davidhurka requested review of this revision.

REVISION SUMMARY
  This adds a new submenu to the View menu, which contains the existing Change 
Colors options.
  There is an action to toggle Change Colors on and off, and an action group to 
choose the Change Colors mode.
  The menu is an KActionMenu, so it can be plugged into the toolbar for quicker 
access.
  The KActionMenu is checkable, so toggling Change Colors just means to click 
the toolbar button once.
  
  This //might// be useful as soon as someone implements a useful Change Colors 
mode.

TEST PLAN
  - Look at menu in menubar, there should be View->Change Colors->...
  
  Then, plug the Change Colors menu into the toolbar:
  
  - Click it -> toggles
  - Open it -> choose another Change Colors mode
  
  Then, assign shortcuts
  
  - Shortcuts shouldn’t be removed after using them
  - Triggering the shortcut for e. g. Change Paper Color also can toggle Change 
Colors on and off

REPOSITORY
  R223 Okular

BRANCH
  create-change-colors-menu (branched from master)

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

AFFECTED FILES
  conf/preferencesdialog.cpp
  conf/preferencesdialog.h
  part.cpp
  part.h
  ui/pageview.cpp
  ui/pageview.h

To: davidhurka, #okular, #vdg
Cc: okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid