Add keyboard modifiers to input event #112

rigtorp · 2017-01-17T18:19:52Z

Making applications track modifier keys results in unnecessary work for
consumers, it's error prone, and it turns out to have unavoidable bugs.
For example, alt-tabbing with x11 results in the alt modifier state
getting stuck.

To resolve these problems, this patch adds a Mods value to the keyboard
input event.

Based on this patch: jwilm/glutin@d287fa9

This PR resolves issues #77 and #108

tomaka · 2017-01-18T08:23:18Z

I have no clue what to do with modifiers.

When it comes to windowing, there are tons of concepts that have existed in the 90s and that are now considered obsolete. I'm not sure whether keyboard modifiers are one of them.

I don't really see why Ctrl/Alt/Shift should be treated in a special way. I'm not buying the "unnecessary work for the user" argument, because that could always be solved by adding some utility struct that tracks the state of the keyboard. The concern about alt+tabbing is justified, but it's maybe just a bug that can be solved in another way?

Usually when I don't know whether something is "modern" I look at what wayland does, because they are the only modern windowing system.
The wayland client-server API has no concept of a modifier (ie. ctrl/alt/shift are not special in any way), but the client-side library tracks the state of modifiers and sends additional events to the application in order to help. Unfortunately that's not really a clear commitment on one way or the other, so I'm still unsure.

rigtorp · 2017-01-18T16:44:27Z

I think ease of use for the API user is important. For a low-level component such as the Wayland protocol it makes sense to just send the key events. As a user of API providing cross-platform window creation and input handling it doesn't make sense if every program needs to have a state machine to track modifier keys.

jwilm · 2017-01-18T18:03:38Z

@tomaka

I'm not sure what you mean by Wayland not having a concept of modifiers. Looking at the protocol, there's a wl_keyboard::modifiers event which provides this state to the application. The wayland-kbd crate consumes both the normal key press event and the modifiers events in order to produce UTF-8 codes.

Windowing APIs on each platform provide access to the current modifiers when a key is pressed. It seems that you would prefer to ignore the data provided by the windowing system and try to reconstruct it ourselves from a series of key presses. But, there's evidence that the latter doesn't work in all situations. It seems that the only correct option is to somehow provide the modifier data from the windowing system.

If the only problem was ergonomics, I would agree with you about providing a sort of state tracking type that user's could opt into.

rigtorp · 2017-01-19T15:32:01Z

@tomaka @jwilm The state machine approach fails if you hold down Alt and then focus the window. If you press another key in this state it won't have the Alt modifier. There really is a reason that the platform windowing APIs provide the set of modifiers with each key press.

tomaka · 2017-01-19T15:48:09Z

I think ease of use for the API user is important.

Ease of use can be done in another way than giving out modifiers.

If all windowing APIs give you modifiers, then I think there's probably a good reason and winit should give them as well. So I guess I'm ok with the change.

Anyway, back to the PR:

I don't think we should use bitflags. Instead the mods should just be a struct containing bools. Of course it's less optimal, but for such a usage is really doesn't matter much and as you said being easy to use is important.
I'm bothered by the fact that it's only implemented on X11. There are already too many features that are only implemented on one platform because the people who added it only implemented the feature on their platform. I don't know how to "solve" this though. Requiring you to implement it on all platforms is probably too demanding, and TODOs would probably be ignored for a while.

rigtorp · 2017-01-19T17:10:45Z

As I see it there is no reliable way to write an application that uses Ctrl-, Alt- etc keyboard shortcuts using the current API.

Example keyboard event APIs:

The other APIs also provides the string representation of the key if it is a printable character. This allows the API user to do any type of complex keyboard event handling from this single event. I will add that too while we are making this API breaking change.

I agree that bitflags is not necessary, performance is not an issue here.

I only did X11 since I wanted to get the API itself reviewed first. I'll do the Wayland and Windows implementations, but I don't have access to MacOS so I can't implement that.

@jwilm What are your thoughts?

jwilm · 2017-01-19T17:33:43Z

@rigtorp

Tomaka just mentioned in the parent comment that providing the modifiers is fine; I'm not sure what you're arguing for in the first half of your comment.

The other APIs also provides the string representation of the key if it is a printable character. This allows the API user to do any type of complex keyboard event handling from this single event. I will add that too while we are making this API breaking change.

I've got a commit with this as well, but I'm not convinced passing the printable character in the event is the right solution because it would require a String. I was planning to open a separate issue to discuss it.

I only did X11 since I wanted to get the API itself reviewed first. I'll do the Wayland and Windows implementations, but I don't have access to MacOS so I can't implement that.

If you had asked me about this before opening the PR, I would have pointed out that there is a Cocoa implementation as well. The Wayland and Windows APIs are definitely unimplemented, though.

@tomaka

I don't think we should use bitflags. Instead the mods should just be a struct containing bools. Of course it's less optimal, but for such a usage is really doesn't matter much and as you said being easy to use is important.

This is your call. bitflags! has the benefit of being able to check for multiple modifiers in a single conditional and other checks like intersections. The API is generally pretty nice as well. Assuming Event::KeyboardInput has a mods parameter, code like this can be written:

if mods == SHIFT | CONTROL

Which is, in my opinion, more ergonomic than a struct.

if mods == Mods::new().shift().control()

It also ends up being more code for Glutin to maintain.

I'm bothered by the fact that it's only implemented on X11. There are already too many features that are only implemented on one platform because the people who added it only implemented the feature on their platform. I don't know how to "solve" this though. Requiring you to implement it on all platforms is probably too demanding, and TODOs would probably be ignored for a while.

Sorry about this. One of the reasons I hadn't opened a PR yet was specifically because it wasn't yet implemented on all platforms. Alacritty aims to support X11, Wayland, macOS, and Windows; we definitely want to implement these APIs on all platforms.

rigtorp · 2017-01-19T18:34:21Z

@jwilm I wanted to get early feed back from @tomaka

@tomaka Let us know what you want to do re bitflags vs struct

I will implement Wayland and Windows support and apply @jwilm macos patch.

tomaka · 2017-01-19T18:46:48Z

Definitely struct.

jwilm · 2017-01-19T19:10:15Z

I don't understand your insistence on a struct. It's going to end up as 4 bytes wide versus 1. The bitflags seem more ergonomic. It's less code for glutin/winit to maintain. Bitflags are a standard thing in the Rust ecosystem so users will immediately be familiar with them. You even said yourself that the struct is less optimal.

So far in this discussion, there has not been one point in favor of using a struct other than it being your preference. Yet, you insist on the struct as the solution here.

rigtorp · 2017-01-19T19:23:38Z

Example struct implementation:

#[derive(Default)]
struct Mods {
  shift: bool,
  ctrl: bool,
  alt: bool,
  meta: bool
}

let mods = Mods { shift: state & ffi::ShiftMask };

Doesn't seem too bad. I'm a C++ expert, not a rust expert, so I couldn't say what is the cleanest most idiomatic way. Considering memory and performance is irrelevant in this case. Only usage ergonomics and rust best practices are relevant.

tomaka · 2017-01-19T19:27:20Z

I disagree that bitfields are standard in Rust and that people are familiar with them.
Using a struct is not only easier to understand when looking at the documentation, but also more straight-forward when you do pattern-matching or when you want to build a struct yourself.

In the end I think it should be the job of the Rust compiler to optimize a struct made of four bools into a bitfield. Eventually in a few years we'll not only be optimal but also idiomatic.

jwilm · 2017-01-19T20:05:33Z

Using a struct is not only easier to understand when looking at the documentation, but also more straight-forward when you do pattern-matching or when you want to build a struct yourself.

I can appreciate that argument.

In the end I think it should be the job of the Rust compiler to optimize a struct made of four bools into a bitfield.

Agreed

Eventually in a few years we'll not only be optimal but also idiomatic.

This part is a bummer, but you've made a solid case for the struct.

Thanks for explaining your position.

Valloric · 2017-01-20T07:08:18Z

In the end I think it should be the job of the Rust compiler to optimize a struct made of four bools into a bitfield.

This can never happen for the same reasons no C++ compiler will ever do this: you can take the address of the bool field, thus it must be addressable (and be at least a byte big).

A struct of bools will never magically become a bitfield through compiler optimizations.

tomaka · 2017-01-20T07:29:39Z

This can never happen for the same reasons no C++ compiler will ever do this: you can take the address of the bool field, thus it must be addressable (and be at least a byte big).

Being able to take the address of a bool field is arguably not very useful. I can totally imagine this being changed in the future if you put some kind of #[repr()] attribute on the struct for example.

rigtorp · 2017-01-20T21:22:56Z

I refactored the code use a struct instead and added implementations for win32 and macos. I couldn't find a machine which can run Wayland (Mesa EGL issues) so I gave up on implementing for wayland.

@tomaka Unless you have additional concerns, this is ready to merge now. Remember to bump the major version since this breaks API compatibility.

Valloric · 2017-01-21T03:19:53Z

Being able to take the address of a bool field is arguably not very useful. I can totally imagine this being changed in the future if you put some kind of #[repr()] attribute on the struct for example.

Is it useful or not is entirely irrelevant; it exist in the language and thus needs to work so you won't see the optimization you're imagining. I'd put the chances of a repr attribute making parts of normal rust syntax not work with such structs at 0% chance of ever happening.

rigtorp · 2017-01-21T03:33:20Z

@Valloric It's the same constraint C++ has on bools, they have to be addressable. We're talking about keyboard events here. Saving some bits or allocations is irrelevant.

Valloric · 2017-01-21T04:31:56Z

@Valloric It's the same constraint C++ has on bools, they have to be addressable.

Exactly my point.

We're talking about keyboard events here. Saving some bits or allocations is irrelevant.

I have no horse in this race; I only wanted to dispel the fantasy that the struct-of-bools will magically become more compact now or in the future. It won't.

tomaka · 2017-01-21T08:21:09Z

it exist in the language and thus needs to work

We are controlling the language.

The language is not something mystical that doesn't obey the laws of nature. It's totally something that can be adjusted as long as backward compatibility is preserved.

tomaka · 2017-01-21T08:27:47Z

src/events.rs

@@ -312,3 +312,11 @@ pub enum VirtualKeyCode {
    WebStop,
    Yen,
 }
+
+#[derive(Default, Debug, Clone, Copy)]
+pub struct Mods {


This needs some documentation. Not everybody knows what a modifier is.
Also the fields should be 4-spaces indented, not 2.

Should I just call it KeyboardModifiers?

tomaka · 2017-01-21T08:27:57Z

src/events.rs

+  pub shift: bool,
+  pub ctrl: bool,
+  pub alt: bool,
+  pub supr: bool


I don't even know what supr is?

It is probably the Super key aka. the Windows key.

I looked into this and super key seems to be the most common platform neutral name for the windows / command key. Super is reserved so I named it supr.

Shame that super is a reserved keyword. I find supr quite a confusing name...

an other common name for this key (in linux world at least) is mod4 (alt is also mod1, I don't know what are 2 and 3), but I don't think it's a clear name either...

super_ would do the work, but is ugly ... I'm out of ideas here... At least it needs to be clearly documented what key it represents (telling in the docs that it is the "windows key" on most keyboards)

Other possible name: internally, xkbcommon calls it the "logo key", which is actually a pretty accurate name

tomaka · 2017-01-21T08:29:42Z

src/platform/macos/mod.rs

@@ -525,9 +526,9 @@ impl Window {

    unsafe fn modifier_event(event: id, keymask: appkit::NSEventModifierFlags, key: events::VirtualKeyCode, key_pressed: bool) -> Option<Event> {
        if !key_pressed && NSEvent::modifierFlags(event).contains(keymask) {
-            return Some(Event::KeyboardInput(ElementState::Pressed, NSEvent::keyCode(event) as u8, Some(key)));
+            return Some(Event::KeyboardInput(ElementState::Pressed, NSEvent::keyCode(event) as u8, Some(key), Mods::default()));


I'm not sure if Mods::default() here is an expected behavior.

You're right, it would not be consistent with x11 and Windows behavior

tomaka · 2017-01-21T08:30:19Z

src/api/wayland/keyboard.rs

@@ -35,7 +37,7 @@ impl wayland_kbd::Handler for KbdHandler {
            };
            let vkcode = key_to_vkey(rawkey, keysym);
            let mut guard = eviter.lock().unwrap();
-            guard.push_back(Event::KeyboardInput(state, rawkey as u8, vkcode));
+            guard.push_back(Event::KeyboardInput(state, rawkey as u8, vkcode, Mods::default()));


Please at least add a TODO if you can't implement on wayland.

tomaka · 2017-01-21T08:32:36Z

Some things I'm not sure about:

Does ctrl/alt/shift mean both left and right control/alt on all platforms?
Are the modifiers correct in a keyboard input event of that same modifiers? For example on a keyboard input that presses Ctrl, is the ctrl modifier correctly set in the event?

rigtorp · 2017-01-21T19:40:12Z

Yes the modifier is the union of all keys with the same name as currently implemented.

Yes on x11 and win32 the ctrl press event has the ctrl modifier.

elinorbgr · 2017-01-29T10:20:23Z

Regarding wayland, this requires some work on wayland-kbd (see Smithay/wayland-kbd#13 ). This is not a huge thing, but I need some time to do this.

I suggest you focus on merging that, and I'll make a PR later to add it. Just add a big TODO as @tomaka suggested, and we could open a follow-up issue.

NickeZ · 2017-02-07T12:07:28Z

Does ctrl/alt/shift mean both left and right control/alt on all platforms?

I'm not sure if this matters, but on my keyboard the right alt is "Alt Gr" and not the regular "Alt"

elinorbgr · 2017-02-12T16:21:51Z

Regarding wayland, I've just released wayland_kbd-0.7 which provides the modifiers state to the events handlers, with this updating the wayland backend should be completely straightforward: https://docs.rs/wayland-kbd/0.7.0/wayland_kbd/struct.ModifiersState.html

Note that I kept "shift" and "caps lock" separated, this might need some merge logic.

rigtorp · 2017-02-19T01:34:24Z

Code review comments fixed. Rebased to master.

rigtorp · 2017-02-27T02:09:29Z

@tomaka Comments?

tomaka · 2017-02-27T05:56:14Z

Please add a TODO comment in the wayland code so that we don't forget about the modifiers not being implemented.
Looks good otherwise.

mitchmindtree · 2017-02-27T09:14:30Z

src/platform/macos/events_loop.rs

+    }
+
+    ev_mods
+}


Not really important, but this whole function body could just be:

let flags = unsafe { NSEvent::modifierFlags(event) }; ModifiersState { shift: flags.contains(appkit::NSShiftKeyMask), ctrl: flags.contains(appkit::NSControlKeyMask), alt: flags.contains(appkit::NSAlternateKeyMask), logo: flags.contains(appkit::NSCommandKeyMask), }

Making applications track modifier keys results in unnecessary work for consumers, it's error prone, and it turns out to have unavoidable bugs. For example, alt-tabbing with x11 results in the alt modifier state getting stuck. To resolve these problems, this patch adds a Mods value to the keyboard input event. Based on this patch: jwilm/glutin@d287fa9

rigtorp · 2017-02-27T19:42:22Z

@tomaka added the TODO comments and simplified macOS implemention as suggested by @mitchmindtree

elinorbgr · 2017-03-02T18:45:53Z

Can we merge this if all is good? I'd like to build on it for the remake of the wayland backend to the new design.

Check for libEGL.dll This would allow rendering through Angle, then capturing and analyzing the WR results on Windows using powerful tools (like RenderDoc, GPU PerfStudio, PIX, NSight, etc). Upstream PR: rust-windowing/glutin#846  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/glutin/112)

…ctive-post-mul Add Perspective::post_mul

…g#112)

rigtorp mentioned this pull request Jan 18, 2017

Update Glutin alacritty/alacritty#234

Closed

rigtorp mentioned this pull request Jan 18, 2017

Windows Support alacritty/alacritty#28

Closed

tomaka suggested changes Jan 21, 2017

View reviewed changes

jwilm mentioned this pull request Jan 25, 2017

Implement xkbcommon stuff for Wayland alacritty/alacritty#354

Closed

elinorbgr mentioned this pull request Jan 26, 2017

Provide access to the modifier state Smithay/wayland-kbd#13

Closed

tomaka approved these changes Feb 27, 2017

View reviewed changes

mitchmindtree reviewed Feb 27, 2017

View reviewed changes

rigtorp added 4 commits February 27, 2017 13:36

Implement KeyboardEvent ModifiersState for X11

a2c6fb7

Implement KeyboardEvent ModifiersState for win32

5d41067

Implement KeyboardEvent ModifiersState for macOS

290040d

tomaka merged commit 4c6e4e8 into rust-windowing:master Mar 2, 2017

Ralith mentioned this pull request May 31, 2017

Improve keyboard event handling #108

Closed

maximbaz mentioned this pull request Jul 25, 2017

Shift + mouse select should work alacritty/alacritty#146

Closed

gilbertw1 mentioned this pull request Oct 20, 2017

Added modifier support to mouse events #328

Merged

francesca64 mentioned this pull request Dec 19, 2017

Missing "alt" release event, OR Why modifier key state should be part of keyboard input event #77

Closed

tmfink pushed a commit to tmfink/winit that referenced this pull request Jan 5, 2022

Merge pull request rust-windowing#112 from asajeffrey/geometry-perspe…

a0aeb12

…ctive-post-mul Add Perspective::post_mul

madsmtm pushed a commit to madsmtm/winit that referenced this pull request Jun 11, 2022

fix(linux): implement missing enabled/selected in menu (rust-windowin…

e0bfa02

…g#112)

+                  }
+                  ev_mods
+              }

Add keyboard modifiers to input event #112

Add keyboard modifiers to input event #112

Conversation

rigtorp commented Jan 17, 2017

tomaka commented Jan 18, 2017

rigtorp commented Jan 18, 2017

jwilm commented Jan 18, 2017

rigtorp commented Jan 19, 2017

tomaka commented Jan 19, 2017

rigtorp commented Jan 19, 2017

jwilm commented Jan 19, 2017

rigtorp commented Jan 19, 2017

tomaka commented Jan 19, 2017

jwilm commented Jan 19, 2017

rigtorp commented Jan 19, 2017

tomaka commented Jan 19, 2017

jwilm commented Jan 19, 2017

Valloric commented Jan 20, 2017

tomaka commented Jan 20, 2017

rigtorp commented Jan 20, 2017

Valloric commented Jan 21, 2017

rigtorp commented Jan 21, 2017

Valloric commented Jan 21, 2017

tomaka commented Jan 21, 2017 • edited Loading

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

tomaka commented Jan 21, 2017

rigtorp commented Jan 21, 2017

elinorbgr commented Jan 29, 2017

NickeZ commented Feb 7, 2017

elinorbgr commented Feb 12, 2017

rigtorp commented Feb 19, 2017

rigtorp commented Feb 27, 2017

tomaka commented Feb 27, 2017

Choose a reason for hiding this comment

rigtorp commented Feb 27, 2017

elinorbgr commented Mar 2, 2017

tomaka commented Jan 21, 2017 •

edited

Loading