From 6372b8370bc3c724a118fd93f65e5c00b03c0eb3 Mon Sep 17 00:00:00 2001 From: Hanjin Liu Date: Sat, 4 Jan 2025 15:47:46 +0900 Subject: [PATCH 1/4] fix recursion check --- src/app_model/backends/qt/_qmenu.py | 32 +++++++++++++++-------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/app_model/backends/qt/_qmenu.py b/src/app_model/backends/qt/_qmenu.py index 11f6cdc..31ce1e5 100644 --- a/src/app_model/backends/qt/_qmenu.py +++ b/src/app_model/backends/qt/_qmenu.py @@ -72,7 +72,7 @@ def findAction(self, object_name: str) -> QAction | QModelMenu | None: return _find_action(self.actions(), object_name) def update_from_context( - self, ctx: Mapping[str, object], _recurse: bool = True + self, ctx: Mapping[str, object], _skip: QModelMenu | None = None ) -> None: """Update the enabled/visible state of each menu item with `ctx`. @@ -85,10 +85,10 @@ def update_from_context( `'when'` expressions provided for each action in the menu. *ALL variables used in these expressions must either be present in the `ctx` dict, or be builtins*. - _recurse : bool + _skip : QModelMenu, optional recursion check, internal use only """ - _update_from_context(self.actions(), ctx, _recurse=_recurse) + _update_from_context(self.actions(), ctx, _skip=_skip) def rebuild( self, include_submenus: bool = True, exclude: Collection[str] | None = None @@ -146,10 +146,10 @@ def __init__( self.setIcon(to_qicon(submenu.icon)) def update_from_context( - self, ctx: Mapping[str, object], _recurse: bool = True + self, ctx: Mapping[str, object], _skip: QModelMenu | None = None ) -> None: """Update the enabled state of this menu item from `ctx`.""" - super().update_from_context(ctx, _recurse=_recurse) + super().update_from_context(ctx, _skip=_skip) self.setEnabled(expr.eval(ctx) if (expr := self._submenu.enablement) else True) # TODO: ... visibility needs to be controlled at the level of placement # in the submenu. consider only using the `when` expression @@ -214,7 +214,7 @@ def findAction(self, object_name: str) -> QAction | QModelMenu | None: return _find_action(self.actions(), object_name) def update_from_context( - self, ctx: Mapping[str, object], _recurse: bool = True + self, ctx: Mapping[str, object], _skip: QModelMenu | None = None ) -> None: """Update the enabled/visible state of each menu item with `ctx`. @@ -227,10 +227,10 @@ def update_from_context( `'when'` expressions provided for each action in the menu. *ALL variables used in these expressions must either be present in the `ctx` dict, or be builtins*. - _recurse : bool + _skip : QModelMenu, optional recursion check, internal use only """ - _update_from_context(self.actions(), ctx, _recurse=_recurse) + _update_from_context(self.actions(), ctx, _skip=_skip) def rebuild( self, include_submenus: bool = True, exclude: Collection[str] | None = None @@ -279,7 +279,7 @@ def __init__( self.addMenu(QModelMenu(id_, app, title, self)) def update_from_context( - self, ctx: Mapping[str, object], _recurse: bool = True + self, ctx: Mapping[str, object], _skip: QModelMenu | None = None ) -> None: """Update the enabled/visible state of each menu item with `ctx`. @@ -292,10 +292,10 @@ def update_from_context( `'when'` expressions provided for each action in the menu. *ALL variables used in these expressions must either be present in the `ctx` dict, or be builtins*. - _recurse : bool + _skip : QModelMenu, optional recursion check, internal use only """ - _update_from_context(self.actions(), ctx, _recurse=_recurse) + _update_from_context(self.actions(), ctx, _skip=_skip) def _rebuild( @@ -328,7 +328,9 @@ def _rebuild( def _update_from_context( - actions: Iterable[QAction], ctx: Mapping[str, object], _recurse: bool = True + actions: Iterable[QAction], + ctx: Mapping[str, object], + _skip: QModelMenu | None = None, ) -> None: """Update the enabled/visible state of each menu item with `ctx`. @@ -343,7 +345,7 @@ def _update_from_context( `'when'` expressions provided for each action in the menu. *ALL variables used in these expressions must either be present in the `ctx` dict, or be builtins*. - _recurse : bool + _skip : QModelMenu, optional recursion check, internal use only """ for action in actions: @@ -358,8 +360,8 @@ def _update_from_context( # but I'm not sure if it's the right thing to do, and it leads to a # recursion error. I stop it with the _recurse flag here, but I wonder # whether that will cause other problems. - if _recurse: - parent.update_from_context(ctx, _recurse=False) + if _skip is not parent: + parent.update_from_context(ctx, _skip=parent) def _find_action(actions: Iterable[QAction], object_name: str) -> QAction | None: From 4ea0bcbe91b5d84b8dd7e1b486dd277ceff12a21 Mon Sep 17 00:00:00 2001 From: Hanjin Liu Date: Sun, 5 Jan 2025 09:30:12 +0900 Subject: [PATCH 2/4] use action.menu() in Qt6 as well --- src/app_model/backends/qt/_qmenu.py | 49 ++++++----------------------- 1 file changed, 10 insertions(+), 39 deletions(-) diff --git a/src/app_model/backends/qt/_qmenu.py b/src/app_model/backends/qt/_qmenu.py index 31ce1e5..7ddd3d3 100644 --- a/src/app_model/backends/qt/_qmenu.py +++ b/src/app_model/backends/qt/_qmenu.py @@ -71,9 +71,7 @@ def findAction(self, object_name: str) -> QAction | QModelMenu | None: """ return _find_action(self.actions(), object_name) - def update_from_context( - self, ctx: Mapping[str, object], _skip: QModelMenu | None = None - ) -> None: + def update_from_context(self, ctx: Mapping[str, object]) -> None: """Update the enabled/visible state of each menu item with `ctx`. See `app_model.expressions` for details on expressions. @@ -85,10 +83,8 @@ def update_from_context( `'when'` expressions provided for each action in the menu. *ALL variables used in these expressions must either be present in the `ctx` dict, or be builtins*. - _skip : QModelMenu, optional - recursion check, internal use only """ - _update_from_context(self.actions(), ctx, _skip=_skip) + _update_from_context(self.actions(), ctx) def rebuild( self, include_submenus: bool = True, exclude: Collection[str] | None = None @@ -145,11 +141,9 @@ def __init__( if submenu.icon: self.setIcon(to_qicon(submenu.icon)) - def update_from_context( - self, ctx: Mapping[str, object], _skip: QModelMenu | None = None - ) -> None: + def update_from_context(self, ctx: Mapping[str, object]) -> None: """Update the enabled state of this menu item from `ctx`.""" - super().update_from_context(ctx, _skip=_skip) + super().update_from_context(ctx) self.setEnabled(expr.eval(ctx) if (expr := self._submenu.enablement) else True) # TODO: ... visibility needs to be controlled at the level of placement # in the submenu. consider only using the `when` expression @@ -213,9 +207,7 @@ def findAction(self, object_name: str) -> QAction | QModelMenu | None: """ return _find_action(self.actions(), object_name) - def update_from_context( - self, ctx: Mapping[str, object], _skip: QModelMenu | None = None - ) -> None: + def update_from_context(self, ctx: Mapping[str, object]) -> None: """Update the enabled/visible state of each menu item with `ctx`. See `app_model.expressions` for details on expressions. @@ -227,10 +219,8 @@ def update_from_context( `'when'` expressions provided for each action in the menu. *ALL variables used in these expressions must either be present in the `ctx` dict, or be builtins*. - _skip : QModelMenu, optional - recursion check, internal use only """ - _update_from_context(self.actions(), ctx, _skip=_skip) + _update_from_context(self.actions(), ctx) def rebuild( self, include_submenus: bool = True, exclude: Collection[str] | None = None @@ -278,9 +268,7 @@ def __init__( id_, title = item if isinstance(item, tuple) else (item, item.title()) self.addMenu(QModelMenu(id_, app, title, self)) - def update_from_context( - self, ctx: Mapping[str, object], _skip: QModelMenu | None = None - ) -> None: + def update_from_context(self, ctx: Mapping[str, object]) -> None: """Update the enabled/visible state of each menu item with `ctx`. See `app_model.expressions` for details on expressions. @@ -292,10 +280,8 @@ def update_from_context( `'when'` expressions provided for each action in the menu. *ALL variables used in these expressions must either be present in the `ctx` dict, or be builtins*. - _skip : QModelMenu, optional - recursion check, internal use only """ - _update_from_context(self.actions(), ctx, _skip=_skip) + _update_from_context(self.actions(), ctx) def _rebuild( @@ -327,11 +313,7 @@ def _rebuild( menu.addSeparator() -def _update_from_context( - actions: Iterable[QAction], - ctx: Mapping[str, object], - _skip: QModelMenu | None = None, -) -> None: +def _update_from_context(actions: Iterable[QAction], ctx: Mapping[str, object]) -> None: """Update the enabled/visible state of each menu item with `ctx`. See `app_model.expressions` for details on expressions. @@ -345,23 +327,12 @@ def _update_from_context( `'when'` expressions provided for each action in the menu. *ALL variables used in these expressions must either be present in the `ctx` dict, or be builtins*. - _skip : QModelMenu, optional - recursion check, internal use only """ for action in actions: if isinstance(action, QMenuItemAction): action.update_from_context(ctx) - elif not QT6 and isinstance(menu := action.menu(), QModelMenu): + elif isinstance(menu := action.menu(), QModelMenu): menu.update_from_context(ctx) - elif isinstance(parent := action.parent(), QModelMenu): - # FIXME: this is a hack for Qt6 that I don't entirely understand. - # QAction has lost the `.menu()` method, and it's a bit hard to find - # how to get to the parent menu now. Checking parent() seems to work, - # but I'm not sure if it's the right thing to do, and it leads to a - # recursion error. I stop it with the _recurse flag here, but I wonder - # whether that will cause other problems. - if _skip is not parent: - parent.update_from_context(ctx, _skip=parent) def _find_action(actions: Iterable[QAction], object_name: str) -> QAction | None: From ad8c85254c8fcb3c7f3b4ebf8bfdc401f9bacfbc Mon Sep 17 00:00:00 2001 From: Talley Lambert Date: Sun, 5 Jan 2025 14:01:31 -0500 Subject: [PATCH 3/4] add exception error, update test --- .github/workflows/ci.yml | 3 --- src/app_model/backends/qt/_qmenu.py | 13 ++++++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3660511..dbf75ca 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -54,9 +54,6 @@ jobs: - python-version: "3.10" os: "ubuntu-latest" qt: "PySide2~=5.15.0" - - python-version: "3.10" - os: "ubuntu-latest" - qt: "PyQt6~=6.2.0" - python-version: "3.10" os: "ubuntu-latest" qt: "PySide6~=6.3.0" diff --git a/src/app_model/backends/qt/_qmenu.py b/src/app_model/backends/qt/_qmenu.py index 7ddd3d3..f9473f3 100644 --- a/src/app_model/backends/qt/_qmenu.py +++ b/src/app_model/backends/qt/_qmenu.py @@ -328,11 +328,14 @@ def _update_from_context(actions: Iterable[QAction], ctx: Mapping[str, object]) *ALL variables used in these expressions must either be present in the `ctx` dict, or be builtins*. """ - for action in actions: - if isinstance(action, QMenuItemAction): - action.update_from_context(ctx) - elif isinstance(menu := action.menu(), QModelMenu): - menu.update_from_context(ctx) + try: + for action in actions: + if isinstance(action, QMenuItemAction): + action.update_from_context(ctx) + elif isinstance(menu := action.menu(), QModelMenu): + menu.update_from_context(ctx) + except AttributeError as e: # pragma: no cover + raise AttributeError(f"This version of Qt is not supported: {e}") from e def _find_action(actions: Iterable[QAction], object_name: str) -> QAction | None: From b59383ca4f467e846bcac16357c8eb86916c7c5a Mon Sep 17 00:00:00 2001 From: Talley Lambert Date: Mon, 6 Jan 2025 10:03:47 -0500 Subject: [PATCH 4/4] add test --- tests/conftest.py | 1 + tests/test_qt/test_qmenu.py | 23 +++++++++++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 5cae879..e98cbb8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -186,6 +186,7 @@ def build_app(name: str = "complete_test_app") -> FullApp: title="Open from B", callback=app.mocks.open_from_b, menus=[{"id": Menus.FILE_OPEN_FROM}], + enablement="sat", ), Action( id=Commands.UNIMPORTABLE, diff --git a/tests/test_qt/test_qmenu.py b/tests/test_qt/test_qmenu.py index cc28d34..aaa591c 100644 --- a/tests/test_qt/test_qmenu.py +++ b/tests/test_qt/test_qmenu.py @@ -77,6 +77,9 @@ def test_submenu(qtbot: QtBot, full_app: FullApp) -> None: assert menu_texts == ["Open From...", "Open..."] submenu = menu.findChild(QModelMenu, app.Menus.FILE_OPEN_FROM) + submenu_action = submenu.findAction(app.Commands.OPEN_FROM_B) + + assert submenu_action assert isinstance(submenu, QModelMenu) submenu.setVisible(True) assert submenu.isVisible() @@ -86,21 +89,29 @@ def test_submenu(qtbot: QtBot, full_app: FullApp) -> None: # "not something_open" is the when clause # "friday" is the enablement clause - menu.update_from_context({"something_open": False, "friday": True}) + menu.update_from_context({"something_open": False, "friday": True, "sat": True}) assert submenu.isVisible() assert submenu.isEnabled() + assert submenu_action.isVisible() + assert submenu_action.isEnabled() - menu.update_from_context({"something_open": False, "friday": False}) + menu.update_from_context({"something_open": False, "friday": False, "sat": True}) assert submenu.isVisible() assert not submenu.isEnabled() + assert submenu_action.isVisible() + assert submenu_action.isEnabled() - menu.update_from_context({"something_open": True, "friday": False}) - # assert not submenu.isVisible() + menu.update_from_context({"something_open": True, "friday": False, "sat": True}) assert not submenu.isEnabled() + assert submenu_action.isEnabled() + + menu.update_from_context({"something_open": True, "friday": True, "sat": True}) + assert submenu.isEnabled() + assert submenu_action.isEnabled() - menu.update_from_context({"something_open": True, "friday": True}) - # assert not submenu.isVisible() + menu.update_from_context({"something_open": True, "friday": True, "sat": False}) assert submenu.isEnabled() + assert not submenu_action.isEnabled() @pytest.mark.filterwarnings("ignore:QPixmapCache.find:")