Add support for more mouse buttons and tracking holding. #843

xStrom · 2020-04-14T14:05:56Z

This PR adds mouse support for more than just two buttons.

Update: It also adds MouseButtons to the MouseEvent to track which buttons are being held down.

cmyr

A potential gotcha here is that MouseButton itself is currently limited to representing five buttons, and on mac unknown buttons get mapped to Left. This is wrong, and should at least be changed to something else; but I think we probably just need an Other member in MouseButton?

(otherwise this looks good)

xStrom · 2020-04-14T17:32:31Z

Okay I added MouseButton::Unknown and use it as a fallback on all the platforms.

xStrom · 2020-04-14T18:55:46Z

After some further testing and thinking I also changed mouse events to contain Option<MouseButton>, because in some cases we know for sure that no button was pressed and that is different from an unknown button.

One slight downside of using an Option is that people now need to import the MouseButtonExt trait to get the convenience is_left etc methods.

cmyr · 2020-04-16T14:12:51Z

Would it make more sense to have MouseButton::None?

cmyr · 2020-04-16T14:14:50Z

An additional (probably useless?) thought: mouse buttons are really a mask, multiple can be down at a time etc. I'm probably over thinking this though, it definitely isn't what we would want to surface as the principle API (although we might want to surface it at some point, for advanced cases?)

cmyr

A few little thoughts; this might be the best path forward but would like to consider the alternatives.

cmyr · 2020-04-16T14:18:41Z

druid-shell/src/mouse.rs

 }

-impl MouseButton {
+/// Adds convenience methods to `MouseButton` and `Option<MouseButton>`.
+pub trait MouseButtonExt {


I get the motivation but this doesn't feel great to me. I do think I might prefer MouseButton::None, and we could sort of redefine MouseButton to be 'the mouse button that triggered a given event, if one exists.' really this is all about mouse moved. Down and up must be associated with a specific button, but moved can have no or all buttons, etc. I wonder if it's worth representing differently in the API?

I've also experimented with ways to build a higher-level API on top of this in runebender; I'm not totally happy with the results, but I think something like that could be useful?

cmyr · 2020-04-16T14:19:39Z

druid-shell/src/platform/gtk/window.rs

+        3 => Some(MouseButton::Right),
+        4 => Some(MouseButton::X1),
+        5 => Some(MouseButton::X2),
+        _ => Some(MouseButton::Unknown),


I might consider making this Unknown(u32)? it doesn't matter too much either way, but if someone wants that info we might as well provide it?

But only if it's not a super big hassle..

xStrom · 2020-04-16T18:50:36Z

I went with Option because that's the rust way but I don't consider that too important in this case.

How about a custom struct MouseButtons which is basically a type-safe bitmask. Something like this:

impl MouseButtons {
    fn has(&self, button: MouseButton) -> bool;

    fn has_left(&self) -> bool;
    fn has_right(&self) -> bool;
    // ....
    fn has_x2(&self) -> bool;

    fn has_any(&self) -> bool;
}

cmyr · 2020-04-16T19:02:30Z

The problem is that these have different uses; so that would work, but would not replace MouseButton.

on MouseDown and MouseUp, it matters which button triggered the event; the mask would be supplementary at best. On MouseMoved, the idea of a MouseButton is meaningless, and something like MouseButtons makes sense.

For all this, though, what's most important is a good API that covers the 99% case; to me this means that MouseDown and MouseUp always have an associated mouse button, and that MouseMoved probably shouldn't.

One option:

MouseEvent does not have a MouseButton, it has only something like the MouseButtons you describe; and then MouseDown and MouseUp are a MouseEvent plus a MouseButton, but MouseMoved is only a MouseEvent? This is the most reasonable thing I can think of right now...

xStrom · 2020-04-16T19:08:19Z

That sounds reasonable. During MouseDown you would have druid-managed state that tells you which other buttons are currently already held down, plus the one new button separately.

Event::MouseDown((MouseEvent, MouseButton)) // Something like this tuple?

xStrom · 2020-04-17T11:12:18Z

After thinking about this some more, especially in relation to #842 which is headed towards adding a new focused bool to the event for clicks, I'm thinking of splitting up the event structs into MouseMoveEvent and MouseClickEvent.

Having the single button info and also focused info only makes sense for clicks. This would also allow for more easy divergence between the event data in the future. It should also break less code because there's no tuple and the following should still work:

Event::MouseUp(e) => {
    if e.button.is_left() { do stuff; }
}

cmyr · 2020-04-17T14:06:45Z

a few options I think:

MouseDown { event: MouseEvent, button: MouseButton }

// or,
MouseClickEvent {
    mouse: MouseEvent,
    button: MouseButton,
    count: u32,
}
// with
Event::MousDown(MouseClickEvent),
Event::MouseUp(MouseClickEvent),
Event::MouseMove(MouseEvent),

(I realized in the course of writing this that the click count is also irrelevant for the purposes of MouseMove).

I think maybe one reason for the current design is that I was thinking about mac, and NSEvent is a monolith; the same underlying 'type' can represent tons of different events, so there are lots of fields that exist that aren't actually meaningful for a given specific event.

xStrom · 2020-04-19T16:24:16Z

I pushed a new iteration of this work. Holding down multiple mouse buttons is now nicely tracked. It works perfectly on macOS and Windows and mostly on GTK. GTK has all sorts of issues due to fragmentation, which will need solving separately. This PR at the very least only improves the situation in druid-shell and doesn't make it worse.

For this iteration I went with ClickEvent and MoveEvent. It's not perfect but doesn't seem too bad either. The benefit is that the struct doesn't contain useless fields, however it does make match code a bit more verbose if it wants to do something on a common field.

// What used to be this ..
Event::MouseDown(e) | Event::MouseUp(e) | Event::MouseMove(e) => {
    self.last_mouse_pos = Some(e.pos)
}
// is now this.
Event::MouseDown(ClickEvent { pos, .. })
| Event::MouseUp(ClickEvent { pos, .. })
| Event::MouseMove(MoveEvent { pos, .. }) => self.last_mouse_pos = Some(*pos),

I'm not 100% convinced that splitting them is the way to go. Maybe the superior ergonomics in handling these three events together is worth the price of wasting those extra bytes of memory and having useless fields in the code editor autocomplete for MouseMove?

cmyr

Sorry, didn't see your update. I have some thoughts; if I were put on the spot I would say let's just use a single event, but include the split between MouseButton and MouseButtons. Some other notes inline. Thanks for digging into this!

cmyr · 2020-04-22T15:04:53Z

druid-shell/src/mouse.rs

-    /// or the released button in the case of a mouse-up event.
+    pub count: u8,
+    /// The button that was pressed down in the case of mouse-down,
+    /// or the button that was released in the case of mouse-up.
    pub button: MouseButton,


okay, thinking about the API:

I think that separating out MouseButtons from MouseButton solves most of our issue here, and shows a viable way to have a single type cover these three events without any clear inconsistency. There will be two fields, button and count, that are not used in MouseMove, and I think that's fine.

count is actually a funny one; the docstring says "will always be 0 on mouse-up", but I'm not sure I actually like that as an implementation; this is something we're going to have to revisit at some point, in that some platforms might provide their own double/triple click counting, and on some platforms we might need to do it ourselves. We probably do want to trust the platform, though, so that we are respecting whatever settings the user has on that platform.

My gut tells me that mouse-up should include count, though? (but this is unrelated to current PR).

MouseMove would also have a useless focused that will be introduced in #842, however even so I'm also thinking a single event is probably the way to go.

As for count, I have additional thoughts on it that I describe in #859. The short of it is that yes I think count should be there for MouseUp but it's not there right now on any platfrom besides macOS so I just wrote the docs to reflect that reality and made macOS compatible with that. This would be a short-lived change in light of #859.

cmyr · 2020-04-22T15:06:40Z

druid-shell/src/mouse.rs

    /// First X button.
    X1,
    /// Second X button.
    X2,
+    /// Some other unknown button.
+    Other,


I think if we're reworking this API we should include precise arbitrary buttons, which means Other should be Other(u8). We will end up needing to do this eventually for something, so we might as well do it now.

Having it be Other(u8) would change the MouseButtons implementation because it couldn't just be a bitmask and would need to store arbitrary u8s in the set as well.

Also it doesn't really look like any of the platforms (Windows, macOS, GTK, web) support more specific buttons via the APIs that we're currently using. GTK is struggling with X1/X2 already. It's of course possible to tap into the mouse driver and get more of them, but I feel like that's a rather large project and probably not even that useful for general GUI apps.

So given the large scope of cross-platfrom support for more buttons, I think we can't reasonably prepare our API for them. When someone wants to take that task on and knows the demands of a practical implementation, then it would be much more reasonable to change the API.

on macOS the mouse buttons are presented as a u32 bitmask. Windows seems to support up to five buttons. I haven't checked elsewhere.

Okay, so my last question would be, is Other even a useful distinction? If the user can't get any more information I think it could be problematic, since two separate buttons could send Other, and you can't tell them apart for the purposes of up & down.

In the very long term, I've been imagining we want to have some API to get the underlying event from the platform in these cases, but that's also another can of worms.

cmyr · 2020-04-22T15:07:52Z

druid-shell/src/mouse.rs

+    /// Adds all the [`MouseButton`]s in `other` to the set.
+    ///
+    /// [`MouseButton`]: enum.MouseButton.html
+    pub fn union(&mut self, other: MouseButtons) {


I would expect this to return a new MouseButtons

You shouldn't, because builder methods start with with.

this wouldn't be a builder method, it would just be a method that happens to return the union of two sets; similar to the union method on collection types like HashSet. (These actually return an iterator over the union, but since our type is Copy I wouldn't worry about that).

If what you want to do is add all the items from one set to the other, the only std api for this would be the Extend trait.

Anyway not a blocker or anything, just something that jumps out at me.

cmyr · 2020-04-22T15:09:09Z

druid-shell/src/mouse.rs

+
+    /// Builder-style method for removing the `button` from the set.
+    #[inline]
+    pub fn without(mut self, button: MouseButton) -> MouseButtons {


I'm getting some YAGNI vibes here; it isn't clear when or where these would be used? Are they just by us, or do we expect external users to need to synthesize mouse buttons?

Well this specific without method is used in the GTK platform code already.

More generally these functions can be used by external users. Yes they might synthesize MouseButtons from some sort of hotkey configuration file and then do a check during a mouse event against that.

cmyr · 2020-04-22T15:09:52Z

druid-shell/src/mouse.rs

+    ///
+    /// [`MouseButton::Other`]: enum.MouseButton.html#variant.Other
+    #[inline]
+    pub fn has_other(self) -> bool {


if we make Other include a specific id, we should allow this to check for that value.

cmyr · 2020-04-22T15:11:31Z

druid-shell/src/mouse.rs

+///
+/// [`MouseButton`]: enum.MouseButton.html
+#[derive(PartialEq, Eq, Clone, Copy, Default)]
+pub struct MouseButtons(u8);


we probably have more than 8 bits of possible buttons; I would just make this a u32 now, that should be enough for anybody.

xStrom · 2020-04-30T12:33:11Z

A new iteration has been pushed. I went back to a single MouseEvent struct and changed the union / extend naming for MouseButtons.

I also thought about your question of whether MouseButton::Other is useful. It can have a bit of value but it does also introduce a lot of problems.

It isn't really cross-platform in a portable way, so hot-key configurations made with Other in druid wouldn't be portable either.
It represents multiple buttons so events can get rather funky with up/down and we couldn't really perform double-click detection on it. Designing a unified multi-click API #859
It isn't supported in our MouseButtons in more than one way. Not just the set itself, but even figuring out what to add to the set. There isn't a clear way to figure out the holding state on all platforms.

I think druid should strive for a consistent API that is reliable across platforms. Given these issues I don't think Other passes the quality bar for druid. For that reason I have removed it from this PR. Now druid-shell just ignores click events on buttons that we don't know.

In the long term, if we want to support more buttons, that can of course be done. However it should be done in a qualitative way and I don't think it can be achieved with the platform mouse APIs druid-shell is using right now. More lower level APIs would need to be used.

cmyr

Okay, I think this is the right direction.

This is basically good to go; only issue is that I think the combination of MouseButton::None and repr(u8) will cause problems as currently written.

There are a few other comments and thoughts inline, but it's mostly just me thinking out loud.

cmyr · 2020-04-30T13:52:49Z

druid-shell/src/mouse.rs

-    pub count: u32,
-    /// The currently pressed button in the case of a move or click event,
-    /// or the released button in the case of a mouse-up event.
+    /// be `0` for a mouse-up and mouse-move events.


Just discussion, but I wonder if it makes more sense for, in the case of mouse-up, for count to be the same as the count of the corresponding mouse-down.

Yes but it's not a trivial change and so will come with #859.

cmyr · 2020-04-30T13:59:07Z

druid-shell/src/mouse.rs

+    /// Add the `button` to the set.
+    #[inline]
+    pub fn add(&mut self, button: MouseButton) {
+        self.0 |= 1 << button as u8;


This plays weird with MouseButton::None; but if we make it the first member in the enum, we can just do something like self.0 |= 1.min(button as u8) << button as u8?

druid-shell itself never adds or checks MouseButton::None but you're right, some external user might do it. I updated the code to ignore MouseButton::None. Hopefully the min will be branchless.

cmyr · 2020-04-30T14:08:42Z

druid-shell/src/mouse.rs

+#[derive(PartialEq, Eq, Clone, Copy, Default)]
+pub struct MouseButtons(u8);
+
+impl MouseButtons {


just sort of riffing on this type; if the idea is to construct it from MouseButtons, one possible thing to do is implement FromIterator<MouseButton>, and then you could write, say,

let buttons: MouseButtons = [MouseButton::Left, MouseButton::X1].iter().copied().collect();

That doesn't feel great really; probably simpler to have a constructor that just takes a &[MouseButton].

In any case I'm just daydreaming, this isn't necessary or anything.

The following one liner isn't too bad either:

let buttons = MouseButtons::new().with(MouseButton::Left).with(MouseButton::Right);

There can be value in adding more methods, however I think I would leave these slice/iterator constructors out of this PR.

cmyr · 2020-04-30T14:09:24Z

druid-shell/src/mouse.rs

+
+    /// Returns `true` if any button is in the set.
+    #[inline]
+    pub fn has_any(self) -> bool {


I might prefer to use is_empty() for this, if we're treating it like a set? Not totally sure, just a thought.

I named as many as I could with has_ prefix because it seemed nicer to me. However it's probably more ergonomic to follow HashSet more closely in naming, because that's what Rust programmers will know. I will rename all the methods that don't match but have equals in HashSet.

yea, this is my general thinking; people are more likely to try and use method names they know from elsewhere.

druid-shell/src/platform/x11/window.rs

cmyr · 2020-04-30T14:11:51Z

druid-shell/src/platform/x11/application.rs

@@ -139,6 +141,8 @@ impl Application {
                                button_release.event_x() as f64,
                                button_release.event_y() as f64,
                            ),
+                            // TODO: Fill with held down buttons


let's open an issue for this when we merge

cmyr

Looks good, only thing is to update the changelog?

xStrom added S-needs-review waits for review shell/mac concerns the macOS backend labels Apr 14, 2020

cmyr reviewed Apr 14, 2020

View reviewed changes

cmyr added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Apr 14, 2020

xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Apr 14, 2020

xStrom force-pushed the mac-mouse-buttons branch 3 times, most recently from e6e0ad4 to 3b3b656 Compare April 14, 2020 18:54

xStrom changed the title ~~Handle more mouse buttons in macOS.~~ Handle more and no mouse buttons. Apr 14, 2020

xStrom added breaking change pull request breaks user code and removed shell/mac concerns the macOS backend labels Apr 14, 2020

xStrom force-pushed the mac-mouse-buttons branch 2 times, most recently from 578a4a0 to 94c2f27 Compare April 14, 2020 19:43

cmyr reviewed Apr 16, 2020

View reviewed changes

xStrom added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Apr 16, 2020

xStrom force-pushed the mac-mouse-buttons branch from 94c2f27 to 04b5950 Compare April 19, 2020 14:59

xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Apr 19, 2020

xStrom mentioned this pull request Apr 19, 2020

Designing a unified multi-click API #859

Closed

cmyr reviewed Apr 22, 2020

View reviewed changes

xStrom added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Apr 24, 2020

xStrom force-pushed the mac-mouse-buttons branch from 459cbef to 158ad19 Compare April 30, 2020 11:48

xStrom changed the title ~~Handle more and no mouse buttons.~~ Add support for more mouse buttons and tracking holding. Apr 30, 2020

xStrom force-pushed the mac-mouse-buttons branch from 158ad19 to f3ae994 Compare April 30, 2020 12:11

xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Apr 30, 2020

cmyr requested changes Apr 30, 2020

View reviewed changes

cmyr approved these changes Apr 30, 2020

View reviewed changes

Add support for more mouse buttons and tracking holding.

4db547f

xStrom force-pushed the mac-mouse-buttons branch from 44ead59 to 4db547f Compare April 30, 2020 15:55

xStrom merged commit 3445361 into linebender:master Apr 30, 2020

xStrom deleted the mac-mouse-buttons branch April 30, 2020 16:17

teddemunnik mentioned this pull request Apr 30, 2020

Provide additional modifiers as part of the mouse wheel event #895

Merged

xStrom removed the S-needs-review waits for review label May 15, 2020

Add support for more mouse buttons and tracking holding. #843

Add support for more mouse buttons and tracking holding. #843

Conversation

xStrom commented Apr 14, 2020 • edited Loading

cmyr left a comment • edited Loading

Choose a reason for hiding this comment

xStrom commented Apr 14, 2020

xStrom commented Apr 14, 2020

cmyr commented Apr 16, 2020

cmyr commented Apr 16, 2020

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xStrom commented Apr 16, 2020

cmyr commented Apr 16, 2020 • edited Loading

xStrom commented Apr 16, 2020

xStrom commented Apr 17, 2020

cmyr commented Apr 17, 2020

xStrom commented Apr 19, 2020

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xStrom commented Apr 30, 2020

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmyr left a comment

Choose a reason for hiding this comment

xStrom commented Apr 14, 2020 •

edited

Loading

cmyr left a comment •

edited

Loading

cmyr commented Apr 16, 2020 •

edited

Loading