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.

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.
  • Loading branch information
legoktm committed Sep 24, 2024
1 parent 0c1cc43 commit 28eb8b1
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 6 deletions.
8 changes: 8 additions & 0 deletions client/.semgrep/custom-rules.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
5 changes: 3 additions & 2 deletions client/securedrop_client/gui/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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)

Expand Down
5 changes: 3 additions & 2 deletions client/securedrop_client/gui/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down
11 changes: 11 additions & 0 deletions client/securedrop_client/gui/shortcuts.py
Original file line number Diff line number Diff line change
@@ -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
4 changes: 2 additions & 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 @@ -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
Expand Down Expand Up @@ -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.
Expand Down
11 changes: 11 additions & 0 deletions client/tests/gui/test_shortcuts.py
Original file line number Diff line number Diff line change
@@ -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"

0 comments on commit 28eb8b1

Please sign in to comment.