D15989: Do not offer to unmount / or /home

[Thomas Surrel]

thsurrel created this revision.
thsurrel added reviewers: Dolphin, VDG.
Herald added a project: Dolphin.
Herald added a subscriber: kfm-devel.
thsurrel requested review of this revision.

REVISION SUMMARY
This removes 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.

REPOSITORY
R318 Dolphin

BRANCH
arc_unmount_root (branched from master)

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

AFFECTED FILES
src/panels/places/placesitemmodel.cpp

To: thsurrel, #dolphin, #vdg
Cc: kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Surrel]

thsurrel added a comment.


Another option would be to still show the 'Unmount' entry but disabled.

REPOSITORY
R318 Dolphin

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

To: thsurrel, #dolphin, #vdg
Cc: kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added subscribers: bruns, broulik, ngraham.
ngraham added a comment.


Agreed in principle.

If some of the program’s settings are only applicable in certain contexts, do not hide the inapplicable ones. Instead, disable them and hint to the user why they’re disabled. Exception: it is acceptable to hide settings for non-existent hardware. For example, it’s okay to hide the touchpad configuration when no touchpad is present.

This doesn't seem like it falls under the exception, so let's disable it instead of not showing it at all. Bonus points if the disabled item's tooltip tells the user why the disk can't be unmounted!

I wonder if this is the correct way to make the change, though. Perhaps `providesTearDown` should get a third condition that checks for "can unmount", if Solid provides that functionality. @bruns or @broulik, do you know if it does?

REPOSITORY
R318 Dolphin

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

To: thsurrel, #dolphin, #vdg
Cc: ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

placesitemmodel.cpp:215
device.as<Solid::StorageAccess>()->isAccessible();
- if (!providesTearDown) {
+ if (!providesTearDown || item->url() == QUrl("file:/") || item->url() == QUrl("file:/home")) {
return nullptr;

Please use `QUrl::fromLocalFile(QDir::rootPath())` and `QUrl::fromLocalFile("/home")` instead.

REPOSITORY
R318 Dolphin

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

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Surrel]

thsurrel updated this revision to Diff 43032.
thsurrel added a comment.


Disable instead of hiding the entry
Fixes as per elvisangelaccio review

I will still wait for @bruns or @broulik whether we should do that in Solid somehow.

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15989?vs=42985&id=43032

BRANCH
arc_unmount_root (branched from master)

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

AFFECTED FILES
src/panels/places/placespanel.cpp

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Stefan Brüns]

bruns added a comment.

Post by Thomas Surrel
Disable instead of hiding the entry
Fixes as per elvisangelaccio review

Can you update the Title and Summary?

REPOSITORY
R318 Dolphin

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

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

placespanel.cpp:185
teardownAction->setParent(&menu);
+ if (item->url() == QUrl::fromLocalFile(QDir::rootPath()) || item->url() == QUrl::fromLocalFile("/home")) {
+ teardownAction->setEnabled(false);

We should not hardcode "/home" here, but derive it from the user home directory.

REPOSITORY
R318 Dolphin

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

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Surrel]

thsurrel retitled this revision from "Do not offer to unmount / or /home" to "Disable unmount option for / or /home".

REPOSITORY
R318 Dolphin

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

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Surrel]

thsurrel updated this revision to Diff 43078.
thsurrel added a comment.


Do not hardcode /home

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15989?vs=43032&id=43078

BRANCH
arc_unmount_root (branched from master)

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

AFFECTED FILES
src/panels/places/placespanel.cpp

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Surrel]

thsurrel updated this revision to Diff 43079.
thsurrel added a comment.


Typo

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15989?vs=43078&id=43079

BRANCH
arc_unmount_root (branched from master)

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

AFFECTED FILES
src/panels/places/placespanel.cpp

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Surrel]

thsurrel marked an inline comment as done.

REPOSITORY
R318 Dolphin

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

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


We just got a bug report for this: https://bugs.kde.org/show_bug.cgi?id=399659

So please add `BUG: 399659` to the summary here in Phab and then do `arc amend` locally to update the git commit message from the Phab diff.

@elvisangelaccio, does this look good now?

REPOSITORY
R318 Dolphin

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

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Surrel]

thsurrel edited the summary of this revision.

REPOSITORY
R318 Dolphin

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

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: sefaeyeoglu, elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

placespanel.cpp:185
if (teardownAction) {
+ KMountPoint::Ptr mp = KMountPoint::currentMountPoints().findByPath(QDir::homePath());
+ if (item->url() == QUrl::fromLocalFile(QDir::rootPath()) ||

Please use a descriptive name for the variable (e.g. `mountPoint`).

placespanel.cpp:185
if (teardownAction) {
+ KMountPoint::Ptr mp = KMountPoint::currentMountPoints().findByPath(QDir::homePath());
+ if (item->url() == QUrl::fromLocalFile(QDir::rootPath()) ||

Small optimization: we could do the mount points lookup (which isn't a trivial operation) only if we are sure `item->url()` is not the root path.

REPOSITORY
R318 Dolphin

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

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: sefaeyeoglu, elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Surrel]

thsurrel updated this revision to Diff 43420.
thsurrel added a comment.


Fixes as per elvisangelaccio comments

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15989?vs=43079&id=43420

BRANCH
arc_unmount_root (branched from master)

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

AFFECTED FILES
src/panels/places/placespanel.cpp

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: sefaeyeoglu, elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
elvisangelaccio added a comment.
This revision is now accepted and ready to land.


Thanks!

REPOSITORY
R318 Dolphin

BRANCH
arc_unmount_root (branched from master)

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

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: sefaeyeoglu, elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Surrel]

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

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15989?vs=43420&id=43426

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

AFFECTED FILES
src/panels/places/placespanel.cpp

To: thsurrel, #dolphin, #vdg, elvisangelaccio
Cc: sefaeyeoglu, elvisangelaccio, ngraham, broulik, bruns, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp