Add scheme selection menu with a "System" entry.
Needs ReviewPublic

Authored by amhndu on Sep 21 2018, 12:26 PM.

Details

Summary

The scheme selection menu does not offer any hint for the current system
scheme. This also makes it hard to "reset" back to the system scheme, once
the scheme has been changed.

Since, this breaks applications using the action name to save the selected
scheme, I've made a new function in which the action data has been changed from
only the path to the model index, this makes it possible to use the data to
differentiate if the selection action is the system scheme action or not.

Test Plan

Run kcolorschemedemo from tests and test the menu from the button.

Diff Detail

Repository
R265 KConfigWidgets
Branch
system-default
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4260
Build 4278: arc lint + arc unit
amhndu created this revision.Sep 21 2018, 12:26 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 21 2018, 12:26 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
amhndu requested review of this revision.Sep 21 2018, 12:26 PM
broulik added inline comments.
src/kcolorschememanager.cpp
243

This needs translation, I also think "System" isn't a very good name for this. How about "System Default" or just "Default"?

254–268

All of this is duplicated from`createSchemeSelectionMenu`, it should be split into a separate method so it can be reused

src/kcolorschememanager.h
127

WithDefaultEntry?

Should I also add the other overloads like createSchemeSelectionMenu does ?

src/kcolorschememanager.cpp
254–268

A private function in KColorSchemeModel or a static function in kcolorschememanager.cpp ?

src/kcolorschememanager.h
127

createSchemeSelectionMenuWithDefaultEntry ?
won't it be too big ?

amhndu updated this revision to Diff 42108.Sep 21 2018, 7:34 PM

Move duplicate code from the two createSelectionMenu methods into one
generic createSelectionMenu in KColorSchemeMangerPrivate

amhndu marked 2 inline comments as done.Sep 21 2018, 7:35 PM

Very much +1 on the end goal!

src/kcolorschememanager.h
127

I think it's okay. Pixels and characters are cheap. :)

shubham added inline comments.
src/kcolorschememanager.cpp
39

KActionMenu* -> KActionMenu *

amhndu updated this revision to Diff 42166.Sep 23 2018, 7:16 AM
amhndu marked 4 inline comments as done.

Updated with styling suggestions.

shubham removed a subscriber: shubham.Sep 23 2018, 8:58 AM
amhndu retitled this revision from [WIP] Add scheme selection menu with a "System" entry. to Add scheme selection menu with a "System" entry..Sep 27 2018, 6:17 PM
amhndu edited the summary of this revision. (Show Details)

Ping. Can someone please review ?

ngraham requested changes to this revision.Oct 26 2018, 11:14 PM

Doesn't build for me:

[ 51%] Building CXX object tests/CMakeFiles/kcodecactiontest.dir/kcodecactiontest.cpp.o
/home/dev/repos/kconfigwidgets/tests/kcolorschemedemo.cpp: In constructor ‘KColorSchemeDemo::KColorSchemeDemo()’:
/home/dev/repos/kconfigwidgets/tests/kcolorschemedemo.cpp:46:34: error: ‘class KColorSchemeManager’ has no member named ‘createSchemeSelectionMenuWithDefault’; did you mean ‘createSchemeSelectionMenuWithDefaultEntry’?
         button->setMenu(manager->createSchemeSelectionMenuWithDefault({}, {}, {}, button)->menu());
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                  createSchemeSelectionMenuWithDefaultEntry
[ 53%] Building CXX object tests/CMakeFiles/kcodecactiontest.dir/kcodecactiontest_autogen/mocs_compilation.cpp.o
tests/kcolorschemedemo.cpp
46–47

Needs to be createSchemeSelectionMenuWithDefaultEntry

This revision now requires changes to proceed.Oct 26 2018, 11:14 PM
amhndu updated this revision to Diff 44279.Oct 27 2018, 5:49 AM
amhndu edited the summary of this revision. (Show Details)

Fixed error in colorschemedemo test.
Sorry about that, I must've forgotton to commit it.

amhndu marked an inline comment as done.Oct 27 2018, 5:50 AM

Thanks, that lets it compile. I'm feeling a bit stupid right now, so could you help me figure out what's needed to test this? kcolorschemedemo.cpp does not actually seem to build an executable. Regardless, wouldn't it make sense to expose this new feature in the existing SettingsColor Theme menu? Or are you planning to do that in another patch?

pino requested changes to this revision.Oct 28 2018, 4:45 AM
pino added a subscriber: pino.
pino added inline comments.
src/kcolorschememanager.cpp
52
  • not the correct way to pass arguments to i18n -- they are extra parameters to the i18n function itself
  • even if %0 is supported, usually the first argument is %1
src/kcolorschememanager.h
127

instead of a custom variant, what about creating new versions of the createSchemeSelectionMenu functions that take flags instead?

This revision now requires changes to proceed.Oct 28 2018, 4:45 AM
amhndu updated this revision to Diff 44342.Oct 28 2018, 9:41 AM

kcolorschemedemo.cpp does not actually seem to build an executable.

It should build kcolorschemedemo in kconfigwidgets/bin (also has kcolorschemedemo target in make)

Regardless, wouldn't it make sense to expose this new feature in the existing Settings → Color Theme menu? Or are you planning to do that in another patch?

Yes, that would require a patch for each application that'd use it. The problem is that with the new menu,
we need a way to distinguish between the reset action and the other actions.
So that applications don't just save the current system scheme in their config, if they did,
the application would still use the previous scheme even after the user changed the system scheme. While resetting should mean the app
should now be following the scheme, just as before setting anything manually.
The new variant has the index as the data instead of the path, this allows checking the index's DisplayRole
and the action's text to distinguish the reset, this is admittedly not a very good solution.
I couldnnott think of anything else that's simple and wouldn't require much change in client code.
What could be an alternative solution here ?
I've thought of creating a subclass of KActionMenu which has two signals - one for reset and one for setting a scheme and returning that,
but that would again require modifying applications to use these signals instead.

instead of a custom variant, what about creating new versions of the createSchemeSelectionMenu functions that take flags instead?

I created a new variant since this isn't compatible with the other overload, as explained above.

amhndu marked an inline comment as done.Oct 28 2018, 9:41 AM

I'm sorry this got lost, @amhndu. Is this reviewable in its current state?

@amhndu are you interested in resuming work on this? If not, please let us know so we can find someone else to take it over. Thanks!

Thanks for picking this up again!
I'm definitely interested to resume but I've been a little busy this week. I can take a look again this weekend.

OK, sounds good!

Gentle ping. :)

amhndu added a comment.Jul 3 2019, 8:42 PM

Sorry about the delay, got caught up in things.

So here's the situation: Currently, the menu's actions store the scheme path as the data and applications that currently use this menu (eg. KDevelop and Kate), just store this scheme path in their config. So if we were to add another action whose data is the system's theme, on triggering this action, the path to the then current scheme will be saved and if the system theme is changed again, the application will continue use the old theme, going out of sync with the system. Thus, there needs to be a way to distinguish the reset action with the other actions. Since it's impossible to this without breaking the applications that use it, I made this as an entirely new function.
My (admittedly not a very good) solution to distinguish the actions were to store the index instead of of the scheme path. That way, you can use the index to get the name of the scheme and check if it matches the action's text.

I've thought of creating a subclass of KActionMenu which has two signals - one for reset and one for setting a scheme and returning that, but that seems a little too complicated.

Is there a simpler solution to this that I'm overlooking?