Explicit convert color to string when compile resources

[Czaki]

Description

One of the big problems when debugging #4995 for me was that get_colorized_svg signature expects color to be a string, and we pass pydantic.color.Color to it. So I lost time digging into what could go wrong and why we passed unexpected arguments.

As follow up we could simply convert color to string before this phase (what is done in this PR) or update the signature of get_colorized_svg to assume that we could accept Color class also to convert it into a string later.

I personally prefer the conversion way, but if you think of another way, the signature update is also ok.

Type of change

• Refactor

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.

[codecov]

Codecov Report

Merging #4997 (cbc949e) into main (5a5392d) will increase coverage by 1.71%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4997      +/-   ##
==========================================
+ Coverage   85.72%   87.43%   +1.71%     
==========================================
  Files         586      586              
  Lines       49498    49538      +40     
==========================================
+ Hits        42430    43314     +884     
+ Misses       7068     6224     -844     

Impacted Files	Coverage Δ
napari/_qt/qt_resources/_svg.py	97.56% <100.00%> (ø)
napari/__main__.py	51.19% <0.00%> (-6.11%)	⬇️
napari/utils/config.py	100.00% <0.00%> (ø)
napari/plugins/_npe2.py	92.85% <0.00%> (ø)
napari/layers/utils/string_encoding.py	100.00% <0.00%> (ø)
napari/components/experimental/monitor/_api.py	0.00% <0.00%> (ø)
napari/utils/_tests/test_key_bindings.py	97.16% <0.00%> (+0.01%)	⬆️
napari/layers/utils/color_encoding.py	96.20% <0.00%> (+0.04%)	⬆️
napari/layers/utils/style_encoding.py	96.22% <0.00%> (+0.07%)	⬆️
napari/layers/shapes/_tests/test_shapes_utils.py	97.22% <0.00%> (+0.07%)	⬆️
... and 62 more

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

[andy-sweet]

Looks good to me.

Not that calling str on a Pydantic Color will return the color name (e.g. cornflowerblue) if its value corresponds to one the known named colors, and otherwise it will return the the hex string that represents the color value.

Based on my understanding Pydantic supports color names (e.g. cornflowerblue) based on the CSS3 spec and Qt supports color names based on the SVG 1.0 spec, and these are identical.

If you wanted to bypass that explanation to anyone else, you could use Color.as_hex which will always return the hex string (or maybe Color.as_rgb which returns the rgb(a) string). But I don't feel strongly here. I'd also be fine with changing type annotations from str to Color.

[Czaki]

This pr reproduce current behaviour when we do implicit conversion by passing color as format argument.

[andy-sweet]

This pr reproduce current behaviour when we do implicit conversion by passing color as format argument.

Understood. Just pointing out that the behavior of string conversion for Pydantic's Color is a little complex, but also entirely reasonable.

[alisterburt]

@andy-sweet I see you have approved - would you suggest that we switch to always using hex codes or keep the current behaviour in this PR?

Marked as 'merge for 0.4.17'!

[Carreau]

I don't have any particular preference on use to_hex or str(color).

My thought is that any weird case heisenbug due to str() might be harder to debug and https://en.wikipedia.org/wiki/Robustness_principle
says "be conservative in what you send, be liberal in what you accept", so maybe a small pref on to_hex, but I think the change we hit a corner case if really small.

[brisvag]

I agree, there's no need to be magic where users are not involved; I vote for hex!

[brisvag]

Multi-approved and simple change. Merging!