Ensure surface normals and wireframes are using Models internally

[brisvag]

Description

Make sure that surface normals are always using the SurfaceNormal model internally (now they can sometimes be dict, which breaks a few assumptions in other parts of the codebase). See napari/napari-animation#145.

I also took the chance to add an event for them, and do the same fix for the wireframe.

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.

[alisterburt]

setters look good, thanks! tests are a bit vague so maybe a docstring with the expected behaviour detailed (dict is accepted, model should always be SurfaceNormals) would be good - non blocking though

[codecov]

Codecov Report

Merging #5501 (e6c168f) into main (465c281) will decrease coverage by 0.04%.
The diff coverage is 96.55%.

❗ Current head e6c168f differs from pull request most recent head 369e05d. Consider uploading reports for the commit 369e05d to get more accurate results

@@            Coverage Diff             @@
##             main    #5501      +/-   ##
==========================================
- Coverage   89.31%   89.27%   -0.04%     
==========================================
  Files         600      600              
  Lines       51019    51075      +56     
==========================================
+ Hits        45566    45596      +30     
- Misses       5453     5479      +26     

Impacted Files	Coverage Δ
napari/layers/surface/surface.py	87.00% <93.33%> (+0.95%)	⬆️
napari/layers/surface/_tests/test_surface.py	100.00% <100.00%> (ø)
napari/_tests/test_draw.py	36.84% <0.00%> (-63.16%)	⬇️
napari/components/experimental/chunk/_pool.py	85.71% <0.00%> (-7.94%)	⬇️
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	80.27% <0.00%> (-0.69%)	⬇️
napari/_qt/qt_main_window.py	74.39% <0.00%> (-0.18%)	⬇️
napari/_qt/dialogs/qt_package_installer.py	81.67% <0.00%> (+0.39%)	⬆️
... and 2 more

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

[Czaki]

Maybe also nice to add type annotation

[brisvag]

Actually, I'm not super happy with the current setters, cause what I really wanted to do was also run model.update(**dict(new_value)) for the cases of dict and SurfaceNormals, which would ensure that we update inplace so we don't mess up event sources etc...

Unfortunately, this fails because anything that has allow_mutation=False (several things in SurfaceNormals model) straight up fails if it's present in the dict. I could pop it, but it seemed convoluted... but maybe that's the better solution

[brisvag]

Ok, I tried to implement it the "proper way". This also allows to add a connection of the normals event to the layer events themselves (mirroring the implementation of Image.events.plane).

[kephale]

This looks great. I also just ran into the napari-animation issue: napari/napari-animation#145 because I had the same error. This looks like it should catch all of the cases that was letting dicts float around as attribute values.

[edit]
I'll also note that I am using this patch to work around the napari-animation issue. Now surface animations work nicely.

[brisvag]

Thanks for the ping, I forgot about this :P Merging!