Allow accelerators based on keyboard-types `Key`

[LeonMatthes]

Hi,
First of all, thank you all for developing and maintaining muda. We are happily using it in Slint for our native menu bars and context menus. 😊

Recently we ran into the limitation that muda only allows creating accelerators based on keyboard-types Code type, which doesn't allow expressing all keyboard shortcuts that Slint supports.
Concretely, the Code type is limited to physical key codes, even though accelerators usually use "logical" keys - e.g. the string that was produced by the keypress, not the physical key position.

That means that Muda currently does not support keyboard shortcuts like Control + +/Control + {/Control + €, etc.

I would like to contribute to muda to add support for these kinds of keyboard shortcuts.

In my opinion the right approach is to add support for the Key type from keyboard-types instead of the Code type.
Key is the right type because it represents a logical keypress that produced a certain string instead of a physical key position.

Ideally, I think accelerators should entirely migrate to using the Key type, but I can understand that for backwards compatibility it is probably better to continue offering the constructor that accepts a Code.

Would you consider merging a pull request that adds an additional constructor for accelerators that use the Key type?
Also, would you prefer keeping the existing Code constructor, or remove it entirely?

[amrbashir]

Thank you, and feel free to open a PR but let's keep the old constructor and implement a new one for Key.

[LeonMatthes]

Hi @amrbashir, thanks for the feedback.

Unfortunately, I found that switching the Accelerator type to use Key internally causes a breaking change, as Key is not Copy, so we would have to remove Copy from the Accelerator type. Overall, it still leads to a reasonably clean implementation, but I don't think it would be possible to avoid a breaking change with this approach.

Alternatively, we could introduce a second accelerator struct, maybe called KeyAccelerator, that holds the key internally. Then we can encourage people to use that over Accelerator and potentially deprecate Accelerator at some point.
This way we can entirely avoid breaking changes, but the API becomes a bit less ergonomic, as the Accelerator struct is still the default (i.e. new still takes Accelerator).

Internally we can convert the Accelerator to KeyAccelerator so that the platform implementations don't have to deal with the two types separately.

To evaluate both of these options, I generated initial implementations with Github Copilot (see the draft PRs that I've opened). You can skim those if you want. They are definitely not final quality, so don't take them too seriously.
I will of course manually go over them before asking for a proper review.

Overall, to avoid breaking changes I think going with the KeyAccelerator variant is the better option.
The Accelerator struct can then optionally be deprecated/removed in the future if it's worth doing so.

Let me know which option you prefer then I will continue with the implementation.

[LeonMatthes]

See #339 and #340 for prototype implementations.

[amrbashir]

Thank you, I prefer the KeyAccelerator approach as it is non breaking, and allows for gradual deprecation in the future.

[LeonMatthes]

Alright, then I'll work on cleaning up #339 👍

[LeonMatthes]

Hi @amrbashir, #339 should now be ready for review :)

[LeonMatthes]

I also tested this by integrating it in Slint and it's working well (on Windows at least):
slint-ui/slint#11363

Also tested in on Linux with Gtk and that seems to work fine as well.