Keybinding code unification #3

[SimonBrandner]

Type: task
Requires element-hq/element-web#21132

---

This PR does a few things:

1. Fixes the call shortcuts to be handled by the KeybindingManager for consistency and possible customizability in the future
2. Moves handling of desktop-only shortcuts (go back/forward, go to space by number...) to the react-sdk. This puts all the keybinding definitions into one place. That makes them easier to work with and will also serve when we want to add customizable keybindings.
3. Does some refactoring to how the getter functions in KeyboardShorctus.ts work
4. Updates and adds a few tests

---

This change is marked as an internal change (Task), so will not be included in the changelog.

Preview: https://pr7850--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

[andybalaam]

Hi @SimonBrandner , I'd like to review this, but I'm struggling to understand it fully. Please could explain what it's for and what changes when we merge it?

Also, I see that element-web/pull/221132 and this depend on each other - would they need to be released simultaneously, or could we live with a hour or two of only one of these being merged? (If so, which should be merged first? If not, is there a way we can restructure the change so this is possible?)

[t3chguy]

(If so, which should be merged first? If not, is there a way we can restructure the change so this is possible?)

Typically with a breaking change refactor, e.g one which changes signatures or enum members, they must land at the same time.

[SimonBrandner]

Hi @SimonBrandner , I'd like to review this, but I'm struggling to understand it fully. Please could explain what it's for and what changes when we merge it?

Yes, sorry for not documenting this in the PR description. I was partially expecting someone with more context to review this but it might not be exactly clear to them either and it's always good to have documentation - I've updated the PR description.

Also, I see that element-web/pull/221132 and this depend on each other - would they need to be released simultaneously, or could we live with a hour or two of only one of these being merged? (If so, which should be merged first? If not, is there a way we can restructure the change so this is possible?)

As Micheal said, both need to be merged at the same time otherwise stuff will go awry

[andybalaam]

Looks good, but I'm not particularly confident about my review, so I'll leave some time a) for the release to be done and b) for someone else to review if they get a chance.

[andybalaam]

Is this TODO now done?

[SimonBrandner]

No, not really, hopefully the next PR will cover that

[andybalaam]

OK, let's go for it!