Keyboard event rework #1049

raphlinus · 2020-06-20T00:38:25Z

As of this checkpoint, we generate an keyboard-types KeyboardEvent,
but don't plumb it through as the actual druid-shell keyboard event.

Also, there is no dead-key handling at this point.

Work towards #1040

As of this checkpoint, we generate an keyboard-types KeyboardEvent, but don't plumb it through as the actual druid-shell keyboard event. Also, there is no dead-key handling at this point. Work towards #1040

WIP, modifiers in mouse event aren't there.

Use the new keyboard-types KeyboardEvent for all of druid, including text input. This is still a mac-only implementation. There are a few loose ends and discussion questions.

WIP

This adds a Windows implementation. There are some missing pieces: Conversion of KeyCompare to ACCEL has a number of holes. Basically I don't think KeyCompare is the right type - I think it can be replaced entirely by keyboard_types::Key, but am still investigating. There are compiler warnings in the places where rework is needed.

Refine implementation of menu hotkeys etc.

The tests now pass on Windows, and documentation is improved, though I wouldn't be surprised if there were still gaps.

Make the menu implementation on mac use Key instead of Code, and get rid of references to KeyCompare.

Make menus on gtk use Key instead of KeyCompare. This should be a better implementation in any case, as Key is better than Code for identifying non-printing keys.

This improves the x11 key event handling beyond what it was before, though there is still a hardcoded keyboard map.

Fix clippy lint. Improve internal docs some. Also add changelog.

Also minor touchups to mac implementations, and formatting.

Switch web implementation of keyboard events to keyboard-types.

cmyr

Okay, this looks great.

The main question I have is around whether we want to use KeyboardEvent and Modifiers from keyboard_types directly; I could see some advantages to having a wrapper for Modifiers, and maybe a slightly different version of KeyboardEvent. This is largely about giving us control over the API, and also making it easier for us to make changes down the road without needing to worry about working with the upstream crate.

cmyr · 2020-06-26T14:18:51Z

druid-shell/src/hotkey.rs

-use crate::keycodes::KeyCode;
+use crate::keyboard_types::{Code, Key, KeyState, KeyboardEvent, Location, Modifiers};
+
+// TODO: fix docstring


druid-shell/src/hotkey.rs

druid-shell/src/lib.rs

cmyr · 2020-06-26T14:33:11Z

druid-shell/src/platform/gtk/keycodes.rs

-            equal => KeyCode::Equals,
-            BackSpace => KeyCode::Backspace,
+#[allow(clippy::just_underscores_and_digits, non_upper_case_globals)]
+pub fn raw_key_to_key(raw: RawKey) -> Option<Key> {


I wonder if this stuff could live in another crate or get upstreamed to keyboard_types or something? It does seem pretty broadly useful.

This feels like a dialog. Is this something that there would be interest in? So far my toes dipped in these waters have met a chilly reception. The major client would be servo, but they're on winit, for which adopting any of this stuff would be a fairly major change.

I think the best way to go about this is a blog and then discussion on reddit or whatnot.

cmyr · 2020-06-26T15:05:21Z

druid/src/widget/textbox.rs

@@ -134,7 +135,7 @@ impl TextBox {
            EditAction::ModifySelection(movement) => self.move_selection(movement, text, true),
            EditAction::SelectAll => self.selection.all(text),
            EditAction::Click(action) => {
-                if action.mods.shift {
+                if action.mods.contains(Modifiers::SHIFT) {


I don't love the ergonomics here, would be nice if we could just do action.has_shift() or something, or at least action.modifiers.includes_shift().

This is maybe related to the idea of still having our own KeyboardEvent type.

I agree with you that the bitflags ergonomics are a downgrade, but one reason I went with it is that it's very much standard Rust practice. I'm also worried about too much timtowtdi, especially if we start having methods on both Modifiers and types that contain it.

cmyr · 2020-06-26T15:48:09Z

druid-shell/src/platform/windows/keyboard.rs

+    WM_KEYDOWN, WM_KEYUP, WM_SYSCHAR, WM_SYSKEYDOWN, WM_SYSKEYUP,
+};
+
+use winapi::um::winuser::{


this can be combined with the block above?

It can be, but I had the vk's in their own block so that it wouldn't be just one mass of symbols. I'll make the change though, because it can create confusion.

cmyr · 2020-06-26T15:49:53Z

druid-shell/src/platform/windows/keyboard.rs

+    ///
+    /// There should be one of these per window. It loads the current keyboard
+    /// layout and retains some mapping information from it.
+    pub fn new() -> KeyboardState {


can this be pub(crate)? here and below

The reason this is not pub(crate) is that I want to keep the same code in win-win, where it's publicly exported. But maybe that should be a diff.

druid-shell/src/platform/windows/keyboard.rs

cmyr · 2020-06-26T15:53:33Z

druid-shell/src/platform/x11/keycodes.rs

+use crate::keyboard_types::{Code, Key, Location, Modifiers};
+use x11rb::protocol::xproto::Keycode;
+
+// TODO: there's duplication here with gtk, as hardware keycodes


maybe worth doing now? I would have a shared module in platform that is cfg'd for linux.

There's also sharing between mac and x11. I'm worried the cfg stuff will get tangly, but it's probably worth it because more than one source of truth is a real problem.

Minor cleanups in response to review. This does not include the creation of shared keyboard utilities, and doesn't touch the type questions.

Also clean up some files that should have been deleted.

As per discussion in PR and Zulip

cmyr

Finding this diff a bit hard to read, but this looks good; I think there's one little oversight but it seems good.

I'd like to just test out porting over runebender and see if I run into any headaches, then I'll feel confident approving. 😁

druid-shell/src/keyboard.rs

cmyr · 2020-06-29T14:24:47Z

Okay, I played around with moving runebender over to this, and I do have a few notes.

direct field access: I wonder if we should be using this less? It does cause headaches when we make changes. This includes a bunch of (perhaps all reasonable) name changes; mods is now modifiers, is_repeat is now repeat, etcetera. This does make migration more of a hassle. If we were using methods for most of these things it would be easier to just deprecate them.

reexporting typealiases with deprecation warnings: I think it might make sense to have something like,

#[deprecated(since = "0.7.0", note = "Use KbKey instead")]
pub type KeyCode = KbKey;
#[deprecated(since = "0.7.0", note = "Use Modifiers instead")]
pub type KeyModifiers = Modifiers;

in druid/src/lib.rs. Again, this just means there's a bunch less red squiggle when updating.

naming, more generally: MouseEvent uses mods, and KeyEvent uses modifiers. It's is_composing but not is_repeat. A few of these feel inconsistent.
IntoKey: since KbKey now uses a String internally, I think it might still make sense to have a KeyCompare trait? In particular we could implement this for &str, without allocating, so you could do event.matches(" "), instead of event.key == KbKey::Character(" ".to_string()) (or something).

If we're moving away from direct field access, it might make sense to use the same names we currently use, but to mark them as deprecated. Playing around, I ended up with something like,

#[derive(Clone, Debug, Default, Eq, Hash, PartialEq)]
pub struct KeyEvent {
    /// Whether the key is pressed or released.
    pub state: KeyState,
    /// Logical key value.
    #[deprecated(since = "0.7.0", note = "use key() method instead")]
    //FIXME: this should be named `key`; we use key_code to ease migration from 0.6
    pub key_code: KbKey,
    /// Physical key position.
    code: Code,
    /// Location for keys with multiple instances on common keyboards.
    location: Location,
    /// Flags for pressed modifier keys.
    #[deprecated(since = "0.7.0", note = "use modifiers() method instead")]
    pub mods: Modifiers,
    /// True if the key is currently auto-repeated.
    #[deprecated(since = "0.7.0", note = "use is_repeat() method instead")]
    pub is_repeat: bool,
    /// Events with this flag should be ignored in a text editor
    /// and instead composition events should be used.
    is_composing: bool,
}
#[allow(deprecated)]
impl KeyEvent {
    /// Flags for pressed modifier keys.
    pub fn modifiers(&self) -> Modifiers {
        self.mods
    }

    /// Return the `KbKey` for this event.
    #[inline]
    pub fn key(&self) -> &KbKey {
        &self.key_code
    }

    /// Physical key position.
    pub fn code(&self) -> Code {
        self.code
    }

    /// Location for keys with multiple instances on common keyboards.
    pub fn location(&self) -> Location {
        self.location
    }
  // etc
}

This doesn't feel great, and may not be worth it, but maybe it is? I'm not sure. The idea would be that after the next release we would make these fields private.

In any case, offering this up for discussion. If some of this is compelling, I can put up a patch.

The "modifiers" field of KeyEvent becomes "mods", and there are deprecated aliases for KeyCode and KeyModifiers, to help with migration.

raphlinus · 2020-06-29T17:20:15Z

I'm not sure how much a KeyCompare trait will help. It doesn't in and of itself reduce allocations when constructing a HotKey, which is the main place IntoKey is used now. (We could of course use a Cow or some TinyStr variant if we're making our own type). I do very much appreciate the desire to be able to match a KbKey without allocation - that might be another question for upstream keyboard-types.

cmyr

Looks good, happy to get this in.

On KeyCompare and allocation: in this world HotKey wouldn't always construct a Key; it might just hold on to a Cow<str> or something, and then we would use that when matching the Key.

In any case this can be future work, I don't want to try and speculate on a design without having played with it more. :)

raphlinus · 2020-06-29T18:09:02Z

Yes, it is I believe possible to improve the implementation of HotKey to allocate less without much changing the external interface, and the pattern of HotKey.matches() pattern seems like it might be the best way to express the match (obviously the other choice is to have a method on KeyEvent or possibly KbKey, the latter of which would require an extension trait or upstream impl).

raphlinus added 4 commits June 19, 2020 17:37

Start work on keyboard rework for mac

29cc45a

As of this checkpoint, we generate an keyboard-types KeyboardEvent, but don't plumb it through as the actual druid-shell keyboard event. Also, there is no dead-key handling at this point. Work towards #1040

Start plumbing new keyboard events through

76c45e3

WIP, modifiers in mouse event aren't there.

Plumb new keyboard types all the way through

7b9e748

Use the new keyboard-types KeyboardEvent for all of druid, including text input. This is still a mac-only implementation. There are a few loose ends and discussion questions.

First draft of gtk keyboard rework

70f95dc

WIP

raphlinus force-pushed the keyboard branch from 30efa80 to 70f95dc Compare June 20, 2020 20:28

raphlinus added 11 commits June 23, 2020 18:35

Replace KeyCompare with Key

3960f47

Refine implementation of menu hotkeys etc.

Fix tests

7e5d787

The tests now pass on Windows, and documentation is improved, though I wouldn't be surprised if there were still gaps.

Fix mac menus

76e3ad2

Make the menu implementation on mac use Key instead of Code, and get rid of references to KeyCompare.

Fix gtk menus

f44baf4

Make menus on gtk use Key instead of KeyCompare. This should be a better implementation in any case, as Key is better than Code for identifying non-printing keys.

Merge branch 'master' into keyboard

057b027

x11 keyboard implementation

a290672

This improves the x11 key event handling beyond what it was before, though there is still a hardcoded keyboard map.

Merge branch 'master' into keyboard

b9e00b9

Minor touchups to Windows impl

8b03eed

Fix clippy lint. Improve internal docs some. Also add changelog.

Fix book examples

f55cd3b

Also minor touchups to mac implementations, and formatting.

web implementation of keyboard-types

a1c89e2

Switch web implementation of keyboard events to keyboard-types.

raphlinus marked this pull request as ready for review June 25, 2020 04:41

raphlinus mentioned this pull request Jun 25, 2020

KeyEvent rework #1040

Open

cmyr reviewed Jun 26, 2020

View reviewed changes

raphlinus added 3 commits June 26, 2020 10:16

Review feedback

c3333fe

Minor cleanups in response to review. This does not include the creation of shared keyboard utilities, and doesn't touch the type questions.

Merge branch 'master' into keyboard

768e454

Elide unneeded lifetime in IntoKey

1902335

raphlinus mentioned this pull request Jun 26, 2020

Modifiers ergonomics improvements pyfisch/keyboard-types#7

Open

raphlinus force-pushed the keyboard branch 3 times, most recently from 27ebcee to 0c8cfa3 Compare June 26, 2020 18:55

Factor duplicate keyboard stuff into shared

a431c81

Also clean up some files that should have been deleted.

raphlinus force-pushed the keyboard branch 3 times, most recently from 01abeb6 to 47667d4 Compare June 26, 2020 22:01

raphlinus changed the title ~~Start work on keyboard rework for mac~~ Keyboard event rework Jun 26, 2020

Wrapper types for keyboard events

06f06c2

As per discussion in PR and Zulip

raphlinus force-pushed the keyboard branch from 47667d4 to 06f06c2 Compare June 26, 2020 22:19

cmyr reviewed Jun 27, 2020

View reviewed changes

druid-shell/src/keyboard.rs Show resolved Hide resolved

druid-shell/src/keyboard.rs Show resolved Hide resolved

raphlinus added 2 commits June 27, 2020 12:55

Use more ergonomic modifier queries

00cfa18

Fix lowercase warning on hotkeys

536da90

Renames as requested in review

a3aa5bf

The "modifiers" field of KeyEvent becomes "mods", and there are deprecated aliases for KeyCode and KeyModifiers, to help with migration.

raphlinus force-pushed the keyboard branch from 3f71153 to a3aa5bf Compare June 29, 2020 16:54

cmyr approved these changes Jun 29, 2020

View reviewed changes

raphlinus merged commit e03c8f3 into master Jun 29, 2020

raphlinus deleted the keyboard branch June 29, 2020 20:23

raphlinus mentioned this pull request Jun 29, 2020

Hotkey mapping on non-Latin keyboard layouts #1069

Open

azymohliad mentioned this pull request Jul 4, 2020

Event::KeyDown reports incorrect repeat value after holding multiple keys at specific order #1080

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyboard event rework #1049

Keyboard event rework #1049

raphlinus commented Jun 20, 2020

cmyr left a comment

cmyr Jun 26, 2020

cmyr Jun 26, 2020

raphlinus Jun 26, 2020

cmyr Jun 26, 2020

raphlinus Jun 26, 2020

cmyr Jun 26, 2020

raphlinus Jun 26, 2020

cmyr Jun 26, 2020

raphlinus Jun 26, 2020

cmyr Jun 26, 2020

raphlinus Jun 26, 2020

cmyr left a comment

cmyr commented Jun 29, 2020

raphlinus commented Jun 29, 2020

cmyr left a comment

raphlinus commented Jun 29, 2020 •

edited

Loading

Keyboard event rework #1049

Keyboard event rework #1049

Conversation

raphlinus commented Jun 20, 2020

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmyr left a comment

Choose a reason for hiding this comment

cmyr commented Jun 29, 2020

raphlinus commented Jun 29, 2020

cmyr left a comment

Choose a reason for hiding this comment

raphlinus commented Jun 29, 2020 • edited Loading

raphlinus commented Jun 29, 2020 •

edited

Loading