Merge pull request #2746 from pythonarcade/gui/fix-memory-leak-2

Gui/fix memory leak 2

Author: eruvanos

arcade/examples/gui/6_size_hints.py

@@ -135,7 +135,6 @@ def on_change(event: UIOnChangeEvent):
         content_anchor.add(UISpace(height=20))
 
         self.ui.execute_layout()
-        self.ui.debug()
 
 
 def main():

arcade/gui/experimental/focus.py

@@ -71,9 +71,9 @@ class UIFocusMixin(UIWidget):
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
 
-        bind(self, "_debug", self.trigger_full_render)
-        bind(self, "_focused_widget", self.trigger_full_render)
-        bind(self, "_focusable_widgets", self.trigger_full_render)
+        bind(self, "_debug", UIFocusMixin.trigger_full_render)
+        bind(self, "_focused_widget", UIFocusMixin.trigger_full_render)
+        bind(self, "_focusable_widgets", UIFocusMixin.trigger_full_render)
 
     def on_event(self, event: UIEvent) -> bool | None:
         # pass events to children first, including controller events

arcade/gui/experimental/scroll_area.py

@@ -44,11 +44,11 @@ def __init__(self, scroll_area: UIScrollArea, vertical: bool = True):
         self.with_border(color=arcade.uicolor.GRAY_CONCRETE)
         self.vertical = vertical
 
-        bind(self, "_thumb_hover", self.trigger_render)
-        bind(self, "_dragging", self.trigger_render)
-        bind(scroll_area, "scroll_x", self.trigger_full_render)
-        bind(scroll_area, "scroll_y", self.trigger_full_render)
-        bind(scroll_area, "rect", self.trigger_full_render)
+        bind(self, "_thumb_hover", UIScrollBar.trigger_render)
+        bind(self, "_dragging", UIScrollBar.trigger_render)
+        bind(scroll_area, "scroll_x", UIScrollBar.trigger_full_render)
+        bind(scroll_area, "scroll_y", UIScrollBar.trigger_full_render)
+        bind(scroll_area, "rect", UIScrollBar.trigger_full_render)
 
     def on_event(self, event: UIEvent) -> bool | None:
         # check if we are scrollable
@@ -234,8 +234,8 @@ def __init__(
             size=canvas_size,
         )
 
-        bind(self, "scroll_x", self.trigger_full_render)
-        bind(self, "scroll_y", self.trigger_full_render)
+        bind(self, "scroll_x", UIScrollArea.trigger_full_render)
+        bind(self, "scroll_y", UIScrollArea.trigger_full_render)
 
     def add(self, child: W, **kwargs) -> W:
         """Add a child to the widget."""

arcade/gui/property.py

@@ -3,6 +3,7 @@
 import traceback
 from collections.abc import Callable
 from contextlib import contextmanager, suppress
+from enum import Enum
 from typing import Any, Generic, TypeVar, cast
 from weakref import WeakKeyDictionary, ref
 
@@ -18,6 +19,41 @@
 AnyListener = NoArgListener | InstanceListener | InstanceValueListener | InstanceNewOldListener
 
 
+class _ListenerType(Enum):
+    """Enum to represent the type of listener"""
+
+    NO_ARG = 0
+    INSTANCE = 1
+    INSTANCE_VALUE = 2
+    INSTANCE_NEW_OLD = 3
+
+    @staticmethod
+    def detect_callback_type(callback: AnyListener) -> "_ListenerType":
+        """Normalizes the callback so every callback can be invoked with the same signature."""
+        signature = inspect.signature(callback)
+
+        # first detect the old *args default listener signatures
+        with suppress(TypeError):
+            signature.bind(..., ...)
+            return _ListenerType.INSTANCE_VALUE
+
+        # check for the most common signature
+        with suppress(TypeError):
+            signature.bind()
+            return _ListenerType.NO_ARG
+
+        # check for the other
+        with suppress(TypeError):
+            signature.bind(..., ..., ...)
+            return _ListenerType.INSTANCE_NEW_OLD
+
+        with suppress(TypeError):
+            signature.bind(...)
+            return _ListenerType.INSTANCE
+
+        raise TypeError("Callback is not callable")
+
+
 class _Obs(Generic[P]):
     """
     Internal holder for Property value and change listeners
@@ -29,46 +65,24 @@ def __init__(self, value: P):
         self.value = value
         # This will keep any added listener even if it is not referenced anymore
         # and would be garbage collected
-        self._listeners: dict[AnyListener, InstanceNewOldListener] = dict()
+        self._listeners: dict[AnyListener, _ListenerType] = dict()
 
     def add(
         self,
         callback: AnyListener,
     ):
         """Add a callback to the list of listeners"""
-        self._listeners[callback] = _Obs._normalize_callback(callback)
+        self._listeners[callback] = _ListenerType.detect_callback_type(callback)
 
     def remove(self, callback):
         """Remove a callback from the list of listeners"""
         if callback in self._listeners:
             del self._listeners[callback]
 
     @property
-    def listeners(self) -> list[InstanceNewOldListener]:
-        return list(self._listeners.values())
-
-    @staticmethod
-    def _normalize_callback(callback) -> InstanceNewOldListener:
-        """Normalizes the callback so every callback can be invoked with the same signature."""
-        signature = inspect.signature(callback)
-
-        with suppress(TypeError):
-            signature.bind(1, 1)
-            return lambda instance, new, old: callback(instance, new)
-
-        with suppress(TypeError):
-            signature.bind(1, 1, 1)
-            return lambda instance, new, old: callback(instance, new, old)
-
-        with suppress(TypeError):
-            signature.bind(1)
-            return lambda instance, new, old: callback(instance)
-
-        with suppress(TypeError):
-            signature.bind()
-            return lambda instance, new, old: callback()
-
-        raise TypeError("Callback is not callable")
+    def listeners(self) -> list[tuple[AnyListener, _ListenerType]]:
+        """Returns a list of all listeners and type, both weak and strong."""
+        return list(self._listeners.items())
 
 
 class Property(Generic[P]):
@@ -147,17 +161,24 @@ def dispatch(self, instance, value, old_value):
 
         """
         obs = self._get_obs(instance)
-        for listener in obs.listeners:
+        for listener, _listener_type in obs.listeners:
             try:
-                listener(instance, value, old_value)
+                if _listener_type == _ListenerType.NO_ARG:
+                    listener()  # type: ignore[call-arg]
+                elif _listener_type == _ListenerType.INSTANCE:
+                    listener(instance)  # type: ignore[call-arg]
+                elif _listener_type == _ListenerType.INSTANCE_VALUE:
+                    listener(instance, value)  # type: ignore[call-arg]
+                elif _listener_type == _ListenerType.INSTANCE_NEW_OLD:
+                    listener(instance, value, old_value)  # type: ignore[call-arg]
             except Exception:
                 print(
                     f"Change listener for {instance}.{self.name} = {value} raised an exception!",
                     file=sys.stderr,
                 )
                 traceback.print_exc()
 
-    def bind(self, instance, callback):
+    def bind(self, instance: Any, callback: AnyListener):
         """Binds a function to the change event of the property.
 
         A reference to the function will be kept.
@@ -200,7 +221,7 @@ def __set__(self, instance, value: P):
         self.set(instance, value)
 
 
-def bind(instance, property: str, callback):
+def bind(instance, property: str, callback: AnyListener):
     """Bind a function to the change event of the property.
 
     A reference to the function will be kept, so that it will be still
@@ -220,6 +241,11 @@ class MyObject:
         my_obj.name = "Hans"
         # > Value of <__main__.MyObject ...> changed to Hans
 
+    Binding to a method of the Property owner itself can cause a memory leak, because the
+    owner is strongly referenced. Instead, bind the class method, which will be invoked with
+    the instance as first parameter.
+
+
     Args:
         instance: Instance owning the property
         property: Name of the property
@@ -228,6 +254,7 @@ class MyObject:
     Returns:
         None
     """
+    # TODO rename property to property_name for arcade 4.0 (just to be sure)
     t = type(instance)
     prop = getattr(t, property)
     if not isinstance(prop, Property):

arcade/gui/ui_manager.py

@@ -529,12 +529,10 @@ def _set_active_widget(self, widget: UIWidget | None):
             return
 
         if self._active_widget:
-            print(f"Deactivating widget {self._active_widget.__class__.__name__}")
             self._active_widget._active = False
 
         self._active_widget = widget
         if self._active_widget:
-            print(f"Activating widget {self._active_widget.__class__.__name__}")
             self._active_widget._active = True
 
     def debug(self):

arcade/gui/widgets/__init__.py

@@ -1,10 +1,12 @@
 from __future__ import annotations
 
+import weakref
 from abc import ABC
 from collections.abc import Iterable
 from enum import IntEnum
 from types import EllipsisType
-from typing import TYPE_CHECKING, NamedTuple, TypeVar
+from typing import Any, Generic, TYPE_CHECKING, NamedTuple, TypeVar, overload
+from weakref import WeakKeyDictionary
 
 from pyglet.event import EVENT_HANDLED, EVENT_UNHANDLED, EventDispatcher
 from pyglet.math import Vec2
@@ -31,6 +33,7 @@
     from arcade.gui.ui_manager import UIManager
 
 W = TypeVar("W", bound="UIWidget")
+P = TypeVar("P")
 
 
 class FocusMode(IntEnum):
@@ -51,6 +54,51 @@ class _ChildEntry(NamedTuple):
     data: dict
 
 
+class WeakRef(Generic[P]):
+    """A weak reference to a UIWidget parent, which is used to prevent memory leaks."""
+
+    __slots__ = ("name", "obs")
+    name: str
+    """Attribute name of the property"""
+    obs: WeakKeyDictionary[Any, weakref.ref[P]]
+    """Weak dictionary to hold the values"""
+
+    def __init__(self):
+        self.obs = WeakKeyDictionary()
+
+    def get(self, instance: Any) -> P | None:
+        """Get value for owner instance"""
+        # If the value is not set, return None
+        value = self.obs.get(instance)
+        return value() if value else None
+
+    def set(self, instance, value: P | None):
+        """Set value for owner instance"""
+        # Store a weak reference to the value
+        if value is None:
+            self.obs.pop(instance, None)
+        else:
+            self.obs[instance] = weakref.ref(value)
+
+    def __set_name__(self, owner, name):
+        self.name = name
+
+    @overload
+    def __get__(self, instance: None, instance_type) -> Self: ...
+
+    @overload
+    def __get__(self, instance: Any, instance_type) -> P | None: ...
+
+    def __get__(self, instance: Any | None, instance_type) -> Self | P | None:
+        """Get the value for the owner instance, or None if not set."""
+        if instance is None:
+            return self
+        return self.get(instance)
+
+    def __set__(self, instance, value: P | None):
+        self.set(instance, value)
+
+
 @copy_dunders_unimplemented
 class UIWidget(EventDispatcher, ABC):
     """The :class:`UIWidget` class is the base class required for creating widgets.
@@ -71,6 +119,9 @@ class UIWidget(EventDispatcher, ABC):
         size_hint_max: max width and height in pixel
     """
 
+    parent: WeakRef[UIManager | UIWidget | None] = WeakRef()
+    """A weak reference to the parent UIManager or UIWidget,
+    which does not prevent garbage collection of the parent."""
     rect = Property(LBWH(0, 0, 1, 1))
     visible = Property(True)
     focused = Property(False)
@@ -113,7 +164,6 @@ def __init__(
     ):
         self._requires_render = True
         self.rect = LBWH(x, y, width, height)
-        self.parent: UIManager | UIWidget | None = None
 
         # Size hints are properties that can be used by layouts
         self.size_hint = size_hint
@@ -126,21 +176,21 @@ def __init__(
         for child in children:
             self.add(child)
 
-        bind(self, "rect", self.trigger_full_render)
-        bind(self, "focused", self.trigger_full_render)
+        bind(self, "rect", UIWidget.trigger_full_render)
+        bind(self, "focused", UIWidget.trigger_full_render)
         bind(
-            self, "visible", self.trigger_full_render
+            self, "visible", UIWidget.trigger_full_render
         )  # TODO maybe trigger_parent_render would be enough
-        bind(self, "_children", self.trigger_render)
-        bind(self, "_border_width", self.trigger_render)
-        bind(self, "_border_color", self.trigger_render)
-        bind(self, "_bg_color", self.trigger_render)
-        bind(self, "_bg_tex", self.trigger_render)
-        bind(self, "_padding_top", self.trigger_render)
-        bind(self, "_padding_right", self.trigger_render)
-        bind(self, "_padding_bottom", self.trigger_render)
-        bind(self, "_padding_left", self.trigger_render)
-        bind(self, "_strong_background", self.trigger_render)
+        bind(self, "_children", UIWidget.trigger_render)
+        bind(self, "_border_width", UIWidget.trigger_render)
+        bind(self, "_border_color", UIWidget.trigger_render)
+        bind(self, "_bg_color", UIWidget.trigger_render)
+        bind(self, "_bg_tex", UIWidget.trigger_render)
+        bind(self, "_padding_top", UIWidget.trigger_render)
+        bind(self, "_padding_right", UIWidget.trigger_render)
+        bind(self, "_padding_bottom", UIWidget.trigger_render)
+        bind(self, "_padding_left", UIWidget.trigger_render)
+        bind(self, "_strong_background", UIWidget.trigger_render)
 
     def add(self, child: W, **kwargs) -> W:
         """Add a widget as a child.
@@ -692,9 +742,9 @@ def __init__(
 
         self.interaction_buttons = interaction_buttons
 
-        bind(self, "pressed", self.trigger_render)
-        bind(self, "hovered", self.trigger_render)
-        bind(self, "disabled", self.trigger_render)
+        bind(self, "pressed", UIInteractiveWidget.trigger_render)
+        bind(self, "hovered", UIInteractiveWidget.trigger_render)
+        bind(self, "disabled", UIInteractiveWidget.trigger_render)
 
     def on_event(self, event: UIEvent) -> bool | None:
         """Handles mouse events and triggers on_click event if the widget is clicked.

arcade/gui/widgets/buttons.py

@@ -134,7 +134,7 @@ def __init__(
         if texture_disabled:
             self._textures["disabled"] = texture_disabled
 
-        bind(self, "_textures", self.trigger_render)
+        bind(self, "_textures", UITextureButton.trigger_render)
 
         # prepare label with default style
         _style = self.get_current_style()

arcade/gui/widgets/image.py

@@ -55,9 +55,9 @@ def __init__(
             height=height if height else texture.height,
             **kwargs,
         )
-        bind(self, "texture", self.trigger_render)
-        bind(self, "alpha", self.trigger_full_render)
-        bind(self, "angle", self.trigger_full_render)
+        bind(self, "texture", UIImage.trigger_render)
+        bind(self, "alpha", UIImage.trigger_full_render)
+        bind(self, "angle", UIImage.trigger_full_render)
 
     @override
     def do_render(self, surface: Surface):