D29397: KPreviewJob : Support for DeviceRatioPixel

meven abandoned this revision. meven added a comment. With the xdg spec now covering this use case https://gitlab.freedesktop.org/xdg/xdg-specs/-/merge_requests/40 I have reworked this into https://invent.kde.org/frameworks/kio/-/merge_requests/266 REPOSITORY R241 KIO REVISION DETAIL

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a comment. Specification work is so slow I am hesitant to bypass it for now and work on code, even if it means resync with spec later. https://gitlab.freedesktop.org/xdg/xdg-specs/-/merge_requests/35 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29397

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a comment. FYI on this patch serie, I am currently focusing on the specification work : https://gitlab.freedesktop.org/xdg/xdg-specs/-/merge_requests/35 Then I am thinking about moving my patch serie to gitlab reflecting on the last feedback of course. Those patchs are

D29397: KPreviewJob : Support for DeviceRatioPixel

bruns added a comment. In D29397#663902 , @broulik wrote: > > At the risk of asking a stupid question, why? > > The text thumbnailer should be able to produce readable text on high dpi, or the folder thumbnailer should render the folder icon

D29397: KPreviewJob : Support for DeviceRatioPixel

bruns added a comment. Most thumbnailers are completely DPR agnostic, and will create identical thumbnails for large@1 and normal@2. Having both is a waste of disk space and CPU time. The thumbnailer should have a key in its metadata to tell if it depends on the DPR, and only then

D29397: KPreviewJob : Support for DeviceRatioPixel

ngraham added a comment. The proposed additions to the spec are non-controversial IMO. Let's push that forward. I left a supportive comment in the email thread, so maybe it's time to put together a patch that people can comment on. REPOSITORY R241 KIO REVISION DETAIL

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a comment. In D29397#674365 , @kossebau wrote: > To not have that block this improvement, you could in parallel for now use a "kde" namespaced directories, say "*@kde2x/", where we/you could just use the for-now-KF-only theme

D29397: KPreviewJob : Support for DeviceRatioPixel

kossebau added a comment. Thanks for starting the discussion about the spec, by what I saw on the mailinglist. Hopefully that soon gets traction by others. To not have that block this improvement, you could in parallel for now use a "kde" namespaced directories, say "*@kde2x/", where

D29397: KPreviewJob : Support for DeviceRatioPixel

ngraham added a comment. Here's where the spec lives, FWIW: https://gitlab.freedesktop.org/xdg/xdg-specs Expanding it is mostly just a matter of writing up a reasonable proposal in the form of a merge request and getting enough people to agree. Discussing on the mailing list first can

D29397: KPreviewJob : Support for DeviceRatioPixel

kossebau added a comment. In D29397#673830 , @ngraham wrote: > The approach makes sense then. I agree that making high DPI a part of the FDO spec would be nice, but IMO that shouldn't block this. The approach currently taken is logical and it

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added inline comments. INLINE COMMENTS > ngraham wrote in previewjob.cpp:401 > What about if I'm using a 250% scale factor? Maybe there should be an `@3x` > folder too. or just trunate devicePixelRatio to int REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29397

D29397: KPreviewJob : Support for DeviceRatioPixel

ngraham added a comment. The approach makes sense then. I agree that making high DPI a part of the FDO spec would be nice, but IMO that shouldn't block this. The approach currently taken is logical and it could be submitted as an extension to the spec later. INLINE COMMENTS > ngraham wrote

D29397: KPreviewJob : Support for DeviceRatioPixel

kossebau added a comment. @meven Have you already got in contact with the other users/maintainers of he thumnbail cache spec about the idea to extend it with support for high dpi? If not, please consider to do so, so things will also work cross-toolkit/platforms in the future there, by

D29397: KPreviewJob : Support for DeviceRatioPixel

meven marked an inline comment as done. meven added a comment. In D29397#673758 , @ngraham wrote: > Overall seems sane. Two questions though: > Is this @2x suffix standardized? No but it is already being used for icon caching.

D29397: KPreviewJob : Support for DeviceRatioPixel

ngraham added a comment. Overall seems sane. Two questions though: INLINE COMMENTS > previewjob.cpp:401 > +thumbPath = thumbRoot + QLatin1String(cacheWidth == 128 ? "normal" : > "large"); > +thumbPath.append(qFuzzyCompare(devicePixelRatio, 2) ? > QStringLiteral("@2x/") :

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 83140. meven added a comment. Rebasing REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=83139=83140 BRANCH arcpatch-D29397 REVISION DETAIL https://phabricator.kde.org/D29397 AFFECTED FILES

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 83139. meven marked 3 inline comments as done. meven added a comment. Typo, const, doxygen fix REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=82709=83139 BRANCH arcpatch-D29397 REVISION DETAIL

D29397: KPreviewJob : Support for DeviceRatioPixel

elvisangelaccio added inline comments. INLINE COMMENTS > previewjob.cpp:587 > +if > (thumb.textKeys().contains(QStringLiteral("Thumb::DevicePixelRatio"))) { > +qreal dpr = > thumb.text(QStringLiteral("Thumb::DevicePixelRatio")).toDouble(); > +thumb.setDevicePixelRatio(dpr);

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a comment. ping dear reviewers REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29397 To: meven, dfaure, broulik, #frameworks, ngraham Cc: elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a comment. In D29397#672557 , @elvisangelaccio wrote: > Would it make sense to initialize `devicePixelRatio` to `QGuiApplication::devicePixelRatio()` and add an API to change it (if desired) ? It would make KPreviewJob

D29397: KPreviewJob : Support for DeviceRatioPixel

elvisangelaccio added a comment. Would it make sense to initialize `devicePixelRatio` to `QGuiApplication::devicePixelRatio()` and add an API to change it (if desired) ? This way we wouldn't need to call a static method in the `main` of every client app (i.e. D29525

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a reviewer: ngraham. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29397 To: meven, dfaure, broulik, #frameworks, ngraham Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a comment. ping reviewers INLINE COMMENTS > previewjob.cpp:754 > thumb.setText(QStringLiteral("Thumb::Mimetype"), > currentItem.item.mimetype()); > +thumb.setText(QStringLiteral("Thumb::DevicePixelRatio"), > QString::number(thumb.devicePixelRatio())); >

D29397: KPreviewJob : Support for DeviceRatioPixel

meven edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29397 To: meven, dfaure, broulik, #frameworks Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 82709. meven added a comment. Ensure to load images with the right devicePixelRatio REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=82339=82709 BRANCH preview-dpr REVISION DETAIL

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 82339. meven added a comment. Rebasing REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=82335=82339 BRANCH preview-dpr REVISION DETAIL https://phabricator.kde.org/D29397 AFFECTED FILES

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 82335. meven added a comment. Fix thumbpath when in devicepixel subdir REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=82331=82335 BRANCH preview-dpr REVISION DETAIL https://phabricator.kde.org/D29397 AFFECTED

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 82331. meven added a comment. Store thumbnails with dpr 2 in @2x folders REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=82292=82331 BRANCH preview-dpr REVISION DETAIL https://phabricator.kde.org/D29397

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a comment. In D29397#666153 , @kossebau wrote: > In D29397#666134 , @meven wrote: > > > In D29397#666132 , @kossebau wrote: > > > > > For

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added inline comments. INLINE COMMENTS > dfaure wrote in thumbcreator.h:215 > docu? I wonder about moving qreal devicePixelRatio before img parameter REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29397 To: meven, dfaure, broulik, #frameworks Cc: kossebau,

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 82292. meven marked an inline comment as done. meven added a comment. Code style REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=82290=82292 BRANCH preview-dpr REVISION DETAIL https://phabricator.kde.org/D29397

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 82290. meven marked 2 inline comments as done. meven added a comment. Improve documentation, code style REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=82255=82290 BRANCH preview-dpr REVISION DETAIL

D29397: KPreviewJob : Support for DeviceRatioPixel

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > previewjob.cpp:173 > + > +void PreviewJob::setDefaultDevicePixelRatio(qreal defaultDevicePixelRatio) { > +s_defaultDevicePixelRatio =

D29397: KPreviewJob : Support for DeviceRatioPixel

kossebau added a comment. In D29397#666134 , @meven wrote: > In D29397#666132 , @kossebau wrote: > > > For another stupid question (the first one was already asked by someone else and answered :)

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a comment. In D29397#666132 , @kossebau wrote: > For another stupid question (the first one was already asked by someone else and answered :) ): > Given some generated thumbnails are cached, does the thumbnail cache specification

D29397: KPreviewJob : Support for DeviceRatioPixel

kossebau added a comment. For another stupid question (the first one was already asked by someone else and answered :) ): Given some generated thumbnails are cached, does the thumbnail cache specification support logical resolution? How would cached thumbnails work cross-screen?

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 82255. meven marked an inline comment as done. meven added a comment. Improve naming of a variable, fix scaling of the resulting preview REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=82079=82255 BRANCH

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a dependent revision: D29526: Thumbnails: make thumbnail generation dpr-aware. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29397 To: meven, dfaure, broulik, #frameworks Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a dependent revision: D29525: Make Previews devicePixelRatio aware. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29397 To: meven, dfaure, broulik, #frameworks Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 82079. meven added a comment. Fix bitmask check REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=82071=82079 BRANCH preview-dpr REVISION DETAIL https://phabricator.kde.org/D29397 AFFECTED FILES

D29397: KPreviewJob : Support for DeviceRatioPixel

meven planned changes to this revision. meven added a comment. In D29397#664605 , @dfaure wrote: > In D29397#664536 , @meven wrote: > > > In D29397#663800 ,

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 82071. meven marked an inline comment as done. meven added a comment. Add @since to setDefaultDevicePixelRatio REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=82056=82071 BRANCH preview-dpr REVISION DETAIL

D29397: KPreviewJob : Support for DeviceRatioPixel

dfaure added a comment. In D29397#664536 , @meven wrote: > In D29397#663800 , @dfaure wrote: > > > Oh, I thought it was sent as an int. But 8 is QImage::Format_ARGB8565_Premultiplied. Did you mean

D29397: KPreviewJob : Support for DeviceRatioPixel

meven marked 2 inline comments as done. meven added a comment. I have pretty much the patch in kio-extras ready. So I am seeking INLINE COMMENTS > broulik wrote in previewjob.cpp:433 > Not a fan. You can have different dpi per screen. > Maybe instead we should have a `QWindow*` method or

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 82056. meven added a comment. Add a static setDefaultDevicePixelRatio method to PreviewJob REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=81982=82056 BRANCH preview-dpr REVISION DETAIL

D29397: KPreviewJob : Support for DeviceRatioPixel

broulik added inline comments. INLINE COMMENTS > meven wrote in previewjob.cpp:433 > Maybe make this static, so that apps have to do it once per app sort of like > we do with `Qt::AA_UseHighDpiPixmaps`, rather than by KPreviewJob. Not a fan. You can have different dpi per screen. Maybe instead

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a comment. In D29397#663800 , @dfaure wrote: > Oh, I thought it was sent as an int. But 8 is QImage::Format_ARGB8565_Premultiplied. Did you mean 0x80? No I meant 8, since format is passed as quint8 (a single byte, 8-bit) and

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added a comment. In D29397#663902 , @broulik wrote: > > At the risk of asking a stupid question, why? > > The text thumbnailer should be able to produce readable text on high dpi, or the folder thumbnailer should render the folder icon

D29397: KPreviewJob : Support for DeviceRatioPixel

broulik added a comment. > At the risk of asking a stupid question, why? The text thumbnailer should be able to produce readable text on high dpi, or the folder thumbnailer should render the folder icon sharply and the picture frames non-pixelated REPOSITORY R241 KIO REVISION DETAIL

D29397: KPreviewJob : Support for DeviceRatioPixel

davidedmundson added a comment. > Allow users of KPreviewJob to request a devicePixelRatio for generated thumbnails. At the risk of asking a stupid question, why? As opposed to just having a width and height always be in device pixels. We're always working with pixmaps is there a

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 81982. meven added a comment. Improve doc, fix @since REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=81971=81982 BRANCH preview-dpr REVISION DETAIL https://phabricator.kde.org/D29397 AFFECTED FILES

D29397: KPreviewJob : Support for DeviceRatioPixel

meven marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D29397 To: meven, dfaure, broulik, #frameworks Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

meven added inline comments. INLINE COMMENTS > previewjob.cpp:433 > +{ > +d_func()->devicePixelRatio = dpr; > +} Maybe make this static, so that apps have to do it once per app sort of like we do with `Qt::AA_UseHighDpiPixmaps`, rather than by KPreviewJob. REPOSITORY R241 KIO REVISION

D29397: KPreviewJob : Support for DeviceRatioPixel

dfaure added a comment. Oh, I thought it was sent as an int. But 8 is QImage::Format_ARGB8565_Premultiplied. Did you mean 0x80? INLINE COMMENTS > thumbcreator.h:191 > + * @class ThumbCreatorV3 thumbcreator.h > + * @since 5.70 > + */ 5.70 is tagged already REPOSITORY R241 KIO REVISION

D29397: KPreviewJob : Support for DeviceRatioPixel

meven updated this revision to Diff 81971. meven marked 5 inline comments as done. meven added a comment. Add Binary compatibility workarounds REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D29397?vs=81836=81971 BRANCH preview-dpr REVISION DETAIL

D29397: KPreviewJob : Support for DeviceRatioPixel

broulik added inline comments. INLINE COMMENTS > thumbcreator.h:139 > */ > -virtual bool create(const QString , int width, int height, QImage > ) = 0; // KF6 TODO: turn first arg into QUrl (see > thumbnail/htmlcreator.cpp) > +virtual bool create(const QString , int width, int

D29397: KPreviewJob : Support for DeviceRatioPixel

meven marked an inline comment as done. meven added inline comments. INLINE COMMENTS > dfaure wrote in previewjob.cpp:717 > This here also breaks compatibility. Add a KF6 TODO to start the > serialization with a version number. > > Meanwhile a hack is needed, like `if (iFormat & 0x1000) {

D29397: KPreviewJob : Support for DeviceRatioPixel

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > previewjob.cpp:717 > +qreal imgDevicePixelRatio; > +str >> width >> height >> iFormat >> imgDevicePixelRatio; > QImage::Format format

D29397: KPreviewJob : Support for DeviceRatioPixel

meven created this revision. meven added reviewers: dfaure, broulik, Frameworks. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. meven requested review of this revision. REVISION SUMMARY Allow users of KPreviewJob to request a devicePixelRatio for generated