Fix unmounting during preview generation
Needs RevisionPublic
Actions

Authored by alnikiforov on Mar 27 2020, 12:24 PM.

Details

Reviewers

elvisangelaccio
ngraham
meven

Summary

Request all preview jobs for specific mount point to stop
and proceed with unmount request only when all such jobs reported stopping.

This change fixes unmounting when it is initiated from dolphin.
Unfortunately, if unmounting is initiated outside of dolphin,
for example from 'mounted devices' widget,
Solid only sends broadcast notification and doesn't wait for any reply,
thus unmounting may fail due to preview jobs not stopping in time.
But in such case repeated unmount request from user should succeed immediately.

Test Plan

Enable thumbnails and 'Places' panel in Dolphin
Prepare removable device with directory with media files. Make sure that thumbnail generation takes some time. At least 15 seconds should be enough
Remove thumbnails cache. I'm just doing 'rm -rf ~/.cache/thumbnails/*'
Mount removable device and open directory with media files on removable device
While thumbnail generation is in process, i.e. files gradually obtain preview thumbnails, request unmounting of removable device from 'Places' panel in Dolphin

Without this change unmounting will fail unless thumbnails generation job finished running.

Diff Detail

Repository

R318 Dolphin

Lint

Lint Skipped

Unit

Unit Tests Skipped

alnikiforov created this revision.Mar 27 2020, 12:24 PM

Restricted Application added a project: Dolphin. · View Herald TranscriptMar 27 2020, 12:24 PM

Restricted Application added a subscriber: kfm-devel. · View Herald Transcript

alnikiforov requested review of this revision.Mar 27 2020, 12:24 PM

It's also may be possible to repeat unmount request from solid a few times with some delays between requests to try waiting until preview jobs are stopped when unmount is requested from 'mounted devices' widget. I've tried 5 retries with 2 seconds between each one, and it worked for me. But waiting for replies to unmount notifications in solid might be preferable to retries.

ngraham added a reviewer: meven.Apr 14 2020, 8:02 PM

Solid only sends broadcast notification and doesn't wait for any reply, thus unmounting may fail due to preview jobs not stopping in time.

That points to a solid shortcoming worth fixing. Adding a wait for apps to ask solid to wait or to wait by default for instance 20-50ms between tearDownRequested and teardownDone

src/kitemviews/kfileitemmodelrolesupdater.cpp
496	The job is finished then, do we really need to kill it ?
581	In the immediate case, the job should be already killed. In the not immediate case, this change is not necessary.
src/kitemviews/kfileitemmodelrolesupdater.h
131	I would favor a new method `stop()` equivalent to the immediate case here.
288	It seems like a "StopPending" or "KillPending" actualy. I wonder if that is necessary to have a new State.

This revision now requires changes to proceed.Apr 15 2020, 11:21 AM

In D28337#648844, @meven wrote:
Solid only sends broadcast notification and doesn't wait for any reply, thus unmounting may fail due to preview jobs not stopping in time.
That points to a solid shortcoming worth fixing. Adding a wait for apps to ask solid to wait or to wait by default for instance 20-50ms between tearDownRequested and teardownDone

On one hand it's shortcoming, on another hand it can be seen as design choice which means that other applications shouldn't be able to make solid wait or to block it from unmounting. There's no way for applications to ask solid to stop unmounting or to wait a bit. Of course, applications can just keep some files open which happens in use-case this change tries to fix at least partially.

And more importantly, such big change to solid architecture is much bigger change.

src/kitemviews/kfileitemmodelrolesupdater.cpp
496	My experiments indicate that until killPreviewJob() is called, thumbnail process may continue to process files, thus blocking unmounting removable device. It might be case that I missed some better way to stop thumbnail process.
581	AFAIK, slotPreviewFailed and slotGotPreview is called for each file processed by thumbnail process. And such process may work on a sequence of files. There's explicit check for immediate case: in immediate case m_state never equals to PausePending. In non-immediate case previewJobFinished would never be called and thumbnail process would continue to work until it's killed.
src/kitemviews/kfileitemmodelrolesupdater.h
131	immediate = true is old case, thus it's default value for sourcecode-level compatibility. By renaming it sourcecode-level compatibility would be broken. I agree that it looks more like 'stopped' state than 'paused', but I didn't want to rename it since it's called this way already.
288	"Pause" is used instead of "Stop" or "Kill" due to already used "Pause" word for similar immediate state/action.

Revision Contents
Changeset List

			Path	Packages
M			src/dolphinmainwindow.h (9 lines)
M			src/dolphinmainwindow.cpp (55 lines)
M			src/dolphintabpage.h (7 lines)
M			src/dolphintabpage.cpp (27 lines)
M			src/dolphintabwidget.h (12 lines)
M			src/dolphintabwidget.cpp (15 lines)
M			src/kitemviews/kfileitemlistview.h (9 lines)
M			src/kitemviews/kfileitemlistview.cpp (10 lines)
M			src/kitemviews/kfileitemmodelrolesupdater.h (10 lines)
M			src/kitemviews/kfileitemmodelrolesupdater.cpp (46 lines)
M			src/views/dolphinview.h (12 lines)
M			src/views/dolphinview.cpp (6 lines)