Make registerShortcut from anki/shortcuts package override conflicting shortcuts

[kleinerpirat]

Sparked by:

• default shortcut conflict with anki built-in ones kleinerpirat/anki-tooltips#1

We discussed the Qt side in #2376 and came to the conclusion it's not worth the effort. But doing it on the JS side might be. This PR allows the following:

registerShortcut(
    (event: KeyboardEvent) => {
        // handle event
    },
    "Control+T",
    { event: "keydown", override: true},
);

As a result, all JS shortcuts (including emacs-style sequences) that conflict with the given keyCombination are disabled.

The example above would disable the LaTeX shortcuts:

• Control+T, T
• Control+T, E
• Control+T, M

---

While it is (almost) reasonable to let add-on authors disable QShortcuts themselves, it is currently impossible to do the same in JS - how would they access the remove callback of native Svelte components that do not expose it?

[kleinerpirat]

./ninja format and ./ninja check seem to have different opinions again.

[kleinerpirat]

An alternative to the override parameter would be to export a different function to the API than what our native components are using. Then add-on shortcuts would take precedence over native ones and existing add-ons would be fixed without extra effort on their side.

[hikaru-y]

At this time, there appear to be a few issues with this.

1. It is not possible to register multiple listeners to a single shortcut, which is currently possible.

   • An add-on may want to register multiple listeners with different event targets or different AddEventListenerOptions for the same shortcut.

2. If a <Shortcut> component is mounted after an add-on executes registerShortcut(), the shortcut key of that component cannot be overridden.

   1. Suppose an addon wants to override Ctrl + Shift + C.
   2. Open an editor with a non-Cloze note type, then change the note type to a Cloze one.
   3. Pressing that shortcut key will execute the original callback as well as the add-on's callback.

[kleinerpirat]

Thanks for testing this, Hikaru!

We could fix the first issue by simply not storing add-on shortcuts in the registeredShortcuts array. We should probably rename it to nativeShortcuts then.

It would of course have the drawback that conflicting add-ons do not disable their shortcuts, but that would be unfair anyway as it's a bit arbitrary which add-on loads first.

The second issue is more involved.. But I feel like that could be solved on the add-on side. I'm at work the next 10 hours, so I won't be able to fix this today @dae fixed the first issue.

[kleinerpirat]

@hikaru-y do you think it's reasonable to let add-on authors manually handle issue 2? I guess the other option is to store calls to require("anki/shortcuts").registerShortcut and reiterate the disabling process when the native registerShortcut is called later.

I'm thinking of something like this:

/**
 * Expose wrapper function which overrides conflicting native shortcuts
 */
registerPackage("anki/shortcuts", {
    registerShortcut: (
        callback: (event: KeyboardEvent) => void,
        keyCombinationString: string,
        restParams: Partial<RegisterShortcutRestParams> = defaultRegisterShortcutRestParams,
    ) => {
        removeConflictingShortcuts(keyCombinationString);
        const removeListener = registerShortcutInner(
            callback,
            keyCombinationString,
            restParams,
        );

        const shortcut = { key: keyCombinationString, remove: removeListener };
        externalShortcuts.push(shortcut);

        const remove = () => {
            externalShortcuts.forEach((item, i) => {
                if (item === shortcut) {
                    shortcut.remove()
                    externalShortcuts.splice(i, 1);
                }
            })
        };

        return remove;
    },
    getPlatformString,
});

Alright, really gotta go to work now :D

[dae]

(I'm tied up with AnkiWeb work at the moment, so I likely won't be able to get to the pending PRs until early next week I'm afraid)

[dae]

We've been focusing on how can we, but I'd like to take a step back and think about the should we first. From what I can tell, the example use case here is turning off the LaTeX shortcuts so that the add-on can use Ctrl+t instead. Do we really want to be encouraging add-ons to replace the default shortcuts with unrelated actions? This is potentially surprising/annoying for users, as it means they can no longer access built-in functionality easily when they install the add-on. Wouldn't it be better to pick an unused shortcut combination instead?

[dae]

(as an aside, I tried out your add-on for the first time today, and it's quite sleek)

[kleinerpirat]

If it were a fixed shortcut, I would simply fix the issue from the add-on side. But a recent update allows users to set their own shortcut for the tooltips.

So any time an add-on allows to set custom shortcuts, there will be trouble with conflicts. I wouldn't exactly call the PR "encouragement" to override native shortcuts, but I get why you're nervous about it.

Alternatively, we could provide a list of editor shortcuts for add-on authors to compare user input against and create an alert in case a user sets a conflicting one.

It's probably best to get some feedback on the issue from other devs.

[dae]

Ah, if the replacement is because the user deliberately chose a shortcut and not because it was the default, that makes more sense. One other option would be to tweak this code so that instead of disabling the existing shortcuts if found, it throws an error that the shortcut is already in use. The add-on could catch it, and report a message like "Ctrl+T is already used; please pick another shortcut". No strong feelings here. WDYT?

[dae]

This should not be necessary with the steps mentioned here: #2395 (comment)

[kleinerpirat]

Thanks!

[kleinerpirat]

Throwing an error is a good idea! However, if a user doesn't care about a particular native shortcut, they may still want to override it. I'll revisit this later.

[dae]

Cleaning up old PRs - please open a new one if you'd like to continue this.