Move KColorScheme to KConfig
Open, HighPublic

Description

Currently in the slightly surprising location in KConfigWidgets (tier 3), and used in many many places.

vkrause created this task.Sep 10 2019, 1:11 PM

KColorScheme uses KConfig and KColorUtils, and so, IIUC, it could be a tier2 but not a tier1.

Right, the KConfig dependency is the problem. KColorUtils is in KGuiAddons, which IMHO would be the natural location for KColorScheme anyway. So the question is whether we can implement KColorScheme without KConfig I guess. Possible ideas:

  • Can we get away with using QSettings rather than KConfig for this, assuming KColorScheme only reads and thus might not need features like immutable entries.
  • How does Kirigami.Theme solve this? That is tier 1 and essentially provides the same features as KColorScheme.

It looks like KColorScheme provides more features, like color intesity and tint... etc. IINM, this is used when editing a color scheme (on the "Disabled" tab). Something similar was used in the icons kcm, i.e. you could change the hover effect of icons, i.e. more dark/gamma/tint... I looked just now (I haven't used that feature in ages) but it seems to be gone from the new kcm which was rewritten in QtQuick, probably dropped on the basis no one actually used it :)

About QSettings, I don't know off-hand.

ahmadsamir added a comment.EditedOct 16 2019, 7:04 PM

Some of the KColorScheme methods have a "config" object parameter, if that parameter is null, it uses a local function, defaultConfig(), to get a default "config" object to work with.

defaultConfig() uses KSharedConfig::openConfig(), so IIUC it's not about just reading values from a configuration store.

If there's unused/no longer reachable stuff in there we should probably be looking at removing that. More possible ideas:

  • Split commonly used read access and the features needed for the configuration KCM, possibly even moving the latter into the KCM if that's the only place needing this.
  • Can we make KColorScheme less needed by applications by adding more colors to QPalette itself? Using a tier 3 framework for more advanced use-cases is far less of a problem than for just getting the warning or error color for example.

[...]

  • Can we make KColorScheme less needed by applications by adding more colors to QPalette itself? Using a tier 3 framework for more advanced use-cases is far less of a problem than for just getting the warning or error color for example.

As far as I understand:

  • QGuiApplication by default uses the color palette of the style
  • KDE has kstyle (from frameworkintegration), which has a standardPalette()[1] method that calls KColorScheme which creates a QPalette and sets the colors, for the various color groups/roles, for it by getting the actual color scheme set in the system/user's settings, which means KColorScheme needs to read the config.

So KColorScheme is needed by kstyle, and the frameworkintegration (the latter is tier 4), and it's used by all apps running under Plasma/KDE.

[1] https://cgit.kde.org/frameworkintegration.git/tree/src/kstyle/kstyle.cpp#n294

I was talking nonsense, Kstyle::standardPalette() passes the config object to kcolorscheme. I need to look more closely at how applications use KColorScheme, it's a lot of bits in different places, I still need to wrap my head around all of the stuff that handle theming in KDE/Plasma... :/

Going back to your original idea, it looks like QSettings can be used to read the values, (and if need be implement the logic KSharedConfig::openConfig() has). So KcolorScheme can go with its sisters in kGuiAddons (it does look like KColorScheme and KColorUtils and co. were in the same place in the monolithic kdelibs era).

I am experimenting with using QSettings in KColorScheme, so if it works:

  • Add new overloads that take a QString configFilename to construct a QSettings from
  • Deprecate the old ctor and port KColorScheme usage throughout the KDE code (not all KColorScheme usage is using the ctor, some instances use some Enum value from KColorScheme).
cfeck added a subscriber: cfeck.Nov 4 2019, 2:17 AM

Does QSettings support cascading config files? Right now, application config files can override colors over kdeglobals to allow per-application color schemes.

ndavis added a subscriber: ndavis.Nov 10 2019, 8:21 AM

Does QSettings support cascading config files? Right now, application config files can override colors over kdeglobals to allow per-application color schemes.

I am awfully sorry for the delay, this got buried in my inbox :/

First a quick update, I think it _is_ possible to use QSettings instead of KConfig, I've been experimenting with it locally (yes, I am quite slow...), and it seems to work o/; I should have a diff soon, which still has a few wrinkles I can't seem to iron yet :).

Also upstream Qt uses similar methods[1] for the platformsupport stuff; I borrowed some code from there.

The per-application color scheme is handled by defaultConfig()[2], basically it uses KDE_COLOR_SCHEME_PATH qApp property set by KColorSchemeManager:

KSharedConfigPtr defaultConfig() {
    // cache the value we'll return, since usually it's going to be the same value
    static thread_local KSharedConfigPtr config;
    // Read from the application's color scheme file (as set by KColorSchemeManager).
    // If unset, this is equivalent to openConfig() and the system scheme is used.
    const auto colorSchemePath = qApp->property("KDE_COLOR_SCHEME_PATH").toString();
    if (!config || config->name() != colorSchemePath) {
        config = KSharedConfig::openConfig(colorSchemePath);
    }
    return config;
}

so the cascading of the config files from KConfig shouldn't be an issue, IIUC.

The color scheme settings are a special case since there are only two places where we'd need to read them from, either a color scheme file set by kcolorschememanager or from kdeglobals and IIUC KConfig[3] searches for a kdeglobals or systemd.kdeglobals. So for example, the "appnamerc" config file doesn't/shouldn't have the color scheme settings AFAICS. So this simplifies matters a bit.

[1] https://code.qt.io/cgit/qt/qtbase.git/tree/src/platformsupport/themes/genericunix/qgenericunixthemes.cpp
[2] https://cgit.kde.org/kconfigwidgets.git/tree/src/kcolorscheme.cpp#n254
[3] https://api.kde.org/frameworks/kconfig/html/kconfig_8cpp_source.html#l00679

Code that uses kcolorscheme can be split into two categories:
a - not passing a config object param, which basically let's kcolorscheme use whatever the system defaults are
b - passing a config object param, which kcolorscheme uses to get the colors it needs

For case a, to use qsettings we'd simply have to look for the config files in the installed system ourselves, this is a bit complicated because we have:
~/.config/kdeglobals
/etc/kdeglobals
~/.config/kdeglobals.system
/etc/kdeglobals.system
/etc/kde5rc

which as Christoph pointed out has its own hierarchy/cascading/overriding, and add over that the kiosk stuff which makes some settings immutable.

For case b, apps would have to either use the config object to get the relevant config file (or use say QStandardPaths locatAll() to get it) then either pass it to kcolorscheme or construct a qsettings object and then pass that to kcolorscheme.

I think for both cases the caching that kconfig does is lost; almost all kde apps use kconfig/ksharedconfig, so the chances are quite high the config file that we need to read is already cached, primed and rearing to go.

And I am not sure if using QSettings between kstyle/apps and kcolorscheme won't have an overhead.

There is interest in Plasma to indicate the current color scheme only with it's name in kdeglobals in the future. Currently we need to have complicated logic in multiple places because in addition to colors and names in kdeglobals, we need to factor a look and feel (and default look and feel!) into it. Also some users rely on actual colors being in kdeglobals which means we depend currently on kde4breeze update script which does this for kcolorscheme to work correctly (see https://bugs.kde.org/show_bug.cgi?id=428771).
This would remove the need for cascading the single colors. Apps could still overwrite colors by overwriting the current scheme name.

To get the values of a specific scheme, you would pass the filename or file.

To get default colors (current active scheme):

  • We need kconfig style cascading to read the current active scheme name or
  • You always have to pass a filename so it's on the application to read the currently active on via kconfig or
  • we find another way how the current colorscheme is set

Does this have a (actionable) conclusion? Or does it need more discussion, e.g. in our Saturday meetings?

Does this have a (actionable) conclusion? Or does it need more discussion, e.g. in our Saturday meetings?

More discussion... :); I'll move it to needs input.

ahmadsamir moved this task from Backlog to Needs Input on the KF6 board.Apr 6 2021, 12:23 PM
dfaure added a subscriber: dfaure.Jun 24 2021, 9:37 AM

@cfeck "Right now, application config files can override colors over kdeglobals to allow per-application color schemes." << technically yes, but there's no GUI for that, so do we really need that feature?

davidre added a comment.EditedJun 24 2021, 10:50 AM

So the idea is

  • don't export colors to kdeglobals, only save current colorscheme
  • because we only need to read one value, we don't need any actual cascading

The overriding a single color user case is not very spread (I would bet actually not all), and altering the QPallette is done and suffices for most cases.

Which means we don't need KConfig and can read with Qsettings

Deprecating KStatefulBrush::brush(const QWidget*) const would make KColorScheme widget-free it seems, and fortunately that is both easy to replace (via KStatefulBrush::brush(const QPalette&) const) and apparently unused in at least Frameworks itself.

https://invent.kde.org/frameworks/kconfigwidgets/-/merge_requests/56 removes the widget dependency, and there's a number of MRs already to adapt PIM to that.

Since QSettings doesn't work ootb see https://mail.kde.org/pipermail/kde-frameworks-devel/2021-December/120057.html
what about

  • moving KColorScheme and KColorSchemeModel to KConfigGui
  • KColorSchemeManger stays

Another topic would be potential API changes for kcolorscheme with not saving colors to kdeglobals/not cascading colors. This is independent from the final location.
I propose:
a) remove KSharedConfig from the constructor instead take a QString, if it's empty return the default colorscheme, relative will try to look it up, and absolute use that file
b) do the same for all the static methods taking KSharedConfig, alternatively make them member functions, especially if they internally create a KColorScheme
c) internally use KConfig::Simple
d) deprecate contrast and contrastF() and make it a member Function, current design of those is a bit weird, also note the differing return types

a), b) and d) can be done now, c) is a behavior change but that we can hide behind the new constructor

Consensus at the meeting was to move it to KConfig

nicolasfella renamed this task from Investigate making KColorScheme tier1 to Move KColorScheme to KConfig.Feb 18 2023, 12:49 AM
nicolasfella moved this task from Needs Input to Backlog on the KF6 board.
nicolasfella triaged this task as High priority.Feb 22 2023, 11:58 AM
nicolasfella moved this task from Backlog to Done on the KF6 board.May 9 2023, 9:47 AM