Fix image loading performance
ClosedPublic

Authored by hoffmannrobert on Oct 28 2019, 1:44 PM.

Details

Summary

Start the dirlister after loading an image whose url is passed on the command line to make sure that this image is shown before the possibly long running dirlister on a potentially slow device is started.

Test Plan

Mount some slow network filesystem (e.g. cifs) and open an image file
from a directory containing a few hundred files using the command line:
$ gwenview imagefile

It will take several seconds/minutes until the image is shown (until all
preview images are loaded).

After applying this patch the image will be shown immediately and the
preview images after that.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
hoffmannrobert created this revision.Oct 28 2019, 1:44 PM
Restricted Application added a project: Gwenview. · View Herald TranscriptOct 28 2019, 1:44 PM
hoffmannrobert requested review of this revision.Oct 28 2019, 1:44 PM
tommo added a subscriber: tommo.Oct 28 2019, 2:49 PM

make sure that an url passed on the command line is loaded and shown before the possibly long running dirlister

Excuse me, but delaying by those arbitrary 1000 msec doesn't make anything "sure". You neither know how slow the remote connection is, nor the size of the image that is being downloaded.

Why not connecting to AbstractDocumentImpl::loaded (and AbstractDocumentImpl::loadingFailed resp.) and starting the dirlister after those signals have been emitted?

  • Start dirlister on document signals

You are right, I changed it to use the signals from the document loaded (or not).

make sure that an url passed on the command line is loaded and shown before the possibly long running dirlister

Excuse me, but delaying by those arbitrary 1000 msec doesn't make anything "sure". You neither know how slow the remote connection is, nor the size of the image that is being downloaded.

Why not connecting to AbstractDocumentImpl::loaded (and AbstractDocumentImpl::loadingFailed resp.) and starting the dirlister after those signals have been emitted?

tommo added a comment.Oct 29 2019, 3:18 PM

That looks better, thanks. Not sure if it's the best place for the connect, but I'll leave that to Nate.

Another nit-pick from my side: In my opinion that if (UrlUtils::urlIsFastLocalFile()) should be removed. IIRC, Floopy devices, CDs and USB1.1 drives are considered to be "FastLocal". So, the else branch should become the only implementation... would also make testing simpler and better reproducible.

That looks better, thanks. Not sure if it's the best place for the connect, but I'll leave that to Nate.

Another nit-pick from my side: In my opinion that if (UrlUtils::urlIsFastLocalFile()) should be removed. IIRC, Floopy devices, CDs and USB1.1 drives are considered to be "FastLocal". So, the else branch should become the only implementation... would also make testing simpler and better reproducible.

An excellent idea.

The connects at ContextManager::setUrlToSelect() only work with the if (UrlUtils::urlIsFastLocalFile()) determination.

The same determination is done in LoadingDocumentImpl::init(). If the url is fast and local, but the file.open() there fails, the loadingFailed() is emitted before being connected.

I cannot find any suitable place to put the connects into, if this should also work with fast local files.

tommo added a comment.Nov 2 2019, 10:57 AM

The same determination is done in LoadingDocumentImpl::init().

True, but I don't like this implementation either. In fact, this code is executed by the UI thread. And if the user (accidentally) tries to load e.g. 100MiB big file from a USB 2.0 drive (...because he was just looking through the pictures on his drive), the UI thread will be stuck in QFile::readAll() until that file was fully transferred to RAM. On the other hand, if the file was loaded via KIO, it is loaded in chunks without blocking the UI thread, with the option to cancel loading.

If the url is fast and local, but the file.open() there fails, the loadingFailed() is emitted before being connected.

Ok, I can reproduce the problem with remote files as well. How about the following: In the constructor Document::Document(), there is a call to reload() at the end. Move it out, and explicitly call Document::reload() within DocumentFactory::load() after all the signal were connected. That way, it should not be possible anymore that the loading of the image has finished before all signals were connected.

  • Move reload(), add DocumentFactory::readyForDirListerStart()

Thanks, I moved the reload() and added a signal readyForDirListerStart() to DocumentFactory, which is emitted when the document is loaded or failed. In ContextManager::setUrlToSelect() this signal is connected to the dirlister start.

  • Remove unnecessary include
tommo added a comment.Nov 4 2019, 3:44 PM

Ok. I would like to test this, will need a few days though.

hoffmannrobert edited the summary of this revision. (Show Details)Nov 5 2019, 8:57 AM
tommo added a comment.Nov 8 2019, 1:30 PM

I've tested it and it works fine for me, thanks. One minor issue is noted below.

lib/contextmanager.cpp
308

This connect should be moved to the constructor. Otherwise every call to setUrlToSelect() will connect the signal again, causing it to be emitted multiple times.

  • Move connect to constructor
hoffmannrobert marked an inline comment as done.Nov 8 2019, 2:14 PM
hoffmannrobert added inline comments.
lib/contextmanager.cpp
308

Yes, right, moved.

ngraham accepted this revision.Nov 8 2019, 3:01 PM

Nice work.

This revision is now accepted and ready to land.Nov 8 2019, 3:01 PM
This revision was automatically updated to reflect the committed changes.