[KColorschemeManager] Add option to reenable following global theme
ClosedPublic

Authored by davidre on Dec 11 2019, 11:08 AM.

Details

Summary

We need remove the titlebar hint property and simply go back to standard palette.

Test Plan

Select "System color scheme" in kcolorschemedemo/kate, change scheme in colors kcm


(Ignore the duplicated themes)

Diff Detail

Repository
R265 KConfigWidgets
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
davidre updated this revision to Diff 71355.Dec 12 2019, 12:28 PM

Really fix typo

Is there actually a way for this to see the name of the system color scheme?

Is there actually a way for this to see the name of the system color scheme?

I think so. We can certainly see the name of the system color scheme at startup. The tricky thing is when it changes but I will try to do it.

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.

davidre added a comment.EditedDec 13 2019, 2:11 PM

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.

ngraham accepted this revision.Dec 19 2019, 3:59 PM
This revision is now accepted and ready to land.Dec 19 2019, 3:59 PM
davidre planned changes to this revision.Sat, Dec 21, 9:18 PM

Changing the UserRole is not the best

Nevermind confused myself, I was not changing the UserRole data but the data of the actions of the menu. For that there are no guarantees documented.

davidre updated this revision to Diff 71974.Sat, Dec 21, 9:33 PM

remove comment

This revision is now accepted and ready to land.Sat, Dec 21, 9:33 PM
davidre updated this revision to Diff 71975.Sat, Dec 21, 9:35 PM

Remove stuff that was just for testing and included in the diff by mistake

davidre updated this revision to Diff 71976.Sat, Dec 21, 9:37 PM

remove qdebug

D15645 tried something similar from what I understood without looking at details, you might want to compare at least :) Sorry, no time myself to look at things currently beyond this comment.

D15645 tried something similar from what I understood without looking at details, you might want to compare at least :) Sorry, no time myself to look at things currently beyond this comment.

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.

I agree. If and when this lands, you could commandeer and close D15645

asemke added a subscriber: asemke.Wed, Dec 25, 9:52 AM
asemke added inline comments.
src/kcolorschememanager.cpp
109

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?

229–233

if (index.row() == 0) would also do the job and is simpler and faster.

src/kcolorschememanager.h
134

What should be the use-case for this new function and should it really be a slot?

ngraham added inline comments.Wed, Dec 25, 6:48 PM
src/kcolorschememanager.cpp
109

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

asemke added inline comments.Thu, Dec 26, 9:27 AM
src/kcolorschememanager.cpp
109

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

ngraham added inline comments.Thu, Dec 26, 3:13 PM
src/kcolorschememanager.cpp
109

Or this is menu is automatically created for KXmlGuiWindow applications...

That's what I was thinking of, yeah.

davidre updated this revision to Diff 72251.Fri, Dec 27, 2:29 PM
davidre marked 3 inline comments as done.
  • Use "Default" as string
  • index.row() == 0
davidre added inline comments.Fri, Dec 27, 2:29 PM
src/kcolorschememanager.h
135

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.

asemke added inline comments.Sat, Dec 28, 9:42 AM
src/kcolorschememanager.h
135

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.

davidre added inline comments.Sun, Dec 29, 1:59 PM
src/kcolorschememanager.h
135

I like that idea with an empty ModelIndex.

davidre added inline comments.Sun, Dec 29, 2:02 PM
src/kcolorschememanager.h
112

Would I need to add @since for a new overload?

ahmadsamir added inline comments.
src/kcolorschememanager.h
112

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:

The @since tag tells users since when the class has existed. It is usual to put a KDE release number here.

Method documentation: We can use @author and @since just like we do for classes.

ahmadsamir added inline comments.Sun, Dec 29, 2:49 PM
src/kcolorschememanager.h
112

s/wanted/wanted to know/ (phabricator doesn't let us edit inline comments.... weird).

kossebau added inline comments.Sun, Dec 29, 3:04 PM
src/kcolorschememanager.h
112

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

davidre updated this revision to Diff 72331.Sun, Dec 29, 3:51 PM
davidre marked 4 inline comments as done.
  • Remove function
  • Select system scheme on invalid model index
  • fix and expand comment
davidre marked 4 inline comments as done.Sun, Dec 29, 3:52 PM
davidre updated this revision to Diff 72334.Sun, Dec 29, 4:04 PM

Fix test

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,

src/kcolorschememanager.cpp
161

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.
// empty string is mapped to "reset to default/system", which is first item in model
That ensures the intention of this code is clear on high level.

161–165

Could start off at 1 now, no?

192–195

also could get a short comment what the highlevel logic is here
// no color theme selected? so it's the default one

225–227

Using // for each comment line is more typical for explanation comments, why the different style here?

src/kcolorschememanager.h
39–40

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.
Same with the existing methods where behaviour was changed.

davidre updated this revision to Diff 72337.Sun, Dec 29, 5:32 PM
davidre marked 5 inline comments as done.
  • note the version in which behavior was changed in the documentation
  • constexpr variable for default scheme row
  • more comments
davidre added inline comments.Sun, Dec 29, 5:33 PM
src/kcolorschememanager.cpp
227

Because I didn't see another multiline comment in this file to compare to ;)

asemke added inline comments.Mon, Dec 30, 10:18 AM
src/kcolorschememanager.cpp
225

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.

davidre updated this revision to Diff 72623.Thu, Jan 2, 3:32 PM
davidre marked an inline comment as done.

Default values

davidre added inline comments.Thu, Jan 2, 3:32 PM
src/kcolorschememanager.cpp
225

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.

asemke added inline comments.Sun, Jan 5, 9:38 AM
src/kcolorschememanager.cpp
225

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.

davidre updated this revision to Diff 73075.Wed, Jan 8, 4:50 PM

Also provide defaults for the other overloads

davidre updated this revision to Diff 73077.Wed, Jan 8, 5:01 PM

@since 5.67

davidre edited the summary of this revision. (Show Details)Wed, Jan 8, 5:04 PM
davidre edited the test plan for this revision. (Show Details)
davidre updated this revision to Diff 73079.Wed, Jan 8, 5:05 PM

Forgot to save file

davidre edited the summary of this revision. (Show Details)Wed, Jan 8, 5:29 PM
This revision was automatically updated to reflect the committed changes.