D28337: Fix unmounting during preview generation

[Aleksei Nikiforov]

alnikiforov added a comment.


  In D28337#648844 <https://phabricator.kde.org/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.

INLINE COMMENTS

> meven wrote in kfileitemmodelrolesupdater.cpp:496
> The job is finished then, do we really need to kill it ?

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.

> meven wrote in kfileitemmodelrolesupdater.cpp:581
> In the immediate case, the job should be already killed.
> In the not immediate case, this change is not necessary.

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.

> meven wrote in kfileitemmodelrolesupdater.h:131
> I would favor a new method `stop()` equivalent to the immediate case here.

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.

> meven wrote in kfileitemmodelrolesupdater.h:288
> It seems like a "StopPending" or "KillPending" actualy.
> I wonder if that is necessary to have a new State.

"Pause" is used instead of "Stop" or "Kill" due to already used "Pause" word for similar immediate state/action.

REPOSITORY
  R318 Dolphin

REVISION DETAIL
  https://phabricator.kde.org/D28337

To: alnikiforov, elvisangelaccio, ngraham, meven
Cc: kfm-devel, waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, fbampaloukas, alexde, Codezela, feverfew, meven, spoorun, navarromorales, firef, ngraham, andrebarros, emmanuelp, rdieter, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20200522/1df7f9d7/attachment.htm>