Implement syncing of theme preferences between SDDM and Plasma
ClosedPublic

Authored by filipf on Jul 1 2019, 3:13 PM.

Details

Summary

This patch exposes UI options in sddm-kcm which enable users to sync their theme preferences used in Plasma with SDDM.

Syncing of UI font, font rendering, color scheme, desktop and icon theme has been implemented.

A reset button has also been added in order to allow easy removal of all SDDM syncing-related customization.

Test Plan

For testing a different font, you need to have a file like this one:


in /home/user/.config/fontconfig/conf.d/

Settings UI:

Diff Detail

Repository
R123 SDDM Configuration Panel (KCM)
Branch
sddm-theme-syncing (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13745
Build 13763: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
filipf planned changes to this revision.Jul 1 2019, 3:27 PM

Definitely have to fix the hardcoding.

The whole .ui file also needs to be redone from scratch since there's leftovers from tinkering with it in QtDesigner.

sddmauthhelper.cpp
117

Initially it was supposed to be a check that the Reset button was pressed (because that's when the argument would get emitted), but I'll check if it can be removed now.

filipf updated this revision to Diff 60968.EditedJul 1 2019, 9:49 PM
  • don't hardcode SDDM user directory
  • code cleanup
  • remove SDDM dir cache cleanup operations
  • remove uncessary references to KCModule
  • redo the .ui file
filipf marked 3 inline comments as done.Jul 1 2019, 9:52 PM
filipf added inline comments.
src/ui/advanceconfig.ui
269

Any tips on how to use theme's smallest font like you can in C++ and QML?

The UI in Spectacle doesn't use a FormLayout and achieves it in some special way AFAIK.

filipf edited the test plan for this revision. (Show Details)Jul 1 2019, 9:52 PM
filipf edited the test plan for this revision. (Show Details)
filipf marked an inline comment as done.
ngraham added inline comments.Jul 1 2019, 10:57 PM
src/ui/advanceconfig.ui
269

Yeah, you have to do it in C++, from within the file that reads the .ui file. Here's how it's done this way in DrKonqi, for example: https://cgit.kde.org/drkonqi.git/tree/src/bugzillaintegration/reportassistantpages_base.cpp#n159

filipf updated this revision to Diff 60976.Jul 1 2019, 11:44 PM
  • have explanation label use the "small" font
  • minor cleanup
filipf updated this revision to Diff 60977.Jul 1 2019, 11:46 PM

also update the .ui file

filipf marked 2 inline comments as done.Jul 1 2019, 11:46 PM
filipf added inline comments.
src/ui/advanceconfig.ui
269

that worked, thanks :)

filipf updated this revision to Diff 60978.Jul 1 2019, 11:49 PM
filipf marked an inline comment as done.

missed an import

ngraham added inline comments.Jul 2 2019, 12:01 AM
sddmauthhelper.cpp
36

This change to alphabetize the includes is good, but should be done in another commit (feel free to commit directly) as it doesn't directly pertain to the changes here.

filipf updated this revision to Diff 60991.Jul 2 2019, 8:10 AM
filipf marked an inline comment as done.
  • Merge branch 'master' into sddm-theme-syncing
filipf updated this revision to Diff 60992.Jul 2 2019, 8:12 AM

get rid of KDevelop files that accidentally got added to the diff

filipf updated this revision to Diff 60993.Jul 2 2019, 8:32 AM

remove 2 unused imports

davidedmundson added inline comments.Jul 2 2019, 8:39 AM
sddmauthhelper.cpp
54–55

do we need two objects representing the same user?

src/advanceconfig.cpp
179

If it's empty, you print a message, which is fine, but then we still add an empty entry to our map, and still try to copy the file?

filipf added inline comments.Jul 2 2019, 12:25 PM
src/advanceconfig.cpp
179

Should I just bring back the code that only does the copy operation if the entry is not empty or would it also be good to not even add the entry to the map if there is no file/folder?

ndavis added a subscriber: ndavis.Jul 2 2019, 10:24 PM

Settings UI:

This control layout seems to imply that a user must press the "Sync" button every time they change their user settings.

Wouldn't a checkbox for continual synchronization and a button for resetting make more sense?

GB_2 added a subscriber: GB_2.Jul 3 2019, 5:12 AM

Settings UI:

This control layout seems to imply that a user must press the "Sync" button every time they change their user settings.

Wouldn't a checkbox for continual synchronization and a button for resetting make more sense?

I think then you would have to type in your password every time you change a setting.

GB_2 added inline comments.Jul 3 2019, 5:31 AM
src/ui/advanceconfig.ui
226

It would be nice if you added the icon view-refresh to this button...

233

...and here the icon document-revert or edit-undo.

262

you theme -> your theme
desktop -> Plasma

GB_2 added inline comments.Jul 3 2019, 5:38 AM
kcm_sddm.actions
301

I think the long form of sync would be better in the description: Synchronizes

ndavis added a comment.EditedJul 3 2019, 6:10 AM
In D22191#489915, @GB_2 wrote:

I think then you would have to type in your password every time you change a setting.

Would you really? There couldn't be a service running with the right permissions that automatically synchronizes settings on logout/user switch/shutdown/when a change is detected? It seems like it should be possible, but I don't know what the security implications would be.

filipf added a comment.Jul 3 2019, 7:52 AM
In D22191#489915, @GB_2 wrote:

I think then you would have to type in your password every time you change a setting.

Would you really? There couldn't be a service running with the right permissions that automatically synchronizes settings on logout/user switch/shutdown/when a change is detected? It seems like it should be possible, but I don't know what the security implications would be.

It's copying to sddm owned directories so it does need root permissions... something would have to be worked out, not sure what the possibilities are though. Also this would have to be enabled by default solely for single user systems, otherwise you run the risk of the login screen always changing appearance based on the user that was last logged in.

ndavis added a comment.EditedJul 3 2019, 4:38 PM
In D22191#489915, @GB_2 wrote:

I think then you would have to type in your password every time you change a setting.

Would you really? There couldn't be a service running with the right permissions that automatically synchronizes settings on logout/user switch/shutdown/when a change is detected? It seems like it should be possible, but I don't know what the security implications would be.

It's copying to sddm owned directories so it does need root permissions... something would have to be worked out, not sure what the possibilities are though. Also this would have to be enabled by default solely for single user systems, otherwise you run the risk of the login screen always changing appearance based on the user that was last logged in.

I think it could be done with a systemd service, but I don't think we want to start directly depending on systemd. Maybe we could just keep this patch like it is and write a separate systemd service file for people to download from https://invent.kde.org/kde/kde-vdg-extras until we have a more user friendly solution.

cfeck added a subscriber: cfeck.Jul 3 2019, 7:43 PM

See also bug 403953 and bug 408863.

I think it could be done with a systemd service

I don't think I follow what part of the puzzle that solves.

We can already run something as root on demand.
I don't think there's any requirement to have something permanently running?

Yes, having some syncing automagically from other places might be nice, but the initial step is to build up those actions.

filipf updated this revision to Diff 61109.Jul 3 2019, 9:33 PM

kcm_sddm.actions

  • "sync" to "synchronize"

sddmauthhelper.cpp

  • don't needlesslydefine SDDM user twice
  • revert to copying files only if entry is not empty

src/ui/advanceconfig.ui

  • add icon "view-refresh" to the Sync button
  • add icon "edit-undo" to Reset button
  • fix typo and change "desktop theme" to "Plasma theme"
filipf marked 8 inline comments as done.Jul 3 2019, 9:34 PM
filipf added inline comments.
src/advanceconfig.cpp
179

I brought back the old code in sddmauthhelper.cpp

filipf marked an inline comment as done.Jul 3 2019, 9:51 PM

See also bug 403953 and bug 408863.

That should also be done this summer and before 5.17.

davidedmundson added inline comments.Jul 9 2019, 2:04 PM
sddmauthhelper.cpp
93

what would happen if:

  • there is no /var/lib/sddm/.config yet
  • the user hasn't got a fontconfig to copy
  • they do have a kdeglobals
filipf updated this revision to Diff 61411.EditedJul 9 2019, 2:35 PM
  • create sddm config dir if not present
  • don't copy fontconfig if folder exists but is empty
  • get rid of 'ignoring return value' warning the chown command spewed
filipf updated this revision to Diff 61479.Jul 10 2019, 1:57 AM

better solution for fontconfig copy operations check

ngraham added a comment.EditedJul 10 2019, 2:11 PM

Overall this is very nice! I have some inline comments, and some design comments:

  1. You need to handle failure conditions for the remove, mkpath, copy, chown etc. operations. Thede functions all return false if they fail, so you can find out easily enough. There's nothing worse than an unhandled error, because then the operation will just fail silently, leaving the user confused. Even showing an ugly error dialog box would probably be better than nothing, though a KMessageWidget would be much nicer.
  1. This should be considered a framework for optionally having the sync operation done automatically when selecting a new theme/icon set/font/etc. The feature is nice, but not very discoverable, and the better UX is to have new settings synced to SDDM automatically for the case where there's only one admin user account on the box.
  1. We need to come up with a way for user-installed content from GHNS etc. to get synced. If detecting when it's installed in a non-system location is unfeasible, we should consider installing new content to a system location rather than in the user's homedir, either optionally of even by default.
sddmauthhelper.cpp
57

Don't ignore the error if the removal fails

60

Don't ignore the error if the copy fails

62

Don't ignore the error if the chown fails

71

Ditto for this and other mkpath() calls. Errors should be handled, even with something as simple as a dialog box saying, "Could not create <path>!"

In general we do need an additional message box which says sync successful or failed. And then in the case of failure it should state what failed.

But as far as I can tell the operations won't fail. They're all conditional on values being existent or not and if things aren't in order they won't get carried out.

I agree with points 2 and 3 as well, but would ask if we could implement it all gradually as working with multiple branches is already getting a bit clumsy.

Implementation wise, point 2 is something Plasma elders would understand better atm.

As for point 3, I've looked it additionally and it does complicate things but might be doable. The question is how to interact with the user. We could copy everything to a global directory and then remove it from the user directory (to avoid duplicates in kcms). What sucks is that users could no longer easily remove these theme files via kcms. So what if we the remove button worked with globally installed? Currently I'm not sure of the repercussions for stuff installed via package manager.

In general we do need an additional message box which says sync successful or failed. And then in the case of failure it should state what failed.

But as far as I can tell the operations won't fail. They're all conditional on values being existent or not and if things aren't in order they won't get carried out.

Famous last words. :) You never know what situations users will get themselves into. Maybe they discover the feature while testing with a live CD where the root filesystem isn't writable, for example.

I agree with points 2 and 3 as well, but would ask if we could implement it all gradually as working with multiple branches is already getting a bit clumsy.

Yeah, that makes sense.

As for point 3, I've looked it additionally and it does complicate things but might be doable. The question is how to interact with the user. We could copy everything to a global directory and then remove it from the user directory (to avoid duplicates in kcms). What sucks is that users could no longer easily remove these theme files via kcms.

Definitely something to ask @leinir about. The GHNS dialog would have to be involved in any event, either to (optionally or by default) install things globally, to know how to remove themes that are globally installed, and to de-duplicate themes that are installed both locally and globally.

But yeah, material for another patch.

In general we do need an additional message box which says sync successful or failed. And then in the case of failure it should state what failed.

But as far as I can tell the operations won't fail. They're all conditional on values being existent or not and if things aren't in order they won't get carried out.

Famous last words. :) You never know what situations users will get themselves into. Maybe they discover the feature while testing with a live CD where the root filesystem isn't writable, for example.

If that's the case then the whole KCM needs to be disabled because any change made within it entails writing to root. This would be relevant to the function though when abstracted and used throughout KCMs. Another thing that crosses my mind in regards to failing is BSD (due to different generic paths), but I have no idea if they even use SDDM.

Well whatever the case, I need to have a look at how to add all this fail/success info.

ngraham accepted this revision.Jul 11 2019, 1:37 PM

If that's the case then the whole KCM needs to be disabled because any change made within it entails writing to root. This would be relevant to the function though when abstracted and used throughout KCMs. Another thing that crosses my mind in regards to failing is BSD (due to different generic paths), but I have no idea if they even use SDDM.

Hmm, that's true, so this is a pre-existing issue then.

Okay, let's get this in and focus on polishing it in subsequent revisions!

This revision is now accepted and ready to land.Jul 11 2019, 1:37 PM
filipf updated this revision to Diff 61669.Jul 12 2019, 5:40 PM

Use KMessageBox to show information whether or not the sync and reset operations succeeded

ngraham added inline comments.Jul 12 2019, 10:39 PM
src/advanceconfig.cpp
206

"Synchronization failed." is a pretty frustrating error message. The user will wonder, "How did it fail? What happened? How can I fix it?" etc. Since we have the error text, let's show it in the message box, since it could provide some clues.

210

I don't think we need a dialog box for the success case. That'll just annoy people.

filipf updated this revision to Diff 61681.Jul 12 2019, 10:56 PM

remove message box on successful operation, use job error text when the operation fails

Failure messages do seem to be generated

ngraham accepted this revision.Jul 12 2019, 11:44 PM

Technical gibberish is better than nothing at least. :)

filipf edited the test plan for this revision. (Show Details)Jul 19 2019, 6:52 PM
This revision was automatically updated to reflect the committed changes.