We need remove the titlebar hint property and simply go back to standard palette.
- Group Reviewers
- Maniphest Tasks
- T12147: KConfigWidgets
- R265:a9e1079eba40: [KColorschemeManager] Add option to reenable following global theme
Select "System color scheme" in kcolorschemedemo/kate, change scheme in colors kcm
(Ignore the duplicated themes)
Then maybe we could even do it like so:
Instead of having a separate "use system default" option, we could make the color scheme that has the same name as the default into the "use system default option, and present it thusly in the menu:
( ) Arc ( ) Arc Dark (o) Breeze (System default) ( ) Breeze Dark
But isn't that going away from the problem that this is trying to solve? To have an option to go back from a fixed colorscheme to the system one.
Also I'm sure it can be done I just need to figure out how stuff works :D
Also if then the color scheme is changed while the application is running it would display "Breeze (system default)" checked, breeze dark is unchecked but the app is dark because breeze dark is the global color scheme.
I think it should be possible to use KConfigWatcher to monitor the global theme.
Yeah, I guess my idea precludes the user forcing a specific color scheme that happens to be equal to the current system default color scheme.
If that's something we think we should support, then feel free to ignore me.
So KConfigWatcher didn't work because the KCM doesn't write with the notify flag. Even if we did we can't depend on a specific version of plasma. Thinking a bit more about this:
This is in frameworks and getting the name probably works only on plasma byreading kdeglobals and listening for the dbus change signal. This means I need to find out if we are on plasma and not on some other desktop. (Maybe it has it's own color scheme support and reading kdeglobals (that still could exist) is wrong),
We set the KDE_COLOR_SCHEME_PATH for plasma-integration. Another idea would be that plasma-integration sets a property that would tell us the default color scheme. Then we don't need to care if we are on plasma we just check if this property exists and is valid.
For now I don't think it would be a showstopper if we can't display the name of the current default theme.
Thanks for the pointer! Looking at that diff what it tried to do was reading the currently active color scheme from kdeglobals. That approach has two problems imo. First it only is correct on Plasma, secondly it sets the wrong scheme when the global color scheme changes. To fix the second issue one could listen to the settings changed signal on DBus but that also only works on Plasma. Just using the standardPalette() is much simpler and more reliable.
Many applications like kdevelop, digikam, labplot, etc. create a menu "Color Scheme" in the main menu bar and add then menu item via KColorSchemeManager. By using "System color scheme" here we'd have "Color Scheme" -> "System color scheme" with this repeated "color scheme" string. Can we simply use "Default" or "System" or "Desktop" here?
if (index.row() == 0) would also do the job and is simpler and faster.
What should be the use-case for this new function and should it really be a slot?
"Default" is probably fine.
FWIW the parent menu item is actually mis-named, at least in Kate. It's called "Color Theme" when it should be "Color Scheme"
Also this menu should be universal, and not re-implemented on a per-app basis.
Yes, was also wrong in LabPlot. Just corrected https://invent.kde.org/kde/labplot/commit/262f37b59193ed88bf680b155dc6ecd37bd11419.
We should have maybe in this class only one public function KActionMenu *createSchemeSelectionMenu(QObject *parent) which internally sets the string to "Color Scheme" and the icon to "preferences-desktop-color". With this we'd enforce a consistent look. Or this is menu is automatically created for KXmlGuiWindow applications...
I thought it would be nice to have as a convenience function if an application has changed the scheme to easily go back to the system color scheme. I don't know if it should be a slot. I put it there because activateScheme is here.
I think the only communication channel with KColorSchemeManager is via the menu created in createSchemeSelectionMenu(). Setting back the default scheme will also be done via this menu. I don't see why an application would need such a helper function - there is simply no other handling of the color schemes in the applications except of that menu.
Also, if somebody would still need such a helper function, he/she could use activateScheme(QModelIndex()) instead of followSystemScheme(). activateScheme() exists already and an empty QModelIndex() could be interpreted as the default color scheme.
I would say yes (your question made me search for an answer since I wanted too:)), from https://community.kde.org/Guidelines_and_HOWTOs/API_Documentation:
"Would I need"... put yourself in the shoes of a user of this class: you would want to know at which version this new overload is available when using it in your code which sets itself a certain version of KF as minimal expected, right? :)
So yes, you want to help the users of the API to know at which versions of KF they can expect which methods and classes to be present. And thus you want to ensure "@since" with any API changes, incl. new methods added, overloaded or not.
Some comments while flying over the code due to being triggered by phab message, but no in-detail review and analysis of approach done, lack of concentration currently, sorry.
I also propose to change all magic number 0s to use some defaultThemeRow from a constexpr int defaultThemeRow = 0: instead,
Propose to add a short comment explaining why we return index(0) here for the future code reader starting off at this code before having read all code and docs. E.g.
Could start off at 1 now, no?
also could get a short comment what the highlevel logic is here
Using // for each comment line is more typical for explanation comments, why the different style here?
Here you might also want to state at which version this feature of "allows going back" was added, to make it clear that with older versions of KF this was not possible.
why not to use reasonable default values here like QIcon::fromTheme(QStringLiteral("preferences-desktop-color") and i18n("Color Scheme")? This function should be used then by all applications and this will help to get a more consistent behavior across the different applications.
Should we also do this for the other overloads? Or would that behavior change be a blocker? ALso an application would want to call KColorSchemeManager::createSchemeSelectionMenu(const QString &selectedSchemeName, QObject *parent) probably if the custom scheme is saved between launches.
I'm not the author of this code but I don't see why this should be a blocker. With this you wouldn't break anything and would simply add more consistency across different applications.