Skip to content

Commit

Permalink
Verify client shortcuts are unique
Browse files Browse the repository at this point in the history
If you have two shortcuts that point to the same key combo, you'll get
"QAction::event: Ambiguous shortcut overload: <sequence>" in the logs
and neither action will be taken.

Add a test that scans the whole codebase to verify the argument to
setShortcut() is globally unique.

To make the test simple to write, standardize on a single way of writing
shortcuts, by adding Qt constants. A semgrep rule tries to guide people
into writing shortcuts in the same style, so the test can match them.
  • Loading branch information
legoktm committed Sep 6, 2024
1 parent b4d8ebf commit aaf3230
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 4 deletions.
7 changes: 7 additions & 0 deletions client/.semgrep/custom-rules.yml
Original file line number Diff line number Diff line change
Expand Up @@ -180,3 +180,10 @@ rules:
# Forbid any other use of assert
- pattern-regex: |
assert.*
- id: enforce-setshortcut-style
patterns:
- pattern-regex: setShortcut\(QKeySequence(.*?)\)
message: "Use `Qt.* + Qt.*` style for setShortcut calls instead of QKeySequence (for test_shortcuts_are_unique test)"
languages: [python]
severity: WARNING
4 changes: 2 additions & 2 deletions client/securedrop_client/gui/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from gettext import gettext as _

from PyQt5.QtCore import Qt
from PyQt5.QtGui import QClipboard, QGuiApplication, QIcon, QKeySequence
from PyQt5.QtGui import QClipboard, QGuiApplication, QIcon
from PyQt5.QtWidgets import QAction, QApplication, QHBoxLayout, QMainWindow, QVBoxLayout, QWidget

from securedrop_client import __version__, state
Expand Down Expand Up @@ -93,7 +93,7 @@ def __init__(
# Actions
quit = QAction(_("Quit"), self)
quit.setIcon(QIcon.fromTheme("application-exit"))
quit.setShortcut(QKeySequence.Quit)
quit.setShortcut(Qt.CTRL + Qt.Key_Q)
quit.triggered.connect(self.close)
self.addAction(quit)

Expand Down
3 changes: 1 addition & 2 deletions client/securedrop_client/gui/widgets.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
QFocusEvent,
QFont,
QIcon,
QKeySequence,
QLinearGradient,
QMouseEvent,
QPalette,
Expand Down Expand Up @@ -3149,7 +3148,7 @@ def __init__(self, source: Source, controller: Controller) -> None:
send_button_icon.addPixmap(load_image("send-disabled.svg"), QIcon.Disabled)
self.send_button.setIcon(send_button_icon)
self.send_button.setIconSize(QSize(56, 47))
self.send_button.setShortcut(QKeySequence("Ctrl+Return"))
self.send_button.setShortcut(Qt.CTRL + Qt.Key_Return)
self.send_button.setDefault(True)

# Set cursor.
Expand Down
24 changes: 24 additions & 0 deletions client/tests/gui/test_shortcuts.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import re
from collections import defaultdict
from pathlib import Path


def test_shortcuts_are_unique(mocker):
"""
Look through the codebase for setShortcut() calls,
verify the argument passed is unique
The `enforce-setshortcut-style` semgrep rule enforces a consistent style
"""
root = Path(__file__).parent.parent.parent / "securedrop_client"
shortcuts = defaultdict(int)
for path in root.glob("**/*.py"):
for match_ in re.findall(r"setShortcut\((.*)\)", path.read_text()):
shortcuts[match_] += 1

# Verify that the parsing actually found stuff
assert "Qt.CTRL + Qt.Key_Q" in shortcuts
# Look for duplicates
for shortcut, instances in shortcuts.items():
if instances > 1:
raise AssertionError(f"Shortcut {shortcut} is used {instances} times")

0 comments on commit aaf3230

Please sign in to comment.