GNOME Bugzilla – Bug 781757
gdk_seat_grab() not working for touchscreen events
Last modified: 2017-09-20 18:50:46 UTC
I'm adding touch screen support to Inkscape for zooming, rotating, and scrolling the canvas. Unforunately after migrating from the deprecated gdk_device_grab() to gdk_seat_grab(GDK_SEAT_CAPABILITY_ALL_POINTING) in r15548, touch screen events are getting lost. After studying the code a bit, I noticed gdk_seat_grab(GDK_SEAT_CAPABILITY_ALL_POINTING) calls gdk_device_grab() internally but without GDK_TOUCH_MASK, which somewhat explains the problem. I'm a little confused because in my case, all my touches are happening inside the Inkscape desktop window, so even if the grab ignores touch events, I should be receiving touch events as long as they're inside the window right? But I'm not getting the events. Regardless, I changed gdk_seat_default_grab() to call gdk_device_grab() with GDK_TOUCH_MASK set, which solves the problem. I see TOUCH_EVENTS defined in that file, so it seems someone was already about to add it. GDK_SEAT_CAPABILITY_ALL_POINTING is defined to include touch screen events, so this seems to be the right thing to do.
Makes sense. Could you attach a rebased version of your patch?
Created attachment 358671 [details] [review] fix gdk_seat_grab() dropping touch events
OK, here's the patch. It's pretty trivial, so I didn't bother uploading one earlier.
Review of attachment 358671 [details] [review]: The general idea seems clearly correct to me, given this definition: typedef enum { GDK_SEAT_CAPABILITY_NONE = 0, GDK_SEAT_CAPABILITY_POINTER = 1 << 0, GDK_SEAT_CAPABILITY_TOUCH = 1 << 1, GDK_SEAT_CAPABILITY_TABLET_STYLUS = 1 << 2, GDK_SEAT_CAPABILITY_KEYBOARD = 1 << 3, GDK_SEAT_CAPABILITY_ALL_POINTING = (GDK_SEAT_CAPABILITY_POINTER | GDK_SEAT_CAPABILITY_TOUCH | GDK_SEAT_CAPABILITY_TABLET_STYLUS), GDK_SEAT_CAPABILITY_ALL = (GDK_SEAT_CAPABILITY_ALL_POINTING | GDK_SEAT_CAPABILITY_KEYBOARD) } GdkSeatCapabilities; However, I would say that it's gdk_seat_default_grab() that should be amended, to grab on POINTER_EVENTS | TOUCH_EVENTS. So, those two would be kept distinct - not that they _need_ to be at this point in time, as they'd only be used in this one place.
Created attachment 358673 [details] [review] gdkseatdefault: Grab touch events for ALL_POINTING gdk_seat_default_grab() grabs POINTER_EVENTS if the capability is GDK_SEAT_CAPABILITY_ALL_POINTING. But that enumerator is a union that includes GDK_SEAT_CAPABILITY_TOUCH, so we should grab TOUCH_EVENTS too. TOUCH_EVENTS was #defined but unused; this may be its intended purpose. This fixes touch events being lost when migrating from gdk_device_grab() to gdk_seat_grab(GDK_SEAT_CAPABILITY_ALL_POINTING), as found by Inkscape -- Thanks for your patch, and no offence with the "obsolete" marker, but this seems a clear way to do it. Please could you test and let me know if this also works? This seems pretty clearly correct to me, but I may wait for an ack from someone with more experience to say for sure.
Daniel, I agree your approach of not lumping together POINTER_EVENTS & TOUCH_EVENTS is cleaner. And I'm sure it will work. Please check this in so that Inkscape & others can enjoy it downstream.
I'd prefer a review from someone familiar with the details, so CCing Carlos, who #define'd TOUCH_EVENTS in the first place
Comment on attachment 358673 [details] [review] gdkseatdefault: Grab touch events for ALL_POINTING Thanks for the patch(es)! It looks alright, but I think it'd be more correct to do: GdkEventMask pointer_evmask; if (capabilities & GDK_SEAT_CAPABILITY_POINTER) pointer_evmask |= POINTER_EVENTS; if (capabilities & GDK_SEAT_CAPABILITY_TOUCH) pointer_evmask |= TOUCH_EVENTS; if (pointer_evmask != 0) gdk_device_grab (..., pointer_evmask, ...) So the capability flags are always respected on the pointer grab.
(In reply to Carlos Garnacho from comment #8) > Comment on attachment 358673 [details] [review] [review] > gdkseatdefault: Grab touch events for ALL_POINTING > > Thanks for the patch(es)! It looks alright, but I think it'd be more correct > to do: > > GdkEventMask pointer_evmask; > > if (capabilities & GDK_SEAT_CAPABILITY_POINTER) > pointer_evmask |= POINTER_EVENTS; > if (capabilities & GDK_SEAT_CAPABILITY_TOUCH) > pointer_evmask |= TOUCH_EVENTS; Where do you envision these going? If you mean within the existing if (capabilities & GDK_SEAT_CAPABILITY_ALL_POINTING) branch, then the latter enum bitmask already includes the two above. Also, a 0-init on the evmask might be needed, depending on where this code got placed.
(In reply to Daniel Boles from comment #9) > Where do you envision these going? If you mean within the existing if > (capabilities & GDK_SEAT_CAPABILITY_ALL_POINTING) branch, then the latter > enum bitmask already includes the two above. Wait, no, of course that should be fine; that checks whether we have any, not all, of the component bits. However, ALL_POINTING also includes TABLET_STYLUS, but we don't handle that (should we?), so if we don't 0-init the pointer_evmask but the capabilities includes TABLET_STYLUS, that's going to explode.
And if pointer_evmask == 0 and we don't grab, what should the returned status be? Currently it would keep the initial value of GDK_GRAB_SUCCESS.
Ah, I completely missed the concept that gdk_seat_grab() can be called for individual capabilities instead of always ALL_POINTING. Then what Carlos proposed looks good, after adding another case to support tablet input (GDK_TABLET_PAD_MASK?). I've thought of another complexity. Since touch events are used to emulate mouse events, you should grab TOUCH_EVENTS even if only GDK_SEAT_CAPABILITY_POINTER is requested. Otherwise, you will never receive emulated touch events. But, I don't think this is important because a good application would not rely on emulated touch events and handle touch events directly.
(In reply to Daniel Boles from comment #10) > (In reply to Daniel Boles from comment #9) > > Where do you envision these going? If you mean within the existing if > > (capabilities & GDK_SEAT_CAPABILITY_ALL_POINTING) branch, then the latter > > enum bitmask already includes the two above. > > Wait, no, of course that should be fine; that checks whether we have any, > not all, of the component bits. > > However, ALL_POINTING also includes TABLET_STYLUS, but we don't handle that > (should we?), so if we don't 0-init the pointer_evmask but the capabilities > includes TABLET_STYLUS, that's going to explode. True, TABLET_STYLUS should also be handled in the POINTER_EVENTS case. On X11 tablet styli take over the pointer cursor, they have no special set of events nor event mask values (just additional axes on motion/button events). Unfortunately there's not much we can tell the windowing to dissociate pointer and stylus input on X11. TABLET_STYLUS is more useful on wayland since those get different sets of GdkDevices (and visible pointer cursors). (In reply to Daniel Boles from comment #11) > And if pointer_evmask == 0 and we don't grab, what should the returned > status be? Currently it would keep the initial value of GDK_GRAB_SUCCESS. evmask == 0 should be prevented on the gdk_seat_grab() entry point. GDK_GRAB_FAILED should be returned in that case. (In reply to yzhang1985 from comment #12) > Ah, I completely missed the concept that gdk_seat_grab() can be called for > individual capabilities instead of always ALL_POINTING. > > Then what Carlos proposed looks good, after adding another case to support > tablet input (GDK_TABLET_PAD_MASK?). Tablet pads are only possibly handled by clients on Wayland (in X11/GNOME, the compositor grabs those entirely and transform button presses there into keystrokes). Maybe GDK_TABLET_PAD_MASK makes sense there however, It may make sense to redirect raw GdkEventPad* for eg. action mapping, even if I think GtkPadController alone should cover the actual user interaction. > > I've thought of another complexity. Since touch events are used to emulate > mouse events, you should grab TOUCH_EVENTS even if only > GDK_SEAT_CAPABILITY_POINTER is requested. Otherwise, you will never receive > emulated touch events. But, I don't think this is important because a good > application would not rely on emulated touch events and handle touch events > directly. Emulated pointer events are constrained to a single touchpoint, so the behavior is pointer-like enough that I don't see this as a big problem. I'm trying to get rid of emulated events altogether for gtk4 to avoid this kind of oddities though.
Created attachment 359196 [details] [review] gdkseatdefault: Grab touch events where applicable gdk_seat_default_grab() grabs POINTER_EVENTS if the capability is GDK_SEAT_CAPABILITY_ALL_POINTING. But that enumerator is a union that includes GDK_SEAT_CAPABILITY_TOUCH, but we never grabbed TOUCH_EVENTS, an unused macro that was presumably created with this purpose in mind. So, check which of the ALL_POINTING capabilities we have, and set the right mask of POINTER_EVENTS and/or TOUCH_EVENTS as required. As part of this, explicitly let TABLET_STYLUS take over pointer events, as this is the intended behaviour and was the effective result before. This should fix touch events being lost in migrating from Device.grab() to Seat.grab(GDK_SEAT_CAPABILITY_ALL_POINTING), as found by Inkscape. -- disclaimer: can't test right now. Please do review/test mercilessly!
I just tested the latest patch in Inkscape (Windows) GTK 3.22.12 and my upcoming 2 finger drag+rotate+scroll gesture works flawlessly. Krita also supports 2 finger drag+rotate, so i guess someone can test that.
Comment on attachment 359196 [details] [review] gdkseatdefault: Grab touch events where applicable Carlos, is this correct, according to what you outlined before?
Comment on attachment 359196 [details] [review] gdkseatdefault: Grab touch events where applicable Yeah, that looks right :)
\o/ Thanks! Attachment 359196 [details] pushed as 5c700cf - gdkseatdefault: Grab touch events where applicable