[RFC] Create a Change Colors menu (with toolbar button)
Needs ReviewPublic

Authored by davidhurka on May 13 2019, 5:47 PM.

Details

Reviewers
None
Group Reviewers
Okular
VDG
Summary

Depends on D21755.

This adds a new submenu to the View menu, which contains the existing Change Colors options.
There are menu items for every color mode, and one Normal Colors.
The menu is an ToggleActionMenu, so it can be plugged into the toolbar for quicker access.
The toolbar button always shows the last used color mode, so it can be enabled or disabled with one click.

This might be useful as soon as someone implements a useful Change Colors mode.

D21196 is related to this, because it gives the View Mode menu a similar usage pattern.

This also fixes 407217, because the button does toggle now. Additionally, every color mode can get a shortcut.

BUG: 407217

Screenshots from before can be found in D21196, or from the bug report: https://bugsfiles.kde.org/attachment.cgi?id=119841

Screenshot after (Left with Invert Colors and menubar menu, right with Normal Colors, and with/out toolbar button menu):

TODO

  • Actions are not syncronized across tabs. Implement this feature in Part or PageView or where? See D21195#118908 (Normal in Okular)
  • Assigning shortcuts doesn’t work. See D21195#464866 (Normal in Okular)
  • Remove CheckableActionMenu by just adding the KActionMenu indepently to the KActionCollection, like done with the ‘Selection Tools’ button? See D21195#468175 (replaced by following)
  • Use ToggleActionMenu instead of CheckableActionMenu.
  • Show the current color mode in the toolbar button.
  • Find Icons for ‘Change Dark & Light Colors’ and ‘Change Paper Color’
  • Expose to D-Bus. See D21195#inline-119670
  • Get the correct ‘&’ into the action text, not ‘&&’. See D21195#464938
  • Cast to int or not? See D21195#inline-119107

Future

  • Add ‘Invert Lightness’ color mode?
Test Plan
  • Look at menu in menubar, there should be View->Change Colors->...

Then, plug the Change Colors menu into the toolbar:

  • Shows last used color mode.
  • Click it -> toggles
  • Open its menu -> choose another color mode

Then, assign shortcuts

  • Triggering a shortcut updates the toolbar button.

Diff Detail

Repository
R223 Okular
Branch
create-change-colors-menu
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13026
Build 13044: arc lint + arc unit
davidhurka created this revision.May 13 2019, 5:47 PM
Restricted Application added a project: Okular. · View Herald TranscriptMay 13 2019, 5:47 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
davidhurka requested review of this revision.May 13 2019, 5:47 PM

Screenshots are appreciated when submitting patches that change or add UI elements. :)

https://community.kde.org/Infrastructure/Phabricator#Include_some_screenshots

davidhurka edited the summary of this revision. (Show Details)May 13 2019, 6:11 PM

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?

If this menu should be inserted to the menubar, I suggest this position. Probably it’s bad to make a submenu checkable:

For more Request For Comment [RFC], see my inline comments.

ui/pageview.cpp
616

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

637

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

672

Unicode ellipsis … instead of periods ... ?

5598

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?

ui/pageview.h
72

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.

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?

aacid added a subscriber: aacid.May 13 2019, 9:04 PM

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

davidre added inline comments.
ui/pageview.cpp
637

You could use a lambda instead

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

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

davidre added inline comments.May 14 2019, 7:22 AM
ui/pageview.cpp
637

Exactly!

  • Renamed the menu entries to Color Mode and Enable Color Changing
  • Use lambda function instead of function-like macro
davidhurka marked 4 inline comments as done.May 14 2019, 11:51 PM

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.

davidhurka updated this revision to Diff 58129.May 15 2019, 2:53 PM

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

GB_2 added a subscriber: GB_2.May 15 2019, 3:00 PM
GB_2 added inline comments.
ui/pageview.cpp
672

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

davidhurka marked 2 inline comments as done.May 15 2019, 3:16 PM

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

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?

ui/pageview.cpp
635

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

672

Ok I’ll add that.

davidhurka updated this revision to Diff 58133.May 15 2019, 4:09 PM
davidhurka marked an inline comment as done.
  • 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
davidhurka marked an inline comment as done.May 15 2019, 4:38 PM

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.

davidhurka added inline comments.May 21 2019, 12:24 PM
ui/pageview.cpp
5592

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.

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

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

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.

davidhurka edited the summary of this revision. (Show Details)May 21 2019, 10:16 PM

So I will keep it this way.

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.

davidhurka edited the summary of this revision. (Show Details)Jun 10 2019, 9:58 PM

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 eeeeeeeverything eeeeeeeeverytime, which takes about an hour...

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

davidhurka edited the summary of this revision. (Show Details)Jun 17 2019, 6:46 PM
  • 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().
davidhurka marked an inline comment as done.Jun 20 2019, 11:01 AM

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?

ui/pageview.h
72

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?

davidhurka edited the summary of this revision. (Show Details)Jun 20 2019, 11:26 AM
davidhurka edited the test plan for this revision. (Show Details)
davidhurka updated this revision to Diff 60144.Jun 20 2019, 3:45 PM
  • Set popup mode of Color Mode menu, because ToggleActionMenu respects it now.
davidre removed a subscriber: davidre.Jun 20 2019, 3:48 PM