Merge pull request #2066 from samschott/ownership-cleanup-samschott

Widget and window reference cleanup

.github/workflows/ci.yml

@@ -12,7 +12,7 @@ on:
 
 env:
   min_python_version: "3.8"
-  max_python_version: "3.11"
+  max_python_version: "3.12"
 
 defaults:
   run:
@@ -65,12 +65,12 @@ jobs:
     strategy:
       matrix:
         platform: [ "macos", "ubuntu", "windows" ]
-        python-version: [ "3.8", "3.9", "3.10", "3.11", "3.12-dev" ]
+        python-version: [ "3.8", "3.9", "3.10", "3.11", "3.12" ]
         include:
           - experimental: false
 
-          - python-version: "3.12-dev"
-            experimental: true
+          # - python-version: "3.13-dev"
+          #   experimental: true
     steps:
     - uses: actions/[email protected]
     - name: Set up Python ${{ matrix.python-version }}

changes/1215.bugfix.rst

@@ -0,0 +1 @@
+Widgets are now removed from windows when the window is closed, preventing a memory leak on window closure.

core/src/toga/widgets/base.py

@@ -2,6 +2,7 @@
 
 from builtins import id as identifier
 from typing import TYPE_CHECKING, Iterator, NoReturn
+from weakref import WeakValueDictionary
 
 from travertino.node import Node
 
@@ -13,10 +14,12 @@
     from toga.window import Window
 
 
-class WidgetRegistry(dict):
-    # WidgetRegistry is implemented as a subclass of dict, because it provides
-    # a mapping from ID to widget. However, it exposes a set-like API; add()
-    # and update() take instances to be added, and iteration is over values.
+class WidgetRegistry(WeakValueDictionary):
+    # WidgetRegistry is implemented as a subclass of WeakValueDictionary, because it
+    # provides a mapping from ID to widget. However, it exposes a set-like API; add()
+    # and update() take instances to be added, and iteration is over values. The
+    # mapping is weak so the registry doesn't retain a strong reference to the widget,
+    # preventing memory cleanup.
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
@@ -43,6 +46,9 @@ def remove(self, id: str) -> None:
     def __iter__(self) -> Iterator[Widget]:
         return iter(self.values())
 
+    def __repr__(self) -> str:
+        return "{" + ", ".join(f"{k!r}: {v!r}" for k, v in self.items()) + "}"
+
 
 class Widget(Node):
     _MIN_WIDTH = 100

core/src/toga/window.py

@@ -116,6 +116,7 @@ def __init__(
         self._impl = None
         self._content = None
         self._is_full_screen = False
+        self._closed = False
 
         self._resizable = resizable
         self._closable = closable
@@ -155,6 +156,11 @@ def app(self, app: App) -> None:
         self._app = app
         self._impl.set_app(app._impl)
 
+    @property
+    def closed(self) -> bool:
+        """Whether the window was closed."""
+        return self._closed
+
     @property
     def _default_title(self) -> str:
         return "Toga"
@@ -298,9 +304,15 @@ def close(self) -> None:
 
         This *does not* invoke the ``on_close`` handler; the window will be immediately
         and unconditionally closed.
+
+        Once a window has been closed, it *cannot* be reused. The behavior of any method
+        or property on a :class:`~toga.Window` instance after it has been closed is
+        undefined, except for :attr:`closed` which can be used to check if the window
+        was closed.
         """
         self.app.windows -= self
         self._impl.close()
+        self._closed = True
 
     ############################################################
     # Dialogs

core/tests/test_window.py

@@ -253,6 +253,7 @@ def test_close_direct(window, app):
     window.close()
 
     # Window has been closed, but the close handler has *not* been invoked.
+    assert window.closed
     assert window.app == app
     assert window not in app.windows
     assert_action_performed(window, "close")
@@ -269,6 +270,7 @@ def test_close_no_handler(window, app):
     window._impl.simulate_close()
 
     # Window has been closed, and is no longer in the app's list of windows.
+    assert window.closed
     assert window.app == app
     assert window not in app.windows
     assert_action_performed(window, "close")
@@ -287,6 +289,7 @@ def test_close_sucessful_handler(window, app):
     window._impl.simulate_close()
 
     # Window has been closed, and is no longer in the app's list of windows.
+    assert window.closed
     assert window.app == app
     assert window not in app.windows
     assert_action_performed(window, "close")
@@ -306,6 +309,7 @@ def test_close_rejected_handler(window, app):
     window._impl.simulate_close()
 
     # Window has *not* been closed
+    assert not window.closed
     assert window.app == app
     assert window in app.windows
     assert_action_not_performed(window, "close")

core/tests/widgets/test_base.py

@@ -123,6 +123,10 @@ def test_add_child(widget):
     assert widget.app == app
     assert widget.window == window
 
+    # Widget is registered with app and window
+    assert widget.id in app.widgets
+    assert widget.id in window.widgets
+
     # Child list is empty
     assert widget.children == []
 
@@ -181,10 +185,7 @@ def test_add_multiple_children(widget):
     child3 = ExampleLeafWidget(id="child3_id")
 
     # App's widget index only contains the parent
-    assert app.widgets["widget_id"] == widget
-    assert "child1_id" not in app.widgets
-    assert "child2_id" not in app.widgets
-    assert "child3_id" not in app.widgets
+    assert app.widgets == {"widget_id": widget}
 
     # Add the children.
     widget.add(child1, child2, child3)
@@ -218,11 +219,20 @@ def test_add_multiple_children(widget):
     assert_action_performed_with(window.content, "refresh")
 
     # App's widget index has been updated
-    assert len(app.widgets) == 4
-    assert app.widgets["widget_id"] == widget
-    assert app.widgets["child1_id"] == child1
-    assert app.widgets["child2_id"] == child2
-    assert app.widgets["child3_id"] == child3
+    assert app.widgets == {
+        "widget_id": widget,
+        "child1_id": child1,
+        "child2_id": child2,
+        "child3_id": child3,
+    }
+
+    # Window's widget index has been updated
+    assert window.widgets == {
+        "widget_id": widget,
+        "child1_id": child1,
+        "child2_id": child2,
+        "child3_id": child3,
+    }
 
 
 def test_reparent_child(widget):
@@ -375,9 +385,16 @@ def test_insert_child(widget):
     assert_action_performed_with(window.content, "refresh")
 
     # App's widget index has been updated
-    assert len(app.widgets) == 2
-    assert app.widgets["widget_id"] == widget
-    assert app.widgets["child_id"] == child
+    assert app.widgets == {
+        "widget_id": widget,
+        "child_id": child,
+    }
+
+    # Window's widget index has been updated
+    assert window.widgets == {
+        "widget_id": widget,
+        "child_id": child,
+    }
 
 
 def test_insert_position(widget):
@@ -402,10 +419,10 @@ def test_insert_position(widget):
     child3 = ExampleLeafWidget(id="child3_id")
 
     # App's widget index only contains the parent
-    assert app.widgets["widget_id"] == widget
-    assert "child1_id" not in app.widgets
-    assert "child2_id" not in app.widgets
-    assert "child3_id" not in app.widgets
+    assert app.widgets == {"widget_id": widget}
+
+    # Windows's widget index only contains the parent
+    assert window.widgets == {"widget_id": widget}
 
     # insert the children.
     widget.insert(0, child1)
@@ -440,11 +457,20 @@ def test_insert_position(widget):
     assert_action_performed_with(window.content, "refresh")
 
     # App's widget index has been updated
-    assert len(app.widgets) == 4
-    assert app.widgets["widget_id"] == widget
-    assert app.widgets["child1_id"] == child1
-    assert app.widgets["child2_id"] == child2
-    assert app.widgets["child3_id"] == child3
+    assert app.widgets == {
+        "widget_id": widget,
+        "child1_id": child1,
+        "child2_id": child2,
+        "child3_id": child3,
+    }
+
+    # Window's widget index has been updated
+    assert window.widgets == {
+        "widget_id": widget,
+        "child1_id": child1,
+        "child2_id": child2,
+        "child3_id": child3,
+    }
 
 
 def test_insert_bad_position(widget):
@@ -467,10 +493,12 @@ def test_insert_bad_position(widget):
     child = ExampleLeafWidget(id="child_id")
 
     # App's widget index only contains the parent
-    assert app.widgets["widget_id"] == widget
-    assert "child_id" not in app.widgets
+    assert app.widgets == {"widget_id": widget}
+
+    # Window's widget index only contains the parent
+    assert window.widgets == {"widget_id": widget}
 
-    # Insert the child at an position greater than the length of the list.
+    # Insert the child at a position greater than the length of the list.
     # Widget will be added to the end of the list.
     widget.insert(37, child)
 
@@ -492,9 +520,16 @@ def test_insert_bad_position(widget):
     assert_action_performed_with(window.content, "refresh")
 
     # App's widget index has been updated
-    assert len(app.widgets) == 2
-    assert app.widgets["widget_id"] == widget
-    assert app.widgets["child_id"] == child
+    assert app.widgets == {
+        "widget_id": widget,
+        "child_id": child,
+    }
+
+    # Window's widget index has been updated
+    assert window.widgets == {
+        "widget_id": widget,
+        "child_id": child,
+    }
 
 
 def test_insert_reparent_child(widget):
@@ -618,6 +653,8 @@ def test_remove_child(widget):
     assert child.parent == widget
     assert child.app == app
     assert child.window == window
+    assert app.widgets == {"widget_id": widget, "child_id": child}
+    assert window.widgets == {"widget_id": widget, "child_id": child}
 
     # Remove the child
     widget.remove(child)
@@ -630,6 +667,10 @@ def test_remove_child(widget):
     assert child.app is None
     assert child.window is None
 
+    # child widget no longer exists in the app or widgets registries.
+    assert app.widgets == {"widget_id": widget}
+    assert window.widgets == {"widget_id": widget}
+
     # The impl's remove_child has been invoked
     assert_action_performed_with(widget, "remove child", child=child._impl)
 
@@ -639,6 +680,9 @@ def test_remove_child(widget):
     # The window's content gets a refresh notification
     assert_action_performed_with(window.content, "refresh")
 
+    # App's widget index does not contain the widget
+    assert "child_id" not in app.widgets
+
 
 def test_remove_multiple_children(widget):
     "Multiple children can be removed from a widget"
@@ -659,6 +703,8 @@ def test_remove_multiple_children(widget):
         assert child.parent == widget
         assert child.app == app
         assert child.window == window
+        assert app.widgets[child.id] == child
+        assert window.widgets[child.id] == child
 
     # Remove 2 children
     widget.remove(child1, child3)
@@ -689,6 +735,14 @@ def test_remove_multiple_children(widget):
     # The window's content gets a refresh notification
     assert_action_performed_with(window.content, "refresh")
 
+    # App's widget index does not contain the widget
+    assert "child1_id" not in app.widgets
+    assert "child3_id" not in app.widgets
+
+    # Windows's widget index does not contain the widget
+    assert "child1_id" not in window.widgets
+    assert "child3_id" not in window.widgets
+
 
 def test_clear_all_children(widget):
     "All children can be simultaneously removed from a widget"
@@ -709,6 +763,8 @@ def test_clear_all_children(widget):
         assert child.parent == widget
         assert child.app == app
         assert child.window == window
+        assert app.widgets[child.id] == child
+        assert window.widgets[child.id] == child
 
     # Clear children
     widget.clear()
@@ -740,6 +796,16 @@ def test_clear_all_children(widget):
     # The window's content gets a refresh notification
     assert_action_performed_with(window.content, "refresh")
 
+    # App's widget index does not contain the widget
+    assert "child1_id" not in app.widgets
+    assert "child2_id" not in app.widgets
+    assert "child3_id" not in app.widgets
+
+    # Window's widget index does not contain the widget
+    assert "child1_id" not in window.widgets
+    assert "child2_id" not in window.widgets
+    assert "child3_id" not in window.widgets
+
 
 def test_clear_no_children(widget):
     "No changes are made (no-op) if widget has no children"
@@ -830,7 +896,7 @@ def test_set_app(widget):
     assert len(app.widgets) == 1
     assert app.widgets["widget_id"] == widget
 
-    # The impl has had it's app property set.
+    # The impl has had its app property set.
     assert attribute_value(widget, "app") == app
 
 

docs/reference/api/window.rst

@@ -46,6 +46,10 @@ If the user attempts to close the window, Toga will call the ``on_close`` handle
 handler must return a ``bool`` confirming whether the close is permitted. This can be
 used to implement protections against closing a window with unsaved changes.
 
+Once a window has been closed (either by user action, or programmatically with
+:meth:`~toga.Window.close()`), it *cannot* be reused. The behavior of any method on a
+:class:`~toga.Window` instance after it has been closed is undefined.
+
 Notes
 -----