| Kirigami |
since we can't directly access the svg file, when we can't use
KIconLoader try to detect monochrome icons on the actual image
with the following heuristic:
non transparent pixels is less than 0.3
this seems to give satisfying results with most icon temes
tested with breeze, oxygen adwayta and some other themes from ghns
| No Linters Available |
| No Unit Test Coverage |
| Buildable 8944 | |
| Build 8962: arc lint + arc unit |
Can you elaborate on the problem we're trying to fix.
so, when we have KiconLoader (ie using the desktop style and we can rely on kf5 dependencies) to colorize icons we use our stylesheet-based color replacement in svgs and everything is always awesome: monochrome icons colorized to the color palette and clolor icons stay as they are.
where we can't use kiconloader due dependencies (kirigami is tier 1) which is usually Android. Or technically one can do a kirigami app with zero dependencies and not have qqc2-desktop-style installed and run it on gnome.
so, we colorize monochrome icons treating them as just masks, and that works ok, tough there isn't any spec to know with 100% certainty if an icon is monochrome or colored (all icon names that end up with -symbolys usually are monochrome, but in breeze and other themes monochrome icons are all over the place and often it depends from the icon size)
in theory all i would need is the access to the svg file and look at the styling inside, but the public QIcon/QIconloader api doesn't let me access the final resolved name of the icon file, being it a png or an svg.
so, the only resort that i have, is actually.. looking at the pixels in the rendering image :/
so, very low entropy -> yay is monochrome, let's paint over it (the actual mask-ification shold be done by a shader ideally, but that's a completely different problem)
high entropy and/or many saturated pixels -> leave it as it is and assume is a colored icon
the final visual problem is that often i get people complaining are things like icons in a menu or in a list view that have here and there a colored breeze icon that became an unrecognizable black splorch
or the opposite problem, a monochrome black icon that is rendered black on a black background
| src/desktopicon.cpp | ||
|---|---|---|
| 522 | Did you mean >= 256? | |
| 545 | You are caching the result per size, but the initial decision depends on the actual icon image, right? Is it possible that the first icon examined is colorful, but the rest is not, or vice versa? If yes, would it make sense to examine a few icons (maybe three) before a decision is made? | |
| 551 | Spaces | |
| 570 | Please add same spaces between binary operators. Also C++ casts are 'type(val)' instead of '(type)val'. | |
| src/desktopicon.h | ||
| 108 | QHash<int, bool> is just a QSet<int>. | |
| src/desktopicon.cpp | ||
|---|---|---|
| 545 | the actual icon image can vary depending on size, usually themes have multiple images per icon separed by those "standard sizes" i'm checking against, so it's possible that like the 16 trough 24 sizes are monochrome and beyond are colored for instance (is often the case in breeze icons) | |
| src/desktopicon.h | ||
| 108 | not really, because with a qset i could only cache those that i know are monochrome but i can't tell the difference between "i didn't test yet" and "i tested and isn't monochrome" | |
so, we colorize monochrome icons treating them as just masks, and that works ok, tough there isn't any spec to know with 100% certainty if an icon is monochrome or colored (all icon names that end up with -symbolys usually are monochrome, but in breeze and other themes monochrome icons are all over the place and often it depends from the icon size)
For breeze there's an alternate approach. We "just" add "-symbolic" symlinks in everywhere. We could even use this code to automate doing that. That would cover the android case.
Or technically one can do a kirigami app with zero dependencies and not have qqc2-desktop-style installed and run it on gnome.
It seems gnome themes also do the -symbolic suffix thing and do SVG replacement like we do. With very similar keys too!
Even ignoring this Kirigami issue we have an issue of Breeze icons on gnome and vice versa that's worth fixing.
If we need to do this as a temporary measure, then fine.
However, I want to hear a plan on what a "correct" cross-desktop KF6 + Qt6 solution would be - and maybe start some ML threads together before accepting.
Otherwise we'll carry on with hacks that only half work forever and ever.
| src/desktopicon.cpp | ||
|---|---|---|
| 494 | Will this code ever get called if we are using the KIconLoader? We want to avoid that. (Even if it's just looking at QIcon.engine()->metaObject) | |
| 496 | Do we need to test: -symbolic (I saw this in some gnome code) | |
| src/libkirigami/platformtheme.cpp | ||
| 695 | Is this useful if DesktopIcon does this anyway? (or to turn the question around, can we do it all here and have DesktopIcon only check isMask) | |
not really, one would still have to use the -symbolic name instead of the real one for every icon.. and only in android, which wouldn't be cross platform, so no.
Or technically one can do a kirigami app with zero dependencies and not have qqc2-desktop-style installed and run it on gnome.
It seems gnome themes also do the -symbolic suffix thing and do SVG replacement like we do. With very similar keys too!
Even ignoring this Kirigami issue we have an issue of Breeze icons on gnome and vice versa that's worth fixing.If we need to do this as a temporary measure, then fine.
However, I want to hear a plan on what a "correct" cross-desktop KF6 + Qt6 solution would be - and maybe start some ML threads together before accepting.
Otherwise we'll carry on with hacks that only half work forever and ever.
this tough is about doing something about it in KIconLoader which is completely ortogonal to this patch.
we can either support both coloring methods or agree on only one (which as usual, would be porting everything to the gnome method)
wouldn't be that hard.. providing iff the gnome method works with QtSvg which is not a given.
the problem remain that: I can't always use KiconLoader in Kirigami, as well as accessing to the icons svg files at all. Changing how breeze does coloring doesn't solve the problem this patch adresses.
Actually we're discussing in VDG-land whether or not this is something we should do anyway, because right now we have no way of forcing the use of a monochrome icon for a >22px size when a larger colorful version exists. Using -symbolic to suffix the monochrome versions would allow us to do this. Adding some VDG folks for comment.
yeah, the decision of "this should always be monochrome" must be done at a semantic level by the app itself, and that's what the -symbolic name is for.
tough some icons wether they are on a way or the other purely depends from a style decision of the icon theme, so while i encourage using the symbolic name for places that should have a simple lineart symbol regardless of the icon theme, it doesn't solve the entrirty of the problem
The problem with using -symbolic for all monochrome icons is that we'd have to create color icons without -symbolic to replace the old monochrome icons without -symbolic. That means we'd be effectively reworking a great deal of Breeze icons. Outside of a few instances in System Settings, Plasmashell and KWin titlebars, where we want to use color icons at small sizes, Breeze looks pretty consistent already, even with 3rd party applications. In places like the Digital Clock widget settings, where a monochrome icon is used when a color icon should be used, that can be fixed in breeze-icons just by making a new color icon at the appropriate size. If all of our monochrome icons had to have -symbolic, then we'd have to depend on 3rd party applications using -symbolic to keep a consistent look as well.
In short, we'd have to create a ton of new icons just to fix an issue that mainly exists in only a few pieces of our own software if we went the route with -symbolic. Maybe with a future icon theme, we can plan things out better, but I don't think we should do anything that greatly changes the look of breeze-icons.
exactly, whether is monochrome or not depends from the file and should be the underlying system be capable of recognizing which is which
The problem with using -symbolic for all monochrome icons is that we'd have to create color icons without -symbolic to replace the old monochrome icons without -symbolic.
Why would breeze need two versions?
If an app requests -symbolic you get a black icon which we know we can colourise.
If the app doesn't request -symbolic you get an icon which happens to be all black.
Also it's worth noting there's an FD.O icon spec rule (that will apply for all QIconEngines hopefully) if an app requests "foo-bar" and "foo-bar" doesn't exist, it will automatically load "foo"
Us requesting "-symbolic" at an app level won't break anything. (though obviously we'll have to port a bunch of app code)
this tough is about doing something about it in KIconLoader which is completely ortogonal to this patch.
That's absolutely not what this is about, because as you note putting it in KIconLoader wouldn't fix anything at all!
We've got code in Plasma that colours icons. Then we put code in KIconLoader because we didn't account for widgets, now we're putting code here because we didn't account for non-plasma - and we know this is a bodge that's going to get replaced as it doesn't work in a lot of cases.
If we are running Kirigami on gnome, you probably want to use a gnome icon set at which point this all breaks. I think the app would /need/ to request the "-symbolic" icon?
I'm fully aware I'm being difficult, but I'm doing it because these half solutions keep going on forever and I'm trying to save us some time in the future.
If we do want changes in GTK4 and Qt6 we should be thinking about them now.
which screws up if it's a dark background
Also it's worth noting there's an FD.O icon spec rule (that will apply for all QIconEngines hopefully) if an app requests "foo-bar" and "foo-bar" doesn't exist, it will automatically load "foo"
so if i requested foo-symbolic but foo was returned and is not monochrome, i will get a broken icon.
Us requesting "-symbolic" at an app level won't break anything. (though obviously we'll have to port a bunch of app code)
tough is still a theme designer decision to put random monochrome icons in the non -symbolic section, that is a valid decision and i don't want to break it
this tough is about doing something about it in KIconLoader which is completely ortogonal to this patch.
That's absolutely not what this is about, because as you note putting it in KIconLoader wouldn't fix anything at all!
We've got code in Plasma that colours icons. Then we put code in KIconLoader because we didn't account for widgets, now we're putting code here because
eh, in kf6 i would love to drop plasmacore.iconitem, having just this item instead.
we didn't account for non-plasma - and we know this is a bodge that's going to get replaced as it doesn't work in a lot of cases.
well, looking at how the gtk one works, i just vastly prefer our approach, exactly because it styles all icons and doesn't require to have -symbolic technically(and that maps a big part of the system color palette, while the gtk one doesn't).
while the approach they taken is only applied to -symbolic, because it would destroy any normal icon, for the following reason:
while instead breeze needs a class set for everything and use fill:currentColor explicitly, so icons that don't want to use it are completely unaffected
(the problem there is that is a bit inconvenient to do because inkscape is quite buggy in saving files using currentColor without breaking stuff)
If we are running Kirigami on gnome, you probably want to use a gnome icon set at which point this all breaks. I think the app would /need/ to request the "-symbolic" icon?
I'm fully aware I'm being difficult, but I'm doing it because these half solutions keep going on forever and I'm trying to save us some time in the future.
If we do want changes in GTK4 and Qt6 we should be thinking about them now.
so, for gtk4/qt6, the base qiconengine included in qt should support whatever coloring is used, in which case i wouldn't eed to do anything on my part.. tough that's for Qt6.
It's important for me tough that in the final version, any icon can be styled, not only those that end with -symbolic
so, for gtk4/qt6, the base qiconengine included in qt should support whatever coloring is used, in which case i wouldn't need to do anything on my part.. tough that's for Qt6.
It won't magically happen on it's own.
I've emailed the XDG mailing list.
| src/desktopicon.cpp | ||
|---|---|---|
| 322 | DesktopIcon::setSource takes a QVariant. The path with the colouring is only executed if this variant is a string. If it's already an icon we go to here directly. | |
added some notes on the thread.
on Qt side the things that would need to be done i think is implementing the coloring stuff in the qiconloader plugin that lives in qtsvg, and (for the method that gnome uses) adding support for the !important keyword in qtsvg css parsing.
also note, the way we color icons is the same we color any plasma svg theme element (for which indeed a full palette is needed) so we would end up with different ways between icons and plasma widgets?