256 really isn't all that big on modern hidpi/4k displays.
Details
Diff Detail
- Repository
- R260 Gwenview
- Lint
Lint Skipped - Unit
Unit Tests Skipped
I love the idea, but what about caching? Cached thumbnails are supposed to follow https://specifications.freedesktop.org/thumbnail-spec/thumbnail-spec-latest.html#THUMBSIZE, which still, sadly, says 256.
it says the sizes are just suggestions, so I don't think bumping it up a bit would violate this.
Thanks for coming back to this. Yes, HiDPI support is still in progress. If you could help with the thumbnail-spec/caching side of things that would be great!
Quotes from the spec:
- Normal: […]The image size must not exceed 128x128 pixel.
- Large: […] must be 256x256 pixel.
- However, these are suggestions. Implementations should cope also with images that are smaller […]
I don't see anything in there about larger thumbnails.
Some observations running Gwenview with this patch:
- Even with the size slider set to 512px, the thumbnails being written to the cache are still 256px. I guess this makes sense, since the "normal" and "large" folders of the cache are meant for 128px and 256px thumbnails respectively as per the spec. However, what's the point of the cache then? Perhaps the spec should be extended to allow for something like a "huge" folder with 512px thumbnails?
- The pixmap used for the thumbnail is still only 256px, this patch merely allows scaling that up to 512px, making it look blurry (can be seen easily by viewing the thumbnail of a screenshot and setting MaxThumbnailSize = 2048). You can achieve the same effect with the default slider maximum of 256px and QT_SCALE_FACTOR=2.
- For large sizes, using the zoom buttons or scrolling over the slider becomes really tedious. It's still acceptable around 256px, but much too slow up to 512px. This should work more like the progressive zoom slider in View mode.
I'm all for allowing larger thumbnail sizes, but I'm not yet comfortable approving the patch. Is showing a large blurry thumbnail really what we want, or should we rather fix things in the proper places? Ideally the spec could be extended, but in any case we'd have to display and (somewhere) store proper 512px thumbnails (which this patch is not implementing – yet?).
@ngraham added reviewers: Konsole, tcanabrava, dhaumann.
Sorry to disagree with you there. Yes, the repo is set wrongly, but title, file list and the diff are quite clear in this case.
Whoops, my bad! It has been so long since I last saw this that I forgot completely what it was about and trusted the Phabricator metadata.
Seems like gnome just went with their own private folder ($XDG_CACHE_HOME/gnome-photos/thumbnails/$SIZE-$GENERATION), though not only because of the size issue:
https://debarshiray.wordpress.com/2018/01/29/gnome-photos-an-overview-of-thumbnailing/
Make sure we avoid the disk cache if we use thumbnails larger than the XDG spec allows.
I am inclined to accept this. It's a nice enhancement that seems to work fine in my testing, and while it doesn't strictly speaking follow the letter of the XDG spec, I don't think there's any sense letting the perfect being the enemy of the good here. We all know how that game would end: tons of wrangling in the FDO world and no official support for 512px thumbnails for years while we wait. I say let's do this now.