D6313: Support Icon Scale from Icon naming specification 0.13

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://pha

D6313: Support Icon Scale from Icon naming specification 0.13

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:

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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 R30

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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://phab

D6313: Support Icon Scale from Icon naming specification 0.13

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 R3

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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.

D6313: Support Icon Scale from Icon naming specification 0.13

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://p

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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 dupli

D6313: Support Icon Scale from Icon naming specification 0.13

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://phabri

D6313: Support Icon Scale from Icon naming specification 0.13

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,

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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:

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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 spe

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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 w

D6313: Support Icon Scale from Icon naming specification 0.13

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 o

D6313: Support Icon Scale from Icon naming specification 0.13

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 mig

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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 h

D6313: Support Icon Scale from Icon naming specification 0.13

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, #v

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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-

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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 KI

D6313: Support Icon Scale from Icon naming specification 0.13

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

D6313: Support Icon Scale from Icon naming specification 0.13

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