Disable unmount option for / or /home
ClosedPublic

Authored by thsurrel on Oct 11 2018, 9:38 PM.

Details

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

BUG: 399659

Diff Detail

Repository
R241 KIO
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 11 2018, 9:38 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 11 2018, 9:38 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
thsurrel requested review of this revision.Oct 11 2018, 9:38 PM
ngraham requested changes to this revision.Oct 11 2018, 10:15 PM
ngraham added a subscriber: ngraham.

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:

(D15989 does work for Dolphin though)

This revision now requires changes to proceed.Oct 11 2018, 10:15 PM
shubham added inline comments.
src/filewidgets/kfileplacesview.cpp
33

Move this header up

54

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

shubham removed a subscriber: shubham.Oct 12 2018, 3:20 AM

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

thsurrel edited the summary of this revision. (Show Details)Oct 12 2018, 7:11 AM

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

thsurrel updated this revision to Diff 43443.Oct 12 2018, 7:45 AM

Moved includes

dfaure added a subscriber: dfaure.Oct 12 2018, 7:56 AM

+1

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

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 ?

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 ?

thsurrel updated this revision to Diff 45378.Nov 12 2018, 10:24 PM

Rebase
Cache placesModel->url(index) in the contextMenuEvent function

@ngraham, could you help me debug the problem you are having ?

ngraham accepted this revision.Nov 13 2018, 1:07 AM

Wouldn't you know it, now it works for me. It probably worked all along; I bet I was doing something wrong, sorry!

Do address @dfaure's comment before landing, please.

This revision is now accepted and ready to land.Nov 13 2018, 1:07 AM
thsurrel marked 2 inline comments as done.Nov 13 2018, 8:11 AM

I did make the modification asked by dfaure in the last diff.
So if there is no other comment, I will land this.

Cool, go for it!

This revision was automatically updated to reflect the committed changes.