re-read color palettes when application color changes
ClosedPublic

Authored by sebas on Fri, Jun 7, 11:38 AM.

Details

Summary

Without this patch, changing the application color scheme from system
settings only affects some widgets. Notably, checkboxes highlighting
colors stays the old color, leading to a hodge-podge color scheme and
bad contrast on some items.

The breeze QStyle caches the colors read via KSharedConfig, so it needs
to re-read the configuration when the application color changes.
QApplication emits a signal (originating in KGlobalSettings), which we
can react to.

This fixes the coloring of various widgets in breeze right after color
changes.

BUG:408416
FIXED-IN: 5.16

Those I haven't tested, but look quite suspicious (so please re-test):
CCBUG:382505
CCBUG:355295

Test Plan
  1. open kcmshell5 colors
  2. change to a theme with a different highlight color
  3. apply
  4. without patch: checkbox in color KCM (and a whole lot of other widgets all over the place) don't change colors
  5. with patch: colors change as expected

Diff Detail

Repository
R31 Breeze
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sebas created this revision.Fri, Jun 7, 11:38 AM
Restricted Application added a project: Plasma. · View Herald TranscriptFri, Jun 7, 11:38 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sebas requested review of this revision.Fri, Jun 7, 11:38 AM
zzag added a subscriber: zzag.Fri, Jun 7, 11:54 AM

A bit noob-ish question: can we listen to paletteChanged signal instead? e.g.

connect(qApp, &QGuiApplication::paletteChanged, this, &Style::configurationChanged);
broulik added inline comments.
kstyle/breezestyle.h
163 ↗(On Diff #59330)

Try connecting to QGuiApplication::paletteChanged. I added that signal specifically so that you avoid having to install an event filter on QApplication. (This codepath is still likely neccessary for Qt 4 compat). Also, I cannot reproduce the issue described.

sebas updated this revision to Diff 59332.Fri, Jun 7, 12:16 PM
  • use QApplication::paletteChanged as Kai suggests
broulik added inline comments.Fri, Jun 7, 12:16 PM
kstyle/breezestyle.cpp
201

Wrap in #if !BREEZE_USE_KDE4

sebas updated this revision to Diff 59333.Fri, Jun 7, 12:18 PM
  • Qt4 compat
cfeck added a subscriber: cfeck.Fri, Jun 7, 12:20 PM

There will be no further 5.15 release. Please set to 5.16 branch (and merge to master when ready).

sebas edited the summary of this revision. (Show Details)Fri, Jun 7, 12:21 PM

Done (assuming editing the phab description is enough).

sebas edited the summary of this revision. (Show Details)Fri, Jun 7, 12:23 PM
broulik accepted this revision.Fri, Jun 7, 1:17 PM

You need to run arc amend to reflect the change to the commit message in your local checkout

This revision is now accepted and ready to land.Fri, Jun 7, 1:17 PM
sitter accepted this revision.Fri, Jun 7, 1:19 PM
sitter added a subscriber: sitter.

LGTM. At a glance breezehelper caches values (loadConfig) and brezestyleconfigdata may as well, so this certainly seems sound. Specifically _viewFocusBrush = KStatefulBrush( KColorScheme::View, KColorScheme::FocusColor, _config ); seems to be used to influence the focus color of checkboxes and that _config is in fact a StyleConfigData. So, this definitely needs a reload.

Is integration of qt4 software still a concern? If so, perhaps the colors KCM should also send the reparseConfiguration signal, seeing as that would cause a double reload on Qt5 we probably don't want that though.

sebas added a comment.Fri, Jun 7, 1:33 PM

@sitter here's a review sending the reparseConfiguration signal from the Colors KCM.

Thanks for the review, all!

This revision was automatically updated to reflect the committed changes.