New keyboard API for Windows

[ArturKovacs]

• Tested on all platforms changed
• Compilation warnings were addressed
• cargo fmt has been run on this branch
• cargo doc builds successfully
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality
• Updated feature matrix, if new features were added or implemented

This contains the Windows implementation of the keyboard API discussed at #753

Update

As of writing this the API implemented by this PR is as follows.

Click for the code

---

ReceivedCharacter(char) was removed. Its functionality is now covered by KeyboardInput and ReceivedImeText.

pub enum WindowEvent {
    /// The user commited an IME string for this window.
    ///
    /// This is a temporary API until #1497 gets completed. See:
    /// https://github.com/rust-windowing/winit/issues/1497
    ReceivedImeText(String),

    KeyboardInput {
        device_id: DeviceId,
        event: KeyEvent,
        is_synthetic: bool,
    },

   // ...
}


/// Contains the platform-native physical key identifier (aka scancode)
#[derive(Debug, Eq, PartialEq, Hash, Serialize, Deserialize)]
pub enum NativeKeyCode {
    Unidentified,
    Windows(u16),
    MacOS(u16),
    XKB(u32),
}

/// Represents the position of a key independent of the
/// currently active layout.
/// 
/// Conforms to https://www.w3.org/TR/uievents-code/
/// 
/// For most keys the name comes from their US-layout equivalent.
#[derive(Debug, Eq, PartialEq, Hash, Serialize, Deserialize)]
#[non_exhaustive]
pub enum KeyCode {
    A,
    B,
    // ...
    ControlLeft,
    ControlRight,
    // ...
    F1,
    F2,
    // ...
    
    /// The native scancode is provided (if available) in order
    /// to allow the user to specify keybindings for keys which
    /// are not defined by this API.
    Unidentified(NativeKeyCode)
}

pub trait KeyCodeExtScancode {
    /// The raw value of the platform specific physical key identifier.
    fn to_scancode(self) -> u32;
    /// Constructs a `KeyCode` from a platform specific physical key identifier.
    fn from_scancode(scancode: u32) -> KeyCode;
}
impl KeyCodeExtScancode for KeyCode {
    fn to_scancode(self) -> u32 {}
    fn from_scancode(scancode: u32) -> KeyCode {}
}

/// Represent a key according to a specific keyboard layout.
#[derive(Debug, Eq, PartialEq, Hash, Serialize, Deserialize, Copy, Clone)]
#[non_exhaustive]
pub enum Key<'a> {
    /// When encoded as UTF-32, consists of one base character and zero or more combining characters
    Character(&'a str),
    Ctrl,
    Shift,
    // ...
    
    /// Contains the text representation of the dead-key
    /// when available.
    /// 
    /// ## Platform-specific
    /// - **Web:** Always contains `None`
    Dead(Option<char>),
    
    /// The native scancode is provided (if available) in order
    /// to allow the user to specify keybindings for keys which
    /// are not defined by this API.
    Unidentified(NativeKeyCode)
}

pub enum KeyLocation {
    Standard,
    Left,
    Right,
    Numpad,
}

pub struct RawKeyEvent {
    pub physical_key: keyboard::KeyCode,
    pub state: ElementState,
}

pub struct KeyEvent {
    pub physical_key: KeyCode,
    
    /// This value is affected by all modifiers except <kbd>Ctrl</kbd>.
    /// 
    /// This has two use cases:
    /// - Allows querying whether the current input is a Dead key
    /// - Allows handling key-bindings on platforms which don't
    /// support `KeyEventExtModifierSupplement::key_without_modifiers`.
    /// 
    /// ## Platform-specific
    /// - **Web:** Dead keys might be reported as the real key instead
    /// of `Dead` depending on the browser/OS.
    pub logical_key: Key<'static>,
    
    /// Contains the text produced by this keypress.
    ///
    /// In most cases this is identical to the content
    /// of the `Character` variant of `logical_key`.
    /// However, on Windows when a dead key was pressed earlier
    /// but cannot be combined with the character from this
    /// keypress, the produced text will consist of two characters:
    /// the dead-key-character followed by the character resulting
    /// from this keypress.
    ///
    /// An additional difference from `logical_key` is that
    /// this field stores the text representation of any key
    /// that has such a representation. For example when 
    /// `logical_key` is `Key::Enter`, this field is `Some("\r")`.
    /// 
    /// This is `None` if the current keypress cannot
    /// be interpreted as text.
    /// 
    /// See also: `text_with_all_modifiers()`
    pub text: Option<&'static str>,
    
    pub location: KeyLocation,
    pub state: ElementState,
    pub repeat: bool,
    
    pub(crate) platform_specific: platform::KeyEventExtra,
}

/// Additional methods for the `KeyEvent` which cannot be implemented on all
/// platforms.
pub trait KeyEventExtModifierSupplement {
    /// Identical to `KeyEvent::text` but this is affected by <kbd>Ctrl</kbd>.
    ///
    /// For example, pressing <kbd>Ctrl</kbd>+<kbd>a</kbd> produces `Some("\x01")`.
    #[inline]
    fn text_with_all_modifiers(&self) -> &Option<String>;

    /// This value ignores all modifiers including
    /// but not limited to <kbd>Shift</kbd>, <kbd>Caps Lock</kbd>,
    /// and <kbd>Ctrl</kbd>. In most cases this means that the
    /// unicode character in the resulting string is lowercase.
    ///
    /// This is useful for key-bindings / shortcut key combinations.
    ///
    /// In case `logical_key` reports `Dead`, this will still report the
    /// real key according to the current keyboard layout. This value
    /// cannot be `Dead`.
    #[inline]
    fn key_without_modifiers(&self) -> Key<'static>;
}

impl Window {
    /// Reset the dead key state of the keyboard.
    ///
    /// This is useful when a dead key is bound to trigger an action. Then
    /// this function can be called to reset the dead key state so that 
    /// follow-up text input won't be affected by the dead key.
    ///
    /// ## Platform-specific
    /// - **Web:** Does nothing
    // ---------------------------
    // (@ArturKovacs): If this cannot be implemented on every desktop platform
    // at least, then this function should be provided through a platform specific
    // extension trait
    pub fn reset_dead_keys(&self) {
        todo!()
    }
}

[maroider]

I'm glad to see that you followed through on the implementation :)

The good

Most buttons seem to work as expected.

The bad

1. Alt Graph emits two key events per state change: once for Alt Graph and once for (left) Ctrl.
2. Print Screen only emits a "key up" event. Not sure if this is fixable at all.
3. Enter emits Key::Character("\r") instead of Key::Enter. Surprisingly , the Enter on the numpad works correctly on "key up".
4. Tab emits Key::Character("\t") instead of Key::Tab
5. Backspace emits Key::Character("\u{8}") instead of Key::Backspace
6. Esc emits Key::Character("\u{1b}") instead of Key::Escape on "key down".
7. / on the numpad emits a Key::Unidentified on "key up". Ctrl+/ always emits Key::Unidentified
8. Win emits Code::Unidentified. Is this because the scancode Windows sends collides with something else?
9. < emits Key::Unidentified on "key up", regardless of shift state. Ctrl+Shift+< always gives Key::Unidentified. The physical key is always Code::IntlBackslash (as expected).
10. With Num Lock on, Shift+Numpad{1|2|3|4|5|6|7|8|9|0|.} produces a false "key up" event for whichever Shift is held down. After the real Numpad{1|2|3|4|5|6|7|8|9|0|.} "key down" event, another false event is sent for whichever Shift is held down, this time a "key down" event.
11. In addition to (10), holding down both Shift keys makes the numpad ignore Num Lock. This one is a bit contrived, but nevertheless unexpected.

A lot of the special cases involving Ctrl are likely also triggered by Alt Graph.

The ugly

12. Repeatedly pressing dead keys leads to the application crashing with some variation of

memory allocation of 18446744073709551615 bytes failederror: process didn't exit successfully: `target\debug\examples\keyboard.rs` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)

(The missing \r\n is authentic)
This does not seem to happen when every dead key is "consumed".

Lastly

(3), (4), (5) and (6) are probably very easy to fix, so those can probably be rectified immediately. (12) should also be more of an "isolated" bug.
The others would probably benefit from a more complete overview of the ways in which Windows mangles keyboard input, since
there's more to test in terms of key combinations, but I've run out of steam for now.
I also need to come up with a more systematic approach, as I keep forgetting how for along I've come in the testing.

Here's my test program for reference.

PS: Good lord, all of those <kbd> tags were a pain to add.

[msiglreith]

For better overview for non-Windows reviewers:

API Changes

/// Describes a keyboard input as a raw device event.
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct RawKeyEvent {
    pub scancode: ScanCode,
    pub physical_key: keyboard_types::Code,
    pub state: keyboard_types::KeyState,
}

/// Describes a keyboard input targeting a window.
// TODO: Implement (de)serialization.
// (This struct cannot be serialized in its entirety because `ScanCode`
// contains platform dependent data so that value cannot be serialized)
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct KeyEvent {
    pub scancode: ScanCode,

    /// Represents the position of a key independent of the
    /// currently active layout.
    /// Conforms to https://www.w3.org/TR/uievents-code/
    ///
    /// Note that `Fn` and `FnLock` key events are not emmited by `winit`.
    /// These keys are usually handled at the hardware or at the OS level.
    pub physical_key: keyboard_types::Code,

    /// This value is affected by all modifiers except <kbd>Ctrl</kbd>.
    ///
    /// This is suitable for text input in a GUI application.
    ///
    /// Note that the `Unicode` variant may contain multiple characters.
    /// For example on Windows when pressing <kbd>^</kbd> using
    /// a US-International layout, this will be `Dead` for the first
    /// keypress and will be `Unicode("^^")` for the second keypress.
    /// It's important that this behaviour might be different on
    /// other platforms. For example Linux systems may emit a  
    /// `Unicode("^")` on the second keypress.
    ///
    /// ## Platform-specific
    /// - **Web:** Dead keys might be reported as the real key instead
    /// of `Dead` depending on the browser/OS.
    pub logical_key: keyboard_types::Key,

    pub location: keyboard_types::Location,
    pub state: keyboard_types::KeyState,
    pub repeat: bool,

    pub(crate) platform_specific: platform_impl::KeyEventExtra,
}

/// Hardware-dependent keyboard scan code.
#[derive(Debug, Copy, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)]
pub struct ScanCode(pub(crate) platform_impl::PlatformScanCode);

and

/// The window received a unicode character.
ReceivedCharacter(char),

replaced by

/// The user commited an IME string for this window.
///
/// This is a temporary API until #1497 gets completed. See:
/// https://github.com/rust-windowing/winit/issues/1497
ReceivedImeText(String),

[msiglreith]

First of all, thanks for spearheading this work!

I have to admit I didn't read/discovered the keyboard API thread so far, therefore, I'm quite confused on the difference between scancode and physical key. Could you add some documentation for scancodes in particular please?

One thing which irks me a bit is allocating a new string for each character event. I assume in the future there might be need to dynamically enable/disable certain events (similar to #1634)

[dhardy]

• scancode is an OS-specific (or maybe keyboard-specific) code. The only reason it is exposed IIUC is to be able to match key-down and key-up events.
• physical_key is roughly-speaking the label assuming a US keyboard layout.
• logical_key is the key affected by layout and shift-key, as used for normal (graphical-mode) text entry
• we also need "console mode" text entry (includes mappings for Ctrl+C, Ctrl+Z etc.)

[kchibisov]

What's the point of even exposing dead keys downstream and other compositing stuff? I think downstream shouldn't care about it and it's hardly platform specific, but we're trying to abstract things away?

Allocating strings for each key press is also costly, and there's no reason to do so? Or is the goal to support multichar compose input?

Also, location stuff is confusing, since I think everything will break on hardware dvorak or other layout, since you generally assume qwerty afaics?

[chrisduerr]

Saying that ReceivedChar is "replaced by ReceivedImeText is also very misleading, they don't even remotely do the same thing. Instead you now have logical_key which might or might not do the ReceivedChar for you.

@maroider's statement on Key::Character("\r") sending a character instead of a key being bad must also be ignored. If that is followed up on and Enter is sent instead, winit becomes completely useless to handle text input.

If keys like enter/escape/backspace don't emit the actual characters they send anymore, winit might as well just let downstream handle all the work themselves, that would be much easier at that point.

I also agree with @kchibisov that allocation is just a bad idea. If an action leads to sending multiple characters, why not emit multiple characters? Windows emitting two ^^ when I press them isn't windows giving me the string ^^, it's giving me two characters. Why then send those two characters connected downstream, that just means that you allocate and then have to just split it into characters again anyways.

[maroider]

Scancodes

scancode is an OS-specific (or maybe keyboard-specific) code.

There are two things called scancodes (at least on Windows): the code the keyboard itself uses to identify a key, and the code the OS sends to applications which also uniquely (well, mostly uniquely) identifies the key. The two used to be the same, IIRC, when PS/2 was the norm. The codes Windows sends to applications is still based on a set of PS/2 scancode values (set 2). I think that Linux may be basing its "keycodes" on "PS/2 scancode set 2", but don't quote me on that.
The scancode being handled here is of the OS-provided variety.

Dead keys and allocating strings

I implore you all to read the "Dead keys" section of the W3C's "UI Events" specification with its accompanying examples (and despair).
In particular, "keydown" events received at the end of a dead key sequence are specified as having "key" value equal to the composed character (ie. ^+a gives a "key" value of "â"). Both Chrome and Firefox implement this. The "double diacritic" case (which is only relevant on Windows) is exposed by Firefox as a "keydown" event with a "key" value of "^^". Chrome only exposes this through the composition half of the API.
I don't particularly like that things are this way, but it is what it is. If we want something different, then that's going to require some interesting (and possibly ill-advised) contortions on the web backend, at the very least.

The other (and admittedly more important) half of the "Why must we use a full string?" equation is that the "UI Events" specification allows for "key" values to contain "0 or 1 non-control characters followed by 0 or more combining characters" (ref). Splitting these up into multiple events each delivering one char is somewhat nonsensical when they arrive as one. Using an internal static cache and handing out &'static strs was proposed, but there was also a desire to use keyboard-types to get some sort of "cross-windowing-library keyboard input API convergence". Notably, win-win and druid already use make use of keyboard-types. Discussion around druid's keyboard input API took place (is still taking place?) in linebender/druid#1040. Using keyboard-types would also allow for projects like iced, which don't want to tie themselves directly to Winit, to rely on keyboard-types as their keyboard input interface, and get Winit integration (almost) for free.

The W3C's "Keyboard Event Viewer" is a useful tool here for testing what browsers actually implement.

Control characters (including \r and \t)

Control characters were intended to be exposed through a desktop-only extension trait (that's how I understood it, at least). AFAICT, they're being stored in this struct, but they're not being exposed through the proposed KeyEventDesktopExt trait.

They should not be exposed through KeyboardEvent.logical_key due to how the uievents-key specification threats Enter, Tab, and control characters in general (ref).

Location, QWERTY and DVORAK

This really shouldn't be an issue. Location is mostly intended for keys which can occur in multiple places in a single layout, either through right/left variants or appearing on the number pad as well as appearing elsewhere. The keys that layouts like DVORAK shift around really shouldn't matter here, since they're all in the alphanumerical section. While their physical "location" may change, the Location value will still be Location::Standard.

QWERTY is only assumed on a "scancodes conform to a QWERTY layout" level (which holds true for most keyboards, even non-QWERTY-ones). At best, it is only used to as a naming scheme for physical_key due to its ubiquity, which in no way should inhibit users of other layouts any more than any other potential naming scheme for physical_key. Admittedly, if "hardware DVORAK" shifts around scancodes, then all bets are off for any assumptions made by code using physical_key, but Location should nevertheless not be a problem.

I'd assume that the kind of people who end up using things such as "hardware DVORAK" are also acutely aware that software generally expects scancodes conforming to QWERTY regardless of what's printed on the keycaps. If they're not aware, they will inevitably become aware of it if they play any games (or other applications) which rely on the physical location of keys. While in an ideal world, there would be some kind of interface for the OS to query stuff like "How are your keys laid out?" from the keyboard, QWERTY-conformant scancodes is the best we've got.

[dhardy]

Dead keys

Is there actually a reason to report dead keys and composestart...composeend? It's not really contorted to only report the final value. Maybe some apps might try to visualise dead chars during input but I haven't seen this.

The other (and admittedly more important) half of the "Why must we use a full string?" ...

If we have a maximum length, then we can use a [u8; N] (internal) field and a method returning &str. This requires unsafe, or we can use a library like arraystring.

[chrisduerr]

Both Chrome and Firefox implement this. The "double diacritic" case (which is only relevant on Windows) is exposed by Firefox as a "keydown" event with a "key" value of "^^".

None of the other things you've mentioned wouldn't work just as well by sending a char. The double diatric case is not a way to input two diatrics at the same time, it's a way to report one delayed diatric after finding out you didn't want to put that above any other character. Reporting that as anything other than two diatric events makes very little sense to me.

The other (and admittedly more important) half of the "Why must we use a full string?" equation is that the "UI Events" specification allows for "key" values to contain "0 or 1 non-control characters followed by 0 or more combining characters" (ref).

I don't see why winit's API should be restricted to the web APIs, when they're basically unmaintained and unused? Sending entire grapheme clusters with a single keyboard press rather than doing that through IME seems very strange to me. The examples for combining characters list one counter example first which does not contain any combining characters (why?) and the second one doesn't seem particularly useful.

Admittedly, if "hardware DVORAK" shifts around scancodes, then all bets are off

It does.

Control characters were intended to be exposed through a desktop-only extension trait (that's how I understood it, at least). AFAICT, they're being stored in this struct, but they're not being exposed through the proposed KeyEventDesktopExt trait.

Why is this a string again? Are desktop platforms expected to handle all input through char_with_all_modifiers? If we want to go in the direction of desktop winit / web winit, what's the benefit of having it all in winit? If I can't even handle text input across platforms and it just causes unnecessary maintenance overhead, one might as well separate the two.

[dhardy]

It does.

Hence why I prefer software remapping. I think all major OSes have built-in support for Colemak and Dvorak now, so there's little reason people should resort to hardware remapping (except that custom software maps can still be a nuisance to set up). But the point is moot anyway because we can't do anything to help with hardware remapping even if we wanted to.

[chrisduerr]

Hardware remapping provides a much better and more consistent experience than having to constantly redo it in software every time.

There's also not a single keyboard layout in the world that would match what I use.

[dhardy]

I've tried both. Software remapping is far more flexible. Go try my layouts.

Sorry people, off-topic.

[maroider]

Dead keys

@dhardy

Is there actually a reason to report dead keys and composestart...composeend? It's not really contorted to only report the final value. Maybe some apps might try to visualise dead chars during input but I haven't seen this.

I can vaguely recall some application or the other enabling superscript when I hit ^, but I can't for the life of me remember which one it was. Might have been Ctrl+^. Regardless of that, it should be of some use for "keybind customization" menus for applications with mnemonic shortcuts.

It's not really contorted

Yeah, calling it "contortions" was perhaps a bit of an exaggeration.

@chrisduerr

The double diatric case is not a way to input two diatrics at the same time, it's a way to report one delayed diatric after finding out you didn't want to put that above any other character.

You are of course entirely correct, and I'm not opposed to moving it somewhere that makes more sense.

Dead keys and the web

It is admittedly a bit of a shame that the W3C has decided to specify the dead-key behaviour that it has, but it's likely for the sake of simplifying Windows implementations. I did propose (in #753) having KeyboardEvent.logical_key not emit dead-key-composed characters, but I quickly conceded in a fit of "I'm tired and don't want to continue arguing about this". I regret that now, as I did not realize before now that there may be a semi-reasonable way to ignore dead-key-composed characters on the web: Unicode normalization form D.

It would be nice if Key::Dead contained an Option<String> (or char or &'static str or whatever), such that platforms which can report which dead key has been pressed (read: everything but the web) can do so immediately. A potentially nicer approach (for the user) would be to use composition events on the web to extract the dead key character so Option can be dropped. I'm not sure of the exact ordering guarantees for these events on the web, though, so I can't say for certain how implementable dropping Option would be.

Do you have any thought on this, @pyfisch?

Control characters

@chrisduerr

If keys like enter/escape/backspace don't emit the actual characters they send anymore, winit might as well just let downstream handle all the work themselves, that would be much easier at that point.

I'm not sure for which exact set of control characters it is you want to expose through Key::Character (since there are a lot more than just the ones mentioned), but I don't think anything other than plain old space (" ") belongs there. I feel like there may be a case for a Key::Space variant, but I'm mostly fine with the W3C's decision here.
I.e. most applications probably don't want to receive \u{3} for Ctrl+c (on Windows), but I'm not sure if you're advocating for this either. Such input should of course be exposed somewhere, just not on KeyboardEvent.logical_key.
When it comes to the things you specifically called out:

• Enter: Line endings are of course platform-dependent, but applications will also likely want to further control the line endings they use. Some will want to not forward line endings and use Enter as a "send" button instead. I think the best default here is to signal to the user: "figure out which one works for you". And if what "works for them" is "whatever the platform exposes", then that's of course supported and exposed elsewhere in the API.
  I.e. VSCode lets you chose between Unix-style (\n) and DOS-style (\r\n) line endings.
  I also find Key::Return to be more intuitive than Key::Character("\r").
• Escape: Now, this is one I find somewhat surprising that you're calling out explicitly. No application I can think of other than terminal emulators would want the escape character (\u{1b} on Windows) here.
• Backspace: This one too, is a bit surprising. Backspace is something you particularly don't want in your text input (as a control character), since you'll be using it to delete some part of your text buffer.
  Additionally, I feel like the intuitiveness argument is even stronger in the case of Escape and Backspace.

I feel like I should also state that I'm viewing this primarily from a gamedev angle, and a bit from a GUI / text editing angle.

Key::Character(String)

If we don't expose the result of dead-key composition here, then I guess there's a lot less to worry about with regard to potentially multi-char inputs, but the "web issue" still remains.
On the whole, though, I feel like this shouldn't be too big of a concern, even WRT the cost of allocation. This should not be a blocker, IMO, and we can change it later if the cost of allocation is too high.

Winit and the web

@chrisduerr

If we want to go in the direction of desktop winit / web winit, what's the benefit of having it all in winit? If I can't even handle text input across platforms and it just causes unnecessary maintenance overhead, one might as well separate the two.

I feel like this is a bit of an exaggeration. While there are definitively a lot of differences between Winit on the desktop and Winit on the web, I don't believe that the differences are so large that the maintenance overhead would become unreasonable. The current lack of a web backend maintainer is rather unfortunate, and I might've thrown my own hat into the ring if I had greater knowledge and interest in the web backend. I certainly have the time for it given the amount of time I use on these comments. Then again, I've never maintained any sort of software project, so take that as you will.

Text input

I'm still not too happy with using KeyboardEvent.logical_key as a part of the text input API. I'd prefer for it to be earmarked (not sure what the best word is here) for mnemonic keyboard shortcuts and the like. Not sure how the rest of you feel, other than @ArturKovacs, who seems to be in favour of it.

PS: I'm really glad you're all showing up to discuss this. #753 has been on my mind for months on end now, and it's nice to see that there may be an end in sight. I am somewhat sorry about the long monolithic comments, particularly when my only qualifications here are: "I spent some time thinking about this".

PPS: I'm also sorry about (half) flip-flopping on you, @ArturKovacs. I truly do appreciate the work you're putting into this, as well as the work you've offered to perform.

PPPS: There's probably something I've forgotten to address here, but I need to post this or I won't be getting any sleep. I've already spent around 6 hours on this.

[chrisduerr]

I'm not sure for which exact set of control characters it is you want to expose through Key::Character (since there are a lot more than just the ones mentioned), but I don't think anything other than plain old space (" ") belongs there.

Just to be clear, this is mostly a question of where you want to put it. But you must have a way for applications to handle text input. That includes all escape characters. Though as previously noted that might be in a desktop extension.

I feel like this is a bit of an exaggeration. While there are definitively a lot of differences between Winit on the desktop and Winit on the web, I don't believe that the differences are so large that the maintenance overhead would become unreasonable.

Just to be very clear on this: Unless you can easily make an application that runs both on the desktop and web with winit, there's zero point in having both in a single library. If we split the API in a web and a desktop way again and again, winit has clearly strayed from the goal to abstract over these platform differences begging the question if the goals it has still make sense.

[msiglreith]

A few comments, haven't looked into all the keyboard logic so far 👀

[msiglreith]

Could Scancode be renamed with PhysicalKey in this context?

[ArturKovacs]

You mean you'd like it to be something like this?

pub trait KeyCodeExtPhysicalKey {
    fn to_physical_key(self) -> Option<u32>;
    fn from_physical_key(scancode: u32) -> KeyCode;
}

[msiglreith]

Right, if we can get rid of scancode in favor of physical_key on the user facing api I think this would be very helpful

[ArturKovacs]

I don't like this because if we go with this, then the term "physical_key" would be used for two different things.

1. physical_key in KeyEvent (and RawKeyEvent) which is of type KeyCode and is a platfom agnostic value.
2. physical_key to refer to platform native physical key identifier such as the u16 scancodes on Windows and the u16 keyCode on macOS

[ArturKovacs]

An alternative name I could think of would be "native key id" as shown below. But I'm not sure what's wrong with "scancode". Is it that scancode is usualy used to refer toPS/2 scancodes, which are only used on Windows, but not on other platforms?

pub trait KeyCodeExtNativeKeyId {
    fn to_native_key_id(self) -> Option<u32>;
    fn from_native_key_id(native_key_id: u32) -> KeyCode;
}

[msiglreith]

Thanks for explanation, it wasn't too clear right now the difference between physical key and scancode to me.

[msiglreith]

But I'm not sure what's wrong with "scancode". Is it that scancode is usualy used to refer toPS/2 scancodes, which are only used on Windows, but not on other platforms?

It was a bit difficult to grasp the differences as we introduce new semantics while also keeping existing ones. It also says in the docs that they are kinda similar. But I'm fine with the current usage as well.

physical key (i.e. it's mostly synonymous with a scancode).

[maroider]

Part of the problem is that scancode is an overloaded term. "Native key ID" seems a lot more neutral.

[ArturKovacs]

I would be okay with changing this to "native key id". I'm wondering if this should be brought up at #753 to give a chance for others to object to it.

[msiglreith]

reported by CI as never read

[ArturKovacs]

Oh this was apparently left here from an earlier state when a completely different approach was used.

[msiglreith]

LGTM!
Only a few small annotations from my side. Examples worked fine and seems also detect anything flawlessly I throw at it (:
(didn't test IME parts)

[ArturKovacs]

I'm glad you mentioned the IME because I just tested it again and of course it panicked :D My latest commit fixes that, and now it seems to work as expected again.

As I mentioned elsewhere this should be regularly merged, not squashed. So if @msiglreith, you just give one more confirmation to the latest changes, then I'll merge this into the new-keyboard branch.

[msiglreith]

thanks! feel free to move forward 🚀

[ArturKovacs]

I thought I can merge it normally but I can't, it says "Not enabled for this repository".

@francesca64 or @vberger could you please enable merging without squashing? My understanding of git may be too limited here but if I'm not mistaken, then all other PRs that are based on this changeset (e.g. #1888 #1890) would have to be rebased on top of the squashed changes, which seems too tedious considering, it can be avoided. Alternatively if you don't want enable this, please merge this if the "Create a merge commit" is available to you.

[chrisduerr]

all other PRs that are based on this changeset (e.g. #1888 #1890) would have to be rebased on top of the squashed changes

That should be no problem at all if they're already rebased on top of the latest changes of this PR.

[maroider]

My Linux branch isn't rebased, it's based off of 42055e3 and a merge with f0474f2.
I'd like to avoid re-writing history if I can get away with it.

EDIT: Had a quick chat with @ArturKovacs on Matrix. I can (probably) figure out how to deal with a squash & merge.
EDIT2: Squash & merge should be fine.

[ArturKovacs]

@chrisduerr thanks, that's reassuring, and based on a quick local test, it does seem to be fine. As @maroider noted as well, squashing should be okay, so I'm doing that.

[ArturKovacs]

Wow I can't believe it really happened. I know we still need to finish/get approved all the remaining platforms but this is a giant milestone and I can barely contain my excitement 😁

[dhardy]

Indeed! Congratulations people on finally getting this merged into the testing branch! 🚀