<table><tr><td style="">sitter 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/D21466">View Revision</a></tr></table><br /><div><div><p>random comment: since we install through polkit the distro should deal with this. polkit can report whether a reboot is necessary, and discover has support for that, so in theory if a distro/polkit backend reports it as necessary (because they added a new group), it should report that to polkit so discover can notify the user. so, maaaaaaaybe this shouldn't actually be decided by us at all. now whether or not we can trust distros to actually take care of this properly is another matter entirely</p>

<p>as for the patch: KSambaShare (part of KIO) ought to get a new function <tt style="background: #ebebeb; font-size: 13px;">bool userCanChange()</tt> or some such which simply runs <tt style="background: #ebebeb; font-size: 13px;">return QFileInfo(userSharePath).isWritable()</tt>. i.e. to determine whether or not a reboot is (like) required you simply need to check whether the user can currently write to the directory where the user shares are stored.<br />
I don't think that is really blocking the diff though. it's a refinement to be sure, but given the reboot notification can be entirely ignored it's hardly a critical blocker for this diff.</p>

<p>LGTM, but I'd prefer if <a href="https://phabricator.kde.org/p/apol/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;">@apol</a> weighs in.</p></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/D21466#inline-152527">View Inline</a><span style="color: #4b4d51; font-weight: bold;">sambausershareplugin.h:63</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: #304a96">#ifdef SAMBA_INSTALL</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">QWidget</span> <span style="color: #aa2211">*</span><span class="n">m_justInstalledSambaWidgets</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">QPushButton</span> <span style="color: #aa2211">*</span><span class="n">m_restartButton</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">shouldn't this rather be a KMessageWidget? As I recall we usually use KMWs for this type of notification.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R432 File Sharing (Samba) integration</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D21466">https://phabricator.kde.org/D21466</a></div></div><br /><div><strong>To: </strong>ngraham, VDG, Frameworks, Dolphin, apol<br /><strong>Cc: </strong>sitter, bruns<br /></div>