Skanlite may create missing output directories automtically (with user's permission)
ClosedPublic

Authored by trufanov on Jan 4 2019, 12:10 AM.

Details

Summary

After first successful scan (or after each one successful scan, depending on settings) Skanlite prompts user with result saving options. These includes output dir and filename. I'm scanning each document to a its own subfolder in my home dir. And at very first scan subfolder usually doesn't exists. So I have to launch Dolphin and create it, or launch directory selection dialog and create subfolder in it.
It would be much simpler for me to just type in subfolder name in output path (it's editable) and let Skanlite create it automatically. That's what this patch proposes.
Now if manually entered output directory doesn't exist then Skanlite asks for user's permission to create them. It should work as mkpath().

Old behavior - Skanlite just fails at file selection and informs user abut that.

Test Plan

Luanch Skanlite, scan something.
It'll ask you for output dir to store result.
Type in non-existing subfolder.
Skanlite shall notice that and ask you to let it create missing folders.
Answer Yes
App shall create dir and save the image.
If No is selected - App informs about impossibility of saving result.

Diff Detail

Repository
R483 Skanlite
Lint
Lint Skipped
Unit
Unit Tests Skipped
trufanov requested review of this revision.Jan 4 2019, 12:10 AM
trufanov created this revision.
trufanov updated this revision to Diff 48653.Jan 4 2019, 12:13 AM

Display dirUrl.path() instead of dir in message as dir will have "file://" schema. It just looks better without it

sars added a comment.Jan 4 2019, 7:05 AM

Hmm... this is only done for local paths. If we only support this feature for local paths it would be much simpler to just use QDir::mkpath(), but it would be nice if it worked for remote folders too...

A second Hmm... should this not be in SaveLocation? The user would be asked and directory created when you click OK. If directory creation fails the SaveLocation dialog would not be closed.

This now create a bit of a dilemma for the dialog... but I don't think it should be impossible to do ;) You would need to special handle the OK button...

Thanks for pushing new features to Skanlite!

trufanov updated this revision to Diff 48680.Jan 4 2019, 5:36 PM

How about that:
Ok for QDir(), but still for local files only
As for SaveLocation - let's just pop up m_saveLocation dialog till user enter a valid directory that we can create.

sars added a comment.Jan 7 2019, 8:25 AM

Besides the processEvents() I'm OK with this! :)

src/skanlite.cpp
392

Is this processEvents() really needed? processEvents() should be avoided... You cannot guarantee what state you are in when you come back from processEvents(). Your object might be deleted.

Besides hasn't the event handler just been running during KMessageBox::questionYesNo() and in m_saveLocation->exec()?

(In the dialogs, you have at least the dialog preventing UI interaction with the parent window.)

trufanov added a comment.EditedJan 7 2019, 1:11 PM
In D17955#387893, @sars wrote:

Besides the processEvents() I'm OK with this! :)

Actually parent->repaint() should be enough. I've tested it with:

on_pushButton_clicked()
{
    if (KMessageBox::questionYesNo(this, "1", "1")) {
        repaint();
        QThread::sleep(5);
    }
}

in newly created app. Dialog disappears with repaint() and still displayed without it.

I tested it, bcs this didn't worked for SkanLite... And it seems that this is libksane's KSaneWidget problem. Bcs if you drag dialog to Skanlite's QDialogButtonBox position at very bottom of the app - then the part of dialog over it is successfully disappear. Only part over KSaneWidget isn't repainted. And Skanlite's main window basically consists of one big KSaneWidget and small QDialogButtonBox near it.
A direct call to m_ksanew->repaint() in Skanlite doesn't help.

So my suggestion is to replace qApp->processEvents(); with parent->repaint();

Later I'll try to find out what's wrong with KSaneWidget's repaint. There are lot of widgets and even some eventFilters,

sars added a comment.Jan 7 2019, 1:18 PM

I'm OK with parent->repaint()

trufanov updated this revision to Diff 48861.EditedJan 7 2019, 1:27 PM

replace qApp->processEvents() with

if (parent) {
    parent->repaint();
}

It seems there are 2 kind of widgets in KSaneWidget that don't repaint if you call KSaneWidget::repaint()
One is QGraphicsView-derived KSaneViewer, bcs it needs viewport()->repaint() instead.
Another are two QScrollAreas in 2 tabs for basic and other options. They need widget()->repaint() instead.

I think there is no way to send anything to KSaneWidget and trigger these. The options are:

  1. Stick with qApp->processEvents()
  2. Implement one more slot in KSaneWidget for full repainting and call d->m_previewViewer->viewport()->repaint() etc. there
  3. In skanlite iterate m_ksanew children, find such objects and call repaint for them. For ex:
for (QAbstractScrollArea* area: parent->findChildren<QAbstractScrollArea*>() ) {
     // findChildren do recursive search by default
     if (area->viewport()) {
         area->viewport()->repaint();
     }
     QScrollArea* scroll = qobject_cast<QScrollArea*>(area);
     if (scroll && scroll->widget()) {
         scroll->widget()->repaint();
     }
 }

Both QScrollAreas and QGraphicsView are derived from QAbstractScrollArea and have viewport(), but only QScrollArea have widget() and its viewport() is null.

I vote for 3rd one.

sars added a comment.EditedJan 8 2019, 10:11 AM

But this all is to just re-paint one time before we freeze for the time it takes to save of the file? Would it be better in the long run to think about moving the saving to a thread?

pino added a subscriber: pino.Jan 8 2019, 10:27 AM
pino added inline comments.
src/skanlite.cpp
380

toLocalFile(), otherwise the result is wrong on Windows (in case you care)

In D17955#388887, @sars wrote:

But this all is to just re-paint one time before we freeze for the time it takes to save of the file? Would it be better in the long run to think about moving the saving to a thread?

Yep. The biggest reason why I want to clear that dialog ghost is bcs from UX it's not clear that the app is hanging and he'll continue to click on dialog buttons thinking that his choice is ignored for some reason. Thats especially wierd for such a small Yes/No dialog.

Moving to a thread would be a better solution but current design won't make it easy to do.
There are 2 modal dialogs and couple of modal message boxes that must be shown. Also 2 time consuming methods after them: KSaneWidget's m_img = m_ksanew->toQImage(m_data, m_width, m_height, m_bytesPerLine, m_format); and QImage::saveToFile(). It seems 1st takes 25% of time and 75% takes the second one.
Also there is a case when toQImage is called just before modal dialog is shown (result preview dialog).
I would leave that case and all modal dialogs in main thread. And consider moving toQImage for other cases and QImage::saveToFile() to a separate thread.
It shouldn't be a big deal for QImage::saveToFile(), but if we do this only for it the KMessageBox's ghost still be hanging for 5 secs bcs of m_ksanew->toQImage();
And moving toQImage to this thread too is more problematic. It's a KSaneWidget's member method. In fact it requires passing of all data as params: QByteArray, size, bytes per line and format. But for some reason it uses current Dpi value of m_ksanew instance.
I would make KSaneWidget's toQImage methods static and add DPI to its parameters, but this'll break compatibility.

Also in Skanlite there is a case when data is saved as QByteArray with KSaneImageSaver::savePngSync. There is async form. This method is called instead of QImage::saveToFile() for 16 bit formats as QImage don't work with them. Change from Sync to Async should be doable.
And for non local file targets there is sync uploading KIO job at very end of saving function. Never tried remote, so can't guess if its moving to another thread will have consequences.

In general I would like to move saving to another thread as it'll allow to start scanning next page a bit faster if previous page was in >600dpi. "A bit faster", bcs Skanlite hanging while saving is partially compensates with a time that scanning caret needs to return to its starting position and user needs to turn the page and put the book back.

So my suggestion is qApp->processEvents or that child iterating func. And I could try moving to QThead but later and by breaking libksane compatibility.

trufanov updated this revision to Diff 49001.Jan 8 2019, 3:54 PM

replaced path() with toLocalFIle()

sars accepted this revision.Jan 9 2019, 7:00 AM

I'm OK with this repaint() and if you want I'm also OK with having the processEvents() until we move the actual saving to a separate thread.

My thread idea is basically that you would add a saveQImage(..) function to KSaneImageSaver and split Skanlite::saveImage() into startSave() and saveDone() (and possibly a third uploadDone())

In stead of using savePngSync() you would use savePng(..) and connect KSaneImageSaver::finished() to saveDone() and the same for the new saveQImage()

This revision is now accepted and ready to land.Jan 9 2019, 7:00 AM
trufanov updated this revision to Diff 49226.Jan 11 2019, 11:03 AM

How about this async approach? I'm still testing. So far it's ok.

@sars could you take a look on thread based version (latest diff)?

sars added a comment.Jan 15 2019, 8:00 PM

Sorry for the late response. A lot of real life activities....

Could we have the create directory and thread stuff in different reviews? The add directory stuff can go in as is.

Thread stuff:

Is the conversion from QByteArray to QImage actually a bottleneck?

If it is a bottleneck we have to figure something out. I'm not totally happy with copying library code to the application. That just means the library has a defect.

Unfortunately the toQimage() functions are not static which means that we cannot use them without an object :(
I don't know if making a function static is a binary compatible change. I suspect not. I tried to make a quick search but could not find a definitive answer.

We can move the convesion code to a static version of the function and have the non-static versions call the static one internally. That is definitely BC.

Another solution is to just pass a KSaneWidget pointer to the KSaneImageSaver object The function does not use any member variables (except the signals which should not be in the static version).

But if we do not need to move toQimage() to the thread we do not have to think about it ;)

Why did you move the image data parameters to the constructor? I can understand that all image types can have the same parameters, but I would prefer to have the parameters passed in the saveXXXASync(...) functions.

Then you can add a KSaneImageSaver member to Skanlite and use a imageSaved() slot in stead of that really long lambda.

In D17955#393880, @sars wrote:

Could we have the create directory and thread stuff in different reviews? The add directory stuff can go in as is.

I would omit create directory step as this message box artifact seems not acceptablde for me. So we can just move to QThread approach and discuss it in this review.

Is the conversion from QByteArray to QImage actually a bottleneck?

It looks like that. There're tight loops but the chunk of raw data is too big to be processed without noticeable delay. I'm scanning in 300 or 600 dpi. 600 dpi full scanner surface gives 5096x7019 image. Even in grayscale with 8bits per pixel this is a 32Mb of raw data.

We can move the convesion code to a static version of the function and have the non-static versions call the static one internally. That is definitely BC.

I'm ok with that approach.

Another solution is to just pass a KSaneWidget pointer to the KSaneImageSaver object The function does not use any member variables (except the signals which should not be in the static version).

It uses. It requests instance's currentDpi() in toQImageSilent(). I don't like that. Even if widgets GUI is blocked one can access it via D-Bus hotkey controls and switch to profile with different dpi before data is saved. That'll corrupt resulting image. So I'm for static approach with additional dpi argument in this function. As this change should be done in libksane I'll create a new review for it. So we'll have 2 reviews. One in libksane for static function and this one for QThread-based image saver.

Why did you move the image data parameters to the constructor? I can understand that all image types can have the same parameters, but I would prefer to have the parameters passed in the saveXXXASync(...) functions.

I just got tired adding new arguments to 4 member functions and decided just move them to constructor as KSaneImageSaver is created and destroyed for each image. I'll rallback to old approach.

Then you can add a KSaneImageSaver member to Skanlite and use a imageSaved() slot in stead of that really long lambda.

That's doable.

trufanov updated this revision to Diff 50042.Jan 22 2019, 5:57 AM

Update to a QThread-based approach of image saving. Now it uses static KSaneWidget::toQImageSilent(). Thus this requires https://phabricator.kde.org/D18446 to be applied.
As it's static it can't emit signals and thus KSaneImageSaver is now getting pointer to KSaneWidget to emit it's signal in case of problems.
Also KSaneImageSaver is now a member of Skanlite class and it's lambda is replaced with new Skanlite's slot.

sars added a comment.Jan 22 2019, 7:25 AM

I think the selection if we save through the PNG 16bit/color or QImage version should be done in Skanlite.cpp not in savePng().

This is almost there :) Thanks for working on this!

src/KSaneImageSaver.cpp
147 ↗(On Diff #50042)

If we replace Private::ImageTypeTIFF with Private::ImageTypeQImage we can do the QImage saving here

158 ↗(On Diff #50042)

We do not need the KSaneWidget here as we already checked for 16bit/color on line 144,145 abd we will never reach this. So the whole KSaneWidget parameter to the constructor can be removed.

162 ↗(On Diff #50042)

This should be moved to a d->saveQImage();

src/KSaneImageSaver.h
41 ↗(On Diff #50042)

Maybe we should just remove the sync versions as we will not be using the sync versions in Skanlite after this.

41 ↗(On Diff #50042)

Why both url and name? From Skanlite.cpp I can see that you have url for the final destination and name for the local file.

I think KSaneImageSaver should not need to know about the KIO remote/local stuff so I would say that we only need a local filename.

41 ↗(On Diff #50042)

Why do you pass a const QImage & to KSaneImageSaver? You don't need to do that from what I can see. I don't think we need a member variable either....

45 ↗(On Diff #50042)

This Tiff saver has always been a stub and I have not heard any request to get it implemented yet so we could just as well remove it until we get a real need for it.

So this would be replaced with saveQImage(.....) for the non 16bit/color saves.

trufanov added inline comments.Jan 22 2019, 9:47 AM
src/KSaneImageSaver.h
41 ↗(On Diff #50042)

It was done just to pass QUrl to imageSaved slot via KSaneImageSaver. I'm trying to avoid case than the slot could be called with wrong url. An alternative would be passing it via SkanLite's class member;

KSaneWIdget allows selection of multiple regions on single page.
In KSaneWidgetPrivate::oneFinalScanDone() after emitting imageReady signal it will continue to scan another region immediately.

So if imageReady() signal is connected with Qt::QueuedConnection we can get Skanlite::imageReady() slot called twice in a raw (each for its region) or something like that.

Skanlite::imageReady slot will launch KSaneImageSaver thread and than could be called again. Second slot will hang on KSaneImageSaver bcs of mutex inside it, but before that it'll change fileUrl.

So passing fileUrl via class member from saveImage() to imageSaved() (that is connected to KSaneImageSaver::imageSaved) is safe unless KSaneWidgetPrivate::imageReady() and Skanlite::imageReady() aren't in different threads and user have several regions selected. And it seems they are definitely in different threads and connection type must be Qt::QueuedConnection.
So current aproach just copies QUrl to KSaneImageSaver instance and protect it from change with QMutex

trufanov updated this revision to Diff 50049.Jan 22 2019, 9:53 AM

One more update, all notes should be addressed in this except for passing fileUrl

sars added a comment.Jan 29 2019, 5:33 PM

Sorry, busy month... new project at work and got a new computer to configure....

I had in mind to have KSaneImageSaver::save16BitPng(...) and KSaneImageSaver::saveQImage(...) so that we can have the warning about supporting 16bit/color only for PNG in Skanlite.cpp and not add that in KSaneImageSaver. As it is right now we don't have the 16bit/color saving.

KSaneImageSaver was originally created to support the 16bit/color images.

Thanks for your patience.

Hi, Kåre!

Sorry, I missed your reply and realized that only now.
I've already forget the code and probably bcs of that didn't get your comment.

As I can see right now the "warning about supporting 16bit/color only for PNG " is already in skanlite.cpp. And no such warning for KSaneImageSaver.
I'll update the diff shortly. Pls check if I got your idea right.

trufanov updated this revision to Diff 51270.Feb 9 2019, 4:41 PM

Now with saveQImage() splitted into saveQImage() and save16BitPng().
And more correct processing of 16bit m_img saving if it's non empty and was already converted inside Skanlite::imageReady bcs of showB4Save->isChecked()

sars added a comment.Feb 10 2019, 9:45 AM

Thanks!

Just these minor adjustments and we are there.

You could also update the commit message to include the move to threaded saving.

This differential is a really good improvement Skanlite :)

src/KSaneImageSaver.cpp
123 ↗(On Diff #51270)

The QString to char* conversion could also be written as qPrintable(m_fileFormat)

src/KSaneImageSaver.h
51 ↗(On Diff #51270)

Move this to the d-pointer Private struct

src/skanlite.cpp
414

enforceSavingAsPng16bit to keep the same variable-naming standard :)

541

This if() statement could theoretically reduce some overall processing time, but I do think that the time to move the data to the disk might be the real bottle neck (m_img.save(...)) so this could give you a UI freeze.

So skip this if() and just do the else :)

sars accepted this revision.Feb 11 2019, 10:11 AM

OK this can go in.

While committing you can change the variable name...

At some point we should also disable the UI so that we don't exit the application or initiate saving the file multiple times while saving the first time. The user might wonder why the dialog is not closed
since he/she already pressed save...

Maybe the m_showImgDialog->close(); should be in Skanlite::saveImage() and m_ksanew disabled until Skanlite::imageSaved(...) is called?

src/skanlite.h
98 ↗(On Diff #51385)

m_imageSaver

trufanov updated this revision to Diff 51408.Feb 11 2019, 1:29 PM

Ok, now with renamed var.

I'm going to commit this as well as required libksane patch today.

Next I would like to push into the project is "D17510 Postponed settings change if scanning is in progress"
https://phabricator.kde.org/D17510

It's a small bugfix. I'll check if its diff requires update after this patch.

This revision was automatically updated to reflect the committed changes.