D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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
  https://phabricator.kde.org/D29397

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: bruns, elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: bruns, elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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 getting old and are going to need some degree of rebasing.

INLINE COMMENTS

> meven wrote in previewjob.cpp:754
> Maybe add it optionally, when different from 1

Not needed.

> bruns wrote in previewjob.cpp:699
> mixing floating point and integers for a memory-allocation like function is a 
> real bad idea.

I am thinking about making devicePixelRatio an int.

> previewjob.h:196
> + *
> + * @since 5.71
> + */

update

> dfaure wrote in thumbcreator.h:191
> 5.70 is tagged already

update

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29397

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: bruns, elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham

D29397: KPreviewJob : Support for DeviceRatioPixel

[Stefan Brüns]

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 sharply and the picture 
frames non-pixelated
  
  
  The folder thumbnailer does not care for DPR, the folder backdrop is rendered 
at full native resolution, and the sub-thumnails are rendered with a scale 
~0.45 on top.
  
  The text thumbnailer currently renders previews in the file list with a 
height of either 256 or 128, and then scales it down. For an icon size of e.g. 
64 the 128px thumb is scaled down, so the effective DPR becomes nativeDpr * 
0.5. Assuming a native DPR of 2.0 (HiDPI), should the text in this case be 
rendered with a DPR of 1.0 or 2.0? One will be blurry, and the other one will 
hardly contain any text at all (how many characters can you squeeze into 
48x64px?).
  
  For dolphin, there is actually another reason for blurriness. The thumbnail 
icons are scaled so after adding the drop shadow the icon has the selected size 
- at 256px, the 256px thumbnail is scaled to 250px. For natural images (Photos, 
Video thumbnails, cover images ...) this is hardly visible, but very noticeable 
for text.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29397

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: bruns, elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham

D29397: KPreviewJob : Support for DeviceRatioPixel

[Stefan Brüns]

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 different thumbnails should be created.

INLINE COMMENTS

> previewjob.cpp:699
>  }
> -shmid = shmget(IPC_PRIVATE, cacheWidth * cacheHeight * 4, IPC_CREAT 
> | 0600);
> +shmid = shmget(IPC_PRIVATE, cacheWidth * cacheHeight * 4 * 
> devicePixelRatio * devicePixelRatio, IPC_CREAT | 0600);
>  if (shmid != -1) {

mixing floating point and integers for a memory-allocation like function is a 
real bad idea.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29397

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: bruns, elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, 
LeGast00n, cblack, michaelh, ngraham

D29397: KPreviewJob : Support for DeviceRatioPixel

[Nathaniel Graham]

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
  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

[Méven Car]

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 extension. And once there is an agreed specification 
extension, the code could be switched to just any non-namespace shared hidpi 
folder matching whatever the specification ended up to be.
  
  
  Well I don't think it is needed, since those @2x are not used by anyone as 
far as I can tell.
  I'd rather move on here without the spec finalized and then update the 
implementation once it is finalized.
  I don't expect it to be much different from the current implementation, and I 
hope this implementation serve the specification process since we will have an 
implementation to debate on.
  And this will save two commits that serves no purpose IMO, we are not Web 
Browsers after all, we are not breaking anyones business.
  
  Alternatively I'd rather wait for the spec to be finalized.
  Just to avoid the friction.

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

[Friedrich W. H. Kossebau]

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 we/you could just use the 
for-now-KF-only theme extension. And once there is an agreed specification 
extension, the code could be switched to just any non-namespace shared hidpi 
folder matching whatever the specification ended up to be.

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

[Nathaniel Graham]

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 help, to gauge people's opinions.

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

[Friedrich W. H. Kossebau]

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 could be submitted as an extension to the 
spec later.
  
  
  It might be seen logical to us from our current POV, but better to do as 
desktop developer generations have done before and try to get others on board 
from the start, once there is a first plan. Would we want others do just extend 
specs on their own and start to write stuff without namespacing onto common 
data grounds? I suspect: no :) So we better behave as well as we would like 
others to do.

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

[Méven Car]

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

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Nathaniel Graham]

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 in previewjob.cpp:401
> Is this @2x suffix standardized? What happens if I'm using 125% scaling, 
> generate some previews, and then switch back to 100% (no scaling?)

What about if I'm using a 250% scale factor? Maybe there should be an `@3x` 
folder too.

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

[Friedrich W. H. Kossebau]

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 being based on an agreed & formalized specification. 
Not being involved here or having full understanding of the topic, but I would 
guess your approach with the separate x2 should run into "make sense" 
responses, so the additional effort might be low for the gain of being based on 
an official spec.
  
  This would be discussed on 
https://lists.freedesktop.org/mailman/listinfo/xdg, possible best by cc:ing 
other potential parties interested in highdpi thumbnails, would need to be 
researched, possibly some Dolphin contributors know their counterparts in 
non-KDE FLOSS projects

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

[Méven Car]

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.
  
  > What happens if I'm using 125% scaling, generate some previews, and then 
switch back to 100% (no scaling?)
  
  When your screen is scaled 125%, the application gets a dpr of 2.
  When you go back to 100%. the thumbnails in `~/.cache/thumbnails/large` and 
`~/.cache/thumbnails/normal` will be used again.
  Meaning you will have two sets of previews.
  
  > The images using a pixel ratio are stored in cache in 
~/.cache/thumbnails/normal@2x for 256*256 (i.e 128*128@2x) and 
~/.cache/thumbnails/large@2x for 512*512 (i.e 256*256@2x)

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

[Nathaniel Graham]

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/") : QStringLiteral("/"));
> +

Is this @2x suffix standardized? What happens if I'm using 125% scaling, 
generate some previews, and then switch back to 100% (no scaling?)

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

[Méven Car]

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
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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
  https://phabricator.kde.org/D29397

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks, ngraham
Cc: elvisangelaccio, kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, 
cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Elvis Angelaccio]

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);

Missing `const`

> previewjob.h:192
> +/**
> + * Request preview to use the device pixel ratio @p.
> + * The returned thumbnail may not respect the device pixel ratio 
> returned.

Did you mean `@p dpr`?

(unless doxygen is smart enough to figure out the name of the parameter?)

> previewjob.h:268
> + */
> +static void setDefaultDevicePixelRatio(qreal devicePixerRatio);
>  };

Typo: `devicePixelRatio`.

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

[Méven Car]

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

[Méven Car]

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 casting a `QCoreApplication::instance` to 
`QGuiApplication::devicePixelRatio` that would be tolerable.
  
  But this would make the feature opt-out : apps could get broken when their 
previews double their size just when changing KF version.
  
  > This way we wouldn't need to call a static method in the `main` of every 
client app (i.e. D29525  wouldn't be 
needed).
  
  This is a single static call for the lifetime of the app, not that hard.
  This makes the feature opt-in, meaning the developer has tested it.

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

[Elvis Angelaccio]

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  wouldn't be 
needed).

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

[Méven Car]

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

[Méven Car]

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()));
>  QString thumbnailerVersion = 
> currentItem.plugin->property(QStringLiteral("ThumbnailerVersion"), 
> QVariant::String).toString();

Maybe add it optionally, when different from 1

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

[Méven Car]

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

[Méven Car]

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
  https://phabricator.kde.org/D29397

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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 FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

meven added a comment.


  In D29397#666153 , @kossebau wrote:
  
  > 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 :) ):
  > > >  Given some generated thumbnails are cached, does the thumbnail cache 
specification support logical resolution?
  > >
  > >
  > > I am evolving here the specification : the images stored are stored with 
their HiDpi size (width * dpr, height * dpr), and then some metadata is written.
  >
  >
  > Specification as in, 
https://specifications.freedesktop.org/thumbnail-spec/thumbnail-spec-latest.html?
 So there is some work on extending the specs not yet reflected on that page?
  
  
  Thanks for the reference.
  I meant I am beyond the spec, adding some by facts, that is not great :/
  This spec is showing its age, and would need to evolve to cover HiDPi use 
case, but I am not editing it ATM.
  
  > How is the metadata written?
  
  line 751 of PreviewJob.
  
  >>> How would cached thumbnails work cross-screen?
  >> 
  >> Cache thumbnails with low dpr will look blurry on hidpi screen, other than 
that's not an issue.
  > 
  > Cached thumbnails rendered for hidpi (so e.g. being 256x256 with bigger 
details due to hidpi context) , but used for lowdpi elsewhere will be an issue 
as well. E.g. when it comes to text rendered with minfontsize like in case of 
plain text documents, it will be too large then.
  > 
  > I wonder if the thumbnail cache would not need the same extension like the 
icon spec had, with a @2x variant, to make up for that. There are still a lot 
of lowdpi devices out there, and they are getting mixed.
  
  That looks like the way to go, I would favor adding normal@2x and large@2x in 
.cache/thumbnails

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

[Méven Car]

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, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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
  https://phabricator.kde.org/D29397

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, cblack, 
michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[David Faure]

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 = defaultDevicePixelRatio;

coding style: '{' on separate line

> thumbcreator.cpp:52
> +
> +bool ThumbCreatorV3::create(const QString , int width, int height, 
> QImage ) {
> +return createV3(path, width, height, img, 1);

'{' on its own line for methods

> thumbcreator.h:215
> + * @param img The QImage to store the preview in.
> + * @param devicePixelRatio
> + *

docu?

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

[Friedrich W. H. Kossebau]

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 :) ):
  > >  Given some generated thumbnails are cached, does the thumbnail cache 
specification support logical resolution?
  >
  >
  > I am evolving here the specification : the images stored are stored with 
their HiDpi size (width * dpr, height * dpr), and then some metadata is written.
  
  
  Specification as in, 
https://specifications.freedesktop.org/thumbnail-spec/thumbnail-spec-latest.html?
 So there is some work on extending the specs not yet reflected on that page?
  How is the metadata written?
  
  >> How would cached thumbnails work cross-screen?
  > 
  > Cache thumbnails with low dpr will look blurry on hidpi screen, other than 
that's not an issue.
  
  Cached thumbnails rendered for hidpi (so e.g. being 256x256 with bigger 
details due to hidpi context) , but used for lowdpi elsewhere will be an issue 
as well. E.g. when it comes to text rendered with minfontsize like in case of 
plain text documents, it will be too large then.
  
  I wonder if the thumbnail cache would not need the same extension like the 
icon spec had, with a @2x variant, to make up for that. There are still a lot 
of lowdpi devices out there, and they are getting mixed.

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

[Méven Car]

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 support logical resolution?
  
  
  I am evolving here the specification : the images stored are stored with 
their HiDpi size (width * dpr, height * dpr), and then some metadata is written.
  When reading the cached version for large, the user expects a 256*256 (modulo 
aspect ratio) and can find a (256 * dpr, 256 * dpr) image (or can read the 
devicePixelRatio from metadata)
  You can see in https://phabricator.kde.org/D29526#C575294OL738 and line 582 
how to detect and handle such a case.
  
  > How would cached thumbnails work cross-screen?
  
  Cache thumbnails with low dpr will look blurry on hidpi screen, other than 
that's not an issue.
  We would need applications to update to use Hidpi thumbnails to renew 
thumbnails.
  I might want to evict low dpr thumbnails.

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

[Friedrich W. H. Kossebau]

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?

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

[Méven Car]

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
  preview-dpr

REVISION DETAIL
  https://phabricator.kde.org/D29397

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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

[Méven Car]

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

[Méven Car]

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
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

meven planned changes to this revision.
meven added a comment.


  In D29397#664605 , @dfaure wrote:
  
  > 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 0x80?
  > >
  > >
  > > No I meant 8, since format is passed as quint8 (a single byte, 8-bit) and 
the format is forced to QImage::Format_ARGB32 "5", so this works.
  >
  >
  > OK, this works today. But if one day we want to start actually using other 
image formats, we'll end up with a clash here.
  >  Why not use 0x80 in order to stay away from valid image format values? 
This seems safer to me, in the long run.
  
  
  I did wrong calculation about how big a 8-bit integer is, believing 0x8 was 
the most significant bit, while you correct me here it is 0x80, thanks.
  Will update accordingly.

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

[Méven Car]

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
  https://phabricator.kde.org/D29397

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[David Faure]

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 0x80?
  >
  >
  > No I meant 8, since format is passed as quint8 (a single byte, 8-bit) and 
the format is forced to QImage::Format_ARGB32 "5", so this works.
  
  
  OK, this works today. But if one day we want to start actually using other 
image formats, we'll end up with a clash here.
  Why not use 0x80 in order to stay away from valid image format values? This 
seems safer to me, in the long run.

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

[Méven Car]

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 constructor to take dpr 
> from

> Not a fan. You can have different dpi per screen.

What you imply is incorrect.
In Wayland, QGuiApplication::devicePixelRatio always returns the maximum scale 
of any screens (and ceiled up, so my 1.25 scale is returned as 2), and always 
the same value regardless on the scale of the screen where the app is. Only at 
painting does the scale makes a difference per screen.

In X, well there is only one scale anyway.

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

[Méven Car]

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
  https://phabricator.kde.org/D29397

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Kai Uwe Broulik]

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 we should have a `QWindow*` method or constructor to take dpr from

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

[Méven Car]

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 the 
format is forced to QImage::Format_ARGB32 "5", so this works.
  From kio-extra thumbnails :
  
if( img.format() != QImage::Format_ARGB32 ) { // KIO::PreviewJob and this 
code below completely ignores colortable :-/,
img = img.convertToFormat(QImage::Format_ARGB32); //  so make sure 
there is none
}
// Keep in sync with kio/src/previewjob.cpp

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

[Méven Car]

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 sharply and the picture 
frames non-pixelated
  
  
  And this also makes the job of Applications easier, not having to make `width 
* devicePixelRatio, height * devicePixelRatio` everywhere but a single 
`setDevicePixelRatio` similar to Qt own functions.

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

[Kai Uwe Broulik]

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
  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

[David Edmundson]

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 reason to do anything with logical 
sizes?
  That's how I thought the current design worked.

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

[Méven Car]

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
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Méven Car]

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

[Méven Car]

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 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

[David Faure]

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 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

[Méven Car]

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
  https://phabricator.kde.org/D29397

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.cpp
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29397: KPreviewJob : Support for DeviceRatioPixel

[Kai Uwe Broulik]

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 height, QImage 
> , qreal devicePixelRatio = 1.0) = 0; // KF6 TODO: turn first arg into 
> QUrl (see thumbnail/htmlcreator.cpp)
>  

You can't do that

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

[Méven Car]

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) { iFormat &= 0xFFF; 
> str >> imgDevicePixelRatio; }`
> and of course setting that 0x1000 flag in the slaves that have been updated 
> to provide the pixelratio.

I am not sure the values you gave for the hack bitmask are correct, iFormat is 
quint8, so how come it contain 0x1000

> dfaure wrote in thumbcreator.h:183
> You need to do like we once did: define an interface that inherits from 
> ThumbCreator.
> Call it V3
> 
> In the job, use dynamic_cast to test whether the object provided by the 
> kioslave supports the V3 interface or not.

I guess you mean in the thumbnail kio regarding instantiating Thumbcreator.
Anyway it works locally.

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

[David Faure]

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 = static_cast(iFormat);

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) { iFormat &= 0xFFF; str 
>> imgDevicePixelRatio; }`
and of course setting that 0x1000 flag in the slaves that have been updated to 
provide the pixelratio.

> thumbcreator.h:183
>  class KIOWIDGETS_DEPRECATED_VERSION(5, 0, "Use ThumbCreator")
>  KIOWIDGETS_EXPORT ThumbCreatorV2 : public ThumbCreator
>  {

You need to do like we once did: define an interface that inherits from 
ThumbCreator.
Call it V3

In the job, use dynamic_cast to test whether the object provided by the 
kioslave supports the V3 interface or not.

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

[Méven Car]

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 
thumbnails.
  The result image is not guaranteed to have this ratio, and users should paint 
it accordingly.
  
  The patch currently breaks binary compatibility though.
  I would like to have some feedback how to work out this issue.
  
  An update to kio-extras thumbnail is in the works.

REPOSITORY
  R241 KIO

BRANCH
  preview-dpr

REVISION DETAIL
  https://phabricator.kde.org/D29397

AFFECTED FILES
  src/widgets/previewjob.cpp
  src/widgets/previewjob.h
  src/widgets/thumbcreator.h

To: meven, dfaure, broulik, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns