Differential D19989

Unmounting busy device doesn't tell who is blocking
ClosedPublic
Actions

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

Details

Reviewers
elvisangelaccio
ngraham
broulik
meven
Group Reviewers
Dolphin
Commits
R318:c3b914a7faed: Unmounting busy device doesn't tell who is blocking
Summary

Unmounting a busy device from the places panel doesn't tell which applications have open files blocking the unmount.

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 17993
Build 18011: 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
476

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

489

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.

broulik added a comment.Mar 23 2019, 8:30 AM

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

hallas added a comment.Mar 23 2019, 8:45 AM
In D19989#436598, @broulik wrote:

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?

elvisangelaccio added a comment.Mar 23 2019, 9:41 AM

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

In D19989#436601, @hallas wrote:
In D19989#436598, @broulik wrote:

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 ;)

davidedmundson added a subscriber: davidedmundson.Mar 23 2019, 10:30 AM

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.

hallas added a comment.Mar 23 2019, 12:21 PM
In D19989#436614, @davidedmundson wrote:

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?

davidedmundson added a comment.Mar 23 2019, 12:46 PM

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

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

hallas added a comment.Mar 23 2019, 9:13 PM

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

hallas added a comment.May 20 2019, 5:24 PM

@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.Aug 16 2019, 10:25 AM
In D19989#467619, @hallas wrote:

@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

davidedmundson added a comment.Aug 16 2019, 11:46 AM

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.

meven added a comment.Aug 16 2019, 12:48 PM
In D19989#512941, @davidedmundson wrote:

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.

meven requested changes to this revision.Oct 2 2019, 9:20 AM

We can now restart this work leveraging KCoreAddon KListOpenFilesJo D21760 .

This revision now requires changes to proceed.Oct 2 2019, 9:20 AM
hallas added a comment.Oct 3 2019, 3:42 AM
In D19989#540686, @meven wrote:

We can now restart this work leveraging KCoreAddon KListOpenFilesJo D21760 .

Yes, I will start rebasing this patch and use KCoreAddon::KListOpenFilesJob :)

hallas added a comment.Oct 3 2019, 3:04 PM
In D19989#540686, @meven wrote:

We can now restart this work leveraging KCoreAddon KListOpenFilesJo D21760 .

Hi @meven , I just realized that I have made a mistake in the commit that adds KListOpenFilesJob, I forgot to add installation of the header file (doh!). I have a fix ready here D24389

hallas updated this revision to Diff 68212.Oct 18 2019, 6:36 AM

Updated to use KListOpenFilesJob

hallas added inline comments.Oct 18 2019, 6:38 AM
CMakeLists.txt
11 ↗(On Diff #68212)

KListOpenFilesJob is available from KDE Frameworks 5.63.0 so I updated this min version, is there a process around this so that packagers are notified about this?

src/panels/places/placesitemmodel.cpp
492 ↗(On Diff #68212)

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.

meven accepted this revision.Oct 18 2019, 4:17 PM

Looks good to me, let's wait for @elvisangelaccio review

This revision is now accepted and ready to land.Oct 18 2019, 4:17 PM
elvisangelaccio requested changes to this revision.Oct 20 2019, 11:18 AM

Please update the commit message, it is no longer true that "this commit copies the code from there to achieve the same thing."

CMakeLists.txt
11 ↗(On Diff #68212)

It's ok to bump the KF5 version as long as we do it before the dependency freeze. No need to notify the packagers.

src/panels/places/placesitemmodel.cpp
489

Feel free to add those errors ;)

481 ↗(On Diff #68212)

Missing this as receiver.

492 ↗(On Diff #68212)

Please use the <application> tag. More details in: https://api.kde.org/frameworks/ki18n/html/prg_guide.html#kuit_markup

493 ↗(On Diff #68212)

Same here

This revision now requires changes to proceed.Oct 20 2019, 11:18 AM
hallas updated this revision to Diff 68474.Oct 21 2019, 7:02 PM
hallas marked 3 inline comments as done.

Review comments

hallas edited the summary of this revision. (Show Details)Oct 21 2019, 7:03 PM
elvisangelaccio requested changes to this revision.Oct 21 2019, 8:09 PM
elvisangelaccio added inline comments.
src/panels/places/placesitemmodel.cpp
481

I meant

connect(listOpenFilesJob, &KIO::Job::result, this, [...](...) { ... });

Without a receiver, the connection could lead to random crashes.

482

Missing const

This revision now requires changes to proceed.Oct 21 2019, 8:09 PM
hallas added inline comments.Oct 22 2019, 4:59 AM
src/panels/places/placesitemmodel.cpp
481

I am not sure I understand what you mean? So is what you are saying that I need to connect the signal to, e.g. a member function?

meven added inline comments.Oct 22 2019, 6:56 AM
src/panels/places/placesitemmodel.cpp
481

I believe he means : connect(listOpenFilesJob, &KIO::Job::result, **this**, [this, listOpenFilesJob](KJob*) {...
Add a receiver at the third parameter before the lambda function.

hallas updated this revision to Diff 68562.Oct 22 2019, 5:37 PM

Review comments

hallas marked 4 inline comments as done.Oct 22 2019, 5:37 PM
hallas added inline comments.
src/panels/places/placesitemmodel.cpp
481

I have made this change, hope it is what you meant :)

elvisangelaccio accepted this revision.Oct 26 2019, 8:24 AM
elvisangelaccio added inline comments.
src/panels/places/placesitemmodel.cpp
481

Yes. Without a receiver, a lambda slot could lead to crashes. More details here: https://medium.com/genymobile/how-c-lambda-expressions-can-improve-your-qt-code-8cd524f4ed9f

This revision is now accepted and ready to land.Oct 26 2019, 8:24 AM
hallas closed this revision.Oct 27 2019, 5:52 AM

Revision Contents
Changeset List

Diff 68474

View Options

CMakeLists.txt

Loading...
View Options

src/CMakeLists.txt

Loading...
View Options

src/panels/places/placesitemmodel.cpp

Loading...