D6313: WIP: Support device pixel ratio in icon loader and engine

2018-03-16 Thread Kai Uwe Broulik
broulik added a comment.


  Not really, I recently wanted to update this patch to include support for 
Icon Scale definition but ran into a dead-end of having to pass through the 
scale in 50 places... I think I'll give it a go once more but only special 
casing SVGs and leaving everything unchanged.

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: rkflx, acrouthamel, ngraham, elvisangelaccio, mart, kvermette, cfeck, 
davidedmundson, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol


D6313: WIP: Support device pixel ratio in icon loader and engine

2018-03-13 Thread Andrew Crouthamel
acrouthamel added a comment.


  Hey there, any movement on this? I've been submitting some patches to fix 
icon scaling in apps and have noticed how they switch from monochrome to 
colored (hires) versions when fixed. Getting the underlying issue fixed would 
be great as more people buy HiDPI/Retina displays and run with scaling.

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: acrouthamel, ngraham, elvisangelaccio, mart, kvermette, cfeck, 
davidedmundson, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol


D6313: WIP: Support device pixel ratio in icon loader and engine

2017-06-26 Thread Marco Martin
mart added a comment.


  In https://phabricator.kde.org/D6313#118285, @kvermette wrote:
  
  > Behaviorally speaking there's justification for ensuring SVGs are treated 
the same as PNGs in this case. Looking at the code we aren't shimming the SVGs 
specifically (unless I'm missing something), but I just wanted to chime in with 
this and nip special treatment for SVGs in the bud. In maintaining the Aether 
icon theme I would create links to the higher resolution icons, so you'd see 
something like a "16x16x2" folder point to my "32x32" 'native' folder, and a 
"16x16x3"->"48x48" folder, "32x32x2"->"64x64", etc. Just because we *can* scale 
SVG icons doesn't mean that behavior should be assumed, should the icon set 
have higher fidelity icons ready for HiDPI.
  
  
  i think for svgs 16x16x2 should be preferred over scaling by 2 the 16x16 
version, but still, scaling the 16x16 version should still be preferred over 
the 32x32 version, so this patch is on the right rirection

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: mart, kvermette, cfeck, davidedmundson, plasma-devel, #frameworks, ZrenBot, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, lukas


D6313: WIP: Support device pixel ratio in icon loader and engine

2017-06-21 Thread Ken Vermette
kvermette added a comment.


  In https://phabricator.kde.org/D6313#118250, @davidedmundson wrote:
  
  > For SVG icons this is fine.
  >
  > For pixmap icons this is only part of the needed changes.
  >
  > We don't want to load the 16px image and then resize it, I think that's 
what this would do? That would be an unacceptable regression.
  >
  > We would need a folder containing the 16px icons at 2x. This is now part of 
the FD.O icon spec [1].  But that means updating all of our icon theme parsing 
and a much much bigger patch. 
  >  Or we could just special case SVGs here.
  >
  > 
[1]https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html
  
  
  Behaviorally speaking there's justification for ensuring SVGs are treated the 
same as PNGs in this case. Looking at the code we aren't shimming the SVGs 
specifically (unless I'm missing something), but I just wanted to chime in with 
this and nip special treatment for SVGs in the bud. In maintaining the Aether 
icon theme I would create links to the higher resolution icons, so you'd see 
something like a "16x16x2" folder point to my "32x32" 'native' folder, and a 
"16x16x3"->"48x48" folder, "32x32x2"->"64x64", etc. Just because we *can* scale 
SVG icons doesn't mean that behavior should be assumed, should the icon set 
have higher fidelity icons ready for HiDPI.

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: kvermette, cfeck, davidedmundson, plasma-devel, #frameworks, ZrenBot, 
spstarr, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, mart, lukas


D6313: WIP: Support device pixel ratio in icon loader and engine

2017-06-21 Thread Christoph Feck
cfeck added inline comments.

INLINE COMMENTS

> cfeck wrote in kiconloader.cpp:863
> Please use some rounding here. Scaling factors such as 1.4 cannot be 
> represented exactly.
> 
> Either add some formatting specifiers, e.g. for three decimal places, or use 
> qRound(scale * 1000).

Just checked that the default precision is 6 digits, so maybe not that 
important.

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: cfeck, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, 
lukas


D6313: WIP: Support device pixel ratio in icon loader and engine

2017-06-21 Thread Christoph Feck
cfeck added inline comments.

INLINE COMMENTS

> kiconloader.cpp:1299
>  
> -if (d->findCachedPixmapWithPath(key, pix, path)) {
> +if (d->findCachedPixmapWithPath(key, pix, path)) {// skip cache
>  if (path_store) {

Is this a left-over change from disabling this check?

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: cfeck, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, 
lukas


D6313: WIP: Support device pixel ratio in icon loader and engine

2017-06-21 Thread Christoph Feck
cfeck added inline comments.

INLINE COMMENTS

> kiconloader.cpp:863
> +   % QLatin1Char('@')
> +   % QString::number(scale)
> % QLatin1Char('_')

Please use some rounding here. Scaling factors such as 1.4 cannot be 
represented exactly.

Either add some formatting specifiers, e.g. for three decimal places, or use 
qRound(scale * 1000).

> kiconloader.h:247
>  
> +// FIXME docs
> +QPixmap loadIcon(const QString , KIconLoader::Group group, qreal 
> scale, int size = 0,

FIXME

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: cfeck, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, 
lukas


D6313: WIP: Support device pixel ratio in icon loader and engine

2017-06-21 Thread David Edmundson
davidedmundson added a comment.


  For SVG icons this is fine.
  
  For pixmap icons this is only part of the needed changes.
  
  We don't want to load the 16px image and then resize it, I think that's what 
this would do? That would be an unacceptable regression.
  
  We would need a folder containing the 16px icons at 2x. This is now part of 
the FD.O icon spec [1].  But that means updating all of our icon theme parsing 
and a much much bigger patch. 
  Or we could just special case SVGs here.
  
  
[1]https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html

INLINE COMMENTS

> kiconloader.cpp:1294
>  QPixmap pix;
> +pix.setDevicePixelRatio(scale);
> +

I don't think you need this?

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas


D6313: WIP: Support device pixel ratio in icon loader and engine

2017-06-21 Thread Kai Uwe Broulik
broulik created this revision.
Restricted Application added projects: Plasma, Frameworks.
Restricted Application added subscribers: Frameworks, plasma-devel.

REVISION SUMMARY
  Since Qt 5.9 there's a ScaledPixmapHook in QIconEngine which is called when 
device pixel ratio is > 1 and it wants a scaled pixmap. In contrast to regular 
pixmap this also knows the scale factor.
  
  This way, when a 32px icon is requested, we can now tell whether we want a 
16px scaled 2x or if it's a legitimate 32px request. It ensures that we keep 
symbolic small icons even on high-dpi screens where we would otherwise load the 
colorful and hard-to-see icons at the given physical size they ends up at.

TEST PLAN
  Ran `QT_SCREEN_SCALE_FACTORS=3 kate`:
  Before:
  F3789197: Screenshot_20170621_151044.png 

  After:
  F3789196: Screenshot_20170621_144649.png 

  
  Before:
  F3789201: Screenshot_20170621_151140.png 

  After:
  F3789199: Screenshot_20170621_151118.png 


REPOSITORY
  R302 KIconThemes

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

AFFECTED FILES
  src/kiconengine.cpp
  src/kiconengine.h
  src/kiconloader.cpp
  src/kiconloader.h

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: plasma-devel, #frameworks, ZrenBot, spstarr, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas