Skip to content

Mouse input refactoring #663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 19 commits into from
Closed

Conversation

psFried
Copy link
Contributor

@psFried psFried commented Jan 8, 2016

This is the beginning of refactoring mouse input handling. The basic idea here is to simplify how widgets handle mouse input in most cases. A new mouse::events module was added that provides higher level event abstractions over low-level mouse events. Widgets can just call Mouse::events(), which returns an iterator over MouseEvents. MouseEvents are simple things like Click, Drag, or Scroll.

Part of the goal here is to obviate the need for widgets to 'capture' the mouse, except in a few rare cases. Turns out, having widgets making calls to capture/uncapture inputs makes it really difficult to add methods to the Ui that control which widgets take focus. Tabbing between text boxes, as well as having external sources capture inputs should get a lot easier once actually capturing input becomes unnecessary in most cases.

There is one breaking change here. Previously, the Mouse struct had four fields for it's buttons: left, right, middle, and unknown. I changed that to instead use a ButtonMap, which maps input::MouseButton enums to ButtonState structs. Where before, one could call mouse.left to get the state of the left mouse button, now you would call mouse.buttons.get(MouseButton::Left). This seems a little more verbose, but it allows for all the mouse buttons to work and makes for some cleaner code when updating the state of the mouse.

I refactored the TextBox widget to use the new mouse events. The rest of the widgets should follow shortly. I made sure to leave all the other state of the mouse visible. Eventually, we can talk about cleaning up some of the fields on ButtonState, but that's a discussion for after all the existing widgets are updated to use the new events.

@mitchmindtree
Copy link
Contributor

Sounds interesting! I'll review this shortly 👍

/// mouse button goes down then up without moving more than the drag threshold
/// while the button is depressed.
#[derive(Copy, Clone, Debug, PartialEq)]
pub struct MouseClick {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are already in the mouse::event module, so we could probably change MouseClick to just Click. This way accessing it via the module would be a little cleaner, i.e. mouse::event::Click. We can still re-export it from the lib.rs as MouseClickEvent, i.e. use mouse::event::Click as MouseClickEvent if we deem it necessary.

@mitchmindtree
Copy link
Contributor

Hmmm, so here are a few thoughts that come to mind while reading through this:

  1. This proposes moving to a more Event oriented handling of user input rather than just tracking the "current state" which was previously the case. I'm very much in favour of this change (see Store all input Events between updates so that they can be inspected by Widgets #569) however rather than only making these changes for the Mouse related input events, it'd be nice to make this changeover for all input events (keyboard as well).

  2. Much of the new event code in the mouse modules looks like a second version of the events that we already receive via the Ui::handle_event method. Perhaps we could just store those events directly rather than mapping them to a different mouse specific event format, as storing the more general Input event would also probably better address the issue I mention in my first point? Would be good to get @bvssvni 's thoughts on this, as he designed the original Input enum which IMO might be a better solution to this.

  3. Your event enum tree seems a little inverted, i.e. you currently have something along the lines of:

    MouseEvent
        Scroll
        Down
            Button
        Click
            Button
        Drag
            Button
    

    whereas this would probably be better described as

    MouseEvent
        Scroll
        Button
            Down
            Click
            Drag
    

    However this is probably doesn't matter if we decide to just store the events given via Ui::handle_input anyway.

  4. I like the idea of automatically handling the capturing/uncapturing of widgets within the Ui! I see that you're doing it for capturing keyboard input via a new widget_under_mouse_captures_keyboard method, however we might as well make this the case for the mouse also? This might remove the need for a lot of the widgets to manually call capture_mouse or uncapture_mouse. I'll have to put some more thought into this.

In general, I like many of the ideas in this PR, however I think they could be implemented in a simpler, more general manner by having the Ui store a Vec<InputEvent>, perhaps in an InputEvents wrapper that provides various methods for filtering through all the events in different ways, depending on whether or not the widget that is trying to access them is capturing or not.

@bvssvni
Copy link
Member

bvssvni commented Jan 8, 2016

The idea behind the design of T: GenericEvent is that you can add custom events and the widgets will receive them, without any parent widget needing to know the constraints on those events. I suggest Vec<T: GenericEvent> if that's possible.

@psFried
Copy link
Contributor Author

psFried commented Jan 8, 2016

Excellent feedback, all. I definitely think that having a generic UpdateAgrs::events() method makes a lot of sense. I will definitely look into the idea of reusing the existing events. The reason that this initial attempt uses a whole new events module is that there is technically a many-to-one relationship between GenericEvents and MouseEvents. The purpose of the Mouse was to aggregate one or more GenericEvents into a single MouseEvent. This is because there may be more GenericEvents per second than there are Ui updates per second.

I really think that just having a single definition of all the events that a widget can handle makes a lot of sense, though. I was actually thinking that even things changing focus should just be handled with generic events.

@mitchmindtree your point #4 is spot on. I think having the Ui handle switching which widgets capture input is important. I don't think that it makes sense to have the it capture the mouse, though. The way i was thinking about capturing is perhaps a little different than how it has been used. Previously, no widget would be given mouse input unless it was capturing. With this change, any widget would just be given the mouse input if the mouse is over that widget. So, the only reason to ever 'capture' the mouse would be in special situations where you want to prevent other widgets from handling mouse input.

I'll take a look into some other approaches this weekend and see what ideas I can come up with.

@psFried
Copy link
Contributor Author

psFried commented Jan 10, 2016

I took some time to look into the code around GenericEvent and I think I agree that using those is the right way to go. The general problem of aggregating many low-level mouse events (like button-down, button-released, etc.) into higher-level events (i.e.- double-click) is still one that I think requires a general solution. That is totally solvable, though, by having some sort of InputsEvents wrapper that wraps a Vec<T: GenericEvent> of events since the last update. We could have a conrod module that interprets a series of low-level events and inserts high-level events into the stream. I actually really like the idea of widgets getting all events (both low- and high-level) from a single source.

We'll just have to figure out a nice clean api for widgets to filter only the events they care about. This is something that I foresee being rather painful given my current understanding of how GenericEvents work. I'm assuming we'll ultimately have to filter on the event_id, but not positive about that. I'll go ahead and close this PR and start on that as soon as I get some time.

@psFried psFried closed this Jan 10, 2016
@psFried psFried mentioned this pull request Jan 10, 2016
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants