feat: support icon color - followup

[brisvag]

Trying to pick up #130!

I fixed conflicts and brought it up to date. @tlambert03 was there something specific with that PR that was incomplete or you wanted to finish?

[codecov]

Codecov Report

❌ Patch coverage is 88.52459% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.42%. Comparing base (8e49944) to head (e559ef2).

Files with missing lines	Patch %	Lines
src/app_model/_app.py	58.33%	5 Missing ⚠️
src/app_model/types/_icon.py	83.33%	2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (88.52%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #277      +/-   ##
==========================================
- Coverage   99.78%   99.42%   -0.37%     
==========================================
  Files          31       31              
  Lines        1851     1903      +52     
==========================================
+ Hits         1847     1892      +45     
- Misses          4       11       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tlambert03]

@brisvag, I think the main reason that PR stalled is because of the problem with theme changes. as mentioned in the first comment:

note that svg-colored icons don't currently change if the parent style sheet changes. This is just a generally harder thing to solve (and one of the few benefits of font-icons). However, we could, as magicgui does, attach QPallete change events here and auto-change icon colors in a future PR.

so, currently here:

  app = Application("myapp")
  app.theme_mode = "dark"  # or let it auto-detect

  Action(
      id="my.action",
      title="Do Thing",
      icon={"dark": "mdi:some-icon", "color_dark": "#FFFFFF", "color_light": "#000000"},
      ...
  )

The icon color is resolved once — at the moment the QAction or QMenuSubMenu is constructed. The resulting QIcon is a static pixmap/SVG baked with whatever color was chosen at creation time. If the user later, changes app.theme_mode from "dark" to "light", or wwitches their OS/Qt theme (causing the QPalette to change), nothing re-runs to_qicon(). The icons stay the old color. There's no signal, no QPalette change event listener, no theme_mode change signal — nothing triggers a re-render.

so, it feels like a partially implemented thing that will pretty quickly have a bug report or feature request

[brisvag]

Does something like this make sense?

One thing I noticed when testing this out with demo/model_app.py is that the "disabled" color seems to be hardcoded. Not sure if this is happening qt the qt level or if it's something that can/should be controlled by superqt when creating the icon.

[tlambert03]

Does something like this make sense?

if you try it out and it works for you, it seems fine to me.

that the "disabled" color seems to be hardcoded. Not sure if this is happening qt the qt level or if it's something that can/should be controlled by superqt when creating the icon.

i need a bit more context: what did you observe and how did it differ from what you expected?

[tlambert03]

pyright is unhappy because it says changeEvent is not a method on QAction (of which this is a subclass), only on QWidget... so are we sure this method actually ever fires?

[brisvag]

Yep, I thought it worked but I tested it wrong. Fixed now using event filters instead.

[brisvag]

Ok so I pushed some changes that should make this work as intended. I also updated temporarily the demo so you test it out, with 2 new buttons: the first changes "system theme" by changing the qpalette. The second changes the model's theme between light and dark manually.

When you swap theme, you'll see that while the icon theme color overrides (blue and red) apply correctly to the two new buttons, they don't affect the scissors icon as long as it's disabled: that grey-out colopr is hardcoded somewhere else. IMO it should be a desaturated version of the provided color?