Experiment with an improved event design

[Osspial]

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

This PR is a draft for an updated event design that tries to fix the old event API design's numerous flaws. These include:

• There are too many structural enum variants. Structural variants are annoyingly verbose to match on, and have been universally replaced with tuple variants.

• Many events aren't flat. Several events contain other structures or enums that need to be imported separately to get matched on, which is an absolute PITA. Examples:

  • If you're matching WindowEvent::MouseInput and want to check if the left mouse button was pressed, you need to import two other types (ElementState and MouseButton). Those shouldn't be necessary.
  • It's impossible to match on keyboard input or touch input without importing separate KeyboardInput or Touch types, which could be removed from the API completely.

  Extraneous types that need to get imported separately have been reduced to an absolute minimum.

• Events expose useless data. Most WindowEvents have a DeviceId field that just gets set to a meaningless values on all backends except X11, and an arbitrary set of events include ModifiersState despite ModifiersChanged existing. Those have been removed.

• There's a lot of inconsistency and redundancy. "Mouse" and "Cursor" are used for different purposes, and it's difficult to remember which one is used where. Touch events and mouse events have completely distinct APIs, despite being used for the same thing in a lot of cases. "Touchpad pressure" and "touch pressure" are two entirely distinct events with no overlap. All of these events have been pulled into a consistent set of Pointer events.

• DeviceEvent doesn't have a design - it's a blob of variants thrown together into one thing. A "Device" isn't a good unit of abstraction, since there are several different device types with almost entirely orthagonal usecases. DeviceEvent has been split into RawPointerEvent and RawKeyboardEvent, and variants that were rarely emitted have been removed. DeviceEvent::Motion has been removed, since the current design is pretty much unusable, it was only emitted on X11, and gamepad support is on the radar.

• Event::(Suspended|Resumed) don't really belong in the root Event enum, and have been moved into an AppEvent variant. AppEvent could in theory be used in the future for additional application lifecycle events.

• WindowEvent's lifetime. Not being able to clone or buffer events is annoying, and the events that do need that lifetime should be quarantined off from the ones that don't. Lifetimed window events have been split into WindowEventImmediate.

I think that's a fairly comprehensive list. Please review the API and provide feedback if you have any issues.

Also, PressFlags are used instead of booleans in the button press events, since booleans are absolutely awful to match on when they're in tuple variants, as true and false doesn't have any inherent semantic meaning when casually reading the source code.

[elinorbgr]

I have not precisely reviewed the actual detailed code, but overall these principles get a :+1: from me.

---

There is just one thing I'd like to mitigate (but it's clearly on the nitpick level):

There are too many structural enum variants. Structural variants are annoyingly verbose to match on, and have been universally replaced with tuple variants.

In terms of matching, I don't think structural patterns are more verbose thant tuple ones, as long as you don't need to rename the fields:

match event {
    // structural variant
    MouseWhell { device_id, delta, phase, modifiers } => {/* ... */},
    /// tuple variant
    MouseWhell(device_id, delta, phase, modifiers) => {/* ... */}
}

And structural patterns come with the advantage of being imo more ergonomic when you only need to process a subset of the fields, and you get the additional bonus of not needing to match the exact order of the fields:

match event {
    // structural variant
    MouseWhell { delta, ... } => {/* ... */},
    /// tuple variant
    MouseWhell(_, delta, _, _) => {/* ... */}
}

[Osspial]

@vberger I don't think those complaints are valid when the new API is taken as a whole, since events have been flattened out significantly. Compare the old MouseWheel event:

MouseWheel {
    device_id: DeviceId,
    delta: MouseScrollDelta,
    phase: TouchPhase,
    modifiers: ModifiersState,
},

pub enum MouseScrollDelta {
    LineDelta(f32, f32),
    PixelDelta(LogicalPosition<f64>),
}

to the new scroll events:

ScrollStarted,
ScrollDiscrete(Vector<i32>),
ScrollSmooth(Vector<f64>),
ScrollEnded,

pub struct Vector<T> {
    pub x: T,
    pub y: T,
}

You don't have to worry about how ergonomic it is to ignore data when the events don't contain data that needs to be ignored. The event flattening also helps out with field ordering; no event has more than three fields, and only three events have more than two fields:

WindowEvent::KeyPress(LogicalKey, ScanCode, PressFlags)
WindowEvent::PointerPress(PointerId, PointerButton, PressFlags)
RawKeyboardEvent::Press(Option<LogicalKey>, ScanCode, RawPressFlags)

Personally speaking, I find it much easier to remember "the first field has the keycode, the second field has the scancode, and the third field has boolean flags" than it is to remember the exact names for each of the fields. I constantly get tripped up when matching on the events from memory, since the exact names for things just don't stick, and additionally it's hard to remember which particular events are tuple variants and which are structural variants.

In terms of matching, I don't think structural patterns are more verbose thant tuple ones, as long as you don't need to rename the fields:

This is true if you're just accessing each field, but is not the case if you're matching on each particular field. Compare matching on an A key press in the old API vs the new one:

// old
WindowEvent::KeyboardInput(KeyboardInput{virtual_keycode: Some(VirtualKeyCode::A), state: ElementState::Pressed, ..}) => ()

// new
WindowEvent::KeyPress(Some(LogicalKey::A), _, flags) if flags.is_down() => (),

That's admittedly a somewhat unfair comparison, since the new API does a lot of additional stuff to reduce verbosity, but it should still be clear that even with noise reduced a minimum tuple variants are significantly more readable than structural variants.

[Ralith]

WindowEvent::KeyPress(Some(LogicalKey::A), _, flags) if flags.is_down() => ()

Some alternative conceptions, since if we're going to break things we should not consider the status quo to be the only alternative to the proposal:

WindowEvent::KeyPress { key: Some(LogicalKey::A), is_down: true, .. } => () WindowEvent::KeyPress(e) if e.key() == Some(LogicalKey::A) && e.is_down() => ()

Both of these similarly require no extra imports, have better forwards-compatibility with future additions, and don't require the reader to guess at the meanings of non-enum (e.g. button number) fields. The former is shorter than the proposal.

[ryanisaacg]

@Ralith's proposed design:

WindowEvent::KeyPress(e) if e.key() == Some(LogicalKey::A) && e.is_down() => ()

appeals to me, especially for the reason of forwards-compat.

[OvermindDL1]

I especially like the WindowEvent::KeyPress { key: LogicalKey::A, is_down: true, .. } => () one, it's nice to put all the matching code in the head. Though to be honest the great majority of the time will be passing them through an action mapper instead of handling them directly.

[Ralith]

In lieu of #[non_exhaustive] (which we may want to avoid for MSRV reasons per discussion in #1355), struct variants do not have genuine forwards-compatibility, because exhaustive matches can be written, but non-exhaustive matches will still be more common, and upgrade friction therefore lower, than would be possible with tuple variants. Conversely, newtype variants offer strong forwards-compatibility in the sense that a semver break is not required for new fields to be added.

[Osspial]

@Ralith's proposed design:

WindowEvent::KeyPress(e) if e.key() == Some(LogicalKey::A) && e.is_down() => ()

appeals to me, especially for the reason of forwards-compat.

I also like that idea, and I've drafted it out for the key-press and pointer-press events. How does this new implementation look to you all?

[Ralith]

Overall those examples look good to me: forwards-compatible without unduly compromising ergonomics.

[chrisduerr]

Besides the points noted by @kchibisov already, there are two other things that seem a bit odd to me.

I'm not really familiar with winit and Wayland (@vberger probably knows more about this), but wouldn't the removal of device ID prevent multiseat support? That seems like a bad idea.

Converting values like button from an enum to a u8 also seems like a strange choice, because at that point you're giving up some great typesystem advantages for the sake of not having to add an import? That seems very strange to me. For a boolean like is_down I can see where this idea is coming from, but for a button it just seems strange.

[Ralith]

Food for thought: an enum could be compared (but not matched) against concisely and without imports by writing as u8.

[Osspial]

I'm not really familiar with winit and Wayland (@vberger probably knows more about this), but wouldn't the removal of device ID prevent multiseat support? That seems like a bad idea.

DeviceIds and SeatIds are distinct concepts and it's misleading from an API design perspective to associate one with the other. I don't know how multiseat support is going to end up being exposed, but I don't think it's a good idea to keep DeviceIds in the API in the meanwhile when doing so poses the problems mentioned in the OP.

Converting values like button from an enum to a u8 also seems like a strange choice, because at that point you're giving up some great typesystem advantages for the sake of not having to add an import?

There's an additional reason which, looking back, I think I forgot to mention. It doesn't make sense to expose the button type as a MouseButton enum, since this overhaul unifies mouse, touch, and (in the future) pen events into a single event class, and the button field represents button state from all of those different input types. The design here was pretty heavily inspired by the web pointer event API, and we're exposing the button state in a fairly similar way to how they do it - 0 represents left-mouse-click/touch contact/pen contact, 1 represents right-mouse-click/pen barrel button, etc. That's done so that naive code written for just one pointer type automatically get a basic level support for the other pointer types.

Still, that's an entirely valid point about type safety. I've redefined the button field to take a PointerButton type here, and added is_{button} methods to allow code to inspect the button variants without importing the button type directly. How does that look to you all?

[Ralith]

I rather liked the maximal concision of == 0, but it's hard to argue with the readability of named constants/predicates, especially since my guess for which integer represented "middle" was evidently wrong. I appreciate that this is forwards-compatible with larger numbers of buttons.

[Osspial]

I've gotten this compiling on Windows, though I still need to some testing (particularly with touch events). I've updated the examples, and they provide good samples for how the new API would impact things.

[Osspial]

I've added code to ensure that the new PointerButton type can deserialize from and (optionally) serialize to the old MouseButton type, to try and minimize breakage to user config files.

[kchibisov]

@Osspial I can contribute Wayland impl once you've done with API on this branch, just ping me at some point, I guess.

[OvermindDL1]

A curiosity, does this give an ID to mice as well? I.E. would multiple mice (on system types that support it like X11 or wayland, or windows with an admin callback hook) be able to distinguish the different mice? I exceptionally hate having to bypass windowing mice handling to be able to support this feature so it would be nice to actually be built in for once.

[dhardy]

@OvermindDL1 it looks like it, given that a PointerId may be a MouseId, which is a platform-defined type (only the Windows variant is implemented here, and that's a unit type, but on X11 it may not be). I am curious how this works with the app though — does winit synthesise a PointerCreated event for each mouse? Is there an easily identifiable "primary pointer"? And how useful is multiple-pointers-per-display anyway (it's such an obscure feature I would guess it's also buggy)?

[OvermindDL1]

And how useful is multiple-pointers-per-display anyway

If a program isn't built to handle them then they usually just handle the 'first' mouse or just kind of combine all their inputs together (bleh). For programs that are aware of it as I try for mine to be then they work quite well and has some very useful usecases even outside of games.

[dhardy]

Multi-mouse may not be exclusive to Linux either (though this sounds limited): https://www.mousemux.com/

[kchibisov]

It's not that buggy on Wayland, but I'm not sure that it's that supported across compositors, in general, multiseat(aka multiple keyboard + pointers) is a thing. winit side here will be simple (when it comes to Wayland at least), since everything is already handed by you, and you know that you, let's say, have multiple pointers, if you have multiple seats with pointer capability. The pointer/keyboard event are per seat, so the only thing where winit should likely do a bit more work here, is 'proper id' for them(not sure how it'll work).

[OvermindDL1]

Plus you want a good DE that's supporting it well as well, otherwise yes, as like on the link it can cause flickering of which program has mouse 'activity'. KDE used to be the best supported, wonder if it still is... Although if both mice are being used in the same program then that's not an issue anyway.

[dhardy]

Just tested on X11 + KDE: it works, but the second cursor causes horrible flickering. Additionally, it appears the keyboard should only follow the first pointer, but the second can shift keyboard focus within most apps, and sometimes it even gets stuck in the wrong app.

Conclusion: it's horribly buggy, as expected, and I doubt it will ever get fixed properly (it's old and thus far remained a barely known niche feature), thus I see little point caring.

[OvermindDL1]

thus I see little point caring.

This is the reason why the support is spotty between apps. Even still, within a single app they work perfectly, and this is my uses for it.

[DorianRudolph]

I implemented stylus support for X11 which might be helpful in porting this to X11 (it looks like only windows is implemented right now) https://github.com/DorianRudolph/winit/tree/stylus

[0x182d4454fb211940]

Hi can I ask what the state of this pr is? I'm interested in using winit in a stylus-based project and so I'd be willing to help with this if needed. I tried suggesting an API in #3001 but this looks more comprehensive.

[kchibisov]

I could say for sure that it's unlikely to get rebased 0x182d4454fb211940 , but you could use it as basis to sketch API.