From 28eb8b1e0fc5f833cfa50f3b974551d39d1d6c18 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. Centralize them in one place so it's easy for humans to spot duplicates. A test verifies uniqueness as well. A semgrep rule enforces that people use the new Shortcuts enum instead of just writing the sequence in the function call directly. --- client/.semgrep/custom-rules.yml | 8 ++++++++ client/securedrop_client/gui/actions.py | 5 +++-- client/securedrop_client/gui/main.py | 5 +++-- client/securedrop_client/gui/shortcuts.py | 11 +++++++++++ client/securedrop_client/gui/widgets.py | 4 ++-- client/tests/gui/test_shortcuts.py | 11 +++++++++++ 6 files changed, 38 insertions(+), 6 deletions(-) create mode 100644 client/securedrop_client/gui/shortcuts.py 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..35eb70a11 100644 --- a/client/.semgrep/custom-rules.yml +++ b/client/.semgrep/custom-rules.yml @@ -180,3 +180,11 @@ rules: # Forbid any other use of assert - pattern-regex: | assert.* + +- id: enforce-setshortcut-style + patterns: + - pattern-not-regex: setShortcut\(Shortcuts\.(.*?)\) + - pattern-regex: setShortcut\((.*?)\) + message: "Use Shortcuts enum to register shortcuts" + languages: [python] + severity: WARNING diff --git a/client/securedrop_client/gui/actions.py b/client/securedrop_client/gui/actions.py index 7b02262fd..0f3088cbe 100644 --- a/client/securedrop_client/gui/actions.py +++ b/client/securedrop_client/gui/actions.py @@ -10,7 +10,7 @@ from gettext import gettext as _ from pathlib import Path -from PyQt5.QtCore import Qt, pyqtSlot +from PyQt5.QtCore import pyqtSlot from PyQt5.QtWidgets import QAction, QApplication, QDialog, QMenu from securedrop_client import state @@ -20,6 +20,7 @@ from securedrop_client.gui.base import ModalDialog from securedrop_client.gui.conversation import PrintDialog from securedrop_client.gui.conversation.export import ExportWizard +from securedrop_client.gui.shortcuts import Shortcuts from securedrop_client.logic import Controller from securedrop_client.utils import safe_mkdir @@ -36,7 +37,7 @@ def __init__( self._state = app_state self._text = _("Download All") super().__init__(self._text, parent) - self.setShortcut(Qt.CTRL + Qt.Key_D) + self.setShortcut(Shortcuts.DOWNLOAD_CONVERSATION.value) self.triggered.connect(self.on_triggered) self.setShortcutVisibleInContextMenu(True) diff --git a/client/securedrop_client/gui/main.py b/client/securedrop_client/gui/main.py index 97e7c0fd6..130e15146 100644 --- a/client/securedrop_client/gui/main.py +++ b/client/securedrop_client/gui/main.py @@ -22,12 +22,13 @@ 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 from securedrop_client.db import Source, User from securedrop_client.gui.auth import LoginDialog +from securedrop_client.gui.shortcuts import Shortcuts from securedrop_client.gui.widgets import BottomPane, LeftPane, MainView from securedrop_client.logic import Controller from securedrop_client.resources import load_all_fonts, load_css, load_icon @@ -93,7 +94,7 @@ def __init__( # Actions quit = QAction(_("Quit"), self) quit.setIcon(QIcon.fromTheme("application-exit")) - quit.setShortcut(QKeySequence.Quit) + quit.setShortcut(Shortcuts.QUIT.value) quit.triggered.connect(self.close) self.addAction(quit) diff --git a/client/securedrop_client/gui/shortcuts.py b/client/securedrop_client/gui/shortcuts.py new file mode 100644 index 000000000..9998a1187 --- /dev/null +++ b/client/securedrop_client/gui/shortcuts.py @@ -0,0 +1,11 @@ +from enum import Enum + +from PyQt5.QtCore import Qt + + +class Shortcuts(Enum): + """Central listing of all keyboard shortcuts""" + + DOWNLOAD_CONVERSATION = Qt.CTRL + Qt.Key_D + QUIT = Qt.CTRL + Qt.Key_Q # Same as QKeySequence.Quit + SEND = Qt.CTRL + Qt.Key_Return diff --git a/client/securedrop_client/gui/widgets.py b/client/securedrop_client/gui/widgets.py index 5421e67ad..74a486524 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, @@ -84,6 +83,7 @@ from securedrop_client.gui.base import SecureQLabel, SvgLabel, SvgPushButton, SvgToggleButton from securedrop_client.gui.conversation import DeleteConversationDialog from securedrop_client.gui.datetime_helpers import format_datetime_local +from securedrop_client.gui.shortcuts import Shortcuts from securedrop_client.gui.source import DeleteSourceDialog from securedrop_client.logic import Controller from securedrop_client.resources import load_css, load_icon, load_image, load_movie @@ -3149,7 +3149,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(Shortcuts.SEND.value) 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..16af6ab77 --- /dev/null +++ b/client/tests/gui/test_shortcuts.py @@ -0,0 +1,11 @@ +from securedrop_client.gui.shortcuts import Shortcuts + + +def test_all_values_are_unique(): + # Get all the values in the enum + enum_values = [] + for value in Shortcuts.__members__.values(): + enum_values.append(value.value) + + # Check that all values are unique + assert len(enum_values) == len(set(enum_values)), "Enum values are not unique"