After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 781757 - gdk_seat_grab() not working for touchscreen events
gdk_seat_grab() not working for touchscreen events
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Input Methods
3.22.x
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2017-04-26 10:51 UTC by yzhang1985
Modified: 2017-09-20 18:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix gdk_seat_grab() dropping touch events (601 bytes, patch)
2017-08-29 11:01 UTC, yzhang1985
none Details | Review
gdkseatdefault: Grab touch events for ALL_POINTING (1.30 KB, patch)
2017-08-29 11:22 UTC, Daniel Boles
none Details | Review
gdkseatdefault: Grab touch events where applicable (2.08 KB, patch)
2017-09-05 14:50 UTC, Daniel Boles
committed Details | Review

Description yzhang1985 2017-04-26 10:51:10 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.
Comment 1 Daniel Boles 2017-08-23 19:45:11 UTC
Makes sense. Could you attach a rebased version of your patch?
Comment 2 yzhang1985 2017-08-29 11:01:20 UTC
Created attachment 358671 [details] [review]
fix gdk_seat_grab() dropping touch events
Comment 3 yzhang1985 2017-08-29 11:02:01 UTC
OK, here's the patch. It's pretty trivial, so I didn't bother uploading one earlier.
Comment 4 Daniel Boles 2017-08-29 11:15:05 UTC
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.
Comment 5 Daniel Boles 2017-08-29 11:22:00 UTC
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.
Comment 6 yzhang1985 2017-08-31 10:03:30 UTC
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.
Comment 7 Daniel Boles 2017-08-31 10:05:30 UTC
I'd prefer a review from someone familiar with the details, so CCing Carlos, who #define'd TOUCH_EVENTS in the first place
Comment 8 Carlos Garnacho 2017-09-04 14:55:08 UTC
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.
Comment 9 Daniel Boles 2017-09-04 15:03:08 UTC
(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.
Comment 10 Daniel Boles 2017-09-04 15:06:06 UTC
(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.
Comment 11 Daniel Boles 2017-09-04 15:08:05 UTC
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.
Comment 12 yzhang1985 2017-09-05 00:17:11 UTC
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.
Comment 13 Carlos Garnacho 2017-09-05 09:58:46 UTC
(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.
Comment 14 Daniel Boles 2017-09-05 14:50:16 UTC
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!
Comment 15 yzhang1985 2017-09-08 09:52:36 UTC
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 16 Daniel Boles 2017-09-17 11:20:05 UTC
Comment on attachment 359196 [details] [review]
gdkseatdefault: Grab touch events where applicable

Carlos, is this correct, according to what you outlined before?
Comment 17 Carlos Garnacho 2017-09-20 18:05:32 UTC
Comment on attachment 359196 [details] [review]
gdkseatdefault: Grab touch events where applicable

Yeah, that looks right :)
Comment 18 Daniel Boles 2017-09-20 18:50:42 UTC
\o/ Thanks!

Attachment 359196 [details] pushed as 5c700cf - gdkseatdefault: Grab touch events where applicable