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 <path>!"

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190710/82e5806d/attachment.html>