D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> volkov wrote in kiconloader.cpp:1368
> Looks like a rebase: 
> https://phabricator.kde.org/D6313?vs=31197&id=34779&whitespace=ignore-most#toc

Feel free to submit a patch to restore this

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: volkov, hein, rkflx, acrouthamel, ngraham, elvisangelaccio, mart, 
kvermette, cfeck, davidedmundson, plasma-devel, LeGast00n, sbergeron, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D6313: Support Icon Scale from Icon naming specification 0.13

[Alexander Volkov]

volkov added inline comments.

INLINE COMMENTS

> broulik wrote in kiconloader.cpp:1368
> Dunno, probably oversight or forgotten to rebase..

Looks like a rebase: 
https://phabricator.kde.org/D6313?vs=31197&id=34779&whitespace=ignore-most#toc

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: volkov, hein, rkflx, acrouthamel, ngraham, elvisangelaccio, mart, 
kvermette, cfeck, davidedmundson, plasma-devel, LeGast00n, sbergeron, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> volkov wrote in kiconloader.cpp:1368
> Why this line reverts D12002 ?

Dunno, probably oversight or forgotten to rebase..

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: volkov, hein, rkflx, acrouthamel, ngraham, elvisangelaccio, mart, 
kvermette, cfeck, davidedmundson, plasma-devel, LeGast00n, sbergeron, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D6313: Support Icon Scale from Icon naming specification 0.13

[Alexander Volkov]

volkov added inline comments.

INLINE COMMENTS

> kiconloader.cpp:1368
>  
> -if (group >= 0 && group < KIconLoader::LastGroup) {
> +if (group >= 0) {
>  img = d->mpEffect.apply(img, group, state);

Why this line reverts D12002 ?

REPOSITORY
  R302 KIconThemes

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

To: broulik, kde-frameworks-devel, #plasma, #vdg
Cc: volkov, hein, rkflx, acrouthamel, ngraham, elvisangelaccio, mart, 
kvermette, cfeck, davidedmundson, plasma-devel, LeGast00n, sbergeron, jraleigh, 
fbampaloukas, GB_2, ragreen, Pitel, michaelh, ZrenBot, bruns, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R302:20f7137145f6: Support Icon Scale from Icon naming 
specification 0.13 (authored by broulik).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D6313?vs=35318&id=35667#toc

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6313?vs=35318&id=35667

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

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[David Edmundson]

davidedmundson added a comment.


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

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik updated this revision to Diff 35318.
broulik added a comment.


  …

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6313?vs=35317&id=35318

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

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik updated this revision to Diff 35317.
broulik added a comment.


  - Fix rebase

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6313?vs=34781&id=35317

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

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kiconloader.h:277
> + * @p canReturnNull.
> + * @since 5.48
> + */

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

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kiconloader.h:527
>   * @see resetPalette
> - * @since 5.39
> + * @since 5.38
>   */

Is this missing a rebase or intended? See 
https://cgit.kde.org/kiconthemes.git/commit/?id=b506a48214a08f56d79e7847a22b0417028a46ff

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik updated this revision to Diff 34781.
broulik added a comment.


  …

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6313?vs=34780&id=34781

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

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik updated this revision to Diff 34780.
broulik added a comment.


  - Remove accidental cruft

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6313?vs=34779&id=34780

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

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik updated this revision to Diff 34779.
broulik added a comment.


  - Rename to `loadIcon` with `scale` to `loadScaledIcon` to avoid ambiguous 
overload

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6313?vs=31197&id=34779

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

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik added a comment.
Restricted Application removed a subscriber: Frameworks.


  Any suggestion how to fix the overload problem?

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> cfeck wrote in kiconloader.h:279
> loadIcon("test", mygroup, 2);
> 
> Which overload is called?

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

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kiconloader.h:279
> + */
> +QPixmap loadIcon(const QString &name, KIconLoader::Group group, qreal 
> scale, int size = 0,
> + int state = KIconLoader::DefaultState, const 
> QStringList &overlays = QStringList(),

loadIcon("test", mygroup, 2);

Which overload is called?

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Andrew Crouthamel]

acrouthamel added a comment.


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

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Christoph Feck]

cfeck added a comment.


  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?

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[David Edmundson]

davidedmundson added a comment.


  RE: QT_SCALE_FACTOR vs QT_SCREEN_SCALE_FACTORS
  
  See: https://paste.kde.org/p0y4ze6dg

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Andrew Crouthamel]

acrouthamel added a comment.


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

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Nathaniel Graham]

ngraham accepted this revision as: VDG.
ngraham added a comment.


  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.

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik added a comment.


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

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik added a comment.


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

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik updated this revision to Diff 31197.
broulik added a comment.


  - Rebase
  - Address Christoph's comments (4x scale and indentation)

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6313?vs=29668&id=31197

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

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Nathaniel Graham]

ngraham added a comment.


  In D6313#238605 , @acrouthamel 
wrote:
  
  > 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?

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Andrew Crouthamel]

acrouthamel added a comment.


  In D6313#238697 , @rkflx wrote:
  
  > In D6313#238605 , @acrouthamel 
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.

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Henrik Fehlauer]

rkflx added a comment.


  In D6313#238599 , @ngraham wrote:
  
  > ...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").
  
  In D6313#238605 , @acrouthamel 
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…

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Andrew Crouthamel]

acrouthamel added a comment.


  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.

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Nathaniel Graham]

ngraham added a comment.


  In D6313#238595 , @acrouthamel 
wrote:
  
  > 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!

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Andrew Crouthamel]

acrouthamel added a comment.


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

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Andrew Crouthamel]

acrouthamel added a comment.


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

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Eike Hein]

hein added a comment.


  I also prefer the "Before" images.

REPOSITORY
  R302 KIconThemes

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

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

D6313: Support Icon Scale from Icon naming specification 0.13

[Christoph Feck]

cfeck added a comment.


  Otherwise looks good. Maybe needs more feedback from testers.

INLINE COMMENTS

> kiconloader.cpp:1264
>  {
> +return loadIcon(_name, group, 1.0 /*scale*/, size, state, overlays, 
> path_store, canReturnNull);
> +}

indent

> kicontheme.cpp:169
> +} else if (scale > 2.1) {
> +integerScale = 3;
> +}

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

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, ragreen, michaelh, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D6313: Support Icon Scale from Icon naming specification 0.13

[Nathaniel Graham]

ngraham added a comment.


  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.

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, ragreen, michaelh, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol

D6313: Support Icon Scale from Icon naming specification 0.13

[Andrew Crouthamel]

acrouthamel added a comment.


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

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: Support Icon Scale from Icon naming specification 0.13

[Kai Uwe Broulik]

broulik updated this revision to Diff 29668.
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.
broulik edited the test plan for this revision.

REPOSITORY
  R302 KIconThemes

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6313?vs=15690&id=29668

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

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

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