Rewrite GTK KCM
ClosedPublic

Authored by gikari on Dec 28 2019, 6:51 PM.

Details

Summary

After several changes, GTK Applications settings are now in
sync with KDE equivalent ones, so that GTK KCM now controls only the
theme of the GTK applications. But GTK KCM was written with the thought
that it controls entire GTK apps appearance in mind. Its code structure
is now redundant and needs to be either rewritten or purged in favour of
new one, that will be present in Application Style KCM code base.

The last approach is preferable, because of the goal of removing GTK KCM
to simplify System Settings structure. However, there are some points to
be noted:

  1. The consensus of where exactly to put the GTK applications settings in App Style KCM is not yet reached.
  2. A couple of changes to the kded settings sync daemon are needed, to provide functionality, that is left in GTK KCM (Themes Installation/Deletion, Preview etc) without depending on GTK libs
  3. There are some bugs in KCM, that are present in the moment, that could only be fixed with architecture changes (for example: not working previews, when the xsettingsd daemon is running; broken icons in the previews; gtk themes do not applying for flatpak apps)

Given the points above I decided to rewrite GTK KCM, so that complex
design task (1) would be solved after changes (2) are made. And also
in the process some bugs (3) will be fixed.

Now, GTK KCM is not responsible for managing GTK configs, the one who
does is the kded module, that is asked through DBus, when it is needed
to do so.
The stuff, that is managed by kded module:

  1. Setting selected GTK2 and GTK3 themes for applications
  2. Getting current GTK2 and GTK3 themes for applications
  3. Previewing GTK2 and GTK3 applications for selected themes

Technically 2 and 3 could be done in KCM directly, because they do not
depend on GTK libs, but I thought, that it would be logically related to
the stuff, that is done via kded module. If you think, that that
approach is not right, feel free to criticise.

The stuff, that is done in KCM directly:

  1. Getting the list of installed GTK themes
  2. Installing and removing the GTK theme

Also there are some visual changes:

  1. Theme deletion is now done via button near GTK theme combo, instead of separate dialog
  2. Theme installation from file is now done via button at the bottom, as in the other similar KCMs
  3. Preview button now has a title with an ellipsis to indicate, that it launches GTK app in a pop up window

BUG: 405405

Test Plan

  1. Relaunch kded5
  2. Open GTK KCM
  3. Check if gtk themes changing works
  4. Check if the previews are working
  5. Check if the local themes deletion works
  6. Check if theme installation from archive works
  7. Check if GHNS theme installation works

Diff Detail

Repository
R99 KDE Gtk Configuration Tool
Branch
set-theme-through-kded
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20797
Build 20815: arc lint + arc unit
gikari created this revision.Dec 28 2019, 6:51 PM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 28 2019, 6:51 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gikari requested review of this revision.Dec 28 2019, 6:51 PM
gikari added a reviewer: VDG.Dec 28 2019, 6:51 PM
gikari edited the test plan for this revision. (Show Details)

Nice work, but I think we should shoot for moving the remaining (small) functionality into the Application Style KCM for Plasma 5.18. Everything that's left here could easily fit into a popup on that KCM, or we could use the multi-page KCM system to put all of this stuff on a sub-page on that KCM.

Everything that's left here could easily fit into a popup on that KCM, or we could use the multi-page KCM system to put all of this stuff on a sub-page on that KCM.

By the way, I've already made a popup in Applications Style KCM. Personally I have no objections whatsoever on that style and would gladly use it.

Nevertheless, this patch is still needed. For the most part the logic and approaches in this code, that I wrote here, will be almost identical to the ones in the future Application KCM code. So I would like to hear comments on how it is good\bad or could be improved, before I make changes in different repository. If I had made changes in two repositories at the same time, it would have been a bit harder to maintain the changes, after revisions.

Everything that's left here could easily fit into a popup on that KCM, or we could use the multi-page KCM system to put all of this stuff on a sub-page on that KCM.

By the way, I've already made a popup in Applications Style KCM. Personally I have no objections whatsoever on that style and would gladly use it.

Nevertheless, this patch is still needed. For the most part the logic and approaches in this code, that I wrote here, will be almost identical to the ones in the future Application KCM code. So I would like to hear comments on how it is good\bad or could be improved, before I make changes in different repository. If I had made changes in two repositories at the same time, it would have been a bit harder to maintain the changes, after revisions.

I don't personally think that fits with the Plasma style, as i never see any pop-ups like that, but maybe that's just me. Either way, something is better than nothing, so that people don't get confused. After 5.18 it can be improved to elegantly integrate the remaining functionality into the style kcm.

May be tabs would be the solution? (This is mockup)

Yeah tabs could work, especially if we ever get a functional icon grid for the GTK themes. You can copy how it's done in the Window Decorations and Audio KCMs.

I think in the VDG channel we decided to put the GTK config stuff on another page accessed by clicking on a button.

ngraham requested changes to this revision.Jan 7 2020, 5:07 PM
This comment was removed by ngraham.
This revision now requires changes to proceed.Jan 7 2020, 5:07 PM

Never mind, I was an idiot and hadn't restarted kded5. Almost everything works perfectly after that. I only found two bugs:

  • When you delete the current theme, the combobox becomes empty. It should probably go back to the default theme or Breeze or something.
  • If you click on the Install from File... button and cancel the dialog, you get an ugly error message:
cblack added a subscriber: cblack.Jan 7 2020, 5:17 PM
cblack added inline comments.
kded/themepreviewer.cpp
86

Taking advantage of the GTK_THEME environment variable would be a more elegant solution than copying the current configuration to another place, applying test configuration, stopping xsettingsd, and then starting the demo application.

gikari added a comment.Jan 7 2020, 5:28 PM
  • When you delete the current theme, the combobox becomes empty. It should probably go back to the default theme or Breeze or something.

So, it needs to select Breeze in otherwise empty combobox and instantly apply it?

  • When you delete the current theme, the combobox becomes empty. It should probably go back to the default theme or Breeze or something.

So, it needs to select Breeze in otherwise empty combobox and instantly apply it?

It doesn't need to instantly apply it, but it should select Breeze and mark the state as dirty so the Apply button becomes enabled.

gikari added a comment.Jan 7 2020, 5:32 PM

It doesn't need to instantly apply it, but it should select Breeze and mark the state as dirty so the Apply button becomes enabled.

But the theme is now deleted and configuration is in the incorrect state - GTK apps are requesting a theme, that does not exist anymore.

If it needs to instantly apply something, sure, I guess it can instantly apply Breeze.

gikari updated this revision to Diff 73006.Jan 7 2020, 5:38 PM

Do not display error, when the theme archive selection is canceled

Nice, works fine for those now. I don't know if this is a bug in the KCM or a bug in the GHNS dialog, but when I delete a theme from the KCM, it's still listed as installed in the dialog. However when I delete a theme from the dialog, it disappears from the KCM as expected.

gikari added a comment.Jan 7 2020, 5:58 PM

I don't know if this is a bug in the KCM or a bug in the GHNS dialog, but when I delete a theme from the KCM, it's still listed as installed in the dialog. However when I delete a theme from the dialog, it disappears from the KCM as expected.

It happens in the old GTK KCM too.

ngraham accepted this revision.Jan 7 2020, 6:00 PM

All right, we can fix that separately then.

Everything works, so I'm giving this my QA blessing. Please wait for code review approval from @cblack or a Plasma person before landing it.

This revision is now accepted and ready to land.Jan 7 2020, 6:00 PM
gikari added a comment.Jan 7 2020, 6:02 PM

Everything works

Wait, I didn't fixed the empty combobox thing yet

that's weird, it started working for me after your last update...

cblack accepted this revision.Jan 7 2020, 6:10 PM

Code-wise, all of the stuff touching GTK looks fine except for the overengineered GTK3 theme preview, but that's relatively minor since it should still work as intended.

gikari added a comment.Jan 7 2020, 6:12 PM

Code-wise, all of the stuff touching GTK looks fine except for the overengineered GTK3 theme preview, but that's relatively minor since it should still work as intended.

I will fix it. My reinventing-the-wheel stupid solution introduces side effect - xsettingsd is not running, when preview window is active.

gikari updated this revision to Diff 73015.Jan 7 2020, 8:15 PM
  • Fixed combobox selection after theme deletion (please test)
  • Minor code style changes
gikari planned changes to this revision.Jan 7 2020, 8:15 PM
gikari requested review of this revision.Jan 7 2020, 8:52 PM
gikari marked an inline comment as done.

Taking advantage of the GTK_THEME environment variable would be a more elegant solution

On the other hand this does not work for GTK2 applications, so I'll leave it as it is.

This revision is now accepted and ready to land.Jan 7 2020, 8:52 PM
gikari updated this revision to Diff 73059.Jan 8 2020, 1:51 PM

Replace dbus signals with dbusinterface calls

gikari updated this revision to Diff 73060.Jan 8 2020, 1:58 PM

Remove forgotten signal connections

gikari updated this revision to Diff 73065.Jan 8 2020, 2:16 PM

Simplify GTK3 Preview algorithm

Unfortunately this can't be applied to GTK2 :(

ngraham accepted this revision.Jan 8 2020, 4:10 PM

Works great for me.

cblack accepted this revision.Jan 11 2020, 6:09 PM

No complaints here.

This revision was automatically updated to reflect the committed changes.