Bug 442533 - Icon re-coloring stopped working
Summary: Icon re-coloring stopped working
Status: RESOLVED FIXED
Alias: None
Product: frameworks-qqc2-desktop-style
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: 5.86.0
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Marco Martin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-16 14:55 UTC by bart
Modified: 2022-02-23 20:04 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
what I see (831.82 KB, image/png)
2021-09-16 23:39 UTC, Aleix Pol
Details
Simple testcase to reproduce the issue (621 bytes, text/plain)
2021-09-17 06:01 UTC, bart
Details
Result before commit (12.03 KB, image/png)
2021-09-17 06:02 UTC, bart
Details
Result after commit (12.01 KB, image/png)
2021-09-17 06:04 UTC, bart
Details

Note You need to log in before you can comment on or make changes to this bug.
Description bart 2021-09-16 14:55:20 UTC
SUMMARY
Icon re-coloring stopped working going from 5.84.0 to 5.85.0.
Using e.g. icon.color on a Button stopped working.

The issue has been traced to this commit: https://invent.kde.org/frameworks/qqc2-desktop-style/-/commit/f5307d10016f398b4371ef72bc145810b3a525b8

EXPECTED RESULT
Icon changes color when using the "icon.color" property on e.g. a qqc2 controls Button.
Comment 1 bart 2021-09-16 15:14:56 UTC
I just noticed that the same bug applies to qqc2-breeze-style.

In that case it's an almost identical commit which is causing the problems: https://invent.kde.org/plasma/qqc2-breeze-style/-/commit/cef3b1f0a6b1965d8f0402b9d515ce7ffa2ce3bd
Comment 2 Aleix Pol 2021-09-16 23:39:40 UTC
Created attachment 141632 [details]
what I see

Are you sure? It's working for me.
Comment 3 Aleix Pol 2021-09-16 23:57:14 UTC
Is it possible that you are seeing this bug?
https://bugs.kde.org/show_bug.cgi?id=442569
Comment 4 bart 2021-09-17 06:01:45 UTC
Created attachment 141636 [details]
Simple testcase to reproduce the issue
Comment 5 bart 2021-09-17 06:02:30 UTC
Created attachment 141637 [details]
Result before commit
Comment 6 bart 2021-09-17 06:04:07 UTC
Created attachment 141638 [details]
Result after commit
Comment 7 bart 2021-09-17 06:05:58 UTC
I'm not sure whether the other bug is related or not.

I've just uploaded a very simple test case, and screenshots of what I see before and after the mentioned commit.  I hope this will help to reproduce the issue.
Comment 8 bart 2021-09-17 06:16:19 UTC
Sorry to spam the bug report.  I've just tested the other bug you mentioned: that one seems to be unrelated because that one already present before this particular commit.
Comment 9 Aleix Pol 2021-09-17 17:20:16 UTC
I could finally reproduce, thanks.

It turns out it's a bit of a race condition situation so I had to force it a bit.

Will send a few MR to see if we can get this one right.
Comment 10 Aleix Pol 2021-09-17 17:29:56 UTC
I suggest porting to the new API on qqc2-desktop-style (frameworks) and qqc2-breeze-style (plasma) master. On qqc2-breeze-style 5.23 branch, reverting my commit so users don't get the regression.
Comment 11 Nate Graham 2021-10-05 21:43:00 UTC
Git commit 513485606d030b7678f54c3e9121f7d43ef94849 by Nate Graham, on behalf of Aleix Pol.
Committed on 05/10/2021 at 21:41.
Pushed by ngraham into branch 'master'.

Make icon colouring a per-icon property rather than a system

At the moment we have icon users setting a global property into a
singleton rather than keeping it on a per-icon basis. This only works as
long as you are not dealing with different icons at the same time.

Instead, keep the QPalette in the KIconEngine (which is per-icon
instance) and the icon will know itself which palette to use.
This commit doesn't change the previous behaviour because it's widely
used although I imagine we can port many of the use cases into this one
and clean a bunch of code.

See PlasmaDesktopTheme::iconFromTheme in qqc2-desktop-style and
qqc2-breeze-style.

M  +21   -3    src/kiconengine.cpp
M  +9    -0    src/kiconengine.h
M  +43   -12   src/kiconloader.cpp
M  +12   -0    src/kiconloader.h

https://invent.kde.org/frameworks/kiconthemes/commit/513485606d030b7678f54c3e9121f7d43ef94849
Comment 12 Gabriel 2022-02-23 15:38:26 UTC
Since there is a commit related to this, does that mean that the bug is now fixed? If it is then I guess the report can be closed.
Comment 13 bart 2022-02-23 20:04:32 UTC
Ah, right.  I forgot that this was still open.
I think that there were a few other dependencies other than that commit that was mentioned which were required to get this solved.  But in the meantime it has been completely solved.  (I just reran the test case to be sure.)

So I'll close it now.

Thanks again to all involved in solving this.