Disable unmount option for / or /home
ClosedPublic

Authored by thsurrel on Oct 6 2018, 8:12 PM.

Details

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.

BUG: 399659

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
thsurrel created this revision.Oct 6 2018, 8:12 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptOct 6 2018, 8:12 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
thsurrel requested review of this revision.Oct 6 2018, 8:12 PM

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

Agreed in principle.

Yes, we generally prefer to disable unavailable options rather than removing them. The HIG says:

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?

elvisangelaccio requested changes to this revision.Oct 7 2018, 9:08 AM
elvisangelaccio added a subscriber: elvisangelaccio.
elvisangelaccio added inline comments.
src/panels/places/placesitemmodel.cpp
215 ↗(On Diff #42985)

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

This revision now requires changes to proceed.Oct 7 2018, 9:08 AM
thsurrel updated this revision to Diff 43032.Oct 7 2018, 1:12 PM

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.

bruns added a comment.Oct 7 2018, 2:08 PM

Disable instead of hiding the entry
Fixes as per elvisangelaccio review

Can you update the Title and Summary?

bruns added inline comments.Oct 7 2018, 2:10 PM
src/panels/places/placespanel.cpp
197

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

thsurrel retitled this revision from Do not offer to unmount / or /home to Disable unmount option for / or /home.Oct 7 2018, 6:34 PM
thsurrel updated this revision to Diff 43078.Oct 7 2018, 8:03 PM

Do not hardcode /home

thsurrel marked an inline comment as done.Oct 9 2018, 7:23 PM

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?

thsurrel edited the summary of this revision. (Show Details)Oct 11 2018, 7:42 PM
elvisangelaccio requested changes to this revision.Oct 11 2018, 8:17 PM
elvisangelaccio added inline comments.
src/panels/places/placespanel.cpp
186

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

186

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.

This revision now requires changes to proceed.Oct 11 2018, 8:17 PM
thsurrel updated this revision to Diff 43420.Oct 11 2018, 9:04 PM

Fixes as per elvisangelaccio comments

This revision is now accepted and ready to land.Oct 11 2018, 9:21 PM
This revision was automatically updated to reflect the committed changes.