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 93.39623% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.44%. Comparing base (82f70b3) to head (0cd8d04).

Files with missing lines	Patch %	Lines
src/app_model/_app.py	92.30%	2 Missing ⚠️
src/app_model/backends/qt/_qmenu.py	88.23%	2 Missing ⚠️
src/app_model/types/_icon.py	83.33%	2 Missing ⚠️
src/app_model/backends/qt/_util.py	96.87%	1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (93.39%) 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.44%   -0.35%     
==========================================
  Files          31       31              
  Lines        1879     1971      +92     
==========================================
+ Hits         1875     1960      +85     
- 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?

[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?

[brisvag]

@tlambert03 ping in case this slipped through the cracks!

[brisvag]

I added a small test. I can't figure out how I'm supposed to test this on submenus, the tests there a quite a bit more complex 🤔

[tlambert03]

thanks!

[tlambert03]

actually... are you guys even using QModelMainWindow? It's not a mandatory feature for using app-model. So maybe it's not the best place to install the event filter? We could install it at the same place you did before, but just have a module-level variable ensuring it only ever gets installed once (could even potentially store the even filter object on the app itself and check for it's presence?). This spot here requires the user to be using the highest level feature (the main window) in order to benefit from a pretty low level feature (icons in menus/toolbars)

[brisvag]

I tried a bunch of ways, but being smart with a proper singleton and checking filter installation did not quite work for me... I ended up with just creating a separate filter object that is stored on the app itself as you suggested, and the places that care about it (qaction and qsubmenu) install the filter unless it's already present. I checked and there's no duplication of events through the filter!

[tlambert03]

I ended up with just creating a separate filter object that is stored on the app itself as you suggested, and the places that care about it (qaction and qsubmenu) install the filter unless it's already present. I checked and there's no duplication of events through the filter!

awesome thanks! this is exactly what I was picturing

[brisvag]

Nice! One thhing that remains to figure out is if/how we can change the color of the disabled icons, cause that issue is still there

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?

[brisvag]

ouch... I just tried this on napari and performance TANKS. It's like 20 seconds to launch O.O There's definitely way too many events going through the filter... I tried to add checks to the icon update to minimize the call to to_qicon, but that doesn't seem to really help, so it must be above that point. A snippet from the profiling:

This is just from launching.

[brisvag]

Ok my bad, the check wasn't properly set up. This does improve things significantly, but the slowdown is still bad (from 2.3 seconds to 3.9).

[brisvag]

A lot better, but still heavy to even just run the filter:

I wonder if at this point it's better to have individual filters but that only run on events that hit the objects of interest?

[brisvag]

Ok I managed to do it via the application filter. I added a check so that this filter only fires once per event, because it will only fire on the QApplication.instance() object. This fully fixes the performance issue for me.
However, I had to add a singleshot timer to delay the app.theme_mode event firing, otherwise it would result in reading the outdated colors because it was firing before the palette of all the widgets were updated.

[brisvag]

I'm not sure what's up with the tests, that seems unrelated and the copy action works fine when I test it manually 🤔

[tlambert03]

I wonder if at this point it's better to have individual filters but that only run on events that hit the objects of interest?

you can certainly give it a try! but make sure to set it up in a realistic way with at least 10-20 actions, including visible toolbuttons. without testing, i would have assumed that a single check for "did the palette change" on the global application would have been faster than a per-action check. but i haven't tested it, so it's definitely worth a check

another option is to just ditch, for now, the automatic event filter... and leave it up to the end use to call some public register_palette_event or something like that.

lastly... do you know why the macos tests are failing now? they seemed to have been passing back in 1744f13 ... but not since c3e7bce

[tlambert03]

I'm not sure what's up with the tests, that seems unrelated and the copy action works fine when I test it manually 🤔

ok, re-running on main, and if they exist there, then don't worry about it

[tlambert03]

yeah, it's on main too. something must have been released. ok, you ok with merge?

[brisvag]

you can certainly give it a try! but make sure to set it up in a realistic way with at least 10-20 actions, including visible toolbuttons. without testing, i would have assumed that a single check for "did the palette change" on the global application would have been faster than a per-action check. but i haven't tested it, so it's definitely worth a check

I wrote that before I figured out the proper solution, so yeah it definitely works best with a global filter as long as the filter is only firing once per event by checking a0 is QApplication.instance(). I think we can merge!

[brisvag]

Side note: I just realized that I we can't actually update icon colors once instantiated, because actions are frozen models. This means that even with this PR, napari won't be able to change icon colors on the fly when the theme colors themselves change. Is this something you think we should be able to do/achieve?

Additionally (and potentially connected), it seems like it would be useful to have default icon colors settable at application level (instead of the currently hardcoded LIGHT/DARK_COLOR) as a fallback, so we don't need to pass them to every action. This could be a followup PR as well.

[tlambert03]

I just realized that I we can't actually update icon colors once instantiated, because actions are frozen models. This means that even with this PR, napari won't be able to change icon colors on the fly when the theme colors themselves change. Is this something you think we should be able to do/achieve?

yeah, I would say that a true dynamic theming approach is far more complicated than what my original icon task here was trying to accomplish (and indeed, beyond what app-model was designed to do). App model was about actions/commands/keybindings/menus/taskbuttons.

Icon colors sit on the edge of this (because of course task buttons need them), and a "real" theming framework. I've done a lot more research into this this year, and was beginning to work on the solution that I personally want (see https://github.com/tlambert03/carina ... which is based on https://github.com/oclero/qlementine and these bindings https://github.com/pyapp-kit/PyQlementine). That has proper first-class support for theme-aware icon coloring... and much more.

I would say that full dynamic re-skinning is outside of the scope of this PR. It's true that we started to go down that rabbit hole with the palette change event, and we're definitely flirting with that territory now in a somewhat unsatisfactory way (given that action defs are immutable). If we don't take that full topic on here, is there anything here you still actually want? What was your direct interest in this PR? Are you hoping to integrate app-model more deeply into a full icon theming approach in napari?

[brisvag]

I think even in this state this would be useful to merge, if you're on board with it. Most use cases do not involve changing theme while the application is running, and for now we can just recommend a restart.

What was your direct interest in this PR? Are you hoping to integrate app-model more deeply into a full icon theming approach in napari?

Yes, I ultimately would like to get icons to be in sync with the theme like everything else. It's already great to be able to switch between provided icon images from dark to light, but for non-image icons (the once via fontawesome and so on) they will be out of sync with the rest of the app's icons if one cannot set these default colors. Would you be opposed to just adding setters for dark and light default colors at application level? In fact, this PR woiuld be a lot simpler and probably cover most use cases if we didn't allow setting individual icon colors, and instead we only had application-wide dark/light colors.

[tlambert03]

Would you be opposed to just adding setters for dark and light default colors at application level?

i'm open to seeing it.

In fact, this PR woiuld be a lot simpler and probably cover most use cases if we didn't allow setting individual icon colors

that's true... but honestly, my primary motivation for #130 was specifically that - to enable things like a red "danger" icon. so, forcing full homogeneity would be a loss in that regard

[brisvag]

Last commit works for me, tested in a branch in napari and it's all working as intended there too!