Skip to content
28 changes: 23 additions & 5 deletions src/app_model/backends/qt/_qaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
from typing import TYPE_CHECKING, ClassVar
from weakref import WeakValueDictionary

from qtpy.QtGui import QKeySequence

from app_model import Application
from app_model.expressions import Expr
from app_model.types import ToggleRule
Expand Down Expand Up @@ -52,6 +54,15 @@ def __init__(
self._keybinding_tooltip = f"({kb.keybinding.to_text()})"
self.triggered.connect(self._on_triggered)

def _update_keybinding(self) -> None:
shortcut = self.shortcut()
if kb := self._app.keybindings.get_keybinding(self._command_id):
self.setShortcut(QKeyBindingSequence(kb.keybinding))
self._keybinding_tooltip = f"({kb.keybinding.to_text()})"
Comment on lines +59 to +61
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code is more or less the same as what's happening in the init at line 52. Can we maybe avoid that duplication by having this be the one place where that logic is held, and call it in init?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is with tooltip. If we call this method from line 52, then, for QCommandRuleAction, the self._tooltip will be undefined on call of _update_keybinding

I think that we may fix this using metaclass and add __post_init__ where _update_keybinding will be called.

elif not shortcut.isEmpty():
self.setShortcut(QKeySequence())
self._keybinding_tooltip = ""

def _on_triggered(self, checked: bool) -> None:
# execute_command returns a Future, for the sake of eventually being
# asynchronous without breaking the API. For now, we call result()
Expand Down Expand Up @@ -84,23 +95,29 @@ def __init__(
) -> None:
super().__init__(command_rule.id, app, parent)
self._cmd_rule = command_rule
self._tooltip = command_rule.tooltip or ""
if use_short_title and command_rule.short_title:
self.setText(command_rule.short_title) # pragma: no cover
else:
self.setText(command_rule.title)
if command_rule.icon:
self.setIcon(to_qicon(command_rule.icon))
self.setIconVisibleInMenu(command_rule.icon_visible_in_menu)
if command_rule.tooltip:
self.setToolTip(command_rule.tooltip)
if command_rule.status_tip:
self.setStatusTip(command_rule.status_tip)
if command_rule.toggled is not None:
self.setCheckable(True)
self._refresh()
tooltip_with_keybinding = (
f"{self.toolTip()} {self._keybinding_tooltip}".rstrip()
)
tooltip_with_keybinding = f"{self._tooltip} {self._keybinding_tooltip}".rstrip()
self.setToolTip(tooltip_with_keybinding)

def setText(self, text: str | None) -> None:
super().setText(text)
self._tooltip = self._tooltip or text or ""

def _update_keybinding(self) -> None:
super()._update_keybinding()
tooltip_with_keybinding = f"{self._tooltip} {self._keybinding_tooltip}".rstrip()
self.setToolTip(tooltip_with_keybinding)

def update_from_context(self, ctx: Mapping[str, object]) -> None:
Expand Down Expand Up @@ -176,6 +193,7 @@ def create(
cache_key = QMenuItemAction._cache_key(app, menu_item)
if cache_key in cls._cache:
res = cls._cache[cache_key]
res._update_keybinding()
res.setParent(parent)
return res

Expand Down
7 changes: 3 additions & 4 deletions src/app_model/registries/_keybindings_reg.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,10 +194,9 @@ def __repr__(self) -> str:
def get_keybinding(self, command_id: str) -> _RegisteredKeyBinding | None:
"""Return the first keybinding that matches the given command ID."""
# TODO: improve me.
return next(
(entry for entry in self._keybindings if entry.command_id == command_id),
None,
)
matches = (kb for kb in self._keybindings if kb.command_id == command_id)
sorted_matches = sorted(matches, key=lambda x: x.source, reverse=True)
return next(iter(sorted_matches), None)

def get_context_prioritized_keybinding(
self, key: int, context: Mapping[str, object]
Expand Down
45 changes: 41 additions & 4 deletions tests/test_qt/test_qactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
Action,
CommandRule,
KeyBindingRule,
KeyBindingSource,
KeyCode,
MenuItem,
ToggleRule,
Expand All @@ -18,7 +19,8 @@
from conftest import FullApp


def test_cache_qaction(qapp, full_app: "FullApp") -> None:
@pytest.mark.usefixtures("qapp")
def test_cache_qaction(full_app: "FullApp") -> None:
action = next(
i for k, items in full_app.menus for i in items if isinstance(i, MenuItem)
)
Expand All @@ -28,7 +30,8 @@ def test_cache_qaction(qapp, full_app: "FullApp") -> None:
assert repr(a1).startswith("QMenuItemAction")


def test_toggle_qaction(qapp, simple_app: "Application") -> None:
@pytest.mark.usefixtures("qapp")
def test_toggle_qaction(simple_app: "Application") -> None:
mock = Mock()
x = False

Expand Down Expand Up @@ -75,6 +78,7 @@ def test_icon_visible_in_menu(qapp, simple_app: "Application") -> None:
assert not q_action.isIconVisibleInMenu()


@pytest.mark.usefixtures("qapp")
@pytest.mark.parametrize(
("tooltip", "expected_tooltip"),
[
Expand All @@ -83,7 +87,7 @@ def test_icon_visible_in_menu(qapp, simple_app: "Application") -> None:
],
)
def test_tooltip(
qapp, simple_app: "Application", tooltip: str, expected_tooltip: str
simple_app: "Application", tooltip: str, expected_tooltip: str
) -> None:
action = Action(
id="test.tooltip", title="Test tooltip", tooltip=tooltip, callback=lambda: None
Expand All @@ -93,6 +97,7 @@ def test_tooltip(
assert q_action.toolTip() == expected_tooltip


@pytest.mark.usefixtures("qapp")
@pytest.mark.parametrize(
("tooltip", "tooltip_with_keybinding", "tooltip_without_keybinding"),
[
Expand All @@ -105,7 +110,6 @@ def test_tooltip(
],
)
def test_keybinding_in_tooltip(
qapp,
simple_app: "Application",
tooltip: str,
tooltip_with_keybinding: str,
Expand All @@ -127,3 +131,36 @@ def test_keybinding_in_tooltip(
# check setting tooltip manually removes keybinding info
q_action.setToolTip(tooltip)
assert q_action.toolTip() == tooltip_without_keybinding


@pytest.mark.usefixtures("qapp")
def test_update_keybinding_in_tooltip(
simple_app: "Application",
) -> None:
action = Action(
id="test.update.keybinding.tooltip",
title="Test update keybinding tooltip",
callback=lambda: None,
tooltip="Initial tooltip",
keybindings=[KeyBindingRule(primary=KeyCode.KeyK)],
)
dispose1 = simple_app.register_action(action)

q_action = QCommandRuleAction(action, simple_app)
assert q_action.toolTip() == "Initial tooltip (K)"

# Update the keybinding
dispose2 = simple_app.keybindings.register_keybinding_rule(
"test.update.keybinding.tooltip",
KeyBindingRule(primary=KeyCode.KeyL, source=KeyBindingSource.USER),
)
q_action._update_keybinding()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little surprised to see this private method called directly in tests.
Whose job is it ultimately to be calling _update_keybinding()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it is called on a rebuild of the menu. app model is not evented. I do not yet identified other places.
But maybe it should be a public method?

assert q_action.toolTip() == "Initial tooltip (L)"

dispose2()
q_action._update_keybinding()
assert q_action.toolTip() == "Initial tooltip (K)"

dispose1()
q_action._update_keybinding()
assert q_action.toolTip() == "Initial tooltip"
Loading