[kcm-colors] Export colorscheme to GTK color definitions
ClosedPublic

Authored by cblack on Wed, Jul 31, 12:07 AM.

Details

Summary

This exports the active colorscheme to ~/.config/gtk-3.0/colors.css and sets up ~/.config/gtk-3.0/gtk.css to read from it.
This will do nothing visibly without changes to Breeze GTK made in D22877

FEATURE: 356006
FIXED-IN: 5.17.0

Test Plan

See that ~/.config/gtk-3.0/colors.css is created, colorscheme is dumped correctly, and that gtk.css links to it.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cblack created this revision.Wed, Jul 31, 12:07 AM
Restricted Application added a project: Plasma. · View Herald TranscriptWed, Jul 31, 12:07 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
cblack requested review of this revision.Wed, Jul 31, 12:07 AM
cblack edited the summary of this revision. (Show Details)Wed, Jul 31, 12:33 AM

Interesting approach!
However, this doesn't seem to apply anything on first login?
Maybe you want to extend the "rdb" thing instead:
runRdb(KRdbExportQtColors | KRdbExportGtkTheme | (m_applyToAlien ? KRdbExportColors : 0)); and add a KRdbExportGtkCssColors which then does all of that, given there's already a "Gtk Theme" in there.

Overall I must say I'm not a fan of adding yet another place where we mess with GTK config files and would prefer someone write that centralized gtk settings daemon I proposed numerous times :) but I don't want to block this on a hypothetical idea that will probably never emerge...

kcms/colors/colors.cpp
323

Pass by const-ref const QString &name, ..

325

return QStringLiteral("@define-color %1 %2").arg(name, color);\n";

452

You probably want to be using QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation) or something along the lines instead of hardcoding ~/.config

456

Coding style, brace on same line as if

469

Not just WriteOnly?

cblack updated this revision to Diff 62847.Wed, Jul 31, 1:13 PM

Incorporate feedback

It seems the header with the new flag is missing from the diff.

It seems the header with the new flag is missing from the diff.

I put it alongside the content in the existing exportColors flag because there's no reason to make a flag for exporting colors to something when there's already a flag for exporting colors to things.

Sorry my bad, I misread the diff. I thought you added a new flag (as proposed above) and looked at exportGtkTheme but that flag is an existing one. And to my shame it doesn't even have a green background.

cblack updated this revision to Diff 62859.Wed, Jul 31, 2:46 PM

Dump the rest of the colors.

cblack retitled this revision from [WIP/RFC] [kcm-colors] Export colorscheme to GTK color definitions to [kcm-colors] Export colorscheme to GTK color definitions.Wed, Jul 31, 2:52 PM
cblack edited the summary of this revision. (Show Details)
ngraham edited the summary of this revision. (Show Details)Thu, Aug 1, 12:16 AM
cblack edited the summary of this revision. (Show Details)Thu, Aug 1, 3:32 PM
ngraham requested changes to this revision.Fri, Aug 2, 7:12 PM
ngraham added a subscriber: ngraham.

Very cool idea. However I'm not seeing it work. No matter how many times I change the color scheme, the .css files never get created. It doesn't work after a reboot either.

This revision now requires changes to proceed.Fri, Aug 2, 7:12 PM
cblack updated this revision to Diff 63001.Fri, Aug 2, 7:57 PM

Make GTK colors its own KRDB flag

ngraham requested changes to this revision.EditedFri, Aug 2, 8:31 PM

Thanks, it works much better now. I'm really impressed with this, and it's very cool to be able to see GTK3 apps respecting the system color scheme:

However this is still not shippable in its current state due to a reproducible issue: GTK apps will only reliably change their color scheme after you apply a new color scheme in the KCM twice. If you only apply it once, most or all of the time, GTK apps will not change, and will retain the old color scheme. This needs to be fixed first so that applying the new color scheme once is all it takes.

Also, it's a bit annoying that you need to restart GTK apps to see the change, but I suppose that's inevitable (and if fixable, we can fix it in a subsequent patch).

This revision now requires changes to proceed.Fri, Aug 2, 8:31 PM
cblack updated this revision to Diff 63009.Sat, Aug 3, 12:19 AM

Colors are now applied from the active colorscheme every time

cblack updated this revision to Diff 63010.Sat, Aug 3, 12:42 AM

Fix WM colors on first go round

Nice, it works every time now! I'm not super thrilled about all the code duplication, though. Can't this live in just one place? If not here, then maybe in plasma-framework?

.gitignore
1 ↗(On Diff #63010)

Unrelated to this patch; please remove from the diff

cblack updated this revision to Diff 63025.Sat, Aug 3, 2:12 PM

Deduplicate colors exporting code

ngraham accepted this revision.Sun, Aug 4, 10:00 PM

Thanks very much. This is excellent work, and I'm impressed. The feature works perfectly and the code structure and implementation makes sense to me. I think people are really going to love this in Plasma 5.17.

Plasma people, what say ye?

This revision is now accepted and ready to land.Sun, Aug 4, 10:00 PM

This doesn't apply the colors on startup, does it?
Probably needs the new flag passed in kcminit_style

cblack updated this revision to Diff 63123.Mon, Aug 5, 2:01 PM

Add KRdbExportGtkColors flag to kcminit_style()

Just a small thing I noticed when scrolling through the patch.

kcms/krdb/krdb.h
31

Since this is treated as a flag, shouldn't this be 0x0020? Otherwise it would be equal to KRdbExportQtColors & KRdbExportGtkTheme which is probably not what you want.

cblack updated this revision to Diff 63185.Tue, Aug 6, 1:53 PM

Fix GtkColors flag

cblack updated this revision to Diff 63187.Tue, Aug 6, 1:57 PM

Update to match master.

cblack updated this revision to Diff 63188.Tue, Aug 6, 2:03 PM

Rebase properly now?

cblack updated this revision to Diff 63196.Tue, Aug 6, 2:09 PM

Rebasing shenanigans, pt 3.

cblack updated this revision to Diff 63207.Tue, Aug 6, 2:31 PM

Fix the rebase shenanigans

broulik accepted this revision.Tue, Aug 6, 2:51 PM

Good job!

ngraham edited the summary of this revision. (Show Details)Tue, Aug 6, 3:05 PM
This revision was automatically updated to reflect the committed changes.