[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

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.

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.

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:

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:

  • 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

Diff Detail

Repository
R223 Okular
Branch
create-change-colors-menu (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11860
Build 11878: 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
743

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

764

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

799

Unicode ellipsis … instead of periods ... ?

5711

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

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
764

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
764

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
764

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
799

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
762

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

799

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.Tue, May 21, 12:24 PM
ui/pageview.cpp
5705

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)Tue, May 21, 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)Mon, Jun 10, 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)Mon, Jun 17, 6:46 PM