User internal model for Container, simplify ContainerWidgetProtocol

magicgui/backends/_qtpy/widgets.py

@@ -241,9 +241,6 @@ def __init__(self):
 #         super().__init__(QtW.QToolButton)
 
 
-# CATEGORICAL
-
-
 class Container(
     QBaseWidget, _protocols.ContainerProtocol, _protocols.SupportsOrientation
 ):
@@ -255,49 +252,19 @@ def __init__(self, layout="vertical"):
             self._layout = QtW.QVBoxLayout()
         self._qwidget.setLayout(self._layout)
 
-    def _mgui_get_margins(self) -> Tuple[int, int, int, int]:
-        m = self._layout.contentsMargins()
-        return m.left(), m.top(), m.right(), m.bottom()
-
-    def _mgui_set_margins(self, margins: Tuple[int, int, int, int]) -> None:
-        self._layout.setContentsMargins(*margins)
-
-    def _mgui_add_widget(self, widget: Widget):
-        _widget = widget.native
-        self._layout.addWidget(_widget)
-
     def _mgui_insert_widget(self, position: int, widget: Widget):
-        _widget = widget.native
-        if position < 0:
-            position = self._mgui_count() + position + 1
-
-        self._layout.insertWidget(position, _widget)
+        self._layout.insertWidget(position, widget.native)
 
     def _mgui_remove_widget(self, widget: Widget):
         self._layout.removeWidget(widget.native)
         widget.native.setParent(None)
 
-    def _mgui_remove_index(self, position: int):
-        # TODO: normalize position in superclass
-        if position < 0:
-            position = self._mgui_count() + position + 1
-        item = self._layout.takeAt(position)
-        item.widget().setParent(None)
-
-    def _mgui_count(self) -> int:
-        return self._layout.count()
-
-    def _mgui_index(self, widget: Widget) -> int:
-        return self._layout.indexOf(widget.native)
+    def _mgui_get_margins(self) -> Tuple[int, int, int, int]:
+        m = self._layout.contentsMargins()
+        return m.left(), m.top(), m.right(), m.bottom()
 
-    def _mgui_get_index(self, index: int) -> Optional[Widget]:
-        # We need to return a magicgui.Widget object, so this currently
-        # requires storing the original object as a hidden attribute.
-        # better way?
-        item = self._layout.itemAt(index)
-        if item:
-            return item.widget()._magic_widget
-        return None
+    def _mgui_set_margins(self, margins: Tuple[int, int, int, int]) -> None:
+        self._layout.setContentsMargins(*margins)
 
     def _mgui_set_orientation(self, value) -> None:
         """Set orientation, value will be 'horizontal' or 'vertical'."""
@@ -313,9 +280,6 @@ def _mgui_get_orientation(self) -> str:
         else:
             return "vertical"
 
-    def _mgui_get_native_layout(self) -> QtW.QLayout:
-        return self._layout
-
 
 class SpinBox(QBaseRangedWidget):
     def __init__(self):
@@ -565,7 +529,7 @@ def _mgui_get_cell(self, row: int, col: int) -> Any:
             return item.data(self._DATA_ROLE)
         widget = self._qwidget.cellWidget(row, col)
         if widget:
-            return widget._magic_widget
+            return getattr(widget, "_magic_widget", widget)
 
     def _mgui_set_cell(self, row: int, col: int, value: Any) -> None:
         """Set current value of the widget."""

magicgui/widgets/_bases/container_widget.py

@@ -81,6 +81,7 @@ def __init__(
         return_annotation: Any = None,
         **kwargs,
     ):
+        self._list: List[Widget] = []
         self._return_annotation = None
         self._labels = labels
         self._layout = layout
@@ -141,19 +142,11 @@ def __getitem__(self, key):  # noqa: F811
         if isinstance(key, str):
             return self.__getattr__(key)
         if isinstance(key, slice):
-            out = []
-            for idx in range(*key.indices(len(self))):
-                item = self._widget._mgui_get_index(idx)
-                if item:
-                    out.append(item)
-            return out
+            return [getattr(item, "_inner_widget", item) for item in self._list[key]]
         elif isinstance(key, int):
-            if key < 0:
-                key += len(self)
-        item = self._widget._mgui_get_index(key)
-        if item is None:
-            raise IndexError("Container index out of range")
-        return getattr(item, "_inner_widget", item)
+            item = self._list[key]
+            return getattr(item, "_inner_widget", item)
+        raise TypeError(f"list indices must be integers or slices, not {type(key)}")
 
     def index(self, value: Any, start=0, stop=9223372036854775807) -> int:
         """Return index of a specific widget instance (or widget name)."""
@@ -172,16 +165,17 @@ def __delattr__(self, name: str):
     def __delitem__(self, key: Union[int, slice]):
         """Delete a widget by integer or slice index."""
         if isinstance(key, slice):
-            for idx in range(*key.indices(len(self))):
-                self._widget._mgui_remove_index(idx)
+            for item in self._list[key]:
+                self._widget._mgui_remove_widget(item)
+        elif isinstance(key, int):
+            self._widget._mgui_remove_widget(self._list[key])
         else:
-            if key < 0:
-                key += len(self)
-            self._widget._mgui_remove_index(key)
+            raise TypeError(f"list indices must be integers or slices, not {type(key)}")
+        del self._list[key]
 
     def __len__(self) -> int:
         """Return the count of widgets."""
-        return self._widget._mgui_count()
+        return len(self._list)
 
     def __setitem__(self, key, value):
         """Prevent assignment by index."""
@@ -207,6 +201,11 @@ def insert(self, key: int, widget: Widget):
                 _widget = _LabeledWidget(widget)
                 widget.label_changed.connect(self._unify_label_widths)
 
+        self._list.insert(key, widget)
+        if key < 0:
+            key += len(self)
+        # NOTE: if someone has manually mucked around with self.native.layout()
+        # it's possible that indices will be off.
         self._widget._mgui_insert_widget(key, _widget)
         self._unify_label_widths()
 
@@ -218,8 +217,7 @@ def _unify_label_widths(self, event=None):
             widest_label = max(
                 measure(w.label) for w in self if not isinstance(w, ButtonWidget)
             )
-            for i in range(len(self)):
-                w = self._widget._mgui_get_index(i)
+            for w in self:
                 if hasattr(w, "label_width"):
                     w.label_width = widest_label  # type: ignore
 
@@ -244,11 +242,6 @@ def layout(self, value):
             "It is not yet possible to change layout after instantiation"
         )
 
-    @property
-    def native_layout(self):
-        """Return the layout widget used by the backend."""
-        return self._widget._mgui_get_native_layout()
-
     def reset_choices(self, event=None):
         """Reset choices for all Categorical subWidgets to the default state.
 
@@ -288,14 +281,14 @@ def __signature__(self) -> MagicSignature:
         return MagicSignature(params, return_annotation=self.return_annotation)
 
     @classmethod
-    def from_signature(cls, sig: inspect.Signature, **kwargs) -> "Container":
+    def from_signature(cls, sig: inspect.Signature, **kwargs) -> Container:
         """Create a Container widget from an inspect.Signature object."""
         return MagicSignature.from_signature(sig).to_container(**kwargs)
 
     @classmethod
     def from_callable(
         cls, obj: Callable, gui_options: Optional[dict] = None, **kwargs
-    ) -> "Container":
+    ) -> Container:
         """Create a Container widget from a callable object.
 
         In most cases, it will be preferable to create a ``FunctionGui`` instead.

magicgui/widgets/_protocols.py

@@ -359,10 +359,6 @@ class SliderWidgetProtocol(RangedWidgetProtocol, SupportsOrientation, Protocol):
 class ContainerProtocol(WidgetProtocol, SupportsOrientation, Protocol):
     """Widget that can contain other widgets."""
 
-    @abstractmethod
-    def _mgui_add_widget(self, widget: "Widget") -> None:
-        raise NotImplementedError()
-
     @abstractmethod
     def _mgui_insert_widget(self, position: int, widget: "Widget") -> None:
         raise NotImplementedError()
@@ -371,27 +367,6 @@ def _mgui_insert_widget(self, position: int, widget: "Widget") -> None:
     def _mgui_remove_widget(self, widget: "Widget") -> None:
         raise NotImplementedError()
 
-    @abstractmethod
-    def _mgui_remove_index(self, position: int) -> None:
-        raise NotImplementedError()
-
-    @abstractmethod
-    def _mgui_count(self) -> int:
-        raise NotImplementedError()
-
-    @abstractmethod
-    def _mgui_index(self, widget: "Widget") -> int:
-        raise NotImplementedError()
-
-    @abstractmethod
-    def _mgui_get_index(self, index: int) -> Optional[Widget]:
-        """(return None instead of index error)."""
-        raise NotImplementedError()
-
-    @abstractmethod
-    def _mgui_get_native_layout(self) -> Any:
-        raise NotImplementedError()
-
     @abstractmethod
     def _mgui_get_margins(self) -> Tuple[int, int, int, int]:
         raise NotImplementedError()

tests/test_widgets.py

@@ -173,9 +173,6 @@ def test_container_widget():
     del container[-1]
     assert not container
 
-    if use_app().backend_name == "qt":
-        assert container.native_layout.__class__.__name__ == "QHBoxLayout"
-
 
 def test_container_label_widths():
     """Test basic container functionality."""
@@ -203,8 +200,9 @@ def test_labeled_widget_container():
 
     w1 = widgets.Label(value="hi", name="w1")
     w2 = widgets.Label(value="hi", name="w2")
-    container = widgets.Container(widgets=[w1, w2], layout="vertical")
-    lw = container._widget._mgui_get_index(0)
+    _ = widgets.Container(widgets=[w1, w2], layout="vertical")
+    assert w1._labeled_widget
+    lw = w1._labeled_widget()
     assert isinstance(lw, _LabeledWidget)
     assert lw.visible
     w1.hide()
@@ -360,3 +358,25 @@ def f(c):
     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