Postponed settings change if scanning is in progress
ClosedPublic

Authored by trufanov on Dec 11 2018, 6:22 PM.

Details

Summary

Libksane contains a widget that blocks UI controls while scan is in progress. Still settings could be changed programmatically as KSaneWidget::setOptVals() has no any checks for that (while some other public methods have).

As I've made a D-Bus based global hotkeys for Skanlite that could be used to switch its settings profiles this situation now could happen.
(Note: this is a feature of Skanlite master branch and it's not in release. So perhaps I'm only one who actually uses it for now).
I made these hotkeys to control scanning process without GUI while watching fullscreen video (as many sane backends doesn't support hardware buttons). And I scan book pages with different Skanlite settings depending on presence of images and color/grayscale content. I realized that sometimes (in rare cases) I may switch settings profiles before scanning caret is returned and resulting image got saved. That may corrupt the resulting image.

So easiest way to fix this would be check if scanning is in progress and just return false from KSaneWidget::setOptVals() in this case. But this wouldn't satisfy my use case as I'm watching video and don't know that profile switch didn't happen bcs I was impatient. And I don't want switch from video to Skanlite GUI every time I switch settings to make sure this really worked bcs it ruins all idea of global hotkeys. And if I scan some pages with wrong dpi that may be unnoticed later.

Thus the best option for me is to ignore settings change if scanning process is ongoing but remember the values and apply them just after scanning process is done. This allows me to make sure resulting image won't be damaged and that i'll scan next page with settings I expected to set up.

The implementation is simple. Only one KSaneWidget::setOptVals argument's record is stored and only in case it's necessary.
I've considered QMutex but it should be put in both QThreads (preview and scanning) or it'll completely lock GUI. Suggested approach is simplest one. In fact it could be extended to make a stack that contains a history of setting changes and add undo/redo functions to KSaneWidget... but I couldn't think out a use case for this feature.

The only thing bothers me in this diff is KSaneWidget::setOptVals() return values in case settings wasn't really changed but may be changed later. Currently method behaves like no changes are made. And there is no way to inform caller if they actually were applied after current scanning ends.

Diff Detail

Repository
R382 KSane Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
trufanov requested review of this revision.Dec 11 2018, 6:22 PM
trufanov created this revision.
sars added a comment.Feb 11 2019, 3:36 PM

The idea is good. Could we have setOptVals() return -1 and setOptVal() false if the scanning is ongoing and the delayed setting of the value implemented in Skanlite?

The delayed option would be then set when we get the scanDone() signal.

trufanov updated this revision to Diff 51447.Feb 11 2019, 10:19 PM

Ok, in this case this we need to patch 2 projects and this review will contain only a minimal changes.

sars accepted this revision.Feb 12 2019, 7:34 AM

Can you also return false in setOptVal() in case the scanning is ongoing? (and the corresponding note in the doxygen comments)

Thanks,

Kåre
This revision is now accepted and ready to land.Feb 12 2019, 7:34 AM
trufanov updated this revision to Diff 51507.Feb 12 2019, 2:22 PM

oops, made a copy-paste mistake. Updated again

This revision was automatically updated to reflect the committed changes.