From 0f3b98f2279825ccd169ad36ed3a4d2b69f3c91a Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Fri, 6 Sep 2024 12:36:25 -0400 Subject: [PATCH] Verify client shortcuts are unique If you have two shortcuts that point to the same key combo, you'll get "QAction::event: Ambiguous shortcut overload: " 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. --- client/.semgrep/custom-rules.yml | 7 +++++++ client/securedrop_client/gui/main.py | 4 ++-- client/securedrop_client/gui/widgets.py | 3 +-- client/tests/gui/test_shortcuts.py | 24 ++++++++++++++++++++++++ 4 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 client/tests/gui/test_shortcuts.py diff --git a/client/.semgrep/custom-rules.yml b/client/.semgrep/custom-rules.yml index 0385449c8..0b2427285 100644 --- a/client/.semgrep/custom-rules.yml +++ b/client/.semgrep/custom-rules.yml @@ -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 diff --git a/client/securedrop_client/gui/main.py b/client/securedrop_client/gui/main.py index 537aea87e..e10a789ac 100644 --- a/client/securedrop_client/gui/main.py +++ b/client/securedrop_client/gui/main.py @@ -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 @@ -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) diff --git a/client/securedrop_client/gui/widgets.py b/client/securedrop_client/gui/widgets.py index 07c9949e1..db0be97f4 100644 --- a/client/securedrop_client/gui/widgets.py +++ b/client/securedrop_client/gui/widgets.py @@ -36,7 +36,6 @@ QFocusEvent, QFont, QIcon, - QKeySequence, QLinearGradient, QMouseEvent, QPalette, @@ -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. diff --git a/client/tests/gui/test_shortcuts.py b/client/tests/gui/test_shortcuts.py new file mode 100644 index 000000000..7828a65f1 --- /dev/null +++ b/client/tests/gui/test_shortcuts.py @@ -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")