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 winit #108

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Update winit #108

merged 2 commits into from
Nov 6, 2023

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Oct 22, 2023

No description provided.

Copy link
Member

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple general comments. I didn't realize that this release of winit was live yet!

Comment on lines -210 to -215
Event::WindowEvent {
event: WindowEvent::ScaleFactorChanged { new_inner_size, .. },
..
} => {
self.resize(**new_inner_size);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure we continue handling this event since it's important for correct display-switching support on Windows.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

winit no longer exposes a new size value in this event. I presumed that the actual resize event covers it. Is there additional work needed?

use yakui_core::input::{KeyCode, Modifiers};

pub fn from_winit_key(key: VirtualKeyCode) -> Option<KeyCode> {
pub fn from_winit_key(key: winit::keyboard::KeyCode) -> Option<KeyCode> {
use winit::keyboard::KeyCode as WinitKey;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is a change in behavior.

  • winit's VirtualKeyCode became Key, referring to a key that should have the given text on its keycap.
  • winit's ScanCode became KeyCode, used here, which refers to a physical location of a key based on its label on a US keyboard.

For what yakui does, I think that Key is the more appropriate default. Using an example of a key combo we don't actually currently have, pressing ctrl-a/cmd-a should select all text, regardless of where a is on the user's keyboard. However, using KeyCode and physical keys, that would mean that users with the AZERTY layout would instead press ctrl-q/cmd-q to select all text.

I think that would be odd, but I'm interested to hear your opinion on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My selection here was motivated by the documentation of keyboard_types::KeyCode:

/// Code is the physical position of a key.
///
/// The names are based on the US keyboard. If the key
/// is not present on US keyboards a name from another
/// layout is used.
///
/// Specification:
/// <https://w3c.github.io/uievents-code/>

Compare winit 0.29's KeyCode:

Code representing the location of a physical key

This mostly conforms to the UI Events Specification’s [KeyboardEvent.code](https://w3c.github.io/uievents-code/#code-value-tables) with a few exceptions

These types appear to directly correspond.

I agree that being able to specify bindings in terms of key labels is desirable, and that physical key identifiers probably don't accomplish that. Should yakui be using keyboard_events::Key instead/additionally? If so, perhaps that should be pursued in a separate change; as written this seems to be most consistent with the documented behavior.

@LPGhatguy LPGhatguy merged commit 05cb678 into SecondHalfGames:main Nov 6, 2023
2 checks passed
@LPGhatguy
Copy link
Member

Merging this and working on iterating a bit on main!

@Ralith Ralith deleted the update-winit branch November 6, 2023 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants