D21466: Recommend rebooting after installing Samba

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R432:8a104aa4fb41: Recommend rebooting after installing Samba 
(authored by ngraham).

REPOSITORY
  R432 File Sharing (Samba) integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21466?vs=58801=77640

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

AFFECTED FILES
  samba/filepropertiesplugin/sambausershareplugin.cpp
  samba/filepropertiesplugin/sambausershareplugin.h

To: ngraham, #vdg, #frameworks, #dolphin, apol, sitter
Cc: anthonyfieroni, sitter, bruns

D21466: Recommend rebooting after installing Samba

[Harald Sitter]

sitter edited the summary of this revision.

REPOSITORY
  R432 File Sharing (Samba) integration

BRANCH
  recommend-rebooting (branched from master)

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

To: ngraham, #vdg, #frameworks, #dolphin, apol, sitter
Cc: anthonyfieroni, sitter, bruns

D21466: Recommend rebooting after installing Samba

[Harald Sitter]

sitter accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R432 File Sharing (Samba) integration

BRANCH
  recommend-rebooting (branched from master)

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

To: ngraham, #vdg, #frameworks, #dolphin, apol, sitter
Cc: anthonyfieroni, sitter, bruns

D21466: Recommend rebooting after installing Samba

[Anthony Fieroni]

anthonyfieroni added a comment.


  Just start services, reboot will not do anything else.

REPOSITORY
  R432 File Sharing (Samba) integration

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

To: ngraham, #vdg, #frameworks, #dolphin, apol
Cc: anthonyfieroni, sitter, bruns

D21466: Recommend rebooting after installing Samba

[Aleix Pol Gonzalez]

apol added a comment.


  In D21466#626419 , @sitter wrote:
  
  > In D21466#625834 , @apol wrote:
  >
  > > So yes, Discover will notify about updates. That doesn't mean it should 
be shown here too.
  >
  >
  > To clarify: are you -1 this diff?
  
  
  no, sorry, I meant to +1. ETOOMANYNEGATIVES.
  
  >> If anything, we should be pushing for such updates to happen at 
startup/shutdown because it doesn't seem like the user benefits from it being 
swapped at runtime, but that's a different question.
  > 
  > "Such updates" being group changes?
  
  I'd like to know which updates will need a reboot before updating and just 
move them to update offline.

REPOSITORY
  R432 File Sharing (Samba) integration

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

To: ngraham, #vdg, #frameworks, #dolphin, apol
Cc: sitter, bruns

D21466: Recommend rebooting after installing Samba

[Harald Sitter]

sitter added a comment.


  In D21466#625834 , @apol wrote:
  
  > So yes, Discover will notify about updates. That doesn't mean it should be 
shown here too.
  
  
  To clarify: are you -1 this diff?
  
  > If anything, we should be pushing for such updates to happen at 
startup/shutdown because it doesn't seem like the user benefits from it being 
swapped at runtime, but that's a different question.
  
  "Such updates" being group changes?

REPOSITORY
  R432 File Sharing (Samba) integration

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

To: ngraham, #vdg, #frameworks, #dolphin, apol
Cc: sitter, bruns

D21466: Recommend rebooting after installing Samba

[Aleix Pol Gonzalez]

apol added a comment.


  So yes, Discover will notify about updates. That doesn't mean it should be 
shown here too.
  
  If anything, we should be pushing for such updates to happen at 
startup/shutdown because it doesn't seem like the user benefits from it being 
swapped at runtime, but that's a different question.
  
  HTH

REPOSITORY
  R432 File Sharing (Samba) integration

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

To: ngraham, #vdg, #frameworks, #dolphin, apol
Cc: sitter, bruns

D21466: Recommend rebooting after installing Samba

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> ngraham wrote in sambausershareplugin.h:63
> Yes, but I'd prefer to do that in another patch and port all the inline 
> messages at once.

Ah! There's more. Fine with me then.

REPOSITORY
  R432 File Sharing (Samba) integration

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

To: ngraham, #vdg, #frameworks, #dolphin, apol
Cc: sitter, bruns

D21466: Recommend rebooting after installing Samba

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> sitter wrote in sambausershareplugin.h:63
> shouldn't this rather be a KMessageWidget? As I recall we usually use KMWs 
> for this type of notification.

Yes, but I'd prefer to do that in another patch and port all the inline 
messages at once.

REPOSITORY
  R432 File Sharing (Samba) integration

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

To: ngraham, #vdg, #frameworks, #dolphin, apol
Cc: sitter, bruns

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, maaaybe 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

D21466: Recommend rebooting after installing Samba

[Nathaniel Graham]

ngraham added a comment.


  In D21466#471371 , @bruns wrote:
  
  > In D21466#471325 , @ngraham 
wrote:
  >
  > > In D21466#471321 , @bruns 
wrote:
  > >
  > > > For group membership changes, only a relogin is required (at most).
  > >
  > >
  > > I know, but telling people to log out and log back in again involves more 
ways to mess up than just telling them to reboot. It's not really much slower 
or more invasive either.
  >
  >
  > I disagree. How could a relogin mess something up? Rebooting requires 
stopping everything outside the session. It may also require unlocking the 
harddisk, waiting for the computer to boot, repopulating caches ... Requesting 
to reboot may also give some bad press - "Rebooting, is this Windows??? ROFL!!!"
  
  
  I'm talking more about UX than technical concerns. Rebooting can be 
accomplished with a single button; logging out and back in requires a 
multi-step process that offers more ways for a user to mess it up.
  
  > 
  > 
  >>> The only relevant group membership here is the usershare group
  >> 
  >> On Arch the group name is actually `sambashare`. I don't believe there is 
a consistent standard here.
  > 
  > The standard is to retrieve the group from the directory. KSambaShare 
already fetches the directory name from the samba config (to add a watch), 
exposing the owner group is trivial.
  
  How can I do that?

REPOSITORY
  R432 File Sharing (Samba) integration

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

To: ngraham, #vdg, #frameworks, #dolphin, apol
Cc: bruns

D21466: Recommend rebooting after installing Samba

[Stefan Brüns]

bruns added a comment.


  In D21466#471325 , @ngraham wrote:
  
  > In D21466#471321 , @bruns wrote:
  >
  > > For group membership changes, only a relogin is required (at most).
  >
  >
  > I know, but telling people to log out and log back in again involves more 
ways to mess up than just telling them to reboot. It's not really much slower 
or more invasive either.
  
  
  I disagree. How could a relogin mess something up? Rebooting requires 
stopping everything outside the session. It may also require unlocking the 
harddisk, waiting for the computer to boot, repopulating caches ... Requesting 
to reboot may also give some bad press - "Rebooting, is this Windows??? ROFL!!!"
  
  >> The only relevant group membership here is the usershare group
  > 
  > On Arch the group name is actually `sambashare`. I don't believe there is a 
consistent standard here.
  
  The standard is to retrieve the group from the directory. KSambaShare already 
fetches the directory name from the samba config (to add a watch), exposing the 
owner group is trivial.

REPOSITORY
  R432 File Sharing (Samba) integration

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

To: ngraham, #vdg, #frameworks, #dolphin, apol
Cc: bruns

D21466: Recommend rebooting after installing Samba

[Nathaniel Graham]

ngraham added a comment.


  In D21466#471321 , @bruns wrote:
  
  > For group membership changes, only a relogin is required (at most).
  
  
  I know, but telling people to log out and log back in again involves more 
ways to mess up than just telling them to reboot. It's not really much slower 
or more invasive either.
  
  > The only relevant group membership here is the usershare group
  
  On Arch the group name is actually `sambashare`. I don't believe there is a 
consistent standard here.
  
  > If the user does not belong to a group which should be able to write to 
, neither a relogin nor a reboot will help.
  
  Right, this is tracked with https://bugs.kde.org/show_bug.cgi?id=407846 and I 
plan to tackle it next.

REPOSITORY
  R432 File Sharing (Samba) integration

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

To: ngraham, #vdg, #frameworks, #dolphin, apol
Cc: bruns

D21466: Recommend rebooting after installing Samba

[Stefan Brüns]

bruns added a comment.


  For group membership changes, only a relogin is required (at most).
  
  The only relevant group membership here is the usershare group, which can be 
determined from the owner/group of the "usershare path = " directory 
(samba config). If the user can write to the directory already, everything is 
fine. If the user does not belong to a group which should be able to write to 
, neither a relogin nor a reboot will help.

REPOSITORY
  R432 File Sharing (Samba) integration

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

To: ngraham, #vdg, #frameworks, #dolphin, apol
Cc: bruns

D21466: Recommend rebooting after installing Samba

[Nathaniel Graham]

ngraham updated this revision to Diff 58801.
ngraham added a comment.


  Revert unintentional whitespace change

REPOSITORY
  R432 File Sharing (Samba) integration

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21466?vs=58800=58801

BRANCH
  recommend-rebooting (branched from master)

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

AFFECTED FILES
  samba/filepropertiesplugin/sambausershareplugin.cpp
  samba/filepropertiesplugin/sambausershareplugin.h

To: ngraham, #vdg, #frameworks, #dolphin, apol

D21466: Recommend rebooting after installing Samba

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: VDG, Frameworks, Dolphin, apol.
ngraham requested review of this revision.

REVISION SUMMARY
  After Samba is installed, very frequently it will not work correctly until 
the machine is
  rebooted. One potential reason is when the installed package has made group 
membership
  changes, which only take effect after a reboot.
  
  This patch implements a reboot recommendation along with a button to reboot 
the machine.
  Both are shown immediately after Samba has been installed.
  
  Because some expert users may understand the technical details of what's 
going on, the
  reboot is only recommended, not required. If the window is closed and then 
re-opened,
  the normal Samba sharing configuration UI is displayed instead of the reboot 
prompt.
  
  Added a new function rather than using a Lambda because it may be useful for 
additional
  purposes too (e.g. https://bugs.kde.org/show_bug.cgi?id=407846)
  
  FEATURE: 407845
  FIXED-IN: 19.08.0

TEST PLAN
  http://s1.webmshare.com/Ry55q.webm (not uploaded to Phab due to file size 
limit)
  
  1. Remove Samba
  2. Go to share a folder
  3. Click "Install Samba"
  4. After the installation has completed, click the "Restart" button and see 
that the machine reboots
  
  5. Remove Samba again
  6. Go to share a folder
  7. Click "Install Samba"
  8. Close the window and re-open it instead of rebooting as recommended
  9. See that the Samba sharing config UI is all there

REPOSITORY
  R432 File Sharing (Samba) integration

BRANCH
  recommend-rebooting (branched from master)

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

AFFECTED FILES
  samba/filepropertiesplugin/sambausershareplugin.cpp
  samba/filepropertiesplugin/sambausershareplugin.h

To: ngraham, #vdg, #frameworks, #dolphin, apol