Don't block unmounting when terminal panel's cwd is the mountpoint
ClosedPublic

Authored by martonmiklos on Sep 16 2017, 2:27 PM.

Details

Summary

When unmounting a removable media the KDE performs check if there are some file opne on the device before performing the unmount. If the terminal window in the dolphin is open and the to be unmounted path is open the unmount process will be blocked.

This patch sets the terminal window current path to the home directory upon unmount request if the terminal directory is set to the mount path.

The unmount request could came from two sources:

  • The user could hit right click on the media in the dolphin's places panel and hit unmount.
  • The user could request an unmount from the indicator applet

This patch was originally written by Arjun AK for the KDE4 I think:
https://git.reviewboard.kde.org/r/121613/

Now I rebased to the current dolphin version and fixed the KF5 related changes.

I have added handling the situations when the umounting of the device is not done from the same Dolphin window's places menu (from the notification tray for e.g.)

BUG: 158264

Test Plan

For testing the places menu removal process:

  1. Insert a removable media.
  2. Open it with dolphin
  3. Hit F4 to open up the terminal
  4. Click on the removable device with right mouse button and select safely remove from the Palaces panel

Expected behaviour:
The device should be unmounted without any problems, and the terminal should cd back to the home directory.
You can ensure that the device is unmounted properly by running mount | grep <device path>

For testing the notification area removal process:

  1. Perform Step 1-3 from the previous step
  2. Click to the devicenotifier on the notification area
  3. Click on the umount button next to the current device

Expected behaviour:
The device should be unmounted and a Device can be safely removed now shall be shown.

Diff Detail

Repository
R318 Dolphin
Lint
Lint Skipped
Unit
Unit Tests Skipped
martonmiklos created this revision.Sep 16 2017, 2:27 PM
zhigalin added inline comments.
src/dolphinmainwindow.cpp
1011

It really should be inside else?

martonmiklos marked an inline comment as done.Sep 16 2017, 6:24 PM
martonmiklos added inline comments.
src/dolphinmainwindow.cpp
1011

Yes.

The purpose of this condition is basically delaying the call of proceedWithTearDown() when the terminal is visible until the terminal changed back to the home folder.

If the terminal was visible the proceedWithTearDown will be called after the terminal successfully cd-ed back to the home directory: in the DolphinMainWindow::slotTerminalDirectoryChanged(const QUrl& url) see line 253.

Rest LGTM, do you have commit rights?

CC @emmanuelp @elvisangelaccio

I'm not sure I agree with this patch.
A standard behavior in other filemanagers when umounting a device is to redirect to the view from the device URL to the home URL.
So imho this patch is using the wrong approach: instead of the mainwindow telling the terminal panel to go to the home, it should be the mainwindow the one that changes its URL to the home.

This way:

  1. We get an usable DolphinView after unmounting the device (matching the behavior of e.g. Nautilus)
  2. We don't need the goHome() method in TerminalPanel, it would be automatic.
martonmiklos marked an inline comment as done.Sep 17 2017, 6:17 PM

I'm not sure I agree with this patch.
A standard behavior in other filemanagers when umounting a device is to redirect to the view from the device URL to the home URL.
So imho this patch is using the wrong approach: instead of the mainwindow telling the terminal panel to go to the home, it should be the mainwindow the one that changes its URL to the home.

This way:

  1. We get an usable DolphinView after unmounting the device (matching the behavior of e.g. Nautilus)
  2. We don't need the goHome() method in TerminalPanel, it would be automatic.

Currently the mainwindow does not go anywhere (keep showing the mount path, but with empty contents) which is a rather odd behaviour I think.

Give me some time to refactor the patch.

Just to doublecheck what I would like to do:

  • If the unmount request come in (from two sources: panel unmount or the Solid::StorageAccess::teardownRequested) then the Dolphin shall:
    • Iterate through all open tabs of the Dolphin instance and if the current tab's path starts with the mounted path then that tab should be redirected to the home directory.
    • There is one corner case what I can think about: if there is a symlink to the mounted content this approach will fail. Technically we could iterate through the directory tree which make the path to see if it is a symlink and check if it points to the mount path, but I think that might not have much added value.
martonmiklos planned changes to this revision.Sep 29 2017, 1:50 PM

@elvisangelaccio
I have implemented the "storage teardown requested -> set the opened tabs to home if mounting path is open" method, but this approach cannot close the terminal in time when the teardown request came externally. (Umount requested from the indicator applet for e.g.). The tabs are set to the home directory properly both the terminal, but the unmount manager says that the mountpath is opened in an application.
I would rather merge the two approaches: asking the terminal to cd to home first and then loop over the tabs and set them to home if mountpath is opened.

ngraham requested changes to this revision.Sep 29 2017, 9:01 PM

Thanks for submitting this patch! I'd like to request some changes:

  • Add "BUG: 158264" to the summary so that bug will be automatically updates and closed once this goes in
  • Rewrite the Title to describe the change, not the problem
  • Rewrite the Summary to offer a slightly more in-depth explanation of the change
martonmiklos retitled this revision from Fix bug 158264 - Terminal panel (konsole part) blocks the umount of removable media to Change the terminal panel opened directory to the home directory if a to be unmounted path is open to prevent the blocking of the unmounting process.Oct 1 2017, 9:04 AM
martonmiklos edited the summary of this revision. (Show Details)

I can't judge the patch, but if this is accepted remember to:

  • use the authorship information of Arjun AK
  • and possibly add a Co-Authored-By: line with the information of martonmiklos
martonmiklos added a comment.EditedOct 1 2017, 10:21 AM

@elvisangelaccio
After several trials I consider the "set unmounted directory to home" out of scope of this patch because I have not found any method which could reliably set the tabs with unmounted paths to home in time.
At the moment I can only think about some QTimer based hack, but I would not like to include it to this patch.

However during these tests I have identified an issue with this pathc: the terminal could block the unmount even if it is hidden, so an updated patch is coming soon.

Thanks for those stylistic changes, @martonmiklos. However the title is still a bit long. How about something more like this:

"Don't block unmounting when terminal panel's cwd is the mountpoint"

Thanks for those stylistic changes, @martonmiklos. However the title is still a bit long. How about something more like this:

"Don't block unmounting when terminal panel's cwd is the mountpoint"

Aye, I felt that the title is not the best. Thanks for the hint, I have changed it!

martonmiklos retitled this revision from Change the terminal panel opened directory to the home directory if a to be unmounted path is open to prevent the blocking of the unmounting process to Don't block unmounting when terminal panel's cwd is the mountpoint.Oct 1 2017, 3:35 PM
elvisangelaccio requested changes to this revision.Oct 1 2017, 9:37 PM

Thanks for the updated diff, please fix the inline issues.

src/dolphinmainwindow.cpp
253

Please use QUrl::fromLocalFile().

src/dolphinmainwindow.h
420

Nitpick, please remove the space between & and the argument. Same in the other occurrences below.

src/panels/places/placesitemmodel.cpp
595

nullptr

962–963

Please use the new syntax:

connect(m_deviceToTearDown, &Solid::StorageAccess::teardownDone,
            this, &PlacesItemModel::slotStorageTearDownDone);
src/panels/places/placesitemsignalhandler.h
67

This signal should have a more descriptive name. Maybe tearDownExternallyRequested() ?

src/panels/places/placespanel.h
50

Missing pass-by-reference

src/panels/terminal/terminalpanel.cpp
67

Prefer QString().

src/panels/terminal/terminalpanel.h
77

boolean arguments are a bad practice (bad readability of code), please use an enum instead.

This revision now requires changes to proceed.Oct 1 2017, 9:37 PM
martonmiklos marked an inline comment as done.
martonmiklos marked 8 inline comments as done.Oct 2 2017, 6:35 PM

Many thanks for the reviews, I have fixed them.

I can't judge the patch, but if this is accepted remember to:

  • use the authorship information of Arjun AK
  • and possibly add a Co-Authored-By: line with the information of martonmiklos

@ltoscano
Is this something what I need to setup here in phabricator, or this is going to be part of the git commit process? (Sorry for the newbie question this is my second KDE patch.)

ngraham accepted this revision.Oct 2 2017, 7:02 PM
cfeck added a subscriber: cfeck.Oct 4 2017, 12:57 PM

You can change the author of the last commit in your local repo (which should be the change in question) with git commit --amend --author="Author Name <email@address.com>"

The authorship information in the git commit is what shows up in the repo.

@elvisangelaccio, is this acceptable now?

elvisangelaccio requested changes to this revision.Oct 11 2017, 9:20 PM
elvisangelaccio added inline comments.
src/panels/terminal/terminalpanel.h
48

Why public? This is only used in a private method.
Also, we are using enum class for new enums, so we should do it also here for consistency.

49

I'd use AddToHistory and SkipHistory.
Not sure about the name of the enum itself, maybe HistoryPolicy ?

This revision now requires changes to proceed.Oct 11 2017, 9:20 PM
martonmiklos marked 2 inline comments as done.
martonmiklos added inline comments.
src/panels/terminal/terminalpanel.h
49

Thanks for the review, I agree and I have updated the requested changes.

martonmiklos marked 2 inline comments as done.Oct 12 2017, 5:54 PM

@elvisangelaccio, is this all good now?

elvisangelaccio accepted this revision.Oct 12 2017, 9:22 PM
This revision is now accepted and ready to land.Oct 12 2017, 9:22 PM
This revision was automatically updated to reflect the committed changes.