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
