Verify client shortcuts are unique

[legoktm]

Status

Ready for review / Work in progress

Description

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.

Test Plan

• CI passes
• Change the Ctrl +D shortcut to be Ctrl +Q, verify the test fails
• Try writing a shortcut as a QKeySequence, semgrep-local should fail

Checklist

• These changes should not need testing in Qubes
• No update to the AppArmor profile is required for these changes
• No database schema changes are needed

[legoktm]

Nerd sniped by @cfm's suggestion at https://github.com/freedomofpress/securedrop-client/wiki/Keyboard-Shortcuts

This test is a bit fragile, e.g. if you write setShortcut(Qt.CTRL+Qt.Key_Q) (no whitespace) it wouldn't catch the duplicate, though ruff would hopefully format it. If the setShortcut() call goes over multiple lines, also wouldn't work.

Two other options:

• Use ast for the parsing, fixes whitespace and formatting gotchas
• eval the argument to setShortcut(), call .toString() on it, and then verify against that. That would also let us use QKeySequence, I think. And this could still be done with regex.

Cory's part two was to have us auto-generate a SHORTCUTS.md file, with documentation for each one. I think this is doable, we'd just want some convention of requiring a documentation line right before the setShortcut() invocation.

[cfm]

No good deed goes unpunished. Forgive me, @legoktm.

In all seriousness: The test plan checks out, and this adds a useful dimension to our test coverage, which we can refine as we expand our use and understanding of keyboard shortcuts in Qt. The discussion I've started inline should be resolvable with a comment, and then I say let's keep it!

[cfm]

I didn't go into this level of detail in https://github.com/freedomofpress/securedrop-client/wiki/Keyboard-Shortcuts, but it's worth noting that this is not a strict requirement, just a policy we could adopt (and use this test to help enforce). Here's what I've realized after sleeping on it:

Shortcuts don't have to be unique globally always; they have to be unique per context per point in time. So this is a valid counterexample across times:

Time	Context	Shortcut	Action
1	WindowShortcut (default)	Ctrl + Q	Quit
2	WindowShortcut (default)	Ctrl + Q	File bug report

Now, this would be a terrible UX, which this test helps prevent. But here's another valid (and more realistic) counterexample across contexts and widgets:

Time	Context	Shortcut	Action
1	WidgetShortcut for ReplyBox 1	Ctrl + Enter	Send from ReplyBox 1
1	WidgetShortcut for ReplyBox 2	Ctrl + Enter	Send from ReplyBox 21

Again, this is valid! The shortcuts don't collide in the UI because they're scoped to the right context. But they also don't collide in the code, and wouldn't be caught by this test, because they're only defined once, on the same widget class before it's instantiated multiple times. With the WindowShortcut context, they would collide, and Qt would complain about them, but they still wouldn't be caught by this test.

So a rigorous test would have to search for duplicate (shortcut, context) pairs² for all combinations of widgets actually displayed in a program run. That's at best a state-space explosion and at worst the halting problem: either way, infeasible.

In conclusion: I still think this test is a good linting measure; it's just both (a) not a complete check and (b) quite nuanced in what it does check. So let's just document that:

Suggested change

verify the argument passed is unique

verify the argument passed is unique. This won't catch collisions between instances of a class that calls `setShortcut()` without also adjusting `setShortcutContext()`. It also enforces a policy that we don't want to reuse the same global shortcut for different purposes (or even with different handlers) at different times. Either of these limitations may be worth revisiting for UX reasons in the future.

Footnotes

1. In some application that displays multiple instances of the same widget at one time. Which we do, just not ReplyBoxes. ↩

2. Actually duplicate shortcuts down the ShortcutContext hierarchy, but I digress.... ↩

[legoktm]

So a rigorous test would have to search for duplicate (shortcut, context) pairs2 for all combinations of widgets actually displayed in a program run. That's at best a state-space explosion and at worst the halting problem: either way, infeasible.

Yeah. I wonder if we can add a hook to QAction::event: Ambiguous shortcut overload that causes tests to fail, but from what I can tell, it only actual triggers when you press the shortcut, so we'd need tests that actually test the shortcuts...

[cfm]

This means giving up QKeySequence's cross-platform defaults, although QKeySequence.Quit seems to resolve to Ctrl + Q everywhere in practice....

[legoktm]

I wonder if the simpler method for this is:

class Shortcuts:
    QUIT = QKeySequence.Quit
    DOWNLOAD_ALL = Qt.CTRL + Qt.Key_D
    ...

Then it is trivial to verify there are no overlaps both by humans visually and a test that enumerates through them. And then a semgrep rule that verifies all setShortcut class use the Shortcuts class.

And then our documentation generator just needs to look at one file in a very specific format instead of scanning through the entire codebase.

[cfm]

I like that approach for checking that our intentions don't collide, and I'm happy to do a pass on that tomorrow just to close out the nerd-swipe. :-) Per #2209 (comment), I still don't think we can do much to check that our instance-level assignments don't collide at runtime, but let's not block on that.