fix: attribute action extension handlers by identity; add enabled_when action registration

CHANGELOG.md

@@ -62,11 +62,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   existing machine configurations continue to work unchanged.
   Machines saved while the driver carried its experimental name keep
   working through a compatibility alias
+- Addon API 22: actions registered via the `register_actions` hook
+  accept an `enabled_when` predicate for their enabled state, and the
+  hook is invoked again when an addon is enabled while the window
+  exists, so addons no longer need `action_extension_registry` for
+  either
 - Updated translations
 
 ### Fixed
 
-- Engrave: the step's Bidirectional Scan Offset had no effect since
+- Addon API: disabling an addon did not reliably remove its
+  `action_extension_registry` handlers when several addons registered
+  same-named handlers, which could leave stale handlers behind or
+  strip other addons' handlers- Engrave: the step's Bidirectional Scan Offset had no effect since
   the pipeline moved to raygeo intents, because the value no longer
   reached the transformer; changing it now also refreshes the cached
   toolpath (#307)

rayforge/core/hooks.py

@@ -4,13 +4,23 @@
 hookimpl = pluggy.HookimplMarker("rayforge")
 
 MINIMUM_API_VERSION = 16
-PLUGIN_API_VERSION = 21
+PLUGIN_API_VERSION = 22
 
 
 """
 API Changelog
 =============
 
+Version 22
+----------
+``action_registry.register()`` accepts an ``enabled_when`` predicate that
+receives the ``ActionManager`` and is evaluated on every action state
+update. Addons that used ``action_extension_registry`` only to enable and
+disable their action can now register directly in the
+``register_actions`` hook. That hook is also invoked again when an addon
+is enabled while the window exists, so runtime-enabled addons no longer
+need ``action_extension_registry`` to have their actions created.
+
 Version 21
 ----------
 Renamed ``WorkOriginElement.set_coordinate_space(space)`` to

rayforge/ui_gtk/action_registry.py

@@ -1,9 +1,14 @@
 import logging
+from collections.abc import Callable
 from dataclasses import dataclass
+from typing import TYPE_CHECKING
 
 from blinker import Signal
 from gi.repository import Gio, Gtk
 
+if TYPE_CHECKING:
+    from .actions import ActionManager
+
 logger = logging.getLogger(__name__)
 
 
@@ -29,6 +34,7 @@ class ActionInfo:
     addon_name: str | None = None
     menu: MenuPlacement | None = None
     toolbar: ToolbarPlacement | None = None
+    enabled_when: 'Callable[["ActionManager"], bool] | None' = None
 
 
 class ActionRegistry:
@@ -64,6 +70,7 @@ def register(
         shortcut: str | None = None,
         menu: MenuPlacement | None = None,
         toolbar: ToolbarPlacement | None = None,
+        enabled_when: 'Callable[["ActionManager"], bool] | None' = None,
     ) -> None:
         """
         Register an action with the window and track it by addon.
@@ -78,6 +85,9 @@ def register(
             menu: Menu placement info if this action should appear in a menu.
             toolbar: Toolbar placement info if this action should appear in
                 the toolbar.
+            enabled_when: Predicate receiving the ActionManager,
+                evaluated on every action state update to set whether
+                the action is enabled.
         """
         if self._window is None:
             logger.warning("Cannot register action: window not set")
@@ -102,6 +112,7 @@ def register(
             addon_name=addon_name or "",
             menu=menu,
             toolbar=toolbar,
+            enabled_when=enabled_when,
         )
         self._actions[action_name] = info
 
@@ -216,6 +227,15 @@ def get_toolbar_items(self, group: str) -> list[ActionInfo]:
             items, key=lambda x: x.toolbar.priority if x.toolbar else 100
         )
 
+    def get_all(self) -> list[ActionInfo]:
+        """
+        Get all registered actions.
+
+        Returns:
+            List of all ActionInfo objects.
+        """
+        return list(self._actions.values())
+
     def get_all_with_shortcuts(self) -> list[ActionInfo]:
         """
         Get all actions that have keyboard shortcuts defined.

rayforge/ui_gtk/actions.py

@@ -111,21 +111,23 @@ class ActionExtensionRegistry:
     def __init__(self):
         self._setup_handlers: list[ActionSetupHandler] = []
         self._state_update_handlers: list[ActionStateUpdateHandler] = []
-        self._setup_addon_map: dict[str, str] = {}
-        self._state_update_addon_map: dict[str, str] = {}
+        self._setup_addon_map: dict[int, str] = {}
+        self._state_update_addon_map: dict[int, str] = {}
 
     def register_setup(self, handler: ActionSetupHandler, addon_name: str):
         """Register a handler to be called during action setup."""
+        if handler in self._setup_handlers:
+            return
         self._setup_handlers.append(handler)
         if addon_name:
-            self._setup_addon_map[handler.__name__] = addon_name
+            self._setup_addon_map[id(handler)] = addon_name
         logger.debug(f"Registered action setup handler: {handler.__name__}")
 
     def unregister_setup(self, handler: ActionSetupHandler) -> bool:
         """Unregister an action setup handler."""
         try:
             self._setup_handlers.remove(handler)
-            self._setup_addon_map.pop(handler.__name__, None)
+            self._setup_addon_map.pop(id(handler), None)
             return True
         except ValueError:
             return False
@@ -134,9 +136,11 @@ def register_state_update(
         self, handler: ActionStateUpdateHandler, addon_name: str
     ):
         """Register a handler to be called during action state updates."""
+        if handler in self._state_update_handlers:
+            return
         self._state_update_handlers.append(handler)
         if addon_name:
-            self._state_update_addon_map[handler.__name__] = addon_name
+            self._state_update_addon_map[id(handler)] = addon_name
         logger.debug(
             f"Registered action state update handler: {handler.__name__}"
         )
@@ -147,7 +151,7 @@ def unregister_state_update(
         """Unregister an action state update handler."""
         try:
             self._state_update_handlers.remove(handler)
-            self._state_update_addon_map.pop(handler.__name__, None)
+            self._state_update_addon_map.pop(id(handler), None)
             return True
         except ValueError:
             return False
@@ -167,21 +171,21 @@ def unregister_all_from_addon(self, addon_name: str) -> int:
         setup_to_remove = [
             h
             for h in self._setup_handlers
-            if self._setup_addon_map.get(h.__name__) == addon_name
+            if self._setup_addon_map.get(id(h)) == addon_name
         ]
         for h in setup_to_remove:
             self._setup_handlers.remove(h)
-            self._setup_addon_map.pop(h.__name__, None)
+            self._setup_addon_map.pop(id(h), None)
         count += len(setup_to_remove)
 
         state_to_remove = [
             h
             for h in self._state_update_handlers
-            if self._state_update_addon_map.get(h.__name__) == addon_name
+            if self._state_update_addon_map.get(id(h)) == addon_name
         ]
         for h in state_to_remove:
             self._state_update_handlers.remove(h)
-            self._state_update_addon_map.pop(h.__name__, None)
+            self._state_update_addon_map.pop(id(h), None)
         count += len(state_to_remove)
 
         if count > 0:
@@ -583,6 +587,19 @@ def update_action_states(self, *args, **kwargs):
         )
 
         action_extension_registry.invoke_state_update_handlers(self)
+        self._update_predicated_actions()
+
+    def _update_predicated_actions(self):
+        """Sets enabled state of actions registered with enabled_when."""
+        for info in action_registry.get_all():
+            if info.enabled_when is None:
+                continue
+            try:
+                info.action.set_enabled(info.enabled_when(self))
+            except Exception:
+                logger.exception(
+                    f"Error updating state of action '{info.action_name}'"
+                )
 
     def on_add_stock(self, action, param):
         """Handler for the 'add-stock' action."""

rayforge/ui_gtk/mainwindow.py

@@ -973,6 +973,10 @@ def _on_job_started(self, sender):
 
     def _on_addon_state_changed(self, sender, addon_name):
         """Handle addon enable/disable to refresh action handlers."""
+        if addon_name in sender.loaded_addons:
+            call_registration_hooks(
+                get_context().plugin_mgr, window_required=True
+            )
         action_extension_registry.invoke_setup_handlers(self.action_manager)
         self.action_manager.update_action_states()
 

tests/ui_gtk/test_action_registries.py

@@ -0,0 +1,155 @@
+# flake8: noqa: E402
+import os
+import sys
+from typing import cast
+
+import pytest
+
+if sys.platform.startswith("linux") and not os.environ.get("DISPLAY"):
+    pytest.skip(
+        "DISPLAY not set on Linux, skipping UI tests. Run with xvfb-run.",
+        allow_module_level=True,
+    )
+
+import gi
+
+gi.require_version("Gtk", "4.0")
+gi.require_version("Adw", "1")
+from gi.repository import Gio, Gtk
+
+from rayforge.ui_gtk.action_registry import ActionRegistry
+from rayforge.ui_gtk.actions import ActionExtensionRegistry, ActionManager
+
+pytestmark = pytest.mark.ui
+
+
+class StubWindow:
+    def __init__(self):
+        self.added: list[Gio.SimpleAction] = []
+        self.removed: list[Gio.SimpleAction] = []
+
+    def add_action(self, action):
+        self.added.append(action)
+
+    def remove_action(self, action):
+        self.removed.append(action)
+
+
+def _make_handler():
+    def _register_actions(action_manager):
+        pass
+
+    return _register_actions
+
+
+def _make_manager() -> ActionManager:
+    return ActionManager.__new__(ActionManager)
+
+
+def test_unregister_all_from_addon_with_same_handler_names():
+    registry = ActionExtensionRegistry()
+    sketcher_handler = _make_handler()
+    print_and_cut_handler = _make_handler()
+    registry.register_setup(sketcher_handler, "sketcher")
+    registry.register_setup(print_and_cut_handler, "print_and_cut")
+
+    assert registry.unregister_all_from_addon("sketcher") == 1
+    assert registry._setup_handlers == [print_and_cut_handler]
+
+    assert registry.unregister_all_from_addon("print_and_cut") == 1
+    assert registry._setup_handlers == []
+
+
+def test_register_setup_is_idempotent():
+    registry = ActionExtensionRegistry()
+    handler = _make_handler()
+    registry.register_setup(handler, "addon")
+    registry.register_setup(handler, "addon")
+
+    assert registry._setup_handlers == [handler]
+    assert registry.unregister_all_from_addon("addon") == 1
+
+
+def test_register_state_update_is_idempotent():
+    registry = ActionExtensionRegistry()
+
+    def _update_action_states(action_manager):
+        pass
+
+    registry.register_state_update(_update_action_states, "addon")
+    registry.register_state_update(_update_action_states, "addon")
+
+    assert registry._state_update_handlers == [_update_action_states]
+
+
+def test_register_stores_enabled_when():
+    registry = ActionRegistry()
+    registry.set_window(cast(Gtk.ApplicationWindow, StubWindow()))
+    action = Gio.SimpleAction.new("test-action", None)
+
+    def enabled_when(action_manager):
+        return True
+
+    registry.register(
+        action_name="test-action",
+        action=action,
+        addon_name="test",
+        enabled_when=enabled_when,
+    )
+
+    info = registry.get("test-action")
+    assert info is not None and info.enabled_when is enabled_when
+    registry.unregister("test-action")
+
+
+def test_update_predicated_actions_evaluates_predicate(monkeypatch):
+    registry = ActionRegistry()
+    registry.set_window(cast(Gtk.ApplicationWindow, StubWindow()))
+    monkeypatch.setattr("rayforge.ui_gtk.actions.action_registry", registry)
+    action = Gio.SimpleAction.new("predicated-test-action", None)
+    action.set_enabled(False)
+    received = []
+
+    def enabled_when(action_manager):
+        received.append(action_manager)
+        return True
+
+    registry.register(
+        action_name="predicated-test-action",
+        action=action,
+        addon_name="test",
+        enabled_when=enabled_when,
+    )
+
+    manager = _make_manager()
+    manager._update_predicated_actions()
+    assert action.get_enabled() is True
+    assert received == [manager]
+
+
+def test_update_predicated_actions_isolates_failing_predicate(monkeypatch):
+    registry = ActionRegistry()
+    registry.set_window(cast(Gtk.ApplicationWindow, StubWindow()))
+    monkeypatch.setattr("rayforge.ui_gtk.actions.action_registry", registry)
+    bad = Gio.SimpleAction.new("predicated-bad-action", None)
+    good = Gio.SimpleAction.new("predicated-good-action", None)
+    good.set_enabled(False)
+
+    def failing(action_manager):
+        raise RuntimeError("boom")
+
+    registry.register(
+        action_name="predicated-bad-action",
+        action=bad,
+        addon_name="test",
+        enabled_when=failing,
+    )
+    registry.register(
+        action_name="predicated-good-action",
+        action=good,
+        addon_name="test",
+        enabled_when=lambda am: True,
+    )
+
+    _make_manager()._update_predicated_actions()
+    assert good.get_enabled() is True