fix: Do not use lambda in QMenuItemAction.destroyed callback (#183)

* do not use lambda in destroyed callback

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

* change caching pattern

* remove old signal

* use WeakValueDictionary

* use parent

* improve cleaning

* fix type error

* Apply suggestions from code review

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

---------

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

Author: 3 people

src/app_model/backends/qt/_qaction.py

@@ -2,6 +2,7 @@
 
 import contextlib
 from typing import TYPE_CHECKING, ClassVar, Mapping
+from weakref import WeakValueDictionary
 
 from app_model import Application
 from app_model.expressions import Expr
@@ -13,6 +14,7 @@
 if TYPE_CHECKING:
     from PyQt6.QtGui import QAction
     from qtpy.QtCore import QObject
+    from typing_extensions import Self
 
     from app_model.types import CommandRule, MenuItem
 else:
@@ -118,52 +120,64 @@ class QMenuItemAction(QCommandRuleAction):
 
     Mostly the same as a `CommandRuleAction`, but aware of the `menu_item.when` clause
     to toggle visibility.
-    """
-
-    _cache: ClassVar[dict[tuple[int, int], QMenuItemAction]] = {}
 
-    def __new__(
-        cls: type[QMenuItemAction],
-        menu_item: MenuItem,
-        app: Application | str,
-        parent: QObject | None = None,
-        *,
-        cache: bool = True,
-    ) -> QMenuItemAction:
-        """Create and cache a QMenuItemAction for the given menu item."""
-        app = Application.get_or_create(app) if isinstance(app, str) else app
-        key = (id(app), hash(menu_item))
-        if cache and key in cls._cache:
-            return cls._cache[key]
+    Parameters
+    ----------
+    menu_item : MenuItem
+        `MenuItem` instance to create an action for.
+    app : Application | str
+        Application instance or name of application instance.
+    parent : QObject | None
+        Optional parent widget, by default None
+    """
 
-        self = super().__new__(cls)
-        if cache:
-            cls._cache[key] = self
-        return self  # type: ignore [no-any-return]
+    _cache: ClassVar[WeakValueDictionary[tuple[int, int], QMenuItemAction]] = (
+        WeakValueDictionary()
+    )
 
     def __init__(
         self,
         menu_item: MenuItem,
         app: Application | str,
         parent: QObject | None = None,
-        *,
-        cache: bool = True,  # used in __new__
     ):
-        initialized = False
-        with contextlib.suppress(RuntimeError):
-            initialized = getattr(self, "_initialized", False)
-
-        if not initialized:
-            super().__init__(menu_item.command, app, parent)
-            self._menu_item = menu_item
-            key = (id(self._app), hash(menu_item))
-            self.destroyed.connect(lambda: QMenuItemAction._cache.pop(key, None))
-            self._app.destroyed.connect(lambda: QMenuItemAction._cache.pop(key, None))
-            self._initialized = True
-
+        super().__init__(menu_item.command, app, parent)
+        self._menu_item = menu_item
         with contextlib.suppress(NameError):
             self.update_from_context(self._app.context)
 
+    @staticmethod
+    def _cache_key(app: Application, menu_item: MenuItem) -> tuple[int, int]:
+        return (id(app), hash(menu_item))
+
+    @classmethod
+    def create(
+        cls,
+        menu_item: MenuItem,
+        app: Application | str,
+        parent: QObject | None = None,
+    ) -> Self:
+        """Create a new QMenuItemAction for the given menu item.
+
+        Prefer this method over `__init__` to ensure that the cache is used,
+        so that:
+
+        ```python
+        a1 = QMenuItemAction.create(action, full_app)
+        a2 = QMenuItemAction.create(action, full_app)
+        a1 is a2  # True
+        ```
+        """
+        app = Application.get_or_create(app) if isinstance(app, str) else app
+        cache_key = QMenuItemAction._cache_key(app, menu_item)
+        if cache_key in cls._cache:
+            res = cls._cache[cache_key]
+            res.setParent(parent)
+            return res
+
+        cls._cache[cache_key] = obj = cls(menu_item, app, parent)
+        return obj
+
     def update_from_context(self, ctx: Mapping[str, object]) -> None:
         """Update the enabled/visible state of this menu item from `ctx`."""
         super().update_from_context(ctx)

src/app_model/backends/qt/_qmenu.py

@@ -305,7 +305,10 @@ def _rebuild(
     exclude: Collection[str] | None = None,
 ) -> None:
     """Rebuild menu by looking up `menu` in `Application`'s menu_registry."""
-    menu.clear()
+    actions = menu.actions()
+    for action in actions:
+        menu.removeAction(action)
+
     _exclude = exclude or set()
 
     groups = list(app.menus.iter_menu_groups(menu_id))
@@ -317,7 +320,7 @@ def _rebuild(
                     submenu = QModelSubmenu(item, app, parent=menu)
                     cast("QMenu", menu).addMenu(submenu)
             elif item.command.id not in _exclude:
-                action = QMenuItemAction(item, app=app, parent=menu)
+                action = QMenuItemAction.create(item, app=app, parent=menu)
                 menu.addAction(action)
         if n < n_groups - 1:
             menu.addSeparator()

tests/test_qt/test_qactions.py

@@ -13,8 +13,8 @@ def test_cache_qaction(qapp, full_app: "FullApp") -> None:
     action = next(
         i for k, items in full_app.menus for i in items if isinstance(i, MenuItem)
     )
-    a1 = QMenuItemAction(action, full_app)
-    a2 = QMenuItemAction(action, full_app)
+    a1 = QMenuItemAction.create(action, full_app)
+    a2 = QMenuItemAction.create(action, full_app)
     assert a1 is a2
     assert repr(a1).startswith("QMenuItemAction")