Switch to event-based input handling

[psFried]

WIP - do not merge yet

Ok, so here's a second crack at refactoring input handling. This ended up being a rather large departure from the current input handling, so I ended up using completely new structs instead of adding onto UserInput. The idea is that we could just deprecate the old UserInput and leave it around for a while for the sake of backwards compatibility. Don't know if that would really be worth it or not, though, since I have no idea how many people are actually using custom widgets.

For the first pass, the only widget that I refactored to use the new WidgetInput was the Button widget. I figured that would be the simplest one to start off with.

All the other widgets still need to be switched over to use the new input methods:

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

I think it would be a good idea to wait until most of these widgets get switched over to using the new input methods before merging, just to make sure that WidgetInput has everything it needs to.

Overall, I think this approach has worked out much better than the previous approach, but any feedback is certainly appreciated. Generally, this should make it really easy to solve #606. This PR already addresses: #569, #642 (though we could use to create a nicer api for sending events), #384, and of course #670.

Anyway, this is a pretty huge PR, so take your time looking it over and let me know if you see any room for improvement.

[mitchmindtree]

😲 wooohoooo!

I'm a bit busy at the moment but will review this as soon as I can! From a quick glance however this already looks like a massive improvement 😸 thanks so much for taking the time for this @psFried, I think conrod and its users will benefit greatly as a result!

[mitchmindtree]

Okydoke, I've had a read through and I really like the new system! Great stuff!

Please don't be put off by the number of points below! Most of the following is related to small changes/improvements - nothing too serious:

1. Perhaps move test modules into a conrod/tests/ directory - specifically the following:

   • global_input/tests.rs -> tests/global_input.rs
   • conrod_event/tests.rs -> tests/conrod_event.rs
   • input_provider::tests -> tests/input_provider.rs
   • ui_tests.rs -> tests/ui.rs

   I think this would mainly be nice to keep the src directory a little less cluttered.

2. IntoIterator impl for GlobalInput is probably unnecessary as it requires moving the whole instance, which is never what we want. We could however implement IntoIterator for &'a GlobalInput, that returns an iterator that yields a borrow of each ConrodEvent.

3. All methods returning a &Vec<ConrodEvent> should instead return a &[ConrodEvent]. This is a little more idiomatic, as you can’t do anything more with a borrowed Vec than you can a borrowed slice (&mut Vec on the other hand can be useful).

4. InputProvider methods should avoid allocating (i.e return Vecs) as this requires dynamically allocating every time a widget needs access to a certain kind of event, which may be the case multiple times for many widgets per update. Instead, return an Iterator type that yields references to the underlying events. This may require writing a new type for each different kind of iterator (i.e. fn keys_just_released(&self) -> KeysJustReleased), as we don't yet have anything like monomorphised, anonymised, return types (i.e. impl Iterator).

5. The method name InputProvider::current_mouse_position could probably just be InputProvider::mouse_position.

6. UiEvent might be a better name for ConrodEvent as it's a bit shorter?

7. RelativePosition trait probably isn’t necessary, I think I'd prefer to impl the method directly on each type. Normally I try to avoid writing traits unless:

   • I need to use them as bounds on a generic type
   • I need to cast some types to trait objects
   • The trait can provide a bunch of useful defaults after having implemented just one required method (like Positionable or Colorable or Iterator).

   I mainly do this to avoid having to import another trait each time I use the method. If it turns out we come across one of the above use-cases, or a user requests it, then we can consider adding a trait and implementations for it in addition to the concrete methods.

8. Perhaps it might be more useful if InputProvider::all_events returned an associated Iterator type over all events? This way GlobalInput could return a std::slice::Iter<'a, ConrodEvent>, while WidgetInput could return some unique type that filters out all events that aren't relevant to that widget.

9. Rather than allocating its own Vec, WidgetInput should borrow the GlobalInput. We could have methods return an iterator that wraps the iterators returned by the GlobalInput methods and filters out events that aren’t relevant to that widget. This way we don’t dynamically allocate by default, but the user still has the option to .collect() into a Vec if they wish. This is important as some projects may have 1000+ widgets, most of which may not need input at all, while the remainder probably only need to check for one kind of event. I think WidgetInput could also borrow the InputState as well rather than copying the whole struct? This isn't as big a deal though.

10. Button Interaction logic isn’t quite the same as it were previously - originally the Interaction::Clicked state indicated that the button is currently pressed (sorry, Pressed would probably have been a better name!). I think with the rest of the changes however, we probably no longer need to track the Interaction of the Button any longer - we could probably remove the type altogether, as it was mainly just to track whether or not it was clicked (which we now have an event for thanks to this PR!).

11. I think we might be better off without the drag_threshold field in GlobalInput - I think it’s best if we consider any movement as dragging, and instead let the user implement their distance threshold functionality.

I have some other ideas related widget-specific events, but I haven't looked closely enough to suggest anything meaningful just yet - this is probably plenty of notes for now anyways!

Again, awesome stuff 😸 Just let me know if I've missed the mark with any of my comments! Otherwise, hopefully you find them useful 👍

[psFried]

@mitchmindtree Good feedback! I think I agree with most of those. I'm not going to have much time until next week, but I'd like to implement those suggestions.

Initially, I actually did try to return Iterators everywhere, but the types of some of the returned iterators just got way out of hand. I think you're right, though. The current approach should allow just having an associated Iterator type on the InputProvider trait, and most methods should just be able to use that. I also really like the idea of the WidgetInput just borrowing the events from GlobalInput and just do the filtering when the InputProvider methods are called instead of up front when WidgetInput is created.

• For 2, I should have just removed the IntoIter implementation. That was an artifact from an approach that just didn't work out. I'll just remove it, since it's probably not going to be useful anyway.
• For 4, I very much agree with this conceptually, but in practice the types of the returned Iterators were beyond unwieldy. As soon as you add a filter on the iterator, you end up with the type of the filter function as a part of the type of the iterator. It looked to me like it might be one of those types that can only be inferred by the compiler but not expressed in the language. I think that the just_pressed and just_released methods will probably have to just return a vec or slice. I haven't really had to use those methods yet, though. I just put them there because that's what was on the old UserInput. It may be that we can just get rid of these methods altogether and replace them with methods that just check if a specific button or set of buttons was just pressed/released.

I think your first point was really a good idea, too. I've found that testing the Ui and Widgets (probably the most important things to test) is very difficult. Putting tests all together will make it easier to share test support code. I still haven't written anything to support testing Widgets, but I'm thinking that some sort of widget_test_support module will be necessary in order to make it less painful. So far, I haven't undertaken adding tests every time I refactor one of the widget's update methods to use the new WidgetInput, but I'm getting close to just doing it.

If there's a lot of interest in starting to add test coverage, I can start working on support for it. Otherwise I'll probably just leave it as is. Do you want to start testing widgets?

[mitchmindtree]

Yeah I know the kinds of crazy iterator types you're talking about! Here's a comment I made about it on an RFC which aimed to solve the problem heheh. There's been a lot of thought on this problem, and a couple follow up RFCs as well. For now unfortunately, the general workaround is to write your own type that implements Iterator, and return that instead of trying to return a Filter or Map etc (whose types are in fact impossible to write when passing closures as their Fn params heh).

So for example, in the case of keys_just_released, you could make a new type called KeysJustReleased and implement the Iterator trait for it, something like:

pub struct KeysJustReleased<'a, I>
    where I: Iterator<Item=&'a ConrodEvent>,
{
    events: I,
    lifetime: ::std::marker::PhantomData<&'a ()>,
}

impl<'a, I> Iterator for KeysJustReleased<'a, I>
    where I: Iterator<Item=&'a ConrodEvent>,
{
    type Item = Key;
    fn next(&mut self) -> Option<Self::Item> {
        while let Some(event) = self.events.next() {
            if let ConrodEvent::Raw(Input::Release(Keyboard(key))) = *event {
                return Some(key);
            }
        }
        None
    }
}

Then the method in InputProvider might look something like this:

pub trait InputProvider<'a> {
    type Events: Iterator<Item=&'a ConrodEvent>;

    // ...

    fn keys_just_released(&'a self) -> KeysJustReleased<'a, Self::Events> {
        KeysJustReleased { events: self.events() }
    }

    // ...
}

As you can see it's definitely a little more verbose :( but it does the job :)

Re testing widgets: I've been super unsure about how to do this effectively myself, but i'm certainly open to adding some tests if you have a nice way of doing so in mind!

[psFried]

I'm finally getting some time to finish up this PR, and just have a few things left to address.

@mitchmindtree

11- I think we might be better off without the drag_threshold field in GlobalInput - I think it’s best if we consider any movement as dragging, and instead let the user implement their distance threshold functionality.

I think there has to be some sort of threshold for this. In testing it out, if the threshold is 0, then it's really easy to end up generating drags instead of clicks, especially with a trackpad. The thing that I really don't like about having the drag threshold here is just that it won't match whatever threshold is set at the OS level. I think that the ultimate solution here would be to have a cross-platform way to get this threshold from the OS instead of just making up our own, but I haven't found a great way of doing that yet. On windows, we would just call this function (exposed in winapi-rs) to get the threshold, but I don't know how that's done on other systems yet. We'll have a similar problem once we start trying to detect double-clicks. There's a system setting for the time between clicks, as well as a distance threshold for those.

tl;dr - I do think that some sort of global threshold is necessary. Is it acceptable to just keep the drag_threshold as it is for now until we can figure out a good way to get it from the system?

Edit:

Looks like on Linux, the drag and double-click thresholds are controlled by GTK, QT, etc. I couldn't find any way to get that setting on OSX. I did find a similar issue here. It now seems to me to be of questionable value to try to come up with a cross-platform way of getting these thresholds. Might be best to just keep them in Conrod.

[mitchmindtree]

@psFried interesting!

One option might be to make drag_threshold a field in the Theme, which could be read by the Ui when constructing events? This way, we could have some reasonable self-assessed default setting in the meantime (while we look into system specific thresholds), while also offering an easy option for users to customise and set their own preferences. Thoughts?

[psFried]

@mitchmindtree Theme sounds like a great place for it to live. I'll go ahead and move it there. I think we'll probably still want to store the threshold in GlobalInput as a private field, but at least then it would be determined by the Theme and can be easily overridden.

[psFried]

At this point, I think I have addressed all of the feedback on the PR. Changing all the InputProvider methods to return Iterators ended up working out really well. Great suggestion!

Still need to finish switching over the rest of the widgets to use the new input methods.

Any thoughts about deprecating UserInput? I see a couple of options:

1. Merge this PR now and wait to remove UserInput until all the conrod widgets have been switched over to the new InputProvider methods. We could just track changing individual widgets in Input Refactoring #670.
2. Merge this once all widgets have been changed to the new InputProvider methods and wait until a future release to remove UserInput.
3. Merge this once all widgets have been changed to the new InputProvider methods and remove UserInput as part of this PR.

I don't have a sense for how many projects are using custom widgets, so I don't know if any projects will be affected by this change. Thoughts?

[mitchmindtree]

Just had one final review - looking great!

There are a couple small changes I'd like to make, but I'd be happy to address them myself!

I'm happy to merge this now as is and wait to remove UserInput once all widgets have been updated 👍

I think there are a few projects using the custom widget API, however as long as we don't break the current version, I'm fine with removing UserInput as the change shouldn't be too drastic for widget implementers and we still have a way to go before 1.0.0. Once we've updated the rest of the widgets, there will be lots of examples of how to migrate widgets to the new event API too.

[psFried]

Excellent 👍 I went ahead and updated the original InputRefactoring issue #670 to track the remaining items to be changed over to use the new InputProvider methods. That way this can be merged and closed.

@mitchmindtree I'd love to hear what other changes you were thinking of. If you want to do that pre-merge, you can just use this branch.

[mitchmindtree]

Nice!

I won't have time to make the changes today, I'll leave them inline in case you get a chance to get to them before I do :)

[mitchmindtree]

I think it would be a little more idiomatic to move this T type parameter to an associated type, i.e. something like type Events: Iterator<Item=&'a UiEvent>. This is because we know that there may only be one type of event iterator per implementation of InputProvider, i.e. GlobalInput's is GlobalInputEventIterator and WidgetInput's is WidgetInputEventIterator.

[psFried]

That all makes sense. You'll probably get to it before I do. I have family coming in from out of town, so I don't see myself getting much free time in the next week :)

[mitchmindtree]

Aiight, I've got about half an hour now so I think I might just merge this and make the adjustments in a follow up PR :)

Thanks again for this @psFried, such an awesome contribution!