<table><tr><td style="">ngraham added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D22191">View Revision</a></tr></table><br /><div><div><p>Overall this is very nice! I have some inline comments, and two macro design comments:</p>

<ol class="remarkup-list">
<li class="remarkup-list-item">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.</li>
</ol>

<ol class="remarkup-list" start="2">
<li class="remarkup-list-item">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.</li>
</ol>

<ol class="remarkup-list" start="3">
<li class="remarkup-list-item">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.</li>
</ol></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D22191#inline-126268">View Inline</a><span style="color: #4b4d51; font-weight: bold;">sddmauthhelper.cpp:57</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">QFile</span><span style="color: #aa2211">::</span><span class="n">exists</span><span class="p">(</span><span class="n">destination</span><span class="p">))</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span class="n">QFile</span><span style="color: #aa2211">::</span><span class="n">remove</span><span class="p">(</span><span class="n">destination</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Don't ignore the error if the removal fails</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D22191#inline-126269">View Inline</a><span style="color: #4b4d51; font-weight: bold;">sddmauthhelper.cpp:60</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">QFile</span><span style="color: #aa2211">::</span><span class="n">copy</span><span class="p">(</span><span class="n">source</span><span class="p">,</span> <span class="n">destination</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">const</span> <span style="color: #aa4000">char</span><span style="color: #aa2211">*</span> <span class="n">destinationConverted</span> <span style="color: #aa2211">=</span> <span class="n">destination</span><span class="p">.</span><span class="n">toLocal8Bit</span><span class="p">().</span><span class="n">data</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Don't ignore the error if the copy fails</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D22191#inline-126270">View Inline</a><span style="color: #4b4d51; font-weight: bold;">sddmauthhelper.cpp:62</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">const</span> <span style="color: #aa4000">char</span><span style="color: #aa2211">*</span> <span class="n">destinationConverted</span> <span style="color: #aa2211">=</span> <span class="n">destination</span><span class="p">.</span><span class="n">toLocal8Bit</span><span class="p">().</span><span class="n">data</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">chown</span><span class="p">(</span><span class="n">destinationConverted</span><span class="p">,</span> <span class="n">sddmUser</span><span class="p">.</span><span class="n">userId</span><span class="p">().</span><span class="n">nativeId</span><span class="p">(),</span> <span class="n">sddmUser</span><span class="p">.</span><span class="n">groupId</span><span class="p">().</span><span class="n">nativeId</span><span class="p">()))</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span style="color: #aa4000">return</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Don't ignore the error if the chown fails</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D22191#inline-126267">View Inline</a><span style="color: #4b4d51; font-weight: bold;">sddmauthhelper.cpp:71</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">sddmConfigLocation</span><span class="p">.</span><span class="n">exists</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span class="n">QDir</span><span class="p">().</span><span class="n">mkpath</span><span class="p">(</span><span class="n">sddmConfigLocation</span><span class="p">.</span><span class="n">path</span><span class="p">());</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Ditto for this and other <tt style="background: #ebebeb; font-size: 13px;">mkpath()</tt> calls. Errors should be handled, even with something as simple as a dialog box saying, "Could not create <path>!"</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R123 SDDM Configuration Panel (KCM)</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D22191">https://phabricator.kde.org/D22191</a></div></div><br /><div><strong>To: </strong>filipf, Plasma, ngraham, davidedmundson, VDG<br /><strong>Cc: </strong>cfeck, GB_2, ndavis, plasma-devel, LeGast00n, jraleigh, fbampaloukas, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart<br /></div>