Update GTK settings according to Plasma settings
ClosedPublic

Authored by gikari on Oct 17 2019, 6:33 PM.

Details

Summary

To increase usability,

  • fonts
  • icon theme
  • cursor theme
  • toolbar style (icons-only, text below buttons etc)
  • icons visibility in menus
  • icons visibility on buttons

settings for gtk applications are now set in respective kcms, instead of separate gtk kcm.

Various kcms are sending signals (dbus or Qt ones) about the configuration change. Then the gtkconfig kded daemon connects to these signals and changes gtk config files to match new settings, that were set in those kcms.

D24701 is needed to notify about cursor change on Wayland.

Live reloading (on a fly) is not going to work for some settings on X11. This applies to cursor theme, it's changed by partially. To illustrate that open Nautilus File Manager, change cursor theme and try to double click folders. You will see, that "loading" cursor changed its theme, but "idle" one did not.

What is not tested:

  • Toolbar style for gtk3 apps other that Geany (absence of live reloading may be a Geany bug)
  • Toolbar style and icons visibility on Wayland

Depends on D24701

BUG: 401507

BUG: 375272

BUG: 411097

FIXED-IN: 5.18.0

Test Plan
  1. To test gtk3 applications live reloading on X11, install xsettingsd.
  2. Restart kded5.
  3. Open gtk2 app, gtk3 app
  4. Change above mentioned settings in respective KCMs
  5. Check if gtk applications are changing their settings before (sometimes) and after their restart.

Rare cases:

  • To test icons on buttons use Inkscape about window.
  • To test toolbar style use Geany (gtk3, live reloading does not work) and BleachBit (gtk2)

Diff Detail

Repository
R99 KDE Gtk Configuration Tool
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
broulik added inline comments.Oct 18 2019, 6:53 AM
kded-module/configvalueprovider.cpp
53 ↗(On Diff #68187)

Pass true as second argument (which is the default), then it will return you a bool directly.

79 ↗(On Diff #68187)

Default value TextBesideIcon

kded-module/gtkconfig.cpp
37 ↗(On Diff #68187)

One variable per line, please

47 ↗(On Diff #68187)

Should this initially apply all the settings?

110 ↗(On Diff #68187)

You'll have to copy those enum values in since KGlobalSettings is deprecated, sorry.

116 ↗(On Diff #68187)

In what way? There's a comment in the style KCM saying

// ##### FIXME - Doesn't apply all settings correctly due to bugs in KApplication/KToolbar
KGlobalSettings::self()->emitChange(KGlobalSettings::ToolbarStyleChanged);
kded-module/gtkconfig.h
24 ↗(On Diff #68187)

Unused

33 ↗(On Diff #68187)

Unused

Nice work! So what's our update story here? If a user has set something from the GTK KCM that's not the same as the data set by the applicable other KCM, who wins, and what happens when said user upgrades to 5.18 (which is presumably the release that will have this)?

Nice work! So what's our update story here? If a user has set something from the GTK KCM that's not the same as the data set by the applicable other KCM, who wins, and what happens when said user upgrades to 5.18 (which is presumably the release that will have this)?

There is a patch, that removes functionality from the gtk kcm: D24744

Without it the gtk kcm will override settings that was set from the daemon. But if the user sets something from plasma kcms again, the gtk style applied by gtk kcm will be overridden (but not all the settings, just the ones that was changed by plasma kcm).

gikari updated this revision to Diff 68256.Oct 18 2019, 4:23 PM
gikari marked 16 inline comments as done.

Fix most of the issues brought by @broulik

gikari planned changes to this revision.Oct 18 2019, 4:32 PM
gikari updated this revision to Diff 68258.Oct 18 2019, 4:58 PM

Fix pidof launching and remove static from the regexp's

gikari added inline comments.Oct 18 2019, 5:08 PM
kded-module/gtkconfig.cpp
116 ↗(On Diff #68187)

@broulik signal from KGlobalSettings is never recieved with KGlobalSettings::ToolbarStyleChanged value as an argument. I debuged style kcm and daemon simultaneously and after stepping over the above line and the previous one which is

KGlobalSettings::self()->emitChange(KGlobalSettings::SettingsChanged, KGlobalSettings::SETTINGS_STYLE);

Only signal with SettingsChanged is received within the daemon.

gikari planned changes to this revision.Oct 18 2019, 5:21 PM
gikari updated this revision to Diff 68260.Oct 18 2019, 6:04 PM

Fix daemon hangup on font change

gikari edited the summary of this revision. (Show Details)Oct 30 2019, 8:38 AM
ervin added a subscriber: ervin.Oct 30 2019, 6:41 PM
ervin added inline comments.
kded-module/configeditor.cpp
49 ↗(On Diff #68260)

Better wrap the const char* in a QStringLiteral.

Also, but more of a nitpick this time: style wise I'd favor using = for those initialization (and others in the file) than parenthesis. It gets too close the most vexing parse territory, I'd rather not potentially expose a future developer touching that file to it.

81 ↗(On Diff #68260)

QStringLiteral please.

96 ↗(On Diff #68260)

QString() or {} instead of ""

102 ↗(On Diff #68260)

QStringLiteral please

There are more of those in the file, I'll stop pointing them out. ;-)

kded-module/configeditor.h
45 ↗(On Diff #68260)

We don't prefix getters with get.

kded-module/configvalueprovider.cpp
35 ↗(On Diff #68260)

I will contradict my previous comment about making those functions static or turning into a namespace. I think it'd be better to avoid creating and ditching those KSharedConfig instances at each call, so why not instantiate it when ConfigValueProvider is created?

You might have to load() from the config at right point in times though.

kded-module/configvalueprovider.h
37 ↗(On Diff #68260)

We don't prefix getters with get

Beside none of those have any state AFAICT, why not make them static or turn this into a namespace?

gikari added inline comments.Oct 31 2019, 6:46 PM
kded-module/configeditor.cpp
49 ↗(On Diff #68260)

Where generally should I use equals sign (=), where parenthesis (()) and where braces ({}) for initialization? I didn't find any note about that in coding style.

ngraham added inline comments.Oct 31 2019, 6:48 PM
kded-module/configeditor.cpp
49 ↗(On Diff #68260)

I believe in general use equals, but you can use braces for lists.

gikari updated this revision to Diff 69133.Oct 31 2019, 7:18 PM
gikari marked 8 inline comments as done.
gikari edited the summary of this revision. (Show Details)
  • Replace () initialization with = one
  • Embrace QStringLiteral madness even more
  • Rename some variables and method names
gikari marked 2 inline comments as done.Oct 31 2019, 7:19 PM
gikari updated this revision to Diff 69141.Oct 31 2019, 9:27 PM
  • Transform ConfigEditor to namespace
  • Make KSharedConfigPtrs of kdeglobals and input configs the ConfigValueProvider members
gikari marked an inline comment as done.Oct 31 2019, 9:28 PM
ervin added a comment.Nov 4 2019, 11:05 AM

Just a minor thing, otherwise LGTM, I'll let Kai give the last stamp of approval.

kded-module/gtkconfig.cpp
113 ↗(On Diff #69141)

Are you sure you want to call setFont() twice here? That looks surprising.

gikari updated this revision to Diff 69259.Nov 4 2019, 11:09 AM
gikari marked an inline comment as done.

Fix the typo

gikari edited the summary of this revision. (Show Details)Nov 7 2019, 6:12 PM
gikari edited the summary of this revision. (Show Details)Nov 12 2019, 10:01 PM

Works really well, good job! Some minor nitpicks

CMakeLists.txt
70

Nitpick, feel free to ignore since it was probably done by me:
usually we call the folder just 'kded'

kded-module/CMakeLists.txt
1 ↗(On Diff #69259)

Move to top-level CMakeLists.txt (if needed at all)

kded-module/configeditor.cpp
176 ↗(On Diff #69259)

QProcess pidof;

kded-module/configvalueprovider.cpp
53 ↗(On Diff #69259)

if (!newIconTheme)

kded-module/gtkconfig.cpp
24 ↗(On Diff #69259)

#include <QDBusConnection>

kded-module/gtkconfig.json
4 ↗(On Diff #69259)

I know it was me who did this, but maybe "gtkconfig" as icon?

gikari updated this revision to Diff 69689.Nov 13 2019, 5:16 PM
gikari marked 6 inline comments as done.
gikari edited the summary of this revision. (Show Details)

Fix the mistakes

gikari updated this revision to Diff 69690.Nov 13 2019, 5:20 PM

Remove the empty line in CMakeLists.txt

gikari updated this revision to Diff 69691.Nov 13 2019, 5:24 PM

Remember to save all files before blindly commiting

gikari planned changes to this revision.Nov 13 2019, 8:10 PM
gikari updated this revision to Diff 69702.Nov 13 2019, 8:11 PM

Add DBusAddons component

cblack added inline comments.Nov 13 2019, 8:15 PM
kded/CMakeLists.txt
30

See below comments about GTK.

kded/configeditor.cpp
36

You shouldn't need to import/use gtk for configuration, only gio which provides gsettings.

52

You should probably remove this call here, as you aren't using any GTK functions.

gikari added inline comments.Nov 13 2019, 8:19 PM
kded/configeditor.cpp
52

You mean remove gtk_init?

cblack added inline comments.Nov 13 2019, 8:21 PM
kded/configeditor.cpp
52

Yes, my bad.

gikari updated this revision to Diff 69704.Nov 13 2019, 8:25 PM
gikari marked 5 inline comments as done.

Remove unnecessary GTK lib usages

broulik accepted this revision.Nov 14 2019, 7:21 PM

Good job!

This revision is now accepted and ready to land.Nov 14 2019, 7:21 PM
GB_2 awarded a token.Nov 14 2019, 7:22 PM
This revision was automatically updated to reflect the committed changes.
zzag added a subscriber: zzag.Nov 14 2019, 7:58 PM

Sweet! I wonder whether this KDED module can be used to synchronize the gtk-decoration-layout property with kwin's decoration button layout.

Maybe a warning about the runtime dependency on xsettingsd should be added to cmake, so distro packagers know about it.

In D24743#562656, @zzag wrote:

Sweet! I wonder whether this KDED module can be used to synchronize the gtk-decoration-layout property with kwin's decoration button layout.

Yes it probably will be used for that.

Maybe a warning about the runtime dependency on xsettingsd should be added to cmake, so distro packagers know about it.

I'm not very proficient in CMake, so how can I do that?

I'm not very proficient in CMake, so how can I do that?

https://cmake.org/cmake/help/latest/command/find_program.html