Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to the new winit keyboard API #6958

Merged
merged 7 commits into from
Jul 11, 2023

Conversation

kchibisov
Copy link
Member

@kchibisov kchibisov commented May 28, 2023

The main highlight of this update is that alacritty will now use new
keyboard API from the winit, which resolves a lot of issues around
key bindings, such as ability to bind dead keys. It also fixes long
standing issues with the virtual key code bindings and make bindings in
general more predictable. It also makes our default Vi key bindings
fully working.

Given that alacritty was using VirtualKey directly in the bindings
from the winit, and winit simply removed the enum, we've added internal
conversions to minimize the fallout, but new way to specify the bindings
should be more intuitive.

Other part of this update fixes some forward compatibility bugs with the
Wayland backend, given that wayland-rs 0.30 is fully forward compatible.
The update also fixes weird Maximized startup issues on GNOME Wayland,
however they were present on any sane compositor.

Fixes #6842.
Fixes #6455.
Fixes #6184.
Fixes #5684.
Fixes #3574.
Fixes #3460.
Fixes #1336.
Fixes #892.
Fixes #458.
Fixes #55.

@kchibisov
Copy link
Member Author

That's a reference what we get when pressing ctrl + shift + c.

[1.288686403s] [INFO ] [alacritty] winit event: WindowEvent { window_id: WindowId(WindowId(94882226519600)), event: KeyboardInput { device_id: DeviceId(Wayland(DeviceId)), event: KeyEvent { physical_key: KeyI, logical_key: Character("C"), text: Some("C"), location: Standard, state: Pressed, repeat: false, platform_specific: KeyEventExtra { key_without_modifiers: Character("c"), text_with_all_modifiers: Some("\u{3}") } }, is_synthetic: false } }
[1.349699787s] [INFO ] [alacritty] winit event: WindowEvent { window_id: WindowId(WindowId(94882226519600)), event: KeyboardInput { device_id: DeviceId(Wayland(DeviceId)), event: KeyEvent { physical_key: KeyI, logical_key: Character("C"), text: Some("C"), location: Standard, state: Released, repeat: false, platform_specific: KeyEventExtra { key_without_modifiers: Character("c"), text_with_all_modifiers: Some("\u{3}") } }, is_synthetic: false } }

@kchibisov kchibisov force-pushed the winit-update branch 3 times, most recently from ec05409 to 1d08a4c Compare May 29, 2023 10:49
@kchibisov
Copy link
Member Author

This branch should sort of work now, except for macOS users given that OptionAsAlt is not ported yet.

@kchibisov
Copy link
Member Author

As it was discussed on IRC, I've added transition from old winit keys into their modern counterparts.

@kchibisov
Copy link
Member Author

I left all complexity wrt char: bindings and adding support for direct key code references out of this PR for now, since it's not yet clear whether we really want them.

@kchibisov kchibisov force-pushed the winit-update branch 2 times, most recently from 94318da to 55bcb00 Compare July 1, 2023 15:01
@kchibisov
Copy link
Member Author

The one more question is what should we do with the Numpad bindings, we had them before, but with the current winit, they are not encoded into the virtual key code directly, but rather reside in the position option.

We still have information whether a binding was from a numpad or not, but it could require config changes, such as adding an implicit field default to None or Standard.

We could also drop support for old numpad keys in the config and solely rely on our new keys, that would require users to delete a few lines, but I don't think that's an issue. I'd prefer this variant btw.

@afh
Copy link
Contributor

afh commented Jul 3, 2023

For what it's worth alacritty has seen config changes in the past that required users to make modifications to their config.

@kchibisov could you give an example of a change that a user would need to make to replace the old numpad keys with the new keys in their config?

@kchibisov
Copy link
Member Author

@afh there's no difference between them and normal keys now. Unless you manually define special set of bindings just for numpad, which differ from regular keys there shouldn't be an issue.

So all NumpadSubtract/Add are now just -,+, etc.

@chrisduerr
Copy link
Member

@perfbot (just testing stuff)

@afh
Copy link
Contributor

afh commented Jul 3, 2023

Thanks for clarifying, @kchibisov. Personally I do not use NumPad keys specifically, yet I could see someone wanting to do that. Would it be possible and useful to introduce a Numpad modifier to Control, Alt, or Shift when specifying a key binding?

For example:

key_bindings:
   { key: Minus, mods: Numpad, action: DecreaseFontSize }

@perfbot
Copy link

perfbot commented Jul 3, 2023

results

@kchibisov
Copy link
Member Author

It's just not clear how default bindings should work then, should we separately define numpad bindings or not, because it affects all the keys. What we get now from the winit is

Key { code: Character("-"), position: Numpad }
Key { code: Character("-"), position: Standard }

We could add implicit Standard field to all the bindings, and translate Numpad to Numpad bindings, and still have the bindings separate... That could work, but not sure how convenient it could be.

@kchibisov
Copy link
Member Author

The numpad thing in the config should be resolved now.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
extra/man/alacritty.5.scd Outdated Show resolved Hide resolved
extra/man/alacritty.5.scd Outdated Show resolved Hide resolved
extra/man/alacritty-bindings.5.scd Outdated Show resolved Hide resolved
alacritty/src/input.rs Outdated Show resolved Hide resolved
alacritty/src/input.rs Show resolved Hide resolved
alacritty/src/config/bindings.rs Outdated Show resolved Hide resolved
alacritty/src/config/bindings.rs Outdated Show resolved Hide resolved
alacritty/src/config/bindings.rs Outdated Show resolved Hide resolved
The main highlight of this update is that alacritty will now use new
keyboard API from the winit, which resolves a lot of issues around
key bindings, such as ability to bind dead keys. It also fixes long
standing issues with the virtual key code bindings and make bindings
in general more predictable. It also makes our default Vi key bindings
fully working.

Given that alacritty was using `VirtualKey` directly in the bindings
from the winit, and winit simply removed the enum, we've added internal
conversions to minimize the fallout, but new way to specify the bindings
should be more intuitive.

Other part of this update fixes some forward compatibility bugs with the
Wayland backend, given that wayland-rs 0.30 is fully forward compatible.
The update also fixes weird Maximized startup issues on GNOME Wayland,
however they were present on any sane compositor.

Fixes alacritty#6842.
Fixes alacritty#6455.
Fixes alacritty#6184.
Fixes alacritty#5684.
Fixes alacritty#3574.
Fixes alacritty#3460.
Fixes alacritty#1336.
Fixes alacritty#892.
Fixes alacritty#458.
Fixes alacritty#55.
extra/man/alacritty.5.scd Outdated Show resolved Hide resolved
extra/man/alacritty.5.scd Outdated Show resolved Hide resolved
extra/man/alacritty.5.scd Outdated Show resolved Hide resolved
kchibisov and others added 2 commits July 11, 2023 00:50
Co-authored-by: Christian Duerr <contact@christianduerr.com>
Co-authored-by: Christian Duerr <contact@christianduerr.com>
@kchibisov kchibisov merged commit db90350 into alacritty:master Jul 11, 2023
5 checks passed
@kchibisov kchibisov deleted the winit-update branch July 11, 2023 02:22
@afh
Copy link
Contributor

afh commented Jul 11, 2023

Thank you so much for working on this, @kchibisov, and keep up the great work!! 🎉 I'm happy to see this merged and look forward to the next alacritty release (candidate) as it should resolve a long standing issue I had with alacritty on OpenBSD (see rust-windowing/winit#2032).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment