Upgrade theme to Breeze GTK on startup
ClosedPublic

Authored by gikari on Jan 24 2020, 10:05 AM.

Details

Summary

Also apply it, if the config is empty. This is a port of kconf_update
script from breeze-gtk repo, but this time it use gtkconfig kded
module methods to manipulate config (to avoid code duplication and to
write config to all possible configuration storages).

BUG: 416635
FIXED-IN: 5.18.0

Test Plan
  1. Remove gtk-theme-name= line in ~/.gtkrc-2.0 file or change it to BreezyGTK, Orion or oxygen-gtk
  2. Run gtk_theme executable from kconf_update build directory. Line should appear/cahnge to Breeze
  3. Remove gtk-theme-name= line in ~/.config/gtk-3.0/settings.ini file or change it to BreezyGTK, Orion or oxygen-gtk
  4. Run gtk_theme executable from kconf_update build directory. Line should appear/cahnge to Breeze, also it should be changed in dconf in org.gnome.desktop.interface.gtk-theme prefix and in ~/.config/xsettingsd/xsettingsd.conf file (Net/ThemeName "Breeze" line)

Diff Detail

Repository
R99 KDE Gtk Configuration Tool
Branch
apply-theme-on-startup (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21675
Build 21693: arc lint + arc unit
gikari created this revision.Jan 24 2020, 10:05 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 24 2020, 10:05 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gikari requested review of this revision.Jan 24 2020, 10:05 AM
fvogt added inline comments.Jan 24 2020, 10:38 AM
kded/gtkconfig.h
76 ↗(On Diff #74299)

Where are those methods called from?

gikari added inline comments.Jan 24 2020, 10:43 AM
kded/gtkconfig.h
76 ↗(On Diff #74299)

GtkConfig::applyAllSettings()?

If this is adjusted to include the migration code for older breeze-like GTK themes from https://cgit.kde.org/breeze-gtk.git/tree/kconf_update/main.cpp?id=b00e12ff39264fd8c2fb8af4168e998726766dfa, it should be possible to drop the kconf_update script there.
It might even cause conflicts otherwise.

kded/gtkconfig.h
76 ↗(On Diff #74299)

Indeed - I didn't notice that hunk extended beyond adding two methods...

gikari marked 3 inline comments as done.Jan 24 2020, 1:07 PM

If this is adjusted to include the migration code for older breeze-like GTK themes from https://cgit.kde.org/breeze-gtk.git/tree/kconf_update/main.cpp?id=b00e12ff39264fd8c2fb8af4168e998726766dfa, it should be possible to drop the kconf_update script there.
It might even cause conflicts otherwise.

If I understood correctly, this script replaces the theme setting from orion/oxygen-gtk to Breeze. But what if a user wants to actually use Oxygen for GTK theme?

Kded Module here is started every time on login, therefore it should not replace the settings from correct ones to correct ones.

fvogt added a comment.Jan 24 2020, 1:12 PM

If this is adjusted to include the migration code for older breeze-like GTK themes from https://cgit.kde.org/breeze-gtk.git/tree/kconf_update/main.cpp?id=b00e12ff39264fd8c2fb8af4168e998726766dfa, it should be possible to drop the kconf_update script there.
It might even cause conflicts otherwise.

If I understood correctly, this script replaces the theme setting from orion/oxygen-gtk to Breeze. But what if a user wants to actually use Oxygen for GTK theme?

The kconf_update script only runs once, which means only for a) Fresh profiles b) Profiles older than the kconf_update script introduction (-> KDE4)
For a), no manual configuration was done and for b) Migration from oxygen -> breeze is done already for the widget theme, so it's needed for consistency.

Kded Module here is started every time on login, therefore it should not replace the settings from correct ones to correct ones.

It should only do the migration once, like the kconf_update script.

It should only do the migration once, like the kconf_update script.

From where migration script is launched? Can the migration script itself be a standalone entity, that uses kded module for requesting the current theme ans setting whatever it wants in every config?

fvogt added a comment.Jan 25 2020, 2:24 PM

It should only do the migration once, like the kconf_update script.

From where migration script is launched? Can the migration script itself be a standalone entity, that uses kded module for requesting the current theme ans setting whatever it wants in every config?

kconf_update is called on kded startup. AFAICT blocking, so calling into kded from a kconf_update script would probably deadlock.

meven added a subscriber: meven.Jan 25 2020, 5:21 PM
meven added inline comments.
kded/gtkconfig.cpp
215 ↗(On Diff #74299)

What if users have a different default / theme set on first launch ?
I would suggest to read the current theme instead of hardcoding Breeze.

gikari added inline comments.Jan 25 2020, 5:24 PM
kded/gtkconfig.cpp
215 ↗(On Diff #74299)

Where can I read the default value from?

@fvogt Do I understand correctly, that kconf_update script also run on Plasma update, if I add a new Id to one of .upd file (or add one with new Id)? For example from 5.17 to 5.18?

@fvogt Do I understand correctly, that kconf_update script also run on Plasma update, if I add a new Id to one of .upd file (or add one with new Id)? For example from 5.17 to 5.18?

Yes. Note that it does not only run on updates but also on fresh installations and I'm not aware whether there are any ordering guarantees between kconf_update scripts.

gikari updated this revision to Diff 74382.Jan 26 2020, 10:26 AM

Provide kconf_script to upgrade themes from old ones. It is mostly the same as the one in breeze-gtk repo, but better.

gikari retitled this revision from Apply Breeze GTK theme on startup, if the config is empty to Upgrade theme to Breeze GTK on startup.Jan 26 2020, 10:29 AM
gikari edited the summary of this revision. (Show Details)
gikari edited the test plan for this revision. (Show Details)
fvogt added inline comments.Jan 26 2020, 11:08 AM
kded/gtkconfig.cpp
207 ↗(On Diff #74382)

Now those two aren't needed anymore?

gikari added inline comments.Jan 26 2020, 11:16 AM
kded/gtkconfig.cpp
207 ↗(On Diff #74382)

Hm, yeah. Actually I think it is redundant to replace theme configuration on each start. It would make sense, if it was about syncing the settings, but since GTK theme is set directly by App Style KCM, I see no reason.

gikari updated this revision to Diff 74386.Jan 26 2020, 11:17 AM
gikari marked 2 inline comments as done.

Remove redundant theme update on startup

gikari edited the summary of this revision. (Show Details)Jan 26 2020, 11:20 AM
gikari edited the test plan for this revision. (Show Details)

@meven wrote:

What if users have a different default theme set on first launch? I would suggest to read the current theme instead of hardcoding Breeze.

But I do not know where the information about the default GTK theme is located, so I do not know what to do here.

fvogt added a comment.Jan 27 2020, 3:16 PM

Tested, confirmed to work.

I wonder what's up with the gtkrc files everywhere:

~/.config/gtkrc-2.0
~/.config/gtkrc
~/.gtkrc-2.0
./.config/gtk-3.0

It seems like the first two are created by krdb, does it read both still?

Tested, confirmed to work.

I wonder what's up with the gtkrc files everywhere:

~/.config/gtkrc-2.0
~/.config/gtkrc
~/.gtkrc-2.0
./.config/gtk-3.0

It seems like the first two are created by krdb, does it read both still?

My ConfigEditor reads only the last one (settings.ini) for GTK3 settings and the ~/.gtkrc-2.0 for GTK2 applications. Because these files are used for configure other settings with the daemon. I do not know what are the first two used for though.

gikari updated this revision to Diff 74815.Jan 31 2020, 8:50 PM
gikari edited the test plan for this revision. (Show Details)

Rebase on master

@fvogt, are you good with this now?

fvogt added a comment.Jan 31 2020, 9:25 PM

The only remaining question from my side is why we have four different gtk configuration files and whether that causes issues.
Besides that, only @meven's comment needs a resolution.

gikari added a comment.Feb 1 2020, 4:32 PM

The differences between gtkrc and gtkrc-2.0 at least is that the first one was used for GTK1 and the second for GTK2.

gikari updated this revision to Diff 74881.Feb 2 2020, 11:34 PM

Rename files to provide ground for more kconf scripts in the future

ngraham accepted this revision.Feb 3 2020, 6:22 PM

LGTM now. @meven and @fvogt, you good with this too?

This revision is now accepted and ready to land.Feb 3 2020, 6:22 PM
fvogt accepted this revision.Feb 3 2020, 6:24 PM

LGTM now. @meven and @fvogt, you good with this too?

Not sure what the rename was about, but yes, still LGTM.

This revision was automatically updated to reflect the committed changes.