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.
Details
- Reviewers
ngraham - Group Reviewers
Gwenview - Commits
- R260:80719d13843f: Fix image loading performance
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
- Branch
- fix_image_loading_performance
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 18495 Build 18513: arc lint + arc unit
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?
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.
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.
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.
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.
I've tested it and it works fine for me, thanks. One minor issue is noted below.
lib/contextmanager.cpp | ||
---|---|---|
304 | 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. |
lib/contextmanager.cpp | ||
---|---|---|
304 | Yes, right, moved. |