search

rust-windowing / winit

Window handling library in pure Rust
https://docs.rs/winit/
Apache License 2.0
4.74k stars 891 forks source link

A keyboard input model #753

Closed pyfisch closed 1 year ago

pyfisch commented 5 years ago

TLDR: I think that Winit needs more expressive keyboards events and to follow a written specification to keep platform inconsistencies to a minimum. I propose to adapt the JS KeyboardEvent for winit and to follow the UI Events specification for keyboard input.

Winit is used for many applications that need to handle different kinds of keyboard input.

Currently there are two events for text input in Winit: KeyboardInput and ReceivedCharacter.

pub struct KeyboardInput {
    pub scancode: ScanCode,
    pub state: ElementState,
    pub virtual_keycode: Option<VirtualKeyCode>,
    pub modifiers: ModifiersState,
}

The KeyboardInput event carries information about keys pressed and released. scancode is a platform-dependent code identifying the physical key. virtual_keycode optionally describes the meaning of the key. It indicates ASCII letters, some punctuation and some function keys. modifiers tells if the Shift, Control, Alt and Logo keys are currently pressed.

The ReceivedCharacter event sends a single Unicode codepoint. The character can be pushed to the end of a string and if this is done for all events the user will see the text they intended to enter.

Shortcomings

This is my personal list in no particular order.

  1. List of VirtualKeyCode is seen as incomplete (#71, #59). Without a given list it is hard to decide which keys to include and when the list is complete. Also it is necessary to define each virtual key code so multiple platforms will map keys to the same virtual key codes. While it probably uncontroversial that ASCII keys should be included for non-ASCII single keys found on many keyboards like é, µ, or ü it is more difficult to decide and to create an exhaustive list.
  2. While VirtualKeyCode should capture the meaning of the key there are different codes for e.g. "0": Key0 and Numpad0 or LControl and RControl.
  3. The ScanCode is platform dependent. Therefore apps wanting to use keys like WASD for navigation will assume an QWERTY layout instead of using the key locations.
  4. It is unclear if a key is repeated or not. Some applications only want to act on the first keypress and ignore all following repeated keys. Right now these applications need to do extra tracking and are probably not correct if the keyboard focus changes while a key is held down. (#310)
  5. A few useful modfiers like AltGraph and NumLock are missing.
  6. There is no relation between ReceivedCharacter and KeyboardInput events. While this is not necessary for every application some (like browsers) need it and have to use ugly (and incorrect) work-arounds. (#34)
  7. Dead-key handling is unspecified and IMEs (Input Method Editors) are not supported.

In general there are many issues that are platform-dependant and where it is unclear what the correct behavior is or it is not documented. Both alacritty and Servo just to name two applications have multiple issues where people mention that keyboard input does not work as expeced.

Proposed Solution

Winit is not the first software that needs to deal with keyboard input on a variety of platforms. In particular the web platform has a complete specification how keyboard events should behave which is implemented on all platforms that Winit aims to support.

While the specification talks about JS objects it can be easily ported to Rust. Some information is duplicated in KeyboardEvent for backwards compatibility but this can be omitted in Rust so Winit stays simpler.

See the keyboard-types for how keyboard events can look like in Rust.

Implementation

This is obviously a breaking change so there needs to be a new release of winit and release notes. While the proposed events are very expressive it is possible to convert Winit to the new events first and then improve each backend to emit the additional information about key-codes, locations, repeating keys etc.

Thank you for writing and maintaining Winit! I hope this helps to get a discussion about keyboard input handling started and maybe some ideas or even the whole proposal is implemented in Winit.

ArturKovacs commented 3 years ago

This seems to indicate that we could omit physical_location from the results and use a function to try mapping (scancode, vkey) to physical_location as well as physical_location → scancode (with both functions returning an Option).

I'm assuming that the purpose of this would be that returning a None from the mapping functions could indicate that the current keyboard doesn't support identifying keys by their position. If this is the case why don't we express this with the Unknown variant or add a new variant expressing specifically this and keep the physical location in the event struct?

ArturKovacs commented 3 years ago

I think I speak for everyone here when I say that this issue is exhausting to participate in which I believe is largely because it's very hard to find the best possible API that satisfies all paltform limitations. This however should not prevent us from at least improving on the current state.

Again here's a link to the updated proposal: https://github.com/rust-windowing/winit/issues/753#issuecomment-695211403

Are we even able to properly associate key, character and IME input? The composition-event branch lists all three as separate events.

Well not on the web at least. In particular the composition start event cannot be associated with keyboard events (and the CompositionUpdate and the CompositionEnd don't emit a keypress event with the correct key when I test on Windows). So as annoying as it is, the keydown and the composition events have to be separated similarly to how it is on the web.

You'd have to load the current keyboard layout, say before every Event::NewEvents, since I don't think Windows notifies you that the layout has changed. There's probably also a case to be made for loading the keyboard layout on every keyboard event, since you can change the layout with a keyboard shortcut (Win+Space bar).

It seems to me that Windows does notify the application about this, by WM_INPUTLANGCHANGEREQUEST.

The proposal that I linked above seems possible to do. It has many compromises for all of us and it turned out to be very similar to the web API due to the limitations there. But as @maroider said, this would be a great improvement so I would love to move to implementation as soon as possible. Even though I know that just like the rest of us I neglected this thread the past month.

@dhardy I know, you've been more or less opposed to this proposal and you wish to take an approach relying on using the minimal amount of data possible and using mapping functions to extract more info. I think the biggest advantage of that approach is that it allows to specify mappings only available on a certain platform using extension traits. But I think that the current set of public fields should be made available on every platform. And adding further extensions like mapping a PhysicalKey to a label can be done later even if we choose the API that I'm and @maroider(?) is suggesting. To be honest you seem to be more knowledagbe in this keyboard input handling topic than I am, but as far as I can tell you never addressed my arguments against choosing an API based on negligible differences in memory usage and speed. And at this point, after almost two years of this issue being opened, I think that starting to implement an API that's not perfect but is a great improvement is more valuable than spending more time discussing what would be the best possible API. Don't get me wrong I'm not trying to shut you out of the conversation because as I said you seem to be more knowledgeable in this, I'd just like the discussion to be focused on the truly concerning aspects and once we agree that the stuff is good enough™ we should move on to getting our hands dirty.

I wrote it earlier but I'll just reinforce it that I'm willing to write the implementation for Windows, macOS, the web, Linux, and Android - in this order of preference.

dhardy commented 3 years ago

I have a slight concern about Unicode(&'static str), — this may require memory leaks with every keypress to implement. Lets just use String since you don't want WindowEvent<'a> and perf. impact is negligible.

I'm not a big fan of the way LogicalKey combines a UTF-8 string with a finite set of values, but practically I don't see a better alternative, so it's still the best option from my point of view. PhysicalKey is possibly worse since extra keys are simply not represented, but it's still the best option we have.

Regarding the other arguments — I agree with you. I'm not 100% sure of the need to include a scancode (one could try matching physical_key instead), but potentially it's the best option to allow matching key-down and -up events. (Maybe see how the implementations pan out.)

And thanks so much for the effort you're putting in here.

maroider commented 3 years ago

@dhardy

I have a slight concern about Unicode(&'static str), — this may require memory leaks with every keypress to implement. Lets just use String since you don't want WindowEvent<'a> and perf. impact is negligible.

@Osspial addressed the memory leak much earlier in the discussion (emphasis mine).

There's a solution for using &str, actually - we could convert unicode Strings that are constructed at runtime into &'static strs as follows, then we can internally store a cache of keypress strings so that we don't consume additional memory for every keypress:

let string: String = "Hello".to_string();
// Construct a 'static string at runtime.
let x: &'static str = Box::leak(string.into_boxed_str());

Since the number of unique strings produced by a given keyboard layout should be fairly limited, leaking some (mostly) bounded amount of memory should be fine.

maroider commented 3 years ago

For posterity's sake, I think it's a good idea to state why keyboard-types isn't being chosen as a solution since what we've landed on is fairly similar. It also isn't easy to spot the why since the discussion on this issue has grown quite long.

Key::Character

Key::Character uses a full String. @Osspial seems to prefer something which is easier to match on:

  • A full String is more difficult to match on than an enum, str, or char.

Code

The Code enum includes some keys (Fn and FnLock in particular) which seem to be pretty much exclusively handled in hardware. @Osspial notes:

I'd like to leave the hardware-handled keys out of our "officially supported" keys

F13-F24: keyboard-types does not enumerate these. macOS and Linux possibly also support up to F35. Physical keyboards rarely (if ever) include those keys these days, but I (and possibly others) assign dedicated macro keys to F13-F24.

Location

keyboard-types sticks the Location enum on the KeyboardEvent struct instead of only sticking it on the keys which have multiple locations. Sticking Location only on the keys which have multiple locations makes it abundantly clear when you need to care about the Location by documenting with the types themselves instead of having to document this elsewhere. @Osspial notes:

I like the idea of having a left/right enum to distinguish between sided keys. However, the Location enum should be exposed through variants in the Key enum (e.g. Ctrl(Location)), rather than on the main KeyboardEvent struct we expose.

There was a concern raised by @pyfisch about having to add a Location field on enum variants, but this shouldn't be an issue since new keyboard layouts aren't really made these days. @Osspial notes:

Regarding adding a location to an existing key being a breaking change - there shouldn't be any reason we ever have to do that! Keyboard layouts are fairly static, and only a limited subset of keys are going to have multiple locations on the keyboard. We should be able to keep track of which ones have multiple locations and structure the enum as necessary.

Modifiers

keyboard-types sticks the Modifiers struct on the KeyboardEvent struct, which goes against the current direction in Winit's API, which makes the user track Modifiers through the ModifiersChanged event. The exact reasoning for this escapes me, so I'm not sure how good of an argument this is against adopting keyboard-types directly.

KeyboardEvent

There's a desire to expose platform-specific keyboard event data, but keyboard-events' KeyboardEvent struct doesn't give us this option. It would be possible to expose this data as a separate event, but the data is currently stuck on KeyboardEvent in @ArturKovacs' latest proposal.

I think that's everything that's been discussed that makes adopting keyboard-types undesirable, but it's entirely possible that I've missed something.

pyfisch commented 3 years ago

@maroider thanks for making this list of differences. Let me treat these items as feature requests for keyboard-types and we maybe avoid having two different APIs.

Key::Character

Key::Character uses a full String.

The current proposal uses &'static str. Since there are usually only a very limited number of key values I think it is reasonable to cache them in winit and use &'static str.

Code

The Code enum includes some keys (Fn and FnLock in particular) which seem to be pretty much exclusively handled in hardware. @Osspial notes:

I'd like to leave the hardware-handled keys out of our "officially supported" keys

It should be sufficient to add a sentence to the documentation that winit does not emit Fn and similar codes because they are handled in hardware.

F13-F24: keyboard-types does not enumerate these. macOS and Linux possibly also support up to F35. Physical keyboards rarely (if ever) include those keys these days, but I (and possibly others) assign dedicated macro keys to F13-F24.

Fair point. Having a variant F(u8) instead of F1 to F12 should resolve this. The web specification says that these keys are valid, I just didn't implement it at the time. We should also have Soft(u8).

Location

keyboard-types sticks the Location enum on the KeyboardEvent struct instead of only sticking it on the keys which have multiple locations. Sticking Location only on the keys which have multiple locations makes it abundantly clear when you need to care about the Location by documenting with the types themselves instead of having to document this elsewhere.

There is one very practical issue with this (I just thought of): The numpad contains keys for digits and operators. They should emit a Key event with a Unicode character and the location is numpad. This means we would need to stick location to the Unicode variant, which unfortunately mostly negates the advantage of knowing which keys can have different location values.

Modifiers

keyboard-types sticks the Modifiers struct on the KeyboardEvent struct, which goes against the current direction in Winit's API, which makes the user track Modifiers through the ModifiersChanged event. The exact reasoning for this escapes me, so I'm not sure how good of an argument this is against adopting keyboard-types directly.

I don't know either why winit chooses to have a separate modifiers changed event. My guess it that other events like mouse events also need this information (For actions like Control + left click), and winit tried to keep the events small.

KeyboardEvent

There's a desire to expose platform-specific keyboard event data, but keyboard-events' KeyboardEvent struct doesn't give us this option. It would be possible to expose this data as a separate event, but the data is currently stuck on KeyboardEvent in @ArturKovacs' latest proposal.

Easiest solution would be to add an extra field to the keyboard-types KeyboardEvent, which can hold any additional data winit wants to send. If the field is not needed users just set it to ().

maroider commented 3 years ago

Key/Code

Fair point. Having a variant F(u8) instead of F1 to F12 should resolve this. The web specification says that these keys are valid, I just didn't implement it at the time. We should also have Soft(u8).

Could you explain why you believe F(u8) is better than enumerating every F-key? I don't think I've seen any indication that anything above F35 is used anywhere. I also don't understand the purpose of the Soft keys. The UI Events specification lists each of them as a "General purpose virtual function key, as index x.", but they don't seem to be discussed anywhere else in the specification.

Location

There is one very practical issue with this (I just thought of): The numpad contains keys for digits and operators. They should emit a Key event with a Unicode character and the location is numpad. This means we would need to stick location to the Unicode variant, which unfortunately mostly negates the advantage of knowing which keys can have different location values.

The LogicalKey/Key enumeration isn't supposed to be where Winit exposes character/text input, so I think it might be reasonable to special case the digits and symbols in question and give them their own Key variants.

Modifiers

I don't know either why winit chooses to have a separate modifiers changed event. My guess it that other events like mouse events also need this information (For actions like Control + left click), and winit tried to keep the events small.

Personally, my only real hang-up here is that keyboard-types' KeyboardEvent would make Winit's API surface inconsistent once the deprecated modifers fields on various events get removed.

KeyboardEvent

Easiest solution would be to add an extra field to the keyboard-types KeyboardEvent, which can hold any additional data winit wants to send. If the field is not needed users just set it to ().

This seems like a good solution to me. It might also be nice to have a without_extra method that always returns a KeyboardEvent<()> so users can throw away the extra information. This would also allow crates which want to interface only with keyboard-types to stick with () instead of sprinkling generics all over their code without giving their users paper cuts.

CompositionEvent

I'm not sure what we should do in terms of composition events. The shape and implementation of that particular API is being discussed in #1497 and it seems there is yet to emerge a consensus on that. My gut is telling me that composition event support should be ignored for the purposes of getting closer to closing this issue, but again, I'm not sure.

maroider commented 3 years ago

One more thing I completely forgot: I've been pushing for the inclusion of a modifier-independent LogicalKey/Key to be delivered alongside the modifier-dependent LogicalKey/Key which is already a part of the UI Events specification. I think that the loss of this part of the API is an acceptable compromise if keyboard-types is able to support Winit's use-case in a satisfactory manner in all other cases. Modifier-independent LogicalKey/Keys can be queried for after the fact once a KeyboardLayout API is added, if I'm not terribly mistaken (although Windows may or may not make that difficult).

pyfisch commented 3 years ago

Key/Code

Fair point. Having a variant F(u8) instead of F1 to F12 should resolve this. The web specification says that these keys are valid, I just didn't implement it at the time. We should also have Soft(u8).

Could you explain why you believe F(u8) is better than enumerating every F-key? I don't think I've seen any indication that anything above F35 is used anywhere.

Well, personally I have only ever seen F1-F12 in hardware. Some obsolete systems appear to have a F36 key. Meanwhile ncurses has a key enumeration listing F57 as the highest key. I don't know if these keys were used anywhere in the last 20 years, and I am doubtful that they will be used by some winit software. I can also add F13 through F35 instead since that appears to be the highest number any modern systems we know of support. If it turns out we are wrong about this, it's easy to add more keys.

I also don't understand the purpose of the Soft keys. The UI Events specification lists each of them as a "General purpose virtual function key, as index x.", but they don't seem to be discussed anywhere else in the specification.

Me neither, the W3C spec seems to inherit them from Qt: Qt.Key, StackOverflow elaborates that they were used on some mobile platform: What key is Keys.Context1 tied to in Qt/QML?.

Location

There is one very practical issue with this (I just thought of): The numpad contains keys for digits and operators. They should emit a Key event with a Unicode character and the location is numpad. This means we would need to stick location to the Unicode variant, which unfortunately mostly negates the advantage of knowing which keys can have different location values.

The LogicalKey/Key enumeration isn't supposed to be where Winit exposes character/text input, so I think it might be reasonable to special case the digits and symbols in question and give them their own Key variants.

A special case wouldn't be the least surprising option imho. If this separate variant is implemented a hotkey on "," needs different code than a hotkey on "." for example. I bet winit will see a few issue reports from confused users. (LogicalKey/Key is useful for text input and used in servo for this purpose. But I don't want to debate this further.)

Modifiers

I don't know either why winit chooses to have a separate modifiers changed event. My guess it that other events like mouse events also need this information (For actions like Control + left click), and winit tried to keep the events small.

Personally, my only real hang-up here is that keyboard-types' KeyboardEvent would make Winit's API surface inconsistent once the deprecated modifers fields on various events get removed.

Yeah, that would be weird. Maybe consider un-deprecating the field and using Modifiers from keyboard-types?

Closing notes

Even if winit decides that the KeyboardEvent from keyboard-types is appropriate for its use-case it is still beneficial to reuse the Key, Code and Modifiers as this means other crates like servo and druid that use keyboard-types don't need to match and convert whole enums but only the keyboard event itself. A modifier-independent logical key could be added to the structure in the extra field, if keyboard-types is used. This can be added without problems after the initial implementation. Similarly composition events can be added later.

ArturKovacs commented 3 years ago

Location

The LogicalKey/Key enumeration isn't supposed to be where Winit exposes character/text input

This is not true with the our latest API proposal. The LogicalKey provides the unicode character input as affected by the modifiers. This isn't exposed anywhere else so this is what users need to use for text input.

I think it might be reasonable to special case the digits and symbols in question and give them their own Key variants.

A special case wouldn't be the least surprising option imho. If this separate variant is implemented a hotkey on "," needs different code than a hotkey on "." for example.

Yeah I completely agree with the latter statement here. Even if it's used for shortcuts, not text this is going to be problematic. I do like the fact that we expose which keys have a location and which don't, but with this issue considered, I'm very much in favour of placing the location in a separate field, just like in keyboard-types.

Modifiers

Yeah, that would be weird. Maybe consider un-deprecating the field and using Modifiers from keyboard-types?

The reasoning behind depracating the modifiers field was to avoid having multiple sources of truth. I remeber that @Osspial formulated this argument in a comment for an issue or a PR but I cannot find it anymore.

Omitting the modifier-independent logical key

I originaly got involved with this issue for this feature. The use case for which I needed this was shortcut keys see #1700. Given that the modifier-dependent key ignores ctrl, I'm fine with removing the modifier-independent key. Ignoring Ctrl is what the web does. To see this, run the following in your browser's console and then press keys in combination with Shift or Ctrl

window.addEventListener("keydown", e => {
   console.log(e.key); 
});

Earlier in the thread @kchibisov argued that ctrl must be included in the translated char. I don't mind this as long as we have access to the character without the ctrl modification. The problem is that if winit emits the control character it's impossible for the application to find the layout-dependent key for it. The situation is similar with going from ctrl-less-char to ctrl-char. As far as I understand, it's practically not possible to do when using a Russian or an Arabic layout.

With that said, I do agree that implementing the modifier-less key is problematic at least on the web. It does seem to be possible with xkbcommon and I'm guessing it's possible on Windows and macOS as well. So my suggestion is to move it to a private, platform dependent field from which this can be accessed using an extension trait. So something like

struct KeyboardEvent {
    // all the public stuff...

    platform: PlatformKeyboardEvent,
}
impl DesktopKeyboardEventExt for KeyboardEvent  {
    pub fn logical_key(&self) -> LogicalKey {
        self.platform.logical_key
    }
}
ArturKovacs commented 3 years ago

Furthermore there's a problem with using the keyboard-types Key type for our "modifier-dependent" key because the keyboard-types documentation states that it's an implementation of the UI Events Specification. However according to that specification a control character is not a valid "key attribute value". Placing a control character in such a type would be just as incorrect as making a String instance which has a non-utf8 character.

The modifier-indipendent key on the other hand could use the keyboard-types Key but I think it would be a bad idea to do that because that would leave us using two enums which are identical in shape and only differ in their meaning. That would be a fine example of code duplication especially considering the size of those enums.

pyfisch commented 3 years ago

The "logical key problem" is back to haunt us. :cry: If I am not mistaken three different "logical keys" have been proposed, each useful for a different set of applications:

Did I miss something important? I will add any missing information to the text above.

It is not possible to infer the web key from the desktop key because some information is lost during the Ctrl transformation. It is also not possible to infer the modifier independent key from the two others because the relation between shifted keys and their unshifted counterparts is dependent on the keyboard layout. It is probably possible to infer the desktop key from the web key, if it is known whether Ctrl is pressed and not already consumed.

With these considerations in mind I propose a slightly different API based on @ArturKovacs proposal and the recent discussion for KeyboardEvent. It gives access to all requested kinds of logical keys, uses only a single enum for logical keys (that can be re-used from keyboard-types ) and still provides access to all needed values.

struct KeyboardEvent {
    pub state: keyboard_types::State,
    // **web key**
    pub key: keyboard_types::Key,
    pub code: keyboard_types::Code,
    pub location: keyboard_types::Location,
    pub repeat: bool,
    // Modifiers are transmitted using the modifiers changed event.
    // Additional private fields necessary to implement all methods.
    necessary_private_fields: (),
}

impl KeyboardEvent {
    // **web key** 
    /// Key value but with Ctrl transformation applied.
    /// The returned string is identical to the result of
    /// https://xkbcommon.org/doc/current/group__state.html#ga0774b424063b45c88ec0354c77f9a247
    /// on X11/Wayland.
    pub fn key_with_ctrl_transformation() -> Option<String> {
        // This function can either use a private flag, whether Ctrl transformation applies
        // and then execute the algorithm from https://github.com/xkbcommon/libxkbcommon/blob/6268ba1c77e248ea7ef829ec6d3ffedabe17086e/src/state.c#L899
        // or the return value is stored in a private field.
        // Control characters are valid UTF-8, so they can be returned in a string without issue.
        unimplemented!()
    }
    // **modifier independent key**
    /// Key value without any modifiers applied.
    pub fn key_without_modifers() -> Option<String> {
        // This needs to be stored in a private field.
        // I don't know if this is possible to correctly implement on Linux.
        unimplemented!()
    }
}
ArturKovacs commented 3 years ago

As far as I can tell neither of key_with_ctrl_transformation and key_without_modifers is possible on the web (which is fine). And here's the reason for each:

key_with_ctrl_transformation: There's the key and the code fields from which one might try infering the ctrl char. If you use the key, you might get я instead of z when using a Russian layout so using the key it's not possible to reliably get the control char. If instead you use the code, you might ket KeyZ when the user pressed the y key on a German layout, so that doesn't work either.

key_without_modifers: The issue here is most easily seen for the numbers below the function keys. If I press Shift+1 I get ! in the key field. But for example on the Hungarian layout ! is not on the 1 key so using the web API it's not possible to tell what would the character be without the Shift key.

But I believe they both should be doable on all desktop platforms. I do like your suggestion however I can still see two points where keyboard-types conflicts with something we did seem to reach a decision about.

1, The usage of Unicode(&'static str). 2, Having the Key, the Code, and the Location be #[non_exhaustive] enums

I'm assuiming that adding non_exhaustive wouldn't be an issue for you but I know that making the unicode variant use a reference is not an option. So after giving it some thought I would be willing to sacrafice static references at the altar of convenience for the rest of the ecosystem. IF we were to do that, below is how the adjusted proposal could look like.

And one more thing. I removed the composition API from this, as I realized that what we had for it so far is pretty much useless and a correct composition (IME) API is out of scope for this issue.

EDIT: The following code has been edited after it was posted. Some comments below might be reflecting to older versions.

Click to see the API ```rust 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 Ctrl. /// /// 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 ^ 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::State, pub repeat: bool, platform_specific: platform::KeyEventExtra, } // This would of course only be compiled for dekstop OSes // like Linux, Windows, macOS, BSD, etc impl KeyEventDesktopExt for KeyEvent { /// This value is affected by all modifiers including but not /// limited to Shift, Ctrl, and Num Lock. /// /// This is suitable for text input in a terminal application. /// /// `None` is returned if the input cannot be translated to a string. /// For example dead key input as well as F1 and /// Home among others produce `None`. /// /// Note that the resulting string may contain multiple characters. /// For example on Windows when pressing ^ using /// a US-International layout, this will be `None` for the first /// keypress and will be `Some("^^")` for the second keypress. /// It's important that this behaviour might be different on /// other platforms. For example Linux systems may emit a /// `Some("^")` on the second keypress. fn char_with_all_modifers(&self) -> Option { self.platform_specific.char_with_all_modifers } /// This value ignores all modifiers including /// but not limited to Shift, Caps Lock, /// and Ctrl. In most cases this means that the /// unicode character in the resulting string is lowercase. /// /// This is useful for 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`. fn key_without_modifers(&self) -> keyboard_types::Key { // This is possible on linux when using xkbcommon // by passing empty modifier masks to `xkb_state_update_mask()` // then calling `xkb_state_key_get_one_sym` self.platform_specific.char_with_all_modifers } } /// An opaque struct that uniquely identifies a single physical key on the /// current platform. /// /// This is distinct from `keyboard_types::Code` because this struct /// will always be a unique identifier for a specific key however /// `keyboard_types::Code` may be `Unidentified` for multiple distinct /// keys. /// /// Furthermore this struct may store a value that cannot be ported /// to another platform, hence it is opaque. To retreive the underlying /// value, use one of the platform-dependent extension traits like /// `XkbScanCodeExt` #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] struct ScanCode { /// platform dependent private fields to uniquely identify a single key } /// For X11 (and maybe wayland as well?) impl XkbScanCodeExt for ScanCode { /// returns `xkb_keycode_t` fn keycode(&self) -> u32 { self.keycode } } ```
pyfisch commented 3 years ago

Having the Key, the Code, and the Location be #[non_exhaustive] enums

I can change this, when I wrote the code #[non_exhaustive] wasn't stable yet.

    /// 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.

This isn't how keys should work. I am aware that some browsers work like that, but the correct behavior would be to use composition events to get the text input for dead keys. (Can we maybe just not specify this right now in detail and consider this again when we implement dead keys and IME?)

maroider commented 3 years ago

Text input

Does the current proposal effectively replace WindowEvent::ReceivedCharacter since you seem to want to expose basic text input inside KeyEvent via logical_key, @ArturKovacs?

Location

Where did it go?

Actually, now that I think about it, I'd like to talk about where to stick Location again. One use-case I haven't already mentioned is using the Key/Code enums' serialized forms in config files. Amethyst already does this (it uses VirtualKeyCode directly in its Button enum). It would be nice to be able to write (in RON) Code(Ctrl(Right)) to specify the right control key with this scheme. Key(Character(Numpad, "/")) is admittedly of less utility, but it would be more consistent to have Location be on both enums instead of being exposed once on the Code enum and once on the KeyboardEvent struct. The most consistent API would be one where Location is exposed only on KeyboardEvent.

It would also be nice to have Key::Caracter(Location::Standard, "w") serialize to and from Character("w").

You can't really say "I don't care about which Key::Ctrl, it just has to be a Key::Ctrl" with this scheme tough, so there's still a case for creating your own (de)serialization logic and all that if you really need to represent such a thing. Such a scheme would also be out-of-scope for Winit.

In the end, this isn't necessarily a blocker for me. I'm certainly willing to make my own (de)serialization logic if I need it.

Dead-keys

This isn't how keys should work. I am aware that some browsers work like that, but the correct behavior would be to use composition events to get the text input for dead keys. (Can we maybe just not specify this right now in detail and consider this again when we implement dead keys and IME?)

@pyfisch Example 25 seemingly contradicts this, but I agree with you in that dead keys should be handled through composition events, and that we should hold off on specifying any exact behaviour.

Sacrificing Unicode(&'static str) at the altar of convenience

I'm mostly fine with this for now.

KeyEventDesktopExt

I'm not entirely sold on the names of the methods, but it looks good enough for now.

ArturKovacs commented 3 years ago

Text input

Does the current proposal effectively replace WindowEvent::ReceivedCharacter [...]?

Yes it does. Once this is implemented, the WindowEvent::ReceivedCharacter variant should be removed.

Location

Oops I accidentally left it out from the API. I just added it.

It would be nice to be able to write (in RON) Code(Ctrl(Right))

It would indeed be nice. However I don't think there's a better way of exposing the location of the key from winit, and you seem to agree with this based on how you expressed yourself and on the fact that you didn't suggest an alternative.

I'm certainly willing to make my own (de)serialization logic if I need it.

That's reassuring because I would say that this is the right approach to the situation you described. Of course one wouldn't have to write (de)serialization from scratch, you could just define a struct SerializedKey(pub Location, pub Key) which you feed into your choice of (de)serializer.

Dead-keys

@pyfisch Example 25 seemingly contradicts this, but I agree with you in that dead keys should be handled through composition events

Composition events will not be exposed by this API as a proper IME API is out of scope for this issue, and is being tackled by #1497 . I think at least for now we should forward Dead keys the way the target platform reports them to keep consitency across applications on a single platform. I don't think this is impactful enough to discuss this further in this thread.

All-in-all to me this API is good enough to start implementing. I probably won't start this weekend, but I hope I won't jinx this but I'd like to start sometime next week.

pyfisch commented 3 years ago

@ArturKovacs I've created a keyboard-types branch for winit: https://github.com/pyfisch/keyboard-types/tree/winit It already contains the F13 to F35 function keys and uses #[non_exhaustive] enums. Just open an issue over there for any other required changes.

maroider commented 3 years ago

Dead Keys

I'd like to back-track a bit on "not specifying how these are handled":

I've thought about this for some hours now, and I think this should be implementable even on the Web.

haata commented 3 years ago

As a note, Fn on Mac keyboards is very different from other keyboards and is actually sent as a separate modifier (which macOS uses for some special keys). Apple makes it impossible for 3rd party keyboard vendors to utilize this Fn key, so it's probably safe to ignore it (you have to spoof an Apple USB VID:PID in order for the macOS HID driver to enable parsing for it). But it is possible to emulate with a custom driver.

Apple includes it in the CGEvents though as maskSecondaryFn (https://developer.apple.com/documentation/coregraphics/cgeventflags).

All other Fn usage is typically done inside the keyboard controller itself (though I think I read some new USB HID proposals from Microsoft that adds a new HID event for Fn keys).

ArturKovacs commented 3 years ago

Thank you @pyfisch. I noticed that you didn't add non_exhatustive to Location. After thinking about it a bit, I believe that's fine. I think this is all for now; new feature requests might come up while implementing this but I don't anticipate any additional change required in keyboard-types.

Thanks for the insight @haata, yeah if I'm not mistaken the decision for ignoring Fn and FnLock keypresses was to maximize consistency across platforms and avoid unpleasant suprises.

I'd like to preface to following with stating that I once again updated the proposal.

Dead keys

I realized that we cannot really remove the IME API like I thought we could. The problem is that logical_key cannot replace ReceivedCharacter because ReceivedCharacter also forwarded IME results but for example on the web there's no keypress event for composition end so there wouldn't be any scancode, yet our API requires scancode to always map to a real, physical key.

A large pain point is that there doesn't seem to be an agreement on IME handling at #1497. That's why I was so quick to jumping to removing IME because I don't see how this will ever be implemented if we include IME in this issue as well. In order to mitigate the previously mentionned issues with including IME input within the keyboard event, I think we should add a single variant to WindowEvent in the shape of:

pub enum WindowEvent {
    ...

    /// A text composition (IME) ended with the provided text.
    ///
    /// This API is limited on purpose. A more feature ritch IME API is
    /// being discussed at:
    /// https://github.com/rust-windowing/winit/issues/1497
    // TODO remove this once 1497 is complete
    ReceivedImeText(String)
}

Then ReceivedCharacter could indeed be removed. As every key-press related text input would be handled by logical_key and IME generated text input would be handled by ReceivedImeText.

KeyboardEvent.logical_key always gives characters without diacritics from dead keys.

As logical_key is for text input for non-ime keyboard events and as such, it should be affected by dead-key-like composition and it should report the Dead varian for dead keys otherwise text input would behave incorrectly.

Some way of accessing "the character/string the platform gave Winit" (ignoring Ctrl, of course) is added [...] This is how you'd get composed characters with diacritics

If I understand you correctly this is also what logical_key is for. And getting the character this way wouldn't be a stop-gap measure according to how I interpret the API. It would simply be the way to do it.

maroider commented 3 years ago

@ArturKovacs You know what? You're probably right about logical_key, even though I find using logical_key for text input in this way to be an unfortunate decision.

ReceivedImeText seems like a good idea for now.

ArturKovacs commented 3 years ago

New API proposal after some discussion at #1788

NOTE: The following code has been edited to address comments that follow, so some comments may be obsolete.

Click for the code ```rust /// 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 { todo!() } fn from_scancode(scancode: u32) -> KeyCode { todo!() } } /// 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), /// 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 KeyEvent { pub physical_key: KeyCode, /// This value is affected by all modifiers except Ctrl. /// /// 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 Ctrl. /// /// For example, pressing Ctrl+a produces `Some("\x01")`. #[inline] fn text_with_all_modifiers(&self) -> &Option; /// This value ignores all modifiers including /// but not limited to Shift, Caps Lock, /// and Ctrl. 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 commented 3 years ago

Key::Character and KeyEvent.text

It's unclear to me why you're proposing &'static str in one place and String in another when you also write the following:

It was pointed out on several occasions by multiple contributors that having a heap-allocated object in this struct seems undesirable. Heap allocations are notorious for being slow. But slow is relative, and a heap allocation takes about a microsecond (or less) whereas key events are produced at most once about every 50 000 microseconds. That is, a heap allocation here has practically no negative effect.

key_without_modifiers

In case logical_key reports Dead, this will still report the real key according to the current keyboard layout. This value cannot be Dead.

I'm not quite certain why you're specifying this behaviour. The key in the base layer could still be a dead key, although not necessarily the one actively modifying the next keypress.

reset_dead_keys

I agree that it should be removed from the cross-platform API if it can't be reasonably implemented on every desktop platform. In that case, I'd still like to have it available through a platform-specific extension trait.

chrisduerr commented 3 years ago

// It was pointed out on several occasions by multiple contributors // that having a heap-allocated object in this struct seems // undesirable. Heap allocations are notorious for being slow. But // slow is relative, and a heap allocation takes about a microsecond // (or less) whereas key events are produced at most once about every // 50 000 microseconds. That is, a heap allocation here has practically // no negative effect.

Saying things are fine without actually having tested it seems very dishonest, especially when there's really not much of a reason why it would be necessary to use an allocated string here.

Also why is it not possible to just get the scancode? What if I don't want a KeyCode or NativeKeyCode?

Why is text_with_modifiers a method that does the same thing as the text variable which isn't a method, just adding in another modifier.

dhardy commented 3 years ago

fn text_with_all_modifiers(&self) -> &Option;

This API is restrictive: it should return Option<&String> (to allow simply returning None) or Option<&str> or just Option<String> (assuming it's rarely called, allocating on usage may be fine and may optimise out anyway — though this reasoning does fall foul of the previous comment).

Moreover though, I find the KeyEvent struct messy: it's not clear cut which fields should be public vs which should be platform-specific details and it requires quite a bit of translation up-front. I still think it would be better to expose everything through methods with an internal trait (for platform implementation).

Edit: the public methods in the API should have #[inline] since without it cross-crate inlining is not enabled without full LTO. For in-crate APIs it's probably better to just let the compiler figure it out.

ArturKovacs commented 3 years ago

It's unclear to me why you're proposing &'static str in one place and String in another

My reasoning for using &'static str is that it's easier to match on than a String. If you have a &'static str you can do the following.

match logical_key {
    Key::Character("a") => do_stuff(),
    _ => (),
}

This does not compile with Charater(String). I guess text could also be a &'static str but that field isn't meant to be matched on and using a String there simplifies the implementation as far as I can see. But I do agree that it's a bit odd to see three different character representations in this small API (the third one being Dead(Option<char>)). With all that considered, I don't really have a preference for one or the other so to satisfy the need to minimize heap allocations I changed the text to be &'static str.

The key in the base layer could still be a dead key, although not necessarily the one actively modifying the next keypress.

The US international layout is almost identical to the US layout; one difference is that while ' (apostrophe) is a dead key on the US internationaly layout it's a regular character key on the US one. If I use both layouts and I have a shortcut key combination including that character, it's best if the action gets triggered on both layouts. Of course the applications could add a branch for this manually but I don't think they should have to do that.

reset_dead_keys

Alright I updated the comment in the proposal, reflecting your suggestion.

Saying things are fine without actually having tested it seems very dishonest, especially when there's really not much of a reason why it would be necessary to use an allocated string here.

I'm trying to be as honest and objective as I can. I do make mistakes however. I changed the text field so that it is &'static str.

Also why is it not possible to just get the scancode? What if I don't want a KeyCode or NativeKeyCode?

We've been trying to focus on a minimal viable product and I don't remember this being raised earlier. Such a function can of course be added fairly easily so I added this as KeyCodeExtScancode::to_scancode to the proposal.

Why is text_with_modifiers a method that does the same thing as the text variable which isn't a method, just adding in another modifier.

I don't know of a reasonably simple way to deduce what the control character is, given the text field. For example on a Russian layout Ctrl+Ф has the same location as Ctrl+A on US layout and the two of the has the same effect in most programs on Windows. For example in CMD with the "Enable Ctrl key shortcuts" turned off, both of these produce the ^A character (character code 1).

chrisduerr commented 3 years ago

For example on a Russian layout Ctrl+Ф has the same location as Ctrl+A on US layout and the two of the has the same effect in most programs on Windows. For example in CMD with the "Enable Ctrl key shortcuts" turned off, both of these produce the ^A character (character code 1).

That sounds very wrong to me and I certainly wouldn't use cmd as a good example for anything at all. Though unfortunately I don't have access to any russians at the moment.

ArturKovacs commented 3 years ago

This API is restrictive: it should return Option<&String> (to allow simply returning None) or Option<&str> or just Option

Good point. Option<&str> seems best out of these.

Moreover though, I find the KeyEvent struct messy: it's not clear cut which fields should be public vs which should be platform-specific details

I don't know what you mean by this. Only platfom_specific is platform specific. The rest of the fields guarantee a reasonably platform agnostic behaviour.

the public methods in the API should have #[inline] since without it cross-crate inlining is not enabled without full LTO

Yeah, this seems very reasonable. I added it where it seemed reasonable (where the function returns a private field).

That sounds very wrong to me and I certainly wouldn't use cmd as a good example for anything at all

I'm experiencing the same effect on the default Terminal on macOS. Nevertheless we could remove text_with_modifiers and only add it when theres a specific demand for it.

ArturKovacs commented 3 years ago

I seemed to remember that someone specifically requested having a way to get the input which includes the ctrl chartarters but I wasn't sure. Now that I checked again, it was @kchibisov in this comment: https://github.com/rust-windowing/winit/issues/753#issuecomment-693280304 . Frankly I don't feel qualified at the moment to argue on either side, so I'd be interested in @kchibisov 's opinion here.

maroider commented 3 years ago

IIIRC, I was the one pushing to have all but a few control characters to be relegated to text_with_modifiers's predecessor key_with_all_modifiers (or something like that). After (re-)reading @kchibisov's comment, I think merging text_with_modifiers into text should be fine. The control characters don't seem to interact with dead keys at all (on Windows), which is the only thing I could think of which may have posed some sort of issue.

The following sequence where ~ is a dead key

  1. ~
  2. Ctrl+a
  3. a

should produce:

  1. ""
  2. "\u{0x1}"
  3. "ã"
chrisduerr commented 3 years ago

I seemed to remember that someone specifically requested having a way to get the input which includes the ctrl chartarters but I wasn't sure. Now that I checked again, it was @kchibisov in this comment: #753 (comment) . Frankly I don't feel qualified at the moment to argue on either side, so I'd be interested in @kchibisov 's opinion here.

Yes, of course it's required. Any application dealing with just raw text input will need this, especially terminal emulators. I'm just saying that pressing a key that isn't A with control and having it do the same thing as Ctrl+A seems very wrong to me.

ArturKovacs commented 3 years ago

I think merging text_with_modifiers into text should be fine

I like this. Removing unncecesarry things is always nice. Although there is one problem, which makes me a bit hesitant about merging the two. As far as I can tell, the web cannot forward control characters from control + key combinations, meaning that the text field would exhibit suprising differences between platforms. Perhaps the benefits outweigh the drawbacks, if they do documentation could just include something like

    /// ## Platform-specific
    /// - **Web:** There *must not* be any assumptions made whether a key
    /// pressed together with <kbd>Ctrl</kbd> would produce control characters or not.
    /// It is best to ignore this field while the `ModifiersState::CONTROL` modifier is active.
    pub text: Option<&'static str>,

I tried to make the wording such that once browsers add support for receiving the control characters, this field can be implemented identical to how it would be on the desktop and the extra note can be removed.

I'm just saying that pressing a key that isn't A with control and having it do the same thing as Ctrl+A seems very wrong to me.

I'm a bit confused and I don't want to ignore you, so please clarify: is this a personal opinion about those terminal emulators or are you arguing for something in relation to the API at hand?

ArturKovacs commented 3 years ago

There are two more topics I would like to discuss

Deserializing a Key

Due to the fact that it contains a static lifetime, it can only be deserialized from a static string/object. I believe this has very little utility in real world applications. We could remove the deserialize attribute, forcing applications to use either the KeyCode or a custom object representing Key in configuration files. The same applies to KeyEvent although I don't see any use for being able to (de)serialize such an object anyways.

Platform friendly line breaks in text

Following the discussion at #477 it seems very likely that most developers will expect text to contain a platform native line break, i.e. "\n" on Unix and "\r\n" on Windows. I think this would fit the current API really well because one could still easily find which key was pressed from the physical_key or the logical_key while this change in text would make text input handling easier downstream.

maroider commented 3 years ago

Platform friendly line breaks in text

If we go this route, then the separation between text and text_with_modifers should stay.

chrisduerr commented 3 years ago

We could remove the deserialize attribute, forcing applications to use either the KeyCode or a custom object representing Key in configuration files.

Winit should definitely be able to provide deserialization facilities for key bindings. One way or another it's going to be needed anyways, winit is the correct place to provide it. KeyCode is obviously not sufficient.

Following the discussion at #477 it seems very likely that most developers will expect text to contain a platform native line break, i.e. "\n" on Unix and "\r\n" on Windows.

I disagree with this. Nothing wrong with always sending \r, in fact it's the correct thing to do, at least on Linux. Sending \n on Linux isn't more correct, but the opposite.

ArturKovacs commented 3 years ago

[...] deserialization facilities for key bindings. One way or another it's going to be needed anyways, winit is the correct place to provide it.

I absolutely agree that this is going to be needed at some layer either way. I do thing though, that there's an important distinction between "key bindings" and a Key value. Key bindings can be a combination of Keys and I don't think that winit should provide any representation for key combinations. This is something that could be provided entiher by an "extension crate" or a higher level framework.

However I'm not fundamentally against allowing Key instances to be deserialized, but if we allow it, it should be done in a way that's practical for most (or all) real use cases. I don't think that only being able to deserialize from a 'static object is practical. So I can think of two solutions here. We either revert back to Character(String) or we make a pub enum GenericKey<T: AsRef<str>>. The second approach could be implemented along these lines:

Click for the code ```rust pub type Key = GenericKey<&'static str>; pub type DeserializedKey = GenericKey; macro_rules! define_generic_key { {$($key_variant: ident),+} => { #[derive(Serialize, Deserialize)] pub enum GenericKey> { Character(T), $($key_variant),+ } impl> GenericKey { fn matches>(&self, other: &GenericKey) -> bool { match self { GenericKey::Character(s) => { if let GenericKey::Character(o) = other { s.as_ref() == o.as_ref() } else { false } } $(GenericKey::$key_variant => { matches!(other, GenericKey::$key_variant) })+ } } } } } define_generic_key! { F1, F2 // ... } ```

I disagree with this. Nothing wrong with always sending \r, in fact it's the correct thing to do, at least on Linux. Sending \n on Linux isn't more correct, but the opposite

Consider tying your view to something external to you. For example "sending \r is better because that's what the OS sends anyways". With that said what is more correct for this field, in my opinion depends on what would be the cleanest and easiest to use. The two use cases for keyboard input are handling key bindings and inserting text. The physical_key and logical_key are designed for the former, while text is designed for the latter. I think it would be better to use platform native line breaks in the text field because that that minimizes the post processing needed for text input downstream.

chrisduerr commented 3 years ago

With that said what is more correct for this field, in my opinion depends on what would be the cleanest and easiest to use.

So a unified \r it is then?

I think it would be better to use platform native line breaks in the text field because that that minimizes the post processing needed for text input downstream.

What is a "text field"? Text input depends strongly on where it is used. For Alacritty at least \r would cause less work. Why would winit post-process it, just for applications to post-process it again. Unless the post-processing is always necessary, might as well delay it until you actually need to do it so you don't do it 17 times. Also pointlessly sending two characters when one is fully sufficient is again just causing useless trouble.

ArturKovacs commented 3 years ago

So a unified \r it is then?

Cheeky :D

For Alacritty at least \r would cause less work. Why would winit post-process it, just for applications to post-process it again.

Hmm I didn't know that. Also I just realized that even text editors might want to use something else than the platform native one, depending on the output file or user preference. Alright let's just use \r and have it be documented. I'll update the proposal in a minute.

Regarding deserialization, what do you think about using Character(String) or enum GenericKey<T: AsRef<str>>?

maroider commented 3 years ago

Couldn't we have Key simply be a enum Key<'a> { .. }, and define KeyEvent.logical_key as a Key<'static>?

chrisduerr commented 3 years ago

Regarding deserialization, what do you think about using Character(String) or enum GenericKey<T: AsRef>?

For me personally, that's just one more reason to use char really.

maroider commented 3 years ago

For me personally, that's just one more reason to use char really.

Which isn't really an option because of the web backend.

EDIT: Never mind about that other bit.

ArturKovacs commented 3 years ago

I updated the proposal: https://github.com/rust-windowing/winit/issues/753#issuecomment-753307584

(Oops I forgot that we already had documented the Enter key translating to \r)

Yeah, using Key<'a> looks much cleaner. It's not as flexible, it cannot be used like this: fn parse_config() -> Key but I think until we get a request to change it, this will suffice.

branpk commented 3 years ago

Due to the fact that it contains a static lifetime, it can only be deserialized from a static string/object.

You could always just leak the string. If excessive memory allocations are a concern, you could use a cache or interner.

maroider commented 3 years ago

You could always just leak the string. If excessive memory allocations are a concern, you could use a cache or interner.

It might then make sense to do something like this

#[derive(Deserialize, ...)]
pub enum Key {
    #[serde(deserialize_with = "deserialize_key_character")] 
    Character(&'static str),
    ...
}

and have deserialize_key_character use the HashMap of leaked strings we are going to have to use anyway to use &'static strs.

Another option would be something like the following:

pub enum Key<'a> {
    Character(&'a str),
    ...
}

impl<'a> Key<'a> {
    pub fn to_static(self) -> Key<'static> {
        match self {
            Self::Character(s) => // Use the `HashMap` of leaked strings to get a `'static str`
            k => k,
        }
    }
}

Of the two, I think I prefer the second approach since it reduces the amount of implicit leaking.

ArturKovacs commented 3 years ago

I'm not happy with either. The second would be sort-of okay, but you can't use the HashMap that winit uses internally because that's going to look like this HashMap<NativeKeyCode, &'static str> but you don't know what's the keycode (scancode) for the Key you are trying to make static. However you could use a global HashSet<&'static str> to at least avoid having duplicates among deserialized strings.

But even then what you wrote @maroider does not compile because due to k => k, the compiler requires self to have a static lifetime. So if we did this, we would either have to copy paste all variants of the enum or would have to do very similar macro magic to what I proposed with GenericKey. So I think that we should either use the GenericKey approach or we just go with Key<'a> without the to_static conversion.

maroider commented 3 years ago

We can't really use a HashMap<NativeKeyCode, &'static str> because the keyboard layout could change while the application is runnig. We'd have to use a HashSet<&'static str> in both cases if we want to avoid leaking more than we absolutely have to.

As for the lifetime thing: we could transmute the lifetime in the catch-all case, but a macro would indeed be better.

ArturKovacs commented 3 years ago

We can't really use a HashMap<NativeKeyCode, &'static str> because the keyboard layout could change while the application is runnig

I believe we have to use the map at least on Windows because if I'm not mistaken that's the only reasonable way to get the proper logical_key for key release events. (There are no WM_CHAR messages for key release). And the way you use a map correctly is that you detect if the current keyboard layout is identical to what you have in the map, and if it's not then you iterate through every possible virtual key value and query the scancode with MapVirtualKey and the character with ToUnicode. This is how Firefox does it too IIRC.

As for the lifetime thing: we could transmute the lifetime in the catch-all case

How do you know that that's sound?

maroider commented 3 years ago

I believe we have to use the map at least on Windows because if I'm not mistaken that's the only reasonable way to get the proper logical_key for key release events. (There are no WM_CHAR messages for key release).

Ah, you are of course correct here. But I think we'd also want a HashSet<&'static str>, anyway, to avoid repeatedly leaking the same strings after layout changes.

How do you know that that's sound?

Well, it's not my favorite trick, but it should be sound since we'd only be transmuting the lifetime on the variants without a reference.

pub enum Key<'a> {
    Character(&'a str),
    Dead(Option<char>),
    F1,
    F2,
    F3,
    ...
}

impl<'a> Key<'a> {
    pub fn to_static(self) -> Key<'static> {
        match self {
            Self::Character(s) => // Use a `HashSet` of leaked strings to get a `'static str`
            // SAFETY: This should be safe since 'a only means something for the `Character` variant.
            k => unsafe { std::mem::transmute::<Key<'a>, Key<'static>>(k) },
        }
    }
}

Using a macro for the conversion would of course be better.

ArturKovacs commented 3 years ago

I've been thinking about merging text_with_all_modifiers into text. It really doesn't make much sense to provide both on platforms that support text_with_all_modifiers. But merging the two is a bit awkward due to the web not providing the control-modified-character. Maybe the best approach is to provide the text exclusively through platform specific traits:

pub trait KeyEventExtModifierSupplement {
    /// ... affected by all modifiers including Ctrl ...
    fn text(&self) -> Option<&str>;
}

pub trait KeyEventExtWeb {
    /// ... affected by all modifiers except Ctrl ...
    fn web_text(&self) -> Option<&str>;
}
Rua commented 3 years ago

I came here wondering how to ignore keydown events if they are associated with character input. This is a rather large discussion, so what is the status on this currently?