Reduce dependence on ViewerModel, using Layer(List) instead by TimMonko · Pull Request #125 · napari/napari-metadata

TimMonko · 2026-03-15T04:42:38Z

References and relevant issues

Built on #124 diff: TimMonko/napari-metadata@enh/layer-events...TimMonko:napari-metadata:enh/reduce-complexity

Born out of discussion about 12 hours ago with @carlosmariorr

Description

Significantly cleans up the code by only placing the ViewerModel inheritance at the npe2 MetadataWidget (_main.py) boundary. Other widgets now only need to inherit Layer (Axis and File widgets) or LayerList (Inheritance Widget), this creates significantly easier reasoning about what should happen in each widget.

Because we know that the subwidgets are only shown in the container if a selection is active, we also can assume there will never be a None layer. So, now we also don't need resolve_layer and instead can just depend directly on public layer API. Effectively now layer_utils.py only contains special logic (a good example of this is the dtype one). Because of widget construction lifetimes, we still do need to check for Nones (unfortuantely), so there is some complexity here still, but at least we don't have if None checks everyone.

This is going to be slightly annoying to review, but one key is that tests are either removed (layer_utils functions being gone) or are simpler because they drop ViewerModel and are typed differently. As such, the test suite passing with no logic changes indicates that this doesn't break functionality.

I also tested in napari itself locally, and all events are still hooked up well.

codecov · 2026-03-15T04:46:30Z

Codecov Report

❌ Patch coverage is 99.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.06%. Comparing base (04a1701) to head (f82d72f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/napari_metadata/widgets/_base.py	98.01%	2 Missing ⚠️
src/napari_metadata/widgets/_main.py	97.14%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
+ Coverage   94.47%   97.06%   +2.58%     
==========================================
  Files          11       11              
  Lines        1429     1429              
==========================================
+ Hits         1350     1387      +37     
+ Misses         79       42      -37

☔ 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.

TimMonko · 2026-03-16T22:08:53Z

after #107 there are lots of conflicts here, which I expected. I'm going to wait until we merge #124 to fix all the conflicts

@carlosmariorr

# References and relevant issues Meeting 12 hours ago with @carlosmariorr we built this with an LLM together. We noticed some bad patterns which is now where #125 was born from. # Description This adds callbacks to layer events so that there is now bidirectional communication between layers and widget values. I'm not sure how important it is to review this, because #125 bludgeons much of the logic. @carlosmariorr what do you think? We discussed this and observed its working performance this morning, and the event connections continue in #125.

carlosmariorr · 2026-03-18T01:38:50Z

+        if self._selected_layer is None:
+            return


I guess I need to leave this comment up.

Is this necessary?

Just context for the archive:
Layer cannot be None anymore in the AxisComponentBase but the inheriting classes (e.g. AxisLabels in _axis.py) have this check in their _on_value_changed() method which I believe is not necessary if the Layer should not be set to None.

I think that the way to handle this is to make it so that in the even of layer changed to None, the widgets are destroyed.

I'll take a look at #127 to check what this is doing and then come back to this.

No more of these should exist now.

…ng LayerName edit

TimMonko · 2026-03-19T20:11:16Z

+        scales = layer.scale
        for value in scales:
            sb = QDoubleSpinBox(parent=self._parent_widget)
            sb.setDecimals(3)
            sb.setSingleStep(0.1)
-            sb.setRange(0.001, 1_000_000)
+            sb.setRange(self._SCALE_MINIMUM, 1_000_000)


we remove the old method which clamped the axis values, and now just use the widget behavior.
This still works fine with tests. We can increase to 4 decimals if needed

TimMonko · 2026-03-19T20:12:23Z

@@ -106,11 +95,70 @@ def component_label(self) -> QLabel:
        return self._component_qlabel

    @abstractmethod
-    def load_entries(self, layer: Layer | None = None) -> None:
-        """Load or refresh widget state for *layer* (defaults to active)."""
+    def load_entries(self, layer: Layer) -> None:
+        """Load or refresh widget state for *layer*."""


Two noticeable changes here that effect everything since this is our base ABC

we don't take viewer anymore

there is no optional layer for load_entries

TimMonko · 2026-03-19T20:12:55Z

+
+    def __init__(self, *args, **kwargs) -> None:
+        super().__init__(*args, **kwargs)
+        self._selected_layer: Layer | None = None


this is where we init layers, if needed, with None, before they get loaded

TimMonko · 2026-03-19T20:13:57Z

+    def _require_selected_layer(self) -> Layer:
+        layer = self._selected_layer
+        if layer is None:
+            raise RuntimeError(
+                f'{type(self).__name__} is not bound to a layer.'
+            )
+        return layer


this is our technical Qt safety for widget references during teardown, so we can call layer = self._required_selected_layer() to ensure that a Layer is returned, and no longer None, this centralizes all the condition if self._selected_layer is None: checks

TimMonko · 2026-03-19T20:14:35Z

+        return layer
+
+
+class BoundLayerCoordinator(BoundLayerOwner, ABC):


This ABC is focused on safely referencing layers as they are selected, because I noticed shared logic in so many widgets

TimMonko · 2026-03-19T20:17:59Z

        """Return the display string for *layer*."""
+
+
+class BoundFileComponentBase(BoundLayerOwner, FileComponentBase):


this class was created for the LayerName widget, which requires safely naming layers and is more than just a callback to a layer property

TimMonko · 2026-03-19T20:18:39Z

-        if self._selected_layer is not None:
-            for component in self._components:
-                component.load_entries(self._selected_layer)
+        layer = self._require_selected_layer()


here is where our None checks live like now

TimMonko · 2026-03-19T20:19:03Z

+        # Wire layer list events directly
+        self._layers.events.inserted.connect(self._update_layers_combobox)
+        self._layers.events.removed.connect(self._update_layers_combobox)
+        self._layers.events.changed.connect(self._update_layers_combobox)


layerlist event wiring, rather than using the separate things in layer_utils

TimMonko · 2026-03-19T20:19:49Z

+            self._general_metadata_instance.unbind_layer()
+            self._axis_metadata_instance.unbind_layer()


again, we see here how things like unbinding takes care of both disconnecting events and other parts of the widget deconstruction/reconstruction

TimMonko · 2026-03-19T20:20:49Z

this file removes all layer objects that already exist, and only is used to query things, like datatype, that are not a guaranteed property. Now we just use layer callbacks everywhere

TimMonko · 2026-03-20T02:08:03Z

MErging after long talk with @jni in community meeting. Will work on codesmells later, and hopefully left enough breadcrumbs here

TimMonko added 15 commits March 14, 2026 14:09

LLM layer events in _axis.py

49a26fd

LLM layer events in _file.py

2bbf858

remove name callback from main and hook to new events

ad7286b

add tests for new callback events

fd4b63a

keep only special logic in layer_utils, no optional layer

8db935a

remove ViewerModel from ABCs

4442881

LayerList for InheritanceWidget

e4613ce

Layer for _axis

28748d6

Layer for _file

ab165b0

_main uses ViewerModel but passes Layer or LL

a8f3868

update test_layer_utils

a7ac577

udate test_base

cda1503

update test_axis

716d105

update test_file

cb4ea09

update test_inheritance

fe6e0ac

TimMonko mentioned this pull request Mar 15, 2026

Layer event callbacks for widget updates #124

Merged

TimMonko mentioned this pull request Mar 17, 2026

Fix sizing and scrolling of main widget and containers #123

Merged

TimMonko added 5 commits March 17, 2026 12:44

CAREFULLY! Merge branch 'main' into enh/reduce-complexity

c1bbdc8

remove dead code and tests

44e7398

add missing axis test coverage

f889021

add coverage for testing inheritance behavior

c8ecfe5

add missing coverage for axis/file/main widget modules

8ebbf58

carlosmariorr reviewed Mar 18, 2026

View reviewed changes

TimMonko added 4 commits March 18, 2026 22:18

Merge branch 'main' into enh/reduce-complexity

8669650

drop set_axis_scales

8cae7a7

fix type of scale

77ead0e

reset dispaly in _file when no layer

7941578

TimMonko added 15 commits March 18, 2026 23:20

rename to event connected layer for inheritance

7cf24bb

drop unnecessary self._viewer assignment in MetadataWidget

6ebf93e

shore up _axis tests

ff75faf

properly block Qt events during teardown of AxisComponents

10cfc9d

almost remove all if layer is None checks

35f6631

remove remaining Layer none references

207b589

remove hopefully the last subwidget None checks

f926743

LLM review found potential for need of block during change layer duri…

bc78dd1

…ng LayerName edit

base classes for layer binding

46c875e

use binding and unbinding for other widgets

2402062

fix bug in main window return, no parent

04d0dc6

simplify base types

f0960b3

fix types in _axis.py

45df1a0

protect _inheritance widget with early returns

0b921e4

cleaner base class inits

f82d72f

TimMonko commented Mar 19, 2026

View reviewed changes

TimMonko merged commit 8adbed8 into napari:main Mar 20, 2026
19 checks passed

TimMonko deleted the enh/reduce-complexity branch March 20, 2026 02:08

		return layer


		class BoundLayerCoordinator(BoundLayerOwner, ABC):

		"""Return the display string for layer."""


		class BoundFileComponentBase(BoundLayerOwner, FileComponentBase):

		self._general_metadata_instance.unbind_layer()
		self._axis_metadata_instance.unbind_layer()

Conversation

TimMonko commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References and relevant issues

Description

Uh oh!

codecov Bot commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TimMonko commented Mar 16, 2026

Uh oh!

carlosmariorr Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimMonko commented Mar 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TimMonko commented Mar 15, 2026 •

edited

Loading

codecov Bot commented Mar 15, 2026 •

edited

Loading

carlosmariorr Mar 18, 2026 •

edited

Loading