Remove layer ndisplay event #5491

andy-sweet · 2023-01-18T21:11:45Z

Description

This removes the private event Layer.events._ndisplay. This was needed to notify the layer control widgets, some of which should only be presented in 2D others in 3D. Now, each layer control widget stores ndisplay as a public mutable property and triggers similar updates when ViewerModel.dims.ndisplay changes.

There are two motivations for this change.

It makes Layer._slice_dims simpler and more clearly similar to Layer.refresh.
We use and test against the public API of the widgets and model rather than rely on a private event.

The main downside of this change is that the layer control widget has some extra state (ndisplay) that we need to keep in sync. Initially, I hoped we could pass through the new value of ndisplay through events (rather than store it as state), but things are complex enough in QtImageControls to make that difficult.

Type of change

This is mostly a refactor with no napari public API changes.

References

Partly motivated by #4795

How has this been tested?

all existing tests pass with my change

andy-sweet · 2023-01-18T21:13:18Z

@jni : this is my attempt at removing Layer.events._ndisplay. I left myself some TODOs to highlight where I'm not happy, but where things might be good enough. That's why this is still a draft/WIP PR.

@alisterburt : you might also be interested in this, as it hits a bit on Image.depiction and implications for related controls.

codecov · 2023-01-18T21:36:59Z

Codecov Report

Merging #5491 (7b6d56f) into main (42f0e7f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5491   +/-   ##
=======================================
  Coverage   89.30%   89.31%           
=======================================
  Files         600      600           
  Lines       51011    51011           
=======================================
+ Hits        45556    45561    +5     
+ Misses       5455     5450    -5

Impacted Files	Coverage Δ
napari/layers/base/base.py	`91.53% <ø> (-0.05%)`	⬇️
...i/_qt/layer_controls/_tests/test_qt_image_layer.py	`100.00% <100.00%> (ø)`
napari/_qt/layer_controls/qt_image_controls.py	`99.48% <100.00%> (-0.03%)`	⬇️
napari/_qt/layer_controls/qt_labels_controls.py	`91.32% <100.00%> (-0.10%)`	⬇️
...apari/_qt/layer_controls/qt_layer_controls_base.py	`92.50% <100.00%> (+1.19%)`	⬆️
.../_qt/layer_controls/qt_layer_controls_container.py	`100.00% <100.00%> (ø)`
napari/_qt/layer_controls/qt_vectors_controls.py	`70.31% <100.00%> (ø)`
napari/_qt/dialogs/qt_package_installer.py	`81.67% <0.00%> (+0.39%)`	⬆️
napari/utils/theme.py	`92.89% <0.00%> (+0.59%)`	⬆️
napari/utils/info.py	`81.44% <0.00%> (+1.03%)`	⬆️
... and 1 more

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

brisvag · 2023-01-19T14:17:08Z

napari/_qt/layer_controls/qt_image_controls.py

@@ -227,11 +227,11 @@ def changeRendering(self, text):
              This will make nearer objects appear more prominent.
        """
        self.layer.rendering = text
-        self._toggle_rendering_parameter_visbility()
+        self._update_rendering_parameter_visbility()


Old typo in "visibility" :)

Good catch! Fixed!

brisvag · 2023-01-19T14:21:47Z

napari/_qt/layer_controls/qt_layer_controls_container.py

+        viewer.dims.events.ndisplay.connect(self._on_ndisplay_change)
+
+        # TODO: I think this is needed if viewer is initialized with ndisplay=3,
+        # but maybe we should do this differently.
+        self._on_ndisplay_change(Event('ndisplay', value=viewer.dims.ndisplay))
+
+    def _on_ndisplay_change(self, event):
+        for widget in self.widgets.values():
+            if widget is not self.empty_widget:
+                widget.ndisplay = event.value


This is kind ahow we handle VispyLayers in the backend (calling reset()), so it makes sense to me. Not sure we need the event actually, since you can simply do:

self._on_display_change()

and then

def _on_ndisplay_change(self): for widget in self.widgets.values(): if widget is not self.empty_widget: widget.ndisplay = viewer.dims.ndisplay

Just like we handle all these things in VispyLayers.

Good to know. I actually realized that _on_ndisplay_change will never do anything at this point since self.widgets only contains self.empty_widget, so I just deleted this.

andy-sweet · 2023-01-19T15:12:56Z

napari/_qt/layer_controls/qt_image_controls.py


    def changeDepiction(self, text):
        self.layer.depiction = text
-        self._toggle_plane_parameter_visibility()
+        self._update_plane_parameter_visibility()


I made these changes to clarify that this is updating based on other state, rather than a true toggle of the plane control visibility.

andy-sweet · 2023-01-19T15:13:11Z

napari/_qt/layer_controls/qt_image_controls.py

-            self.attenuationLabel.hide()
-
-    def _toggle_plane_parameter_visibility(self):
+        iso_threshold_visible = rendering == ImageRendering.ISO


I made this changes to be a bit more concise and functional.

andy-sweet · 2023-01-19T15:17:12Z

Thanks for the review @brisvag ! As you gave this a first pass and I addressed your feedback, I changed this from draft to ready to review. If you're happy to approve/reject this, please do!

I think the main talking point of this PR is adding the ndisplay state to these control panels. Though I think that's an unavoidable consequence of our long term plan to move slice/display state off of the layer.

brisvag

I think the changes make perfect sense, the state should be held here, imo. I like the small logic simplification as well. I'm not sure I like the kwarg approach, but I don't consider this blocking!

brisvag · 2023-01-19T15:22:49Z

napari/_qt/layer_controls/qt_image_controls.py

-    def __init__(self, layer):
-        super().__init__(layer)


One thing that I'm not sure about is having this as a parameter; is that necessary, or can we just initialize it like everything else and only update it via events?

The main reason I did it this way is that if a layer is added to the viewer (i.e. without ndisplay changing), we have to assign the correct value of from QtLayerControlsContainer to the layer control panel somehow. Passing the value as an initialization parameter means that any other initialization that might be dependent on that value has access to it immediately. It's also more atomic - i.e. we can avoid initializing then setting either explicitly or through manually calling a callback/event.

Does that explanation seem good enough? Just wondering before I change **kwargs to *, ndisplay: int = 2 for the other layer types.

I actually lean more towards explicitly calling the callback at the end of the init. This is what we do with all the VispyLayers (and soon Overlays), and since I'm familiar with that part of the codebase, doing it that way feels more "consistent" to me.

However, since here the controls do not have references to the viewer, we can just do it in the container by adding this line to _add():

class QtLayerControlsContainer(QStackedWidget): def _add(self, event): ... controls.ndisplay = viewer.ndisplay ...

I lean the other way, but don't feel too strongly. So in the interest of trying new things and being more consistent with other parts of the code base, I made the changes you suggested.

andy-sweet · 2023-01-19T17:16:48Z

I'm not sure I like the kwarg approach, but I don't consider this blocking!

Yeah, I don't like that too much either. I did it out of laziness, especially since this was a draft PR. But now, I'm happy to go in and just explicitly expand the signature to include ndisplay as a keyword argument for all the layer control classes.

andy-sweet · 2023-01-19T18:59:35Z

napari/_qt/layer_controls/qt_layer_controls_container.py

@@ -148,7 +165,6 @@ def _remove(self, event):
        layer = event.value
        controls = self.widgets[layer]
        self.removeWidget(controls)
-        # controls.close()


I have a passion for deleting commented code that lacks any explanatory comments.

andy-sweet · 2023-01-20T17:33:26Z

napari/_qt/layer_controls/_tests/test_qt_image_layer.py

    qtctrl = QtImageControls(layer)
    qtbot.addWidget(qtctrl)

-    layer._slice_dims(ndisplay=3)


Since QtImageControls.ndisplay == 2 is the default on initialization now, I reordered the test here to make it a little shorter and understandable (i.e. one mutation instead of two).

andy-sweet · 2023-01-20T17:37:30Z

Since this has approval and I've addressed some optional comments, I'll assume this is good to merge. But it's also Friday for me, so I'll probably wait until Monday comes around to push the green button. So feel free to object and suggest alternatives before then.

This removes the private event `Layer.events._ndisplay`. This was needed to notify the layer control widgets, some of which should only be presented in 2D others in 3D. Now, each layer control widget stores `ndisplay` as a public mutable property and triggers similar updates when `ViewerModel.dims.ndisplay` changes. There are two motivations for this change. 1. It makes `Layer._slice_dims` simpler and more clearly similar to `Layer.refresh`. 2. We use and test against the public API of the widgets and model rather than rely on a private event. The main downside of this change is that the layer control widget has some extra state (`ndisplay`) that we need to keep in sync. Initially, I hoped we could pass through the new value of `ndisplay` through events (rather than store it as state), but things are complex enough in `QtImageControls` to make that difficult. This is mostly a refactor with no napari public API changes. Partly motivated by #4795 - [x] all existing tests pass with my change

andy-sweet added 3 commits December 23, 2022 12:43

Initial changes

dc6017f

Merge branch 'main' into remove-layer-ndisplay-event

a3acd23

Store ndisplay as state in controls widget

4517a9f

andy-sweet requested a review from jni January 18, 2023 21:11

github-actions bot assigned andy-sweet Jan 18, 2023

github-actions bot added qt Relates to qt tests Something related to our tests labels Jan 18, 2023

brisvag reviewed Jan 19, 2023

View reviewed changes

andy-sweet added 2 commits January 19, 2023 07:10

Address PR feedback

1843149

Add docstring

68d0d52

andy-sweet commented Jan 19, 2023

View reviewed changes

andy-sweet marked this pull request as ready for review January 19, 2023 15:13

Merge branch 'main' into remove-layer-ndisplay-event

eb39ed3

brisvag approved these changes Jan 19, 2023

View reviewed changes

andy-sweet changed the title ~~[WIP] Remove layer ndisplay event~~ Remove layer ndisplay event Jan 19, 2023

andy-sweet commented Jan 19, 2023

View reviewed changes

andy-sweet added 3 commits January 20, 2023 09:12

Merge branch 'main' into remove-layer-ndisplay-event

604ab9d

Inject ndisplay through property

ec94ef9

Change existing test to use property

f21f3f4

andy-sweet commented Jan 20, 2023

View reviewed changes

Remove unnecessary kwargs

7b6d56f

andy-sweet merged commit 7b1be74 into napari:main Jan 23, 2023

andy-sweet deleted the remove-layer-ndisplay-event branch January 23, 2023 18:58

brisvag mentioned this pull request Jan 26, 2023

Overlays 2.0 #4894

Merged

7 tasks

andy-sweet mentioned this pull request Feb 9, 2023

Move layer editable change from slicing to controls #5546

Merged

Czaki mentioned this pull request Jun 7, 2023

v0.4.18 - release process #5911

Closed

Czaki added this to the 0.5.0 milestone Jun 13, 2023

Czaki added maintenance PR with maintance changes, triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process labels Jun 13, 2023

Czaki modified the milestones: 0.5.0, 0.4.18 Jun 18, 2023

Chris-N-K mentioned this pull request Aug 2, 2023

Genrally broken with napari 0.4.18 bauerdavid/napari-bbox#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove layer ndisplay event #5491

Remove layer ndisplay event #5491

andy-sweet commented Jan 18, 2023 •

edited

andy-sweet commented Jan 18, 2023

codecov bot commented Jan 18, 2023 •

edited

brisvag Jan 19, 2023

andy-sweet Jan 19, 2023

brisvag Jan 19, 2023

andy-sweet Jan 19, 2023

andy-sweet Jan 19, 2023 •

edited

andy-sweet Jan 19, 2023

andy-sweet commented Jan 19, 2023

brisvag left a comment

brisvag Jan 19, 2023

andy-sweet Jan 19, 2023

andy-sweet Jan 19, 2023

brisvag Jan 20, 2023

andy-sweet Jan 20, 2023

andy-sweet commented Jan 19, 2023

andy-sweet Jan 19, 2023

andy-sweet Jan 20, 2023

andy-sweet commented Jan 20, 2023 •

edited

Remove layer ndisplay event #5491

Remove layer ndisplay event #5491

Conversation

andy-sweet commented Jan 18, 2023 • edited

Description

Type of change

References

How has this been tested?

andy-sweet commented Jan 18, 2023

codecov bot commented Jan 18, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet Jan 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet commented Jan 19, 2023

brisvag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet commented Jan 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet commented Jan 20, 2023 • edited

andy-sweet commented Jan 18, 2023 •

edited

codecov bot commented Jan 18, 2023 •

edited

andy-sweet Jan 19, 2023 •

edited

andy-sweet commented Jan 20, 2023 •

edited