D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

This revision was automatically updated to reflect the committed changes.
Closed by commit R123:f9e753551cdd: Implement syncing of theme preferences 
between SDDM and Plasma (authored by filipf).

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=61681=62073

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: leinir, cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  Technical gibberish is better than nothing at least. :)

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: leinir, cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf added a comment.


  Failure messages do seem to be generated
  
  F7013711: image.png 

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: leinir, cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 61681.
filipf added a comment.


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

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=61669=61681

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: leinir, cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> advanceconfig.cpp:207
> +qDebug() << job->errorText();
> +KMessageBox::error(this, i18n("Synchronization failed."));
> +} else {

"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.

> advanceconfig.cpp:211
> +qDebug() << "Synchronization successful";
> +KMessageBox::information(this, i18n("Synchronization successful."));
> +}

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

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: leinir, cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 61669.
filipf added a comment.


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

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=61480=61669

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: leinir, cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  In D22191#493985 , @filipf wrote:
  
  > 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!

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: leinir, cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf added a comment.


  In D22191#493949 , @ngraham wrote:
  
  > In D22191#493928 , @filipf wrote:
  >
  > > 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.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: leinir, cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Nathaniel Graham]

ngraham added a subscriber: leinir.
ngraham added a comment.


  In D22191#493928 , @filipf wrote:
  
  > 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.
  
  In D22191#493928 , @filipf wrote:
  
  > 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.
  
  In D22191#493928 , @filipf wrote:
  
  > 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.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: leinir, cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, 
fbampaloukas, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf added a comment.


  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.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Nathaniel Graham]

ngraham added a comment.


  Overall this is very nice! I have some inline comments, and two macro 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.
  
  2. 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.
  
  3. 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.

INLINE COMMENTS

> sddmauthhelper.cpp:57
> +if (QFile::exists(destination)) {
> +QFile::remove(destination);
> +}

Don't ignore the error if the removal fails

> sddmauthhelper.cpp:60
> +
> +QFile::copy(source, destination);
> +const char* destinationConverted = destination.toLocal8Bit().data();

Don't ignore the error if the copy fails

> sddmauthhelper.cpp:62
> +const char* destinationConverted = destination.toLocal8Bit().data();
> +if (chown(destinationConverted, sddmUser.userId().nativeId(), 
> sddmUser.groupId().nativeId())) {
> +return;

Don't ignore the error if the chown fails

> sddmauthhelper.cpp:71
> +if (!sddmConfigLocation.exists()) {
> +QDir().mkpath(sddmConfigLocation.path());
> +}

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

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 61480.
filipf added a comment.


  .

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=61479=61480

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 61479.
filipf added a comment.


  better solution for fontconfig copy operations check

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=61411=61479

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 61411.
filipf added a comment.


  - Merge branch 'master' into sddm-theme-syncing

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=61109=61411

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> sddmauthhelper.cpp:93
> +// copy kdeglobals (color scheme)
> +if (!args[QStringLiteral("kdeglobals")].isNull()) {
> +QDir kdeglobalsSource(args[QStringLiteral("kdeglobals")].toString());

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

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf marked an inline comment as done.
filipf added a comment.


  In D22191#490399 , @cfeck wrote:
  
  > See also bug 403953 and bug 408863.
  
  
  That should also be done this summer and before 5.17.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf marked 8 inline comments as done.
filipf added inline comments.

INLINE COMMENTS

> filipf wrote in 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?

I brought back the old code in sddmauthhelper.cpp

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 61109.
filipf added a comment.


  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"

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=60993=61109

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[David Edmundson]

davidedmundson added a comment.


  >   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.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Christoph Feck]

cfeck added a comment.


  See also bug 403953 and bug 408863.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, 
ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Noah Davis]

ndavis added a comment.


  In D22191#489958 , @filipf wrote:
  
  > In D22191#489925 , @ndavis wrote:
  >
  > > 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 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.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf added a comment.


  In D22191#489925 , @ndavis wrote:
  
  > 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.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Noah Davis]

ndavis added a comment.


  In D22191#489915 , @GB_2 wrote:
  
  > In D22191#489838 , @ndavis wrote:
  >
  > > > Settings UI:
  > > >  F6953661: Screenshot_20190701_234413.png 

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

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Björn Feber]

GB_2 added inline comments.

INLINE COMMENTS

> kcm_sddm.actions:301
> +Name=Sync Settings
> +Description=Syncs user settings with SDDM settings
> +Policy=auth_admin

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

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Björn Feber]

GB_2 added inline comments.

INLINE COMMENTS

> advanceconfig.ui:226
> +   
> +
> + 

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

> advanceconfig.ui:233
> +   
> +
> + 

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

> advanceconfig.ui:256
> +   
> +Settings synchronization allows you to transfer you theme 
> customization (color scheme, font, font rendering, desktop and icon theme) to 
> SDDM.
> 

you theme -> your theme
desktop -> Plasma

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Björn Feber]

GB_2 added a comment.


  In D22191#489838 , @ndavis wrote:
  
  > > Settings UI:
  > >  F6953661: Screenshot_20190701_234413.png 

  >
  > 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.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Noah Davis]

ndavis added a comment.


  > Settings UI:
  >  F6953661: Screenshot_20190701_234413.png 

  
  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?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, 
Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf added inline comments.

INLINE COMMENTS

> davidedmundson wrote in 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?

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?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> sddmauthhelper.cpp:54-55
> +{
> +KUser sddmUser(QStringLiteral("sddm"));
> +KUser sddmGroup(QStringLiteral("sddm"));
> +

do we need two objects representing the same user?

> advanceconfig.cpp:179
> +
> +if (fontconfigPath.isEmpty()) {
> +qDebug() << "fontconfig folder not found";

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?

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 60993.
filipf added a comment.


  remove 2 unused imports

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=60992=60993

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf marked an inline comment as done.
filipf added inline comments.

INLINE COMMENTS

> ngraham wrote in sddmauthhelper.cpp:37
> 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.

https://cgit.kde.org/sddm-kcm.git/commit/?id=edaae61c36de21dda224b3e40d9abc1d5e8c057a

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 60992.
filipf added a comment.


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

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=60991=60992

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 60991.
filipf marked an inline comment as done.
filipf added a comment.


  - Merge branch 'master' into sddm-theme-syncing

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=60978=60991

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  SYNCINGsddm-kcm.kdev4
  kcm_sddm.actions
  sddm-kcm.kdev4
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> sddmauthhelper.cpp:37
> +#include 
> +#include 
>  

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.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 60978.
filipf marked an inline comment as done.
filipf added a comment.


  missed an import

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=60977=60978

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf marked 2 inline comments as done.
filipf added inline comments.

INLINE COMMENTS

> ngraham wrote in advanceconfig.ui:281
> 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

that worked, thanks :)

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 60977.
filipf added a comment.


  also update the .ui file

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=60976=60977

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 60976.
filipf added a comment.


  - have explanation label use the "small" font
  - minor cleanup

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=60968=60976

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> filipf wrote in advanceconfig.ui:281
> 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.

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

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf marked 3 inline comments as done.
filipf added inline comments.

INLINE COMMENTS

> advanceconfig.ui:281
> +
> + 8
> +

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.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf updated this revision to Diff 60968.
filipf added a comment.


  - don't hardcode SDDM user directory
  
  _ code cleanup
  
  - remove SDDM dir cache cleanup operations
  - remove uncessary references to KCModule
  - redo the .ui file

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22191?vs=60934=60968

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf planned changes to this revision.
filipf added a comment.


  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.

INLINE COMMENTS

> davidedmundson wrote in sddmauthhelper.cpp:118
> What's the point of this argument when you hardcode/evaluate it so much in 
> this file.

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.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> sddmauthhelper.cpp:76
> +QStringList sourceDirEntries = sourceDirectory.entryList 
> (QDir::AllDirs);
> +QDir 
> destination(QStringLiteral("/var/lib/sddm/.config/fontconfig/"));
> +

Don't hardcode /var/lib/sddm

KUser("sddm").homeDir() would be better

> sddmauthhelper.cpp:118
> +//QDir cacheDir(QStringLiteral("/var/lib/sddm/.cache/"));
> +if (!args[QStringLiteral("sddmUserConfig")].isNull()) {
> +fontconfigDir.removeRecursively();

What's the point of this argument when you hardcode/evaluate it so much in this 
file.

> advanceconfig.cpp:40
>  
> -AdvanceConfig::AdvanceConfig(const KSharedConfigPtr , QWidget 
> *parent) :
> +AdvanceConfig::AdvanceConfig(const KSharedConfigPtr , KCModule 
> *parent) :
>  QWidget(parent),

we ended up not needing to access KCModule from this class

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

REVISION DETAIL
  https://phabricator.kde.org/D22191

To: filipf, #plasma, ngraham, davidedmundson, #vdg
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart

D22191: Implement syncing of theme preferences between SDDM and Plasma

[Filip Fila]

filipf created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
filipf requested review of this revision.

REVISION 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.

REPOSITORY
  R123 SDDM Configuration Panel (KCM)

BRANCH
  sddm-theme-syncing (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D22191

AFFECTED FILES
  kcm_sddm.actions
  sddmauthhelper.cpp
  sddmauthhelper.h
  src/advanceconfig.cpp
  src/advanceconfig.h
  src/ui/advanceconfig.ui

To: filipf
Cc: plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart