D16141: Disable unmount option for / or /home

[Thomas Surrel]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:45597c32627f: Disable unmount option for / or /home 
(authored by thsurrel).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16141?vs=45378=45437

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

AFFECTED FILES
  src/filewidgets/kfileplacesview.cpp

To: thsurrel, #frameworks, ngraham
Cc: dfaure, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[Nathaniel Graham]

ngraham added a comment.


  Cool, go for it!

REPOSITORY
  R241 KIO

BRANCH
  arc_unmount (branched from master)

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

To: thsurrel, #frameworks, ngraham
Cc: dfaure, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[Thomas Surrel]

thsurrel added a comment.


  In D16141#341553 , @ngraham wrote:
  
  > However, I'm afraid this doesn't actually work for me:
  
  
  That's strange, it is the same code than in Dolphin (and it works for me). 
Could you have a look at the placesModel->url(index) != 
QUrl::fromLocalFile(QDir::rootPath()) comparison at tell me what each side 
contains ?

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: dfaure, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[Thomas Surrel]

thsurrel added a comment.


  In D16141#341738 , @dfaure wrote:
  
  > This whole method could put placesModel->url(index) into a local variable 
to avoid calling it so many times, though.
  
  
  Should I do that here or in a different patch ?

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: dfaure, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[David Faure]

dfaure added a comment.


  +1
  
  This whole method could put placesModel->url(index) into a local variable to 
avoid calling it so many times, though.

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: dfaure, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[Thomas Surrel]

thsurrel added a comment.


  If logged in as root, wouldn't QDir::homePath() return /root and not 
/home/ ? You should be able to unmount /home in that case.

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[Thomas Surrel]

thsurrel edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[Kai Uwe Broulik]

broulik added a comment.


  Why would you be unable to unmount `/home`? If you're logged in as root (not 
saying that one should or can), whose home is `/root` you can surely unmount 
`/home`

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[Shubham]

shubham added inline comments.

INLINE COMMENTS

> kfileplacesview.cpp:33
> +#include 
>  
>  #include 

Move this header up

> kfileplacesview.cpp:54
> +#include 
>  
>  #include "kfileplaceeditdialog_p.h"

Move the header up, to its appropriate position letter wise.

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: shubham, ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  Can you add `CCBUG: 399659` to this so that bug report reflects that two 
commits were necessary to fully resolve it? Thanks!
  
  However, I'm afraid this doesn't actually work for me:
  
  F6322782: can still unmount.png 
  
  (D15989  does work for Dolphin though)

REPOSITORY
  R241 KIO

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

To: thsurrel, #frameworks, ngraham
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D16141: Disable unmount option for / or /home

[Thomas Surrel]

thsurrel created this revision.
thsurrel added a reviewer: Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
thsurrel requested review of this revision.

REVISION SUMMARY
  This disables the 'Unmount' context menu in the Places panel for discs
  corresponding to / and /home.
  
  It does not make much sense to offer an option that will always fail.
  
  Twin of D15989  for Dolphin

REPOSITORY
  R241 KIO

BRANCH
  arc_unmount (branched from master)

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

AFFECTED FILES
  src/filewidgets/kfileplacesview.cpp

To: thsurrel, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns