-
Notifications
You must be signed in to change notification settings - Fork 295
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
Conversation
…n mouse is clicked, started refactoring TextBox to use new Ui methods
…instead of just a time and Point, for consistency
… mouse events into their own file
…use.left and such
…ouseButtonDown, handle mouse button down in text box
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 { |
There was a problem hiding this comment.
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.
Hmmm, so here are a few thoughts that come to mind while reading through 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 |
The idea behind the design of |
Excellent feedback, all. I definitely think that having a generic 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. |
I took some time to look into the code around 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 |
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 overMouseEvent
s. 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
, andunknown
. I changed that to instead use aButtonMap
, which mapsinput::MouseButton
enums toButtonState
structs. Where before, one could callmouse.left
to get the state of the left mouse button, now you would callmouse.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.