Fix recursion error when connecting to container self.method during subclass init (#539)

* fix: fix getattr during init

* test: add test

* style(pre-commit.ci): auto fixes [...]

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: tlambert03

src/magicgui/widgets/bases/_container_widget.py

@@ -81,6 +81,7 @@ class ContainerWidget(Widget, _OrientationMixin, MutableSequence[WidgetVar]):
     )
     _widget: protocols.ContainerProtocol
     _initialized = False
+    _list: list[WidgetVar] = []
 
     def __init__(
         self,
@@ -107,15 +108,15 @@ def __init__(
 
     def __getattr__(self, name: str) -> WidgetVar:
         """Return attribute ``name``.  Will return a widget if present."""
-        for widget in self:
+        for widget in self._list:
             if name == widget.name:
                 return widget
         return object.__getattribute__(self, name)  # type: ignore
 
     def __setattr__(self, name: str, value: Any) -> None:
         """Set attribute ``name``.  Prevents changing widget if present, (use del)."""
         if self._initialized:
-            for widget in self:
+            for widget in self._list:
                 if name == widget.name:
                     raise AttributeError(
                         "Cannot set attribute with same name as a widget\n"
@@ -188,7 +189,7 @@ def __setitem__(self, key: Any, value: Any) -> NoReturn:
     def __dir__(self) -> list[str]:
         """Add subwidget names to the dir() call for this widget."""
         d = list(super().__dir__())
-        d.extend([w.name for w in self if not w.gui_only])
+        d.extend([w.name for w in self._list if not w.gui_only])
         return d
 
     def insert(self, key: int, widget: WidgetVar) -> None:
@@ -217,7 +218,7 @@ def _unify_label_widths(self) -> None:
         if not self._initialized:
             return
 
-        need_labels = [w for w in self if not isinstance(w, ButtonWidget)]
+        need_labels = [w for w in self._list if not isinstance(w, ButtonWidget)]
         if self.layout == "vertical" and self.labels and need_labels:
             measure = use_app().get_obj("get_text_width")
             widest_label = max(measure(w.label) for w in need_labels)
@@ -254,15 +255,17 @@ def reset_choices(self, *_: Any) -> None:
         choices as when the widget was instantiated, if the callable relies on external
         state.
         """
-        for widget in self:
+        for widget in self._list:
             if hasattr(widget, "reset_choices"):
                 widget.reset_choices()
 
     @property
     def __signature__(self) -> MagicSignature:
         """Return a MagicSignature object representing the current state of the gui."""
         params = [
-            MagicParameter.from_widget(w) for w in self if w.name and not w.gui_only
+            MagicParameter.from_widget(w)
+            for w in self._list
+            if w.name and not w.gui_only
         ]
         # if we have multiple non-default parameters and some but not all of them are
         # "bound" to fallback values, we may have  non-default arguments
@@ -315,7 +318,9 @@ def labels(self, value: bool) -> None:
     def asdict(self) -> dict[str, Any]:
         """Return state of widget as dict."""
         return {
-            w.name: getattr(w, "value", None) for w in self if w.name and not w.gui_only
+            w.name: getattr(w, "value", None)
+            for w in self._list
+            if w.name and not w.gui_only
         }
 
     def update(
@@ -342,7 +347,7 @@ def _dump(self, path: str | Path) -> None:
         path = Path(path)
         path.parent.mkdir(parents=True, exist_ok=True)
         _dict = {}
-        for widget in self:
+        for widget in self._list:
             try:
                 # not all values will be pickleable and restorable...
                 # for now, don't even try

tests/test_container.py

@@ -0,0 +1,205 @@
+import pytest
+
+from magicgui import magicgui, use_app, widgets
+
+
+@pytest.mark.parametrize("scrollable", [False, True])
+def test_container_widget(scrollable):
+    """Test basic container functionality."""
+    container = widgets.Container(labels=False, scrollable=scrollable)
+    labela = widgets.Label(value="hi", name="labela")
+    labelb = widgets.Label(value="hi", name="labelb")
+    container.append(labela)
+    container.extend([labelb])
+    # different ways to index
+    assert container[0] == labela
+    assert container["labelb"] == labelb
+    assert container[:1] == [labela]
+    assert container[-1] == labelb
+
+    with pytest.raises(NotImplementedError):
+        container[0] = "something"
+
+    assert container.layout == "vertical"
+    with pytest.raises(NotImplementedError):
+        container.layout = "horizontal"
+
+    assert all(x in dir(container) for x in ["labela", "labelb"])
+
+    assert container.margins
+    container.margins = (8, 8, 8, 8)
+    assert container.margins == (8, 8, 8, 8)
+
+    del container[1:]
+    del container[-1]
+    assert not container
+    container.close()
+
+
+@pytest.mark.parametrize("scrollable", [False, True])
+def test_container_label_widths(scrollable):
+    """Test basic container functionality."""
+    container = widgets.Container(layout="vertical", scrollable=scrollable)
+    labela = widgets.Label(value="hi", name="labela")
+    labelb = widgets.Label(value="hi", name="I have a very long label")
+
+    def _label_width():
+        measure = use_app().get_obj("get_text_width")
+        return max(
+            measure(w.label)
+            for w in container
+            if not isinstance(w, widgets.bases.ButtonWidget)
+        )
+
+    container.append(labela)
+    before = _label_width()
+    container.append(labelb)
+    assert _label_width() > before
+    container.close()
+
+
+@pytest.mark.parametrize("scrollable", [False, True])
+def test_labeled_widget_container(scrollable):
+    """Test that _LabeledWidgets follow their children."""
+    from magicgui.widgets._concrete import _LabeledWidget
+
+    w1 = widgets.Label(value="hi", name="w1")
+    w2 = widgets.Label(value="hi", name="w2")
+    container = widgets.Container(
+        widgets=[w1, w2], layout="vertical", scrollable=scrollable
+    )
+    assert w1._labeled_widget
+    lw = w1._labeled_widget()
+    assert isinstance(lw, _LabeledWidget)
+    assert not lw.visible
+    container.show()
+    assert w1.visible
+    assert lw.visible
+    w1.hide()
+    assert not w1.visible
+    assert not lw.visible
+    w1.label = "another label"
+    assert lw._label_widget.value == "another label"
+    container.close()
+
+
+@pytest.mark.parametrize("scrollable", [False, True])
+def test_visible_in_container(scrollable):
+    """Test that visibility depends on containers."""
+    w1 = widgets.Label(value="hi", name="w1")
+    w2 = widgets.Label(value="hi", name="w2")
+    w3 = widgets.Label(value="hi", name="w3", visible=False)
+    container = widgets.Container(widgets=[w2, w3], scrollable=scrollable)
+    assert not w1.visible
+    assert not w2.visible
+    assert not w3.visible
+    assert not container.visible
+    container.show()
+    assert container.visible
+    assert w2.visible
+    assert not w3.visible
+    w1.show()
+    assert w1.visible
+    container.close()
+
+
+def test_delete_widget():
+    """We can delete widgets from containers."""
+    a = widgets.Label(name="a")
+    container = widgets.Container(widgets=[a])
+    # we can delete widgets
+    del container.a
+    with pytest.raises(AttributeError):
+        container.a
+
+    # they disappear from the layout
+    with pytest.raises(ValueError):
+        container.index(a)
+
+
+def test_reset_choice_recursion():
+    """Test that reset_choices recursion works for multiple types of widgets."""
+    x = 0
+
+    def get_choices(widget):
+        nonlocal x
+        x += 1
+        return list(range(x))
+
+    @magicgui(c={"choices": get_choices})
+    def f(c):
+        pass
+
+    assert f.c.choices == (0,)
+
+    container = widgets.Container(widgets=[f])
+    container.reset_choices()
+    assert f.c.choices == (0, 1)
+    container.reset_choices()
+    assert f.c.choices == (0, 1, 2)
+
+
+def test_container_indexing_with_native_mucking():
+    """Mostly make sure that the inner model isn't messed up.
+
+    keeping indexes with a manipulated native model *may* be something to do in future.
+    """
+    l1 = widgets.Label(name="l1")
+    l2 = widgets.Label(name="l2")
+    l3 = widgets.Label(name="l3")
+    c = widgets.Container(widgets=[l1, l2, l3])
+    assert c[-1] == l3
+    # so far they should be in sync
+    native = c.native.layout()
+    assert native.count() == len(c)
+    # much with native layout
+    native.addStretch()
+    # haven't changed the magicgui container
+    assert len(c) == 3
+    assert c[-1] == l3
+    # though it has changed the native model
+    assert native.count() == 4
+
+
+def test_containers_show_nested_containers():
+    """make sure showing a container shows a nested FunctionGui."""
+
+    @magicgui
+    def func(x: int, y: str):
+        pass
+
+    assert not func.visible
+    c2 = widgets.Container(widgets=[func])
+    assert not c2.visible
+    c2.show()
+    assert c2.visible and func.visible
+    c2.close()
+    assert not func.visible
+
+
+def test_container_removal():
+    c = widgets.Container()
+    s = widgets.Slider(label="label")
+    assert len(c) == 0
+    assert c.native.layout().count() == 0
+
+    c.append(s)
+    assert len(c) == 1
+    assert c.native.layout().count() == 1
+
+    c.pop()
+    assert len(c) == 0
+    assert c.native.layout().count() == 0
+
+
+def test_connection_during_init() -> None:
+    class C(widgets.Container):
+        def __init__(self) -> None:
+            btn = widgets.PushButton()
+            btn.changed.connect(self._on_clicked)
+            super().__init__(widgets=[btn])
+
+        def _on_clicked(self):
+            ...
+
+    assert isinstance(C(), widgets.Container)