| broulik | |
| cfeck | |
| elvisangelaccio | |
| ngraham | |
| pino |
| Frameworks |
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.
Run kcolorschemedemo from tests and test the menu from the button.
| No Linters Available |
| No Unit Test Coverage |
| Buildable 3105 | |
| Build 3123: arc lint + arc unit |
| 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? | |
Move duplicate code from the two createSelectionMenu methods into one
generic createSelectionMenu in KColorSchemeMangerPrivate
Very much +1 on the end goal!
| src/kcolorschememanager.h | ||
|---|---|---|
| 127 | I think it's okay. Pixels and characters are cheap. :) | |
| src/kcolorschememanager.cpp | ||
|---|---|---|
| 39 | KActionMenu* -> KActionMenu * | |
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 | |
Fixed error in colorschemedemo test.
Sorry about that, I must've forgotton to commit it.
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 Settings → Color Theme menu? Or are you planning to do that in another patch?
| src/kcolorschememanager.cpp | ||
|---|---|---|
| 52 |
| |
| src/kcolorschememanager.h | ||
| 127 | instead of a custom variant, what about creating new versions of the createSchemeSelectionMenu functions that take flags instead? | |
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 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.
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?