Bug 451538

Summary: Kirigami.Icon perhaps inappropriately assumes that icon names with the -symbolic suffix are monochrome
Product: [Frameworks and Libraries] frameworks-kirigami Reporter: Martin Fritz <Fritz.Martin99>
Component: generalAssignee: Marco Martin <notmart>
Status: RESOLVED FIXED    
Severity: normal CC: andysem, mfbernardes, mtmcp, mvourlakos, nate, notmart, riddervancocagne
Priority: HI    
Version: 5.92.0   
Target Milestone: Not decided   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed In: 6.0
Attachments: the sidebar in a small window

Description Martin Fritz 2022-03-15 17:34:39 UTC
Created attachment 147516 [details]
the sidebar in a small window

SUMMARY

STEPS TO REPRODUCE
1.  use any non-breeze icon theme and enable sidebar mode in systemsettings

OBSERVED RESULT
all arrow icons are painted black, including the "back button"

EXPECTED RESULT
show icon themes arrow icons unchanged or "normally

SOFTWARE/OS VERSIONS
Linux/KDE Plasma: 5.16.14
(available in About System)
KDE Plasma Version:  5.24.3
KDE Frameworks Version: 5.91.0
Qt Version: 5.15.3 

ADDITIONAL INFORMATION
I've attached a screenshot, for reference
Comment 1 Nate Graham 2022-03-25 17:21:40 UTC
We're using a custom contentItem for this ToolButton, and it uses a Kirigami.Icon for the icon. The issue is that Kirigami.Icon assumes that an icon ending with -symbolic or -symbolic-rtl is a monochrome icon that needs to be recolored and made monochrome.

This kind of needs to be discussed and fixed in Kirigami IMO, because that doesn't seem like it's the safest assumption. There is in fact an open merge request relating to this (https://invent.kde.org/frameworks/kirigami/-/merge_requests/511) but in its current form, it actually makes the situation worse for me, not better. Hopefully that'll get fixed.

In the meantime, I can change the icon name to omit -symbolic.
Comment 2 Nate Graham 2022-03-28 16:19:56 UTC
Git commit b579a5d94a3afbd6f85a95cf368a29e8c96c2b92 by Nate Graham.
Committed on 26/03/2022 at 15:01.
Pushed by ngraham into branch 'Plasma/5.24'.

Don't let back arrow be re-colored to monochrome

The back button uses a custom content item with the icon being provided
by Kirigami.Icon. Unfortunately, Kirigami.Icon currently suffers from an
issue that causes colored icons that end with "-symbolic" to be forced
to monochrome. See https://bugs.kde.org/show_bug.cgi?id=451538.

There is an open merge request to fix that
(https://invent.kde.org/frameworks/kirigami/-/merge_requests/511), but
the fix is in Frameworks, which means it will only get to people who use
rolling release distros or whose packagers backport the fix.

This commit targeted at the Plasma/5.24 branch only is a local
workaround intended to alleviate the situation for LTS distro users who
are less likely to get the frameworks fix, once it's merged. The master
branch doesn't need the workaround since we can hope  that the
Frameworks fix will be merged before Plasma 5.25.

M  +6    -1    sidebar/package/contents/ui/SubCategoryPage.qml

https://invent.kde.org/plasma/systemsettings/commit/b579a5d94a3afbd6f85a95cf368a29e8c96c2b92
Comment 3 Bug Janitor Service 2022-05-17 11:35:25 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/systemsettings/-/merge_requests/132
Comment 4 Nate Graham 2022-05-17 16:47:52 UTC
*** Bug 453914 has been marked as a duplicate of this bug. ***
Comment 5 Nate Graham 2022-08-25 13:20:24 UTC
*** Bug 394800 has been marked as a duplicate of this bug. ***
Comment 6 Nate Graham 2022-08-25 13:20:32 UTC
*** Bug 421189 has been marked as a duplicate of this bug. ***
Comment 7 Nate Graham 2022-08-25 13:20:38 UTC
*** Bug 435873 has been marked as a duplicate of this bug. ***
Comment 8 Nate Graham 2022-08-25 13:20:55 UTC
*** Bug 458232 has been marked as a duplicate of this bug. ***
Comment 9 Nate Graham 2022-08-25 13:23:44 UTC
So the basic issue here is that Kirigami assumes that any icon with the `-symbolic` suffix is a monchrome icon, and it re-colors the icon to respect the color scheme. This isn't a safe assumption though. A variety of icon themes use colorful icons for this, and it's not against any spec because there is no spec to govern this. It's a *convention* from GNOME that icons whose names end with "-symbolic" are monochrome but it's simply not safe to assume.

We probably need to determine monochrome-ness by sampling the colors.
Comment 10 Nate Graham 2023-05-15 12:49:36 UTC
Git commit 66c6a75d44353c15101d872396be66b92d1c33e6 by Nate Graham, on behalf of Alexander Volkov.
Committed on 15/05/2023 at 12:49.
Pushed by ngraham into branch 'master'.

Fix painting of non-symbolic icons which are fallbacks for symbolic

A symbolic icon may be missing in an icon theme which may result in
loading the non-symbolic fallback. For example "window-close" may be
loaded instead of "window-close-symbolic". Then this fallback icon
should not be treated as symbolic.
FIXED-IN: 5.107

M  +14   -3    src/icon.cpp
M  +1    -0    src/icon.h

https://invent.kde.org/frameworks/kirigami/commit/66c6a75d44353c15101d872396be66b92d1c33e6
Comment 11 Nate Graham 2023-05-15 13:06:17 UTC
Git commit c6559f7142f71a80b5c92700fc7e51ca7bab697d by Nate Graham, on behalf of Alexander Volkov.
Committed on 15/05/2023 at 12:51.
Pushed by ngraham into branch 'kf5'.

Fix painting of non-symbolic icons which are fallbacks for symbolic

A symbolic icon may be missing in an icon theme which may result in
loading the non-symbolic fallback. For example "window-close" may be
loaded instead of "window-close-symbolic". Then this fallback icon
should not be treated as symbolic.
FIXED-IN: 5.107


(cherry picked from commit 66c6a75d44353c15101d872396be66b92d1c33e6)

M  +14   -3    src/icon.cpp
M  +1    -0    src/icon.h

https://invent.kde.org/frameworks/kirigami/commit/c6559f7142f71a80b5c92700fc7e51ca7bab697d
Comment 12 Nate Graham 2023-08-05 15:47:52 UTC
Git commit bdb322527fbc910ce7124a45dc43dfaa0fad1522 by Nate Graham.
Committed on 04/08/2023 at 16:58.
Pushed by ngraham into branch 'master'.

Icon: Remove automatic heuristic-based monochrome icon re-coloring

This feature has caused many bugs over the years and its value is
dubious to begin with. Automatically masking a colorful icon to instead
be monochrome based on this or that heuristic violates the integrity of
the user's icon theme and enforces a stylistic preference for monochrome
icons that is not appropriate to do at the library level.

If the user is using an icon theme whose icons have any color in them
despite being 16 or 22px size or using the `-symbolic` suffix, this
needs to be considered an intentional stylistic choice on the part of
the icon theme, and it isn't something that we should monkey with here.

As a bonus, we also reap an efficiency win by simply doing less
unnecessary work most of the time.

If a developer wants to do this masking anyway, they are still free to
set `mask: true` in their code and then it will happen on an opt-in
basis.
Related: bug 473001, bug 471803, bug 469978, bug 465422
FIXED-IN: 6.0

M  +2    -93   src/icon.cpp

https://invent.kde.org/frameworks/kirigami/-/commit/bdb322527fbc910ce7124a45dc43dfaa0fad1522