Unmounting busy device doesn't tell who is blocking
Needs ReviewPublic

Authored by hallas on Mar 23 2019, 8:21 AM.

Details

Reviewers
elvisangelaccio
ngraham
broulik
Group Reviewers
Dolphin
Summary

Unmounting a busy device from the places panel doesn't tell which
applications have open files blocking the unmount. This has been a
feature in the Device Notifier for some time, so this commit copies the
code from there to achieve the same thing.

Test Plan

Mount a USB stick using Dolphin
Open a file from the USB stick
Unmount the USB stick using Dolphin
Observe the new error message.

FEATURE: 189302

Diff Detail

Repository
R318 Dolphin
Branch
show_which_files_are_blocking_unmount (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9997
Build 10015: arc lint + arc unit
hallas created this revision.Mar 23 2019, 8:21 AM
Restricted Application added a project: Dolphin. · View Herald TranscriptMar 23 2019, 8:21 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
hallas requested review of this revision.Mar 23 2019, 8:21 AM
hallas added inline comments.Mar 23 2019, 8:23 AM
src/panels/places/placesitemmodel.cpp
484

I just copied this code from the Device Notifier, but it would be nice to have this as general functionality somewhere

529

What do you think about the wording of these error messages? Currently I just copied the ones from the Device Notifier, but actually I think that it should also state that unmounting has filed, or maybe that Removal has failed.

Solid's signal void setupDone(Solid::ErrorType error, QVariant errorData, const QString &udi); has an errorData where you could probably send this to all interested parties without having to recreate this code all over the place

Solid's signal void setupDone(Solid::ErrorType error, QVariant errorData, const QString &udi); has an errorData where you could probably send this to all interested parties without having to recreate this code all over the place

So what you are saying is to move the queryBlockingApps functionality to Solid? What about the dependency to KSysGuard, is that ok to add to Solid?

I'd also try to avoid copying the code here, is possible.

Solid's signal void setupDone(Solid::ErrorType error, QVariant errorData, const QString &udi); has an errorData where you could probably send this to all interested parties without having to recreate this code all over the place

So what you are saying is to move the queryBlockingApps functionality to Solid? What about the dependency to KSysGuard, is that ok to add to Solid?

As long as it'd be an optional dependency, it should be fine, as I don't see it forbidden here: https://community.kde.org/Frameworks/Policies

But maybe send a mail to kde-frameworks-devel to double check ;)

As long as it'd be an optional dependency, it should be fine, as I don't see it forbidden here: https://community.kde.org/Frameworks/Policies

We can't.
Even ignoring the super confusing dependency loop trying to build frameworks and plasma at best this will break when one of us gets to Qt6 first.

But, there's good news:

This has come up a few times. Kdevelop has a plasma dep because of this same goal, and I want to get rid of that.
I've been meaning for ages to make a class in KF5::CoreAddon that is a super super minimal process list, listing running processes and getting a command line for them.

Effectively we just need to copy the files: https://github.com/KDAB/GammaRay/blob/master/launcher/ui/processlist.h (with the two backends) which are LGPL and currently copied about in a bunch of projects already.

This is yet another motivation.

As long as it'd be an optional dependency, it should be fine, as I don't see it forbidden here: https://community.kde.org/Frameworks/Policies

We can't.
Even ignoring the super confusing dependency loop trying to build frameworks and plasma at best this will break when one of us gets to Qt6 first.

But, there's good news:

This has come up a few times. Kdevelop has a plasma dep because of this same goal, and I want to get rid of that.
I've been meaning for ages to make a class in KF5::CoreAddon that is a super super minimal process list, listing running processes and getting a command line for them.

Effectively we just need to copy the files: https://github.com/KDAB/GammaRay/blob/master/launcher/ui/processlist.h (with the two backends) which are LGPL and currently copied about in a bunch of projects already.

This is yet another motivation.

@davidedmundson - Thanks for the info!

So I am thinking to do the following:

  1. Create a simple ProcessList class as @davidedmundson suggest in KF5::CoreAddon
  2. Move the queryBlockingApps functionality to Solid, but use the ProcessList implementation from KF5::CoreAddon (implement the functionality like @broulik suggests)
  3. Use the new functionality from Solid in Dolphin to show a nice end-user message
  4. Party?

Does that make sense?

Maybe after all this is done, use the general functionality from Solid in the Device Notifier as well? Does anyone know of other places that has the lsof functionality duplicated?

Does anyone know of other places that has the lsof functionality duplicated?

plasma-vault
libksysguard (KLsofWidget)
dataengines/devicenotifications/ksolidnotify

I have pushed the first commit with adding a GetProcessList function in KCoreAddons: D20007

@davidedmundson - Hi David! I am (finally) starting to look at implementing the lsof functionality in Solid, but I am unsure of where exactly to place it in Solid? Also, currently Solid does not depend on any KF5 libraries, this would be the first, is that ok?

meven added a subscriber: meven.Fri, Aug 16, 10:25 AM

@davidedmundson - Hi David! I am (finally) starting to look at implementing the lsof functionality in Solid, but I am unsure of where exactly to place it in Solid? Also, currently Solid does not depend on any KF5 libraries, this would be the first, is that ok?

ping @davidedmundson

Perhaps queryBlockingApps feature should be moved to kcoreaddons as well ?
Then it would depend on apps directly to use it.
Keeping solid clean of process miscellaneous utils.

And since D20007 has landed, this might be a good time to de-duplicate related code in

plasma-vault
libksysguard (KLsofWidget)
dataengines/devicenotifications/ksolidnotify

Also, currently Solid does not depend on any KF5 libraries, this would be the first, is that ok?

Probably ok.

Keeping solid clean of process miscellaneous utils.

It's not really cleaner if it just puts it somewhere else.
If you're dealing with lsof you're probably doing something with a drive, at which point you're doing something with a drive (and using solid) anyway?

And since D20007 has landed, this might be a good time to de-duplicate related code in

Ack. I've been porting the code that uses libksysguard just to get a process name. I haven't touched the lsof cases yet.

Also, currently Solid does not depend on any KF5 libraries, this would be the first, is that ok?

Probably ok.

Keeping solid clean of process miscellaneous utils.

It's not really cleaner if it just puts it somewhere else.
If you're dealing with lsof you're probably doing something with a drive, at which point you're doing something with a drive (and using solid) anyway?

My point was also that KCoreAddons has already KProcessInfo ands KFileSystemType to find files fs types.
So a utility in the same area such as queryBlockingApps might make sense there. It would keep solid from getting a dependency.