Only generate high resolution thumbs if the user cares to not throw them away when closing gwenview, that is when deleteThumbnailCacheOnExit() is set to false. If set to true, prefer speed over quality: always use the embedded thumbnail regardless of its size.
Details
- Reviewers
ngraham - Group Reviewers
Gwenview - Commits
- R260:081efe6e1547: Optionally use images' embedded thumbnails for greater performance, even if…
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Attached is a screenshot demonstrating this suggestion. Even at 512px big thumbs, the embedded thumbnail (160x120) is big enough to get a rough idea of what's on the picture: A landscape, an animal, maybe which one? (not sure if you can tell the difference between a horse and a donkey, but IMO this isn't the purpose of a thumbnail preview anyway.)
Note that this feature is esp. useful when GwenviewConfig::deleteThumbnailCacheOnExit() is true.
One might argue, that those blurred thumbs are not pleasant to look at. In this case one might think about switching off the smooth scaling interpolation then and just show the pixelated preview, which would look like:
P.S.: The unit test fails expectedly. However, I'd first like to get some feedback before fixing it.
I think blurry is better than pixellated.
Speed is indeed important, but so is quality. I'm worried that if we do this, we'll get a huge flood of bugs complaining about blurry or pixellated thumbnaile. As a user, I would file one such bug.
What might make more sense is to load the small thumbs (if available) immediately for maximum speed, and then once that's done, begin lazily loading larger and more detailed thumbnails for any image where a small one is being used. That way we'll get the best of both worlds: speed and quality! What do you think?
I really can't get used to high quality thumbnails. The problem with your approach is that it puts (IMO unnecessary) load on memory bus and CPU: First we would crawl through all files in the directory to quickly display the embedded thumbnail and in a second run we open and decode picture by picture just for generating nice high quality thumbs. This reminds me of https://bugs.kde.org/show_bug.cgi?id=331435
Counter-proposal: Only generate high resolution thumbs if the user cares to not throw them away when closing gwenview, that is when deleteThumbnailCacheOnExit() is set to false. If set to true, always use the embedded thumbnail regardless of its size.
Oh, and I would prefer pixelated.
Okay, that seems reasonable to me since it's an option that's not on by default. With this proposal, that option basically turns into a high performance/low resource usage toggle, which I can get behind. We might want to adjust the wording in the settings dialog to mention the new behavior. Maybe something like this:
Thumbnails: [ ] Low resource usage mode Don't store thumbnails on disk, and prefer lower-quality thumbnails that are faster to load, when available Be careful: blablabla
BTW:
I really can't get used to high quality thumbnails.
Oh, and I would prefer pixelated.
You may be a bit different from Gwenview's target user, JFYI. :) That's just fine, most of us software developers are weirdos with very different preferences than our users, but it's important to keep that in mind. :) As a general rule most people prefer smoothness and quality over blistering speed, when a choice must be made for one or the other. We should default to beauty, even if the quality/speed trade-off is configurable.
Docs updated. Need to apply https://phabricator.kde.org/D21329 first, to avoid conflict.
Thanks in advance for fixing this minor typo.
lib/thumbnailprovider/thumbnailgenerator.cpp | ||
---|---|---|
123 | Typo: thumnail -> thumbnail |
Thanks. To account for this behavioral change, I think we need to adjust the UI strings in the settings window as well. Did my suggestion above make sense?
Thumbnails: [ ] Low resource usage mode Don't store thumbnails on disk, and prefer lower-quality thumbnails that are faster to load, when available Be careful: blablabla
app/advancedconfigpage.ui | ||
---|---|---|
51 ↗ | (On Diff #58530) | avoid to store -> avoid storing |
doc/index.docbook | ||
849 ↗ | (On Diff #58530) | Same ^^ |
Thanks, just one last little thing...
tests/auto/thumbnailprovidertest.cpp | ||
---|---|---|
226 ↗ | (On Diff #58583) | space after the if |
You guys really have to setup clang-format or astyle to automate those whitespace formatting issues, IMO.
app/advancedconfigpage.ui | ||
---|---|---|
51 ↗ | (On Diff #58615) | I haven't checked which folder Gwenview actually tries to delete, but the thumbnails are stored in ~/.cache/thumbnails instead of ~/.thumbnails. |
I haven't checked which folder Gwenview actually tries to delete, but the thumbnails are stored in ~/.cache/thumbnails instead of ~/.thumbnails.
Indeed, the thumbnail cache location was changed in 574ff87ed9f1e761cd958045a57f937c0c2becac to ~/.cache/thumbnails which will be deleted in case.
Regarding astyle, see
Oh, I see, thanks!