require("anki/shortcuts").registerShortcut doesn't disable QShortcuts (but probably should)

[kleinerpirat]

Anki exports the anki/shortcuts package to set a user-defined keyboard shortcut in webviews. Currently such shortcuts do not prevent QShortcuts registered on the editor from firing, so keys like Esc or Ctrl+Enter (or my default shortcut for Anki Tooltips) cannot be used without unwanted side effects like closing the window, adding a new note etc.

Proposal

We could send a

bridgeCommand(`registerShortcut:${keyCombinationString}`);

on registerShortcut from shortcuts.ts to AnkiWebView and setEnabled(False) on any conflicting shortcut of the parent widget.

(This is an over-simplification. In reality it would be a bit more involved, as we're currently assigning shortcuts to individual widgets, not the QMainWindow/QDialog.)

Add-ons already have the ability to override shortcuts in Python, so it seems reasonable to allow it via JS too.

[dae]

If an add-on author adds a Qt shortcut that conflicts with an existing one, both shortcuts fail to work, so it's up to the add-on author to disable the existing one first. So isn't the situation the same for JS? Add-on authors can disable the existing Qt shortcut if required.

[kleinerpirat]

So isn't the situation the same for JS?

Not really. If an add-on sets a shortcut via the JS API, the Qt shortcut continues to work.

If an add-on author allows users to set a custom shortcut, he'd have to know all shortcuts and which widget of each window they're set to and then provide a complex method to handle all cases. It's not like all shortcuts are set to the QMainWindow or QDialog: a lot of them are set on subwidgets.

I suggest cleaning house and creating a standard way to register shortcuts. E.g. subclass QMainWindow to AnkiWindow and QDialog to AnkiDialog, then give those subclasses a ShortcutHandler with registerShortcut and unregisterShortcut as well as a setShortcutEnabled method for temporary suppression. Then move all subwidget shortcuts to the parent window.

It would also make the code cleaner:

-        self.compat_add_shorcut = QShortcut(QKeySequence("Ctrl+Enter"), self)
-        qconnect(self.compat_add_shorcut.activated, self.addButton.click)
+        self.registerShortcut("Ctrl+Enter", self.addButton.click)

Another advantage would be the ability to provide feedback if a shortcut would conflict with an existing one. We could also create nice overviews of all registered shortcuts from the dictionary entries of ShortcutHandler.

[dae]

So in the JS case we have shortcuts being triggered twice, instead of not at all. It's different, but in both cases it's broken :-)

To be clear, I'm not saying what you propose is a bad idea per se. The problem is that we have lots of good ideas, and only limited resources (both for the person writing the PR, and also for me to review them, and for us to deal with any follow-up issues that are accidentally introduced, and possible follow-ups/scope creep).

Personally, I am rather concerned about the state of Qt. Pretty much every day on the forums we have posts from users who are either getting graphical glitches, crashes, or who are even unable to open the app at all, and all of those issues appear to be Qt related. We're not going to be able to drop Qt any time soon, but I'd at least like to see us edging closer towards being less dependent on it. PRs that make non-trivial changes to the Qt parts of the codebase feel a bit like a step in the wrong direction, even if they may be useful features in their own right.

[kleinerpirat]

I hear you. Then I'll try to create a good method to disable Qt shortcuts from the add-on side and share it with the community. Ultimately what matters is that people who follow the New Format Pack way of attaching Svelte buttons do not encounter unexpected issues.

[dae]

Thanks Matthias :-)