Support Icon Scale from Icon naming specification 0.13
ClosedPublic

Authored by broulik on Jun 21 2017, 1:12 PM.

Details

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.

Implement support for Icon Scale properties introduced in Icon naming specification 0.13 [1] to choose an appropriate icon, if available, rather than blatantly loading a larger icon that may not fit in the context and also could be hard to tell as its physical size will be smaller than what it was designed for.

[1] https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-0.13.html

BUG: 365941

Test Plan

Ran QT_SCREEN_SCALE_FACTORS=3 kate:
Before:


After:

Before:


After:

For testing I put a symlink from places/32 to places/16@2x and added the following to the index.theme of Breeze:

ScaledDirectories=places/16@2x
...
[places/16@2x]
Size=16
Scale=2
Context=Places
Type=Fixed

This way you designers can now create dedicated 2x SVGs for those usecases, ie. we can have a 16px icon as well as a 16px@2x icon rather than it just taking the 32px icon which might not fit. In case a 16px@2x icon is not present it will load the 32px icon instead as it did before. This way one could even create a high dpi Oxygen theme.

Diff Detail

Repository
R302 KIconThemes
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Jun 21 2017, 1:12 PM
Restricted Application added projects: Plasma, Frameworks. · View Herald TranscriptJun 21 2017, 1:12 PM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript

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

src/kiconloader.cpp
1294

I don't think you need this?

cfeck added a subscriber: cfeck.Jun 21 2017, 1:53 PM
cfeck added inline comments.
src/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).

src/kiconloader.h
247

FIXME

cfeck added inline comments.Jun 21 2017, 1:55 PM
src/kiconloader.cpp
1299

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

cfeck added inline comments.Jun 21 2017, 1:58 PM
src/kiconloader.cpp
863

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

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.

mart added a subscriber: mart.Jun 26 2017, 8:58 AM

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

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.

rkflx added a subscriber: rkflx.Mar 13 2018, 8:33 PM

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.

broulik updated this revision to Diff 29668.Mar 16 2018, 9:47 AM
broulik retitled this revision from WIP: Support device pixel ratio in icon loader and engine to Support Icon Scale from Icon naming specification 0.13.
broulik edited the summary of this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)

Thanks for working on this again, it really helps. :)

TBH, I prefer the "Before" images. The only reason why the symbolic line-art versions exist is because the prettier icons look bad at small sizes on low-dpi screens. Especially for the document icons, IMHO the Before versions look vastly better.

ngraham edited the summary of this revision. (Show Details)Mar 29 2018, 4:49 PM
cfeck added a comment.Mar 30 2018, 1:31 AM

Otherwise looks good. Maybe needs more feedback from testers.

src/kiconloader.cpp
1271

indent

src/kicontheme.cpp
169

Unless the XDG spec forbids this, please add scale 4, too. Those 8K screens are everywhere ;)

hein added a subscriber: hein.Mar 30 2018, 9:02 AM

I also prefer the "Before" images.

@ngraham and @hein, while I agree that in some instances I like the colored hi-res icons showing, this issue causes a mixture of icons to be displayed depending on the app. The examples here show a nice homogeneous selection of icons that change from A to B, so I can see why you like the colored versions better. But in diffs such as D11669 and D10688, you can see how there are now colored icons where they were once symbolic, mixed within the interface.

This really boils down to an issue with Breeze, as it has special symbolic versions in the "low-res" folders, and fancy colored icons in the "hi-res" folders. Some other icon themes do not do this, and rather use just one generic icon for all sizes, so the issue really is not prevalent there. My opinion is that if you prefer the colored icons, you should recommend to Breeze that they phase out the symbolic versions, or create new colored icons for everything, to place into the "hi-res" folders, so they all show as colored for those running HiDPI/scaling. Otherwise we are left with inconsistent interfaces for users.

This change fixes a legitimate issue with an implemented freedesktop standard, so I hope it gets approved and committed.

If Breeze has been taking advantage of a bug in our implementation of this spec, then we really need to fix Breeze before we fix the bug.

acrouthamel added a comment.EditedApr 2 2018, 4:04 PM

I'm just saying if you prefer colored over symbolic line art, that is something for Breeze or VDG to work out. Not that they are really taking advantage of anything.

The bug here is that the system isn't aware of scaling at the moment. So the icons it should be displaying are the symbolic ones, regardless of whether you are running HiDPI or not (and whether you like them or not). That's because the menu should be displaying 24x24 or 32x32 icons, or whatever it should be. But in HiDPI environments it is doubling that (or whatever scale you have set), and displaying icons from high-res folders such as 64x64 or 96x96, where they shouldn't be displayed.

This patch fixes that issue and forces the system to load the correct size, as originally intended. Menus and stuff shouldn't be loading icons from these high-res folders. Symbolic vs. colored is a design thing to work out separately.

I'm just saying if you prefer colored over symbolic line art, that is something for Breeze or VDG to work out. Not that they are really taking advantage of anything.

The bug here is that the system isn't aware of scaling at the moment. So the icons it should be displaying are the symbolic ones, regardless of whether you are running HiDPI or not (and whether you like them or not). That's because the menu should be displaying 24x24 or 32x32 icons, or whatever it should be. But in HiDPI environments it is doubling that (or whatever scale you have set), and displaying icons from high-res folders such as 64x64 or 96x96, where they shouldn't be displayed.

This patch fixes that issue and forces the system to load the correct size, as originally intended. Menus and stuff shouldn't be loading icons from these high-res folders. Symbolic vs. colored is a design thing to work out separately.

Right, and I'm saying we need to work that out before we fix this. Visually, the whole point of making line-art versions for full-color icons is to look better at really small sizes on low-resolution screens. If we fix this bug without resolving the issues in Breeze, then people with HiDPI screens will see line-art icons where the full-color ones would work fine and look better.

...Unless I'm misunderstanding something obvious, in which case, feel free to ignore my deluded ramblings!

I think both of us missed this part at the bottom of @broulik's description:

This way you designers can now create dedicated 2x SVGs for those usecases, ie. we can have a 16px icon as well as a 16px@2x icon rather than it just taking the 32px icon which might not fit. In case a 16px@2x icon is not present it will load the 32px icon instead as it did before. This way one could even create a high dpi Oxygen theme.

So Breeze will show the hi-res icons like it does now since it doesn't have any @2x folders or symlinks (an example of that can be found in Papirus). Moving forward, Breeze could create these @2x folders, populating them with beautiful Hi-DPI-acceptable icons, or whatever they want, to ensure interface consistency.

But for now, it would be status-quo, even with this patch committed. So no one will notice, except those of us with @2x-compatible, 3rd party icon themes right now.

rkflx added a comment.Apr 2 2018, 7:18 PM

...Unless I'm misunderstanding something obvious, in which case, feel free to ignore my deluded ramblings!

You might be mixing up two separate topics:

  • Optimizing rendering of SVGs by snapping to the pixel grid and changing line widths ("sharpness").
  • Adapting the level of detail of the icon to the apparent size for the user, e.g. even with a HiDPI monitor there is a lower limit as to what details a user can still tell apart ("squinting").

@2x-compatible

How will this work for 4x / 2.7x / 1.4x / etc. scaling? Of course we could define a point where it snaps from 1x to 2x icons, but eventually this approach would also need 4x + larger versions…

In D6313#238697, @rkflx wrote:

@2x-compatible

How will this work for 4x / 2.7x / 1.4x / etc. scaling? Of course we could define a point where it snaps from 1x to 2x icons, but eventually this approach would also need 4x + larger versions…

It looks like the freedesktop spec has Scale listed only as an integer. So that would need updating first in my opinion. So 4x would be possible, but decimal scaling would need to be snapped for now.

I think both of us missed this part at the bottom of @broulik's description:

This way you designers can now create dedicated 2x SVGs for those usecases, ie. we can have a 16px icon as well as a 16px@2x icon rather than it just taking the 32px icon which might not fit. In case a 16px@2x icon is not present it will load the 32px icon instead as it did before. This way one could even create a high dpi Oxygen theme.

So Breeze will show the hi-res icons like it does now since it doesn't have any @2x folders or symlinks (an example of that can be found in Papirus). Moving forward, Breeze could create these @2x folders, populating them with beautiful Hi-DPI-acceptable icons, or whatever they want, to ensure interface consistency.

But for now, it would be status-quo, even with this patch committed. So no one will notice, except those of us with @2x-compatible, 3rd party icon themes right now.

Oops, so I did. Let me actually test this before I run my mouth any more...

[goes to apply the patch]

Kai, could you re-base this on master so it applies?

broulik updated this revision to Diff 31197.Apr 3 2018, 7:17 AM
  • Rebase
  • Address Christoph's comments (4x scale and indentation)

The only reason why the symbolic line-art versions exist is because the prettier icons look bad at small sizes on low-dpi screens.

Note that the physical size of the icon is the same for 1x on low dpi and 2x on high dpi so the icon will be hard to tell which is the main motivation for this patch. I have to look closely to actually see the tiny dark blue overlay denoting the folder type (music, downloads, etc)

Okay I've deployed the patch, and without applying the change specified in the "For testing..." sentence, I was unable to find any visual changes from the status quo resulting from this patch. Is that intentional? For example QT_SCREEN_SCALE_FACTORS=3 kate does not show me the "After" picture when I open a file open dialog. Might be worth updating the Test Plan section to describe exactly what's necessary to test this patch.

Question: what is the technical difference between QT_SCALE_FACTOR=2 and QT_SCREEN_SCALE_FACTORS=2?

Okay I've deployed the patch, and without applying the change specified in the "For testing..." sentence, I was unable to find any visual changes from the status quo resulting from this patch. Is that intentional?

Yes, as Breeze currently does not offer any specific high dpi icons.

ngraham accepted this revision as: VDG.Apr 3 2018, 2:27 PM

OK, +1 then! If we can safely implement this part of the spec without causing any unexpected visual changes, then I don't see a problem with it.

@ngraham, in the initial description @broulik forced a @2x folder via symlink to test. Otherwise no change will be observed. :)

RE: QT_SCALE_FACTOR vs QT_SCREEN_SCALE_FACTORS

See: https://paste.kde.org/p0y4ze6dg

Thanks David! HiDPI turns out to be complicated... ;-)

cfeck added a comment.Apr 3 2018, 6:09 PM

In other words, the icon theme designer can now decide if he makes HiDPI only bigger or more detailed by symlinking to either the less detailed or the more detailed svg, without duplicating the icon files?

In D6313#239212, @cfeck wrote:

In other words, the icon theme designer can now decide if he makes HiDPI only bigger or more detailed by symlinking to either the less detailed or the more detailed svg, without duplicating the icon files?

Exactly, that is correct. They could also make special icons if they desire, just like the other folders (32x32, 64x64, etc).

cfeck added inline comments.Apr 3 2018, 10:06 PM
src/kiconloader.h
317

loadIcon("test", mygroup, 2);

Which overload is called?

broulik added inline comments.Apr 24 2018, 9:21 AM
src/kiconloader.h
317

Looks like it's always taking the one without scale. Meh.

Any suggestion how to fix the overload problem?

Restricted Application removed a subscriber: Frameworks. · View Herald TranscriptMay 16 2018, 7:36 AM
broulik updated this revision to Diff 34779.May 24 2018, 8:13 AM
  • Rename to loadIcon with scale to loadScaledIcon to avoid ambiguous overload
broulik updated this revision to Diff 34780.May 24 2018, 8:15 AM
  • Remove accidental cruft
cfeck added inline comments.May 24 2018, 2:53 PM
cfeck added inline comments.May 24 2018, 2:55 PM
src/kiconloader.h
315

Next release is 5.47, but I am fine with waiting for additional feedback if this is controversial.

broulik updated this revision to Diff 35317.Jun 1 2018, 10:48 AM
  • Fix rebase

+1 from me, but wait till after this release tagging

This revision was not accepted when it landed; it landed in state Needs Review.Jun 6 2018, 7:42 AM
This revision was automatically updated to reflect the committed changes.