From dbb3ad2853372899beffde20b861e255fe2d0bb3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bokota Date: Sat, 23 Mar 2024 09:02:04 +0100 Subject: [PATCH 01/10] do not use lambda in destroyed callback --- src/app_model/backends/qt/_qaction.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/app_model/backends/qt/_qaction.py b/src/app_model/backends/qt/_qaction.py index 7b23eba9..37536d6d 100644 --- a/src/app_model/backends/qt/_qaction.py +++ b/src/app_model/backends/qt/_qaction.py @@ -157,13 +157,17 @@ def __init__( 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.destroyed.connect(self._remove_from_cache) + self._app.destroyed.connect(self._remove_from_cache) self._initialized = True with contextlib.suppress(NameError): self.update_from_context(self._app.context) + def _remove_from_cache(self) -> None: + key = (id(self._app), hash(self._menu_item)) + QMenuItemAction._cache.pop(key, None) + 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) From beb9b6e0c52857c97c446d923689fce9637d517c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 23 Mar 2024 08:04:30 +0000 Subject: [PATCH 02/10] style: [pre-commit.ci] auto fixes [...] --- src/app_model/backends/qt/_qaction.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app_model/backends/qt/_qaction.py b/src/app_model/backends/qt/_qaction.py index 37536d6d..6ab5ceed 100644 --- a/src/app_model/backends/qt/_qaction.py +++ b/src/app_model/backends/qt/_qaction.py @@ -156,7 +156,7 @@ def __init__( if not initialized: super().__init__(menu_item.command, app, parent) self._menu_item = menu_item - key = (id(self._app), hash(menu_item)) + (id(self._app), hash(menu_item)) self.destroyed.connect(self._remove_from_cache) self._app.destroyed.connect(self._remove_from_cache) self._initialized = True From 1fb13fbbcec8b4903ad78f8d6245e9f0e35cf00e Mon Sep 17 00:00:00 2001 From: Talley Lambert Date: Sun, 24 Mar 2024 11:37:55 -0400 Subject: [PATCH 03/10] change caching pattern --- src/app_model/backends/qt/_qaction.py | 81 +++++++++++++++------------ src/app_model/backends/qt/_qmenu.py | 3 +- tests/test_qt/test_qactions.py | 4 +- 3 files changed, 50 insertions(+), 38 deletions(-) diff --git a/src/app_model/backends/qt/_qaction.py b/src/app_model/backends/qt/_qaction.py index 6ab5ceed..8724da55 100644 --- a/src/app_model/backends/qt/_qaction.py +++ b/src/app_model/backends/qt/_qaction.py @@ -13,6 +13,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,55 +119,65 @@ class QMenuItemAction(QCommandRuleAction): Mostly the same as a `CommandRuleAction`, but aware of the `menu_item.when` clause to toggle visibility. + + 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 """ _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] - - self = super().__new__(cls) - if cache: - cls._cache[key] = self - return self # type: ignore [no-any-return] - 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 - (id(self._app), hash(menu_item)) - self.destroyed.connect(self._remove_from_cache) - self._app.destroyed.connect(self._remove_from_cache) - 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)) + def _remove_from_cache(self) -> None: - key = (id(self._app), hash(self._menu_item)) - QMenuItemAction._cache.pop(key, None) + cache_key = QMenuItemAction._cache_key(self._app, self._menu_item) + QMenuItemAction._cache.pop(cache_key, None) + + @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: + return cls._cache[cache_key] + + cls._cache[cache_key] = obj = cls(menu_item, app, parent) + obj.destroyed.connect(obj._remove_from_cache) + app.destroyed.connect(obj._remove_from_cache) + return obj def update_from_context(self, ctx: Mapping[str, object]) -> None: """Update the enabled/visible state of this menu item from `ctx`.""" diff --git a/src/app_model/backends/qt/_qmenu.py b/src/app_model/backends/qt/_qmenu.py index 6b88ed0a..d362e6fb 100644 --- a/src/app_model/backends/qt/_qmenu.py +++ b/src/app_model/backends/qt/_qmenu.py @@ -242,6 +242,7 @@ def rebuild( include_submenus=include_submenus, exclude=self._exclude if exclude is None else exclude, ) + self.rebuilt.emit() def _disconnect(self) -> None: self._app.menus.menus_changed.disconnect(self._on_registry_changed) @@ -317,7 +318,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) menu.addAction(action) if n < n_groups - 1: menu.addSeparator() diff --git a/tests/test_qt/test_qactions.py b/tests/test_qt/test_qactions.py index 8c82b1da..630d4346 100644 --- a/tests/test_qt/test_qactions.py +++ b/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") From a025309073f946a270d37adbbd26783b6a477746 Mon Sep 17 00:00:00 2001 From: Talley Lambert Date: Sun, 24 Mar 2024 11:40:59 -0400 Subject: [PATCH 04/10] remove old signal --- src/app_model/backends/qt/_qmenu.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app_model/backends/qt/_qmenu.py b/src/app_model/backends/qt/_qmenu.py index d362e6fb..45705fd0 100644 --- a/src/app_model/backends/qt/_qmenu.py +++ b/src/app_model/backends/qt/_qmenu.py @@ -242,7 +242,6 @@ def rebuild( include_submenus=include_submenus, exclude=self._exclude if exclude is None else exclude, ) - self.rebuilt.emit() def _disconnect(self) -> None: self._app.menus.menus_changed.disconnect(self._on_registry_changed) From 335ee5f2b98eab95046fab9a987ce423fa41d8e5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bokota Date: Sun, 24 Mar 2024 17:47:29 +0100 Subject: [PATCH 05/10] use WeakValueDictionary --- src/app_model/backends/qt/_qaction.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/app_model/backends/qt/_qaction.py b/src/app_model/backends/qt/_qaction.py index 8724da55..d641054a 100644 --- a/src/app_model/backends/qt/_qaction.py +++ b/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 @@ -130,7 +131,7 @@ class QMenuItemAction(QCommandRuleAction): Optional parent widget, by default None """ - _cache: ClassVar[dict[tuple[int, int], QMenuItemAction]] = {} + _cache: ClassVar[dict[tuple[int, int], QMenuItemAction]] = WeakValueDictionary() def __init__( self, From eb8211b8ae9c3e4e4dbd0ee8ad68298ad5b08943 Mon Sep 17 00:00:00 2001 From: Grzegorz Bokota Date: Sun, 24 Mar 2024 22:03:30 +0100 Subject: [PATCH 06/10] use parent --- src/app_model/backends/qt/_qmenu.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app_model/backends/qt/_qmenu.py b/src/app_model/backends/qt/_qmenu.py index 45705fd0..cb8d9157 100644 --- a/src/app_model/backends/qt/_qmenu.py +++ b/src/app_model/backends/qt/_qmenu.py @@ -317,7 +317,7 @@ def _rebuild( submenu = QModelSubmenu(item, app, parent=menu) cast("QMenu", menu).addMenu(submenu) elif item.command.id not in _exclude: - action = QMenuItemAction.create(item, app=app) + action = QMenuItemAction.create(item, app=app, parent=menu) menu.addAction(action) if n < n_groups - 1: menu.addSeparator() From d357d6bfd34e1fbbae240e1e4f275e77c405afd3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bokota Date: Sun, 24 Mar 2024 22:13:05 +0100 Subject: [PATCH 07/10] improve cleaning --- src/app_model/backends/qt/_qaction.py | 4 +++- src/app_model/backends/qt/_qmenu.py | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/app_model/backends/qt/_qaction.py b/src/app_model/backends/qt/_qaction.py index d641054a..5bdf64df 100644 --- a/src/app_model/backends/qt/_qaction.py +++ b/src/app_model/backends/qt/_qaction.py @@ -173,7 +173,9 @@ def create( 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: - return cls._cache[cache_key] + res = cls._cache[cache_key] + res.setParent(parent) + return res cls._cache[cache_key] = obj = cls(menu_item, app, parent) obj.destroyed.connect(obj._remove_from_cache) diff --git a/src/app_model/backends/qt/_qmenu.py b/src/app_model/backends/qt/_qmenu.py index cb8d9157..0ddc4068 100644 --- a/src/app_model/backends/qt/_qmenu.py +++ b/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)) From 6d45f20118cb9d897b1f61d3fcdbe9644f8350cb Mon Sep 17 00:00:00 2001 From: Grzegorz Bokota Date: Sun, 24 Mar 2024 22:37:47 +0100 Subject: [PATCH 08/10] fix type error --- src/app_model/backends/qt/_qaction.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/app_model/backends/qt/_qaction.py b/src/app_model/backends/qt/_qaction.py index 5bdf64df..882e166f 100644 --- a/src/app_model/backends/qt/_qaction.py +++ b/src/app_model/backends/qt/_qaction.py @@ -131,7 +131,9 @@ class QMenuItemAction(QCommandRuleAction): Optional parent widget, by default None """ - _cache: ClassVar[dict[tuple[int, int], QMenuItemAction]] = WeakValueDictionary() + _cache: ClassVar[WeakValueDictionary[tuple[int, int], QMenuItemAction]] = ( + WeakValueDictionary() + ) def __init__( self, From c52989dc70478bc50fbdc9e8d9c82f7403f4cabe Mon Sep 17 00:00:00 2001 From: Grzegorz Bokota Date: Mon, 25 Mar 2024 00:04:13 +0100 Subject: [PATCH 09/10] Apply suggestions from code review --- src/app_model/backends/qt/_qaction.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/app_model/backends/qt/_qaction.py b/src/app_model/backends/qt/_qaction.py index 882e166f..18b412a7 100644 --- a/src/app_model/backends/qt/_qaction.py +++ b/src/app_model/backends/qt/_qaction.py @@ -150,9 +150,6 @@ def __init__( def _cache_key(app: Application, menu_item: MenuItem) -> tuple[int, int]: return (id(app), hash(menu_item)) - def _remove_from_cache(self) -> None: - cache_key = QMenuItemAction._cache_key(self._app, self._menu_item) - QMenuItemAction._cache.pop(cache_key, None) @classmethod def create( @@ -180,8 +177,6 @@ def create( return res cls._cache[cache_key] = obj = cls(menu_item, app, parent) - obj.destroyed.connect(obj._remove_from_cache) - app.destroyed.connect(obj._remove_from_cache) return obj def update_from_context(self, ctx: Mapping[str, object]) -> None: From c9a2b33568c6615e893702aeca76ca3e0d9a4fda Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 24 Mar 2024 23:04:22 +0000 Subject: [PATCH 10/10] style: [pre-commit.ci] auto fixes [...] --- src/app_model/backends/qt/_qaction.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app_model/backends/qt/_qaction.py b/src/app_model/backends/qt/_qaction.py index 18b412a7..74c2e80b 100644 --- a/src/app_model/backends/qt/_qaction.py +++ b/src/app_model/backends/qt/_qaction.py @@ -150,7 +150,6 @@ def __init__( def _cache_key(app: Application, menu_item: MenuItem) -> tuple[int, int]: return (id(app), hash(menu_item)) - @classmethod def create( cls,