D21466: Recommend rebooting after installing Samba

[Harald Sitter]

sitter added a comment.


  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
  
  as for the patch: KSambaShare (part of KIO) ought to get a new function `bool userCanChange()` or some such which simply runs `return QFileInfo(userSharePath).isWritable()`. 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.
  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.
  
  LGTM, but I'd prefer if @apol weighs in.

INLINE COMMENTS

> sambausershareplugin.h:63
> +#ifdef SAMBA_INSTALL
> +    QWidget *m_justInstalledSambaWidgets;
> +    QPushButton *m_restartButton;

shouldn't this rather be a KMessageWidget? As I recall we usually use KMWs for this type of notification.

REPOSITORY
  R432 File Sharing (Samba) integration

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

To: ngraham, #vdg, #frameworks, #dolphin, apol
Cc: sitter, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20200128/7114d1f6/attachment.htm>