fix theme id not being used correctly

[brisvag]

As discussed in this zulip conversation, napari is currently not respecting icons when it comes to theme contributions.

Turns out, the theme_dict was constructed by updating the base theme of the same type (dark or light) with the new values, but only the colors were being updated, leaving any non-color contribution the same (such as the theme name [which I renamed id for consistency with the schema; this made it harder to debug]). Since name was left to dark, and this was the field used to pick the correct generated icons, we ended up with a half theme.

The only "real" changes here (among the many variable renames) are inside utils/theme.py:

• label is now read from the contribution instead of ignored (though it's not used anywhere in napari yet)
• the dict is updated with all the contribution info, see at the bottom of the file

Description

Type of change

• Bug-fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

References

How has this been tested?

• example: the test suite for my feature covers cases x, y, and z
• example: all tests pass with my change
• example: I check if my changes works with both PySide and PyQt backends
  as there are small differences between the two Qt bindings.

Final checklist:

• My PR is the minimum possible work for the desired functionality
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• If I included new strings, I have used trans. to make them localizable.
  For more information see our translations guide.

[Czaki]

Looks ok. Also tested locally (need to update gruvbox plugin) and it works well.

[codecov]

Codecov Report

Merging #5412 (b7a27bc) into main (96f99e2) will increase coverage by 0.03%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##             main    #5412      +/-   ##
==========================================
+ Coverage   89.01%   89.05%   +0.03%     
==========================================
  Files         595      595              
  Lines       50564    50584      +20     
==========================================
+ Hits        45012    45048      +36     
+ Misses       5552     5536      -16     

Impacted Files	Coverage Δ
napari/utils/theme.py	92.85% <95.83%> (+0.78%)	⬆️
napari/_qt/_tests/test_qt_window.py	100.00% <100.00%> (ø)
napari/_qt/qt_resources/__init__.py	79.16% <100.00%> (ø)
napari/plugins/_plugin_manager.py	75.44% <100.00%> (ø)
napari/utils/_tests/test_theme.py	100.00% <100.00%> (ø)
napari/_tests/test_draw.py	36.84% <0.00%> (-63.16%)	⬇️
napari/_qt/widgets/qt_color_swatch.py	71.54% <0.00%> (-3.26%)	⬇️
napari/utils/interactions.py	74.52% <0.00%> (-2.84%)	⬇️
napari/utils/info.py	78.35% <0.00%> (-2.07%)	⬇️
napari/_qt/qt_event_loop.py	81.63% <0.00%> (-0.69%)	⬇️
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[brisvag]

As this is relatively small and hits a not-yet used plugin interface, I'll start the 24h merge countdown :)

[psobolewskiPhD]

Nice to see this work!
I think this is the easiest solution, rather than changing the manifest stuff (id->name or adding name), even if I doubt that there are too many (any?) theme plugins.

Thinking out loud, not this PR, just for the record: I wonder if we can use that light dark type to make custom themes work with system?
Like use theme type internally? So the theme wouldn't be Dark or Light but napari and get_system_theme could return theme.type and to make it Light vs Dark.
Then gruvbox could have two types, instead of two theme id, and would work with light and dark system settings?

Edit: BTW this makes me thing that system shouldn't be a theme.id (ex name) but a theme.type that gets set by get_system...

Also, should make a PR to have settings use label and not id for the user facing things.

[Czaki]

Thinking out loud, not this PR, just for the record: I wonder if we can use that light dark type to make custom themes work with system?

Yes. We need to add an option to setting to add a selection of custom dark, and custom light themes.

[brisvag]

rather than changing the manifest stuff (id->name or adding name),

I think we should also do that, but this change at least fixes the current behaviour. In fact, I think we should just remove the double id/name and just use name to generate an id by snake_casing it.

Then gruvbox could have two types, instead of two theme id, and would work with light and dark system settings?

That makes sense, but we should consider the possibility of a contribution of a single type of theme.

Also, should make a PR to have settings use label and not id for the user facing things.

Yup; I tried to include it here but it was getting complicated so I just left it for later.

[brisvag]

@andy-sweet we got what looks like a random failure here, you might be interested: https://github.com/napari/napari/actions/runs/3714126707/jobs/6297620663#step:8:306

[brisvag]

Will merge now that spurious tests passed :)

[andy-sweet]

@andy-sweet we got what looks like a random failure here, you might be interested: https://github.com/napari/napari/actions/runs/3714126707/jobs/6297620663#step:8:306

Thanks, I suspect the first task there hasn't actually been started. Will fix that and any others shortly.