Input Refactoring

[psFried]

There's a few issues with the way that input is currently handled.

• There are no high-level abstractions over common inputs such as mouse drags, double clicks, or even regular mouse clicks. This adds complexity to any widget that wants to handle these types of events.
• Widgets currently make explicit calls to capture the keyboard/mouse in order to receive input events from them. This also adds unneeded complexity.
• It's currently rather difficult to handle programmatically generated events such as tabbing between widgets, scrolling, etc.

I already took a stab at refactoring input events ( #663 ) and discussion on that PR indicated that we could come up with a more general approach that would allow for all types of events to be handled by widgets. The best idea so far would be to have the Ui store a Vec<T: GenericEvent> of all events that have occured since the last update. We would then have a wrapper around that information that would peek at those events and add high-level events to that stream as appropriate. The idea would be for widgets to get all events from a single source and just provide a clean api for them to filter out only the events they care about. This might look something like:

fn update<C: CharacterCache>(self, update_args: UpdateArgs<Self, C>) {
  for event in update_args.events() {
    event.left_click(|position| {
      // closure only called if event is a left click
      handle_left_click(position, update_args.state);
    });

    event.drag(|drag_info| {
      // would be called if this event is a drag event
      handle_drag(drag_info.from, drag_info.to, update_args.state);
    });
    ...
}

The conditional closure thing seems to be the established pattern for handling different types of events, and it makes it very clear which events the widget cares about. Ideally, we would add events for things like mouse clicks, double-clicks, drags, widget taking/losing focus. These wouldn't have to be added all at once, though.

The wrapper around this Vec<T: GenericEvent> would have to do two things, though. It would need to 'interpret' a series of event in order to provide the high-level (click, drag, etc.) events, and it would also need to provide those events relative to a specific widget, filtering out the events that don't apply to the widget. It may eventually also be useful to provide feedback on which events were handled in order to support things like event 'bubbling'. I'm thinking that it makes sense to just have single conrod events trait that defines/re-exports all the methods for filtering the events using the callback pattern.

This will be a pretty big refactor, but I think well worth it. It shouldn't immediately cause any breaking changes. Eventually, though, we should be able to completely eliminate the Mouse class since everything could more easily be handled just using the events. I'll try to start on this as soon as I have time. I'll post a link to my working branch whenever I get it pushed up so people can start commenting on specifics.

Edit:

The initial PR to add event based input handling (#684) is getting merged. We still have some widgets that need to get switched over to use the new WidgetInput methods instead of the old UserInput. Once everything is using the new input handler, then UserInput will be removed. This issue will track progress towards that.

Things that need switched over:

• Button
• DropDownList
• EnvelopeEditor
• NumberDialer
• Slider
• Tabs (maybe?)
• TextBox
• TitleBar
• Toggle
• XYPad
• Custom widget example
• Change draggable widgets to use new input methods

[mitchmindtree]

Nice, this all sounds really promising :)

I'm still unsure about storing generic-ly typed events - this may add to the type param noise significantly throughout conrod, increasing the initial perceived complexity. We wouldn't be able to do things like capturing/uncapturing for custom events, without providing some way for the user to do this from their custom widgets that require these custom events after having downcast them.

I think I'd prefer to leave fully generic events as a non-goal of conrod, and instead store a Vec of something like the piston Input event. This should make it a little easier to reason about capturing, filtering, etc and remove the need for type params. If in the future we come across a really motivating reason for custom event support, perhaps we can add a Custom variant that stores unique unrecognised input events as Box<GenericEvent>, allowing the user to downcast and handle these themselves.

For higher-level events (where we do our own logic to create widget-specific events related to capturing etc, like dragging, double-clicking, etc) I'm unsure whether these would be better as part of the same enum, or whether it would be better to have them as a separate event type that is produced? Perhaps the events that we store could be some conrod-specific input event like this:

pub struct Input {
    time: Time,
    kind: Kind,
}

pub enum Kind {
    Raw(piston::input::Input),
    HighLevel(HighLevel),
}

or perhaps it would be better to store both in their own Vecs? This might require some experimenting/research to work out what suits us better.

Anyway, I'm really excited about this! Thanks a lot for taking the time to think this through and have a go, it's been on the back of my mind for a while now. I think it'll be a huge benefit offering these high-level abstractions along with a more event oriented approach, and will likely reduce a lot of the load in widget implementations 😸

BTW, the UserInput type (in ui.rs) might be a good type to refactor as the wrapper around the Vec of events - it's already set up to be delivered to widgets during their Widget::update for handling (its current functionality is just a lot more primitive than what we're discussing here).

[bvssvni]

Unfortunately GenericEvent can not be used as a trait object. Perhaps we could redesign it?

Alternative is create a new trait that is implemented for all T: GenericEvent, which can be used as a trait object but does not require construction.

[psFried]

Looking at GenericEvent, I feel a little torn. On the one hand, having a trait object seems like it would offer the best mix of extensibility and convenience. Redesigning GenericEvent seems like a big undertaking and a hugely invasive breaking change.

@bvssvni Creating a new trait that can be used as a trait object sounds like it would give us what we want (dynamic dispatch), but I don't have a good enough understanding of trait objects and 'object-safety' to know for sure if it's possible. I'd need to read up/experiment.

Ultimately, I do feel like an enum would be the best way to store events and provide them to widgets just because it enables widgets to use pattern matching. Something like:

// Approach 1
for event in update_args.input().events() {
  match event {
    Raw(Move(MouseCursor(x, y))) => { /* handle raw event */ },
    HIghLevel(Click(x, y)) => { /* handle high level event */ },
    _ => {}
  }
}

Seems better than:

// Approach 2
for event in update_args.input().events() {
  event.mouse_cursor(|x, y| { /* handle raw event */ });
  event.left_click(|x, y| { /* handle high level event */ });
}

I thought about just making something like:

pub enum Event<T: GenericEvent> {
  Raw(T),
  HighLevel(HighLevelEvent) // where HighLevelEvent is some sort of enum
}

The problem with that approach is you can't filter GenericEvents and enums the same way. This would make it more difficult to provide a simple and clean api for widgets to filter out the events they care about. My guess is that we'd end up with something like 'Approach 2' for handling all events. One option would be to take @mitchmindtree suggestion and have the Raw type just wrap a piston::input::Input instead of GenericEvent.

[mitchmindtree]

For the record, I'm considerably more in favour of using the piston::input::Input event instead of dynamically dispatched events, at least to start with.

So far, we haven't run into any need for (or had any requests for) events that piston itself doesn't already provide, so I think it's safe to use the Input enum for now.

Also, remember that if a user does require some special widget to handle some fancy event, it is trivial for a user to pass their own unique/custom events directly to the arguments of the widget due to conrod's immediate API. I think I'd much prefer requiring a user to do this, rather than providing full dynamically dispatched events for the rare case that a user would find it slightly more convenient.

API-wise, going with the Input option allows for using either Approach 1 or Approach 2 at the user's discretion. However, I imagine the kind of helper methods that we will provide will greatly reduce the number of occasions that a user needs to match directly on the events anyway.

[mitchmindtree]

@psFried btw, you might already know this, but can do rust syntax highlighting on your code blocks by adding rust after the first three back ticks that indicate the code block, i.e.

    ```rust
    // code
    ```

[psFried]

@mitchmindtree I agree. Looking more into GenericEvent, I think just using Input gives us everything we need for right now. If we decide to support arbitrary custom events later, we could easily add another type to wrap a GenericEvent later. For now, though, handling Input as well as the new high-level events should cover the vast majority of use cases.

Any thoughts on what to call these new high-level events? I kind of hate the name HighLevelEvent. Maybe:

pub enum ConrodEvent {
  Raw(piston::input::Input),
  Interpreted(WidgetEvent)
}

Not sure I'm too hot on those names either...

Thanks also for the syntax highlighting tip. That was news to me.

[bvssvni]

@mitchmindtree That seems like a reasonable assumption for now. Can use Input and switch later if needed.

One idea is to use same naming convention as in piston-input, where names ending with Event are traits. I recommend using traits for methods here, since you can redesign the enum without breaking too much code. Could be ConrodInput or simply Input.

[psFried]

One thing I'm not quite understanding is the difference between some of the input::Input enums. Is input::Input::Text(String) really just an aggregation of all the Press/Release events? If that's the case, then it seems like that one is already sort of a high-level event. Just looking for a confirmation if that's how it works.

Also, coercing a T: GenericEvent back into an input::Input is actually pretty ugly. I think that the best approach is to just create new Inputs in each of the closures in Ui::handle_event like:

fn handle_event<T: GenericEvent>(&mut self, event: T) {
  event.resize(|w, h| {
    self.widget_events.push_event(Input::Resize(w, h))
   // ...

This is clearly a little hacky, but I think a real long-term solution would involve some redesign of the GenericEvent trait, which seems like a little to great an undertaking right now. If it turns out that this whole event-based approach is going to be a long-term solution for Conrod, then I think it would be worth revisiting GenericEvent design at that time. @bvssvni @mitchmindtree You guys think that sounds about right, or would you rather get into the whole GenericEvent design now?

[mitchmindtree]

Hey @psFried, going to close this in favour #700 in order to keep the pre-#684 and post-#684 discussion distinct 👍