Created attachment 307269 [details] [review]
events: Ensure touchpad gesture events go through clutter

They otherwise fall through paths that enable bypass_clutter, this
is necessary so they can be picked by captured-event handlers
along the actor hierarchy.

Review of attachment 307269 [details] [review]:

I'm very skeptical of this change. Why are they being set to bypass_clutter?

Right, not always, only on events above windows, due to:
https://git.gnome.org/browse/mutter/tree/src/core/events.c#n268

Actually all the "if (window)" branch seems pointless on touchpad events, that's why I thought it's best to entirely skip it.

Why don't touchpad events have an event sequence?

event sequences are useful to recognize and keep tracking of individual touches, touchpad gesture events are a high level interpretation, without access to individual touchpoints, as such event sequences aren't useful.

If you wonder why touchpad events are the way they are, not all touchpads provide touchpoint info, some are limited to bounding rect for all fingers, ellipse sizes and whatnot. libinput is still able to provide gesture events uniformly on those.

So what you're saying is that that "clutter_event_get_event_sequence" branch and the comment in that branch are incorrect. We should fix that instead of adding event trickery.

That's not what I'm saying. If you see https://git.gnome.org/browse/mutter/commit/?id=62e0c42803b2a9c617cb260374230bc777eaaf83 (the commit which added that branch), bypass_clutter was TRUE on all events over windows.

I needed to punch the hole back then for touch events, so events from a not yet "claimed" touch sequence could make it to the stage (already claimed sequences go directly through https://git.gnome.org/browse/mutter/tree/src/core/events.c#n237, so don't need any further forwarding to clutter).

But we're now talking of a different event set, with different semantics (even if in the high level we pursue the same info), I could eg. fold the condition in this patch into the event sequence checks, but I consider both things too loosely related.

Well, we have some events that should be routed to windows, and some other events that shouldn't be routed to windows. Currently, the set of events that should be routed to a window are "all non-touch events". You want to change that to "all non-touch events, and also all gestures".

Perhaps we should invert the check, and only send events to windows that we know should be gobbled.

There's 2 criteria relevant for this patch:

A) whether the window (or its frame) should see the event through meta_window_handle_ungrabbed_event()
B) whether the event should propagate to clutter

Pointer events want A, touch events want A and B, and touchpad gesture events want B.

I guess we can hide the event sets for A and B in #defines, so the code ends up:

  if (window && EVENT_DELIVERABLE_TO_WINDOWS (event))
    ...

  bypass_clutter = !EVENT_DELIVERABLE_TO_CLUTTER (event);

Now that we are at bikeshedding over three-liners, I consider the logic in meta_display_handle_event() broken. The wayland/x11 window event handling paths should be run in the bubbling phase of the window/surface actors, so it is possible to intercept the events from other points in the hierarchy. That's how I'd do it, if Clutter had the same client-side grab behavior and gesture state propagation logic that GTK+ has, we're unfortunately not quite close.

In the current code we deliver the event to the wayland client *before* it gets to Clutter. Specifically for touchpad gestures, we'll need to distinguish compositor from client gestures before even sending the first event, which makes the checks iffy (anything above 2fg goes to the compositor? or?).

The reason I'm bikeshedding over three lines is because it's a special case. We already have a few of those, but building up enough of those special cases is enough of an attempt to try to figure out where we went wrong and think of a better system.

I initially tried the bubbling thing by connecting to events on the surface actor, but I ran into issues with the shell intercepting things with captured-event, so that's why I do it early now.

I know why it's done the way it's done. It doesn't help me see random ::captured-event handlers as a viable option to toolkit/stage-level grabs :(. The toolkit would have enough information to send notifications, so we could be safely undoing things instead of just getting silence on previous ::captured-event handlers (with the involved dangers of leaving these potentially dangling).

This is drifting offtopic anyway... I'm folding touchpad and touch cases into a single IS_GESTURE_EVENT define, and made the comment more ellaborate and up-to-date. Hope the next patch looks alright.

Created attachment 307345 [details] [review]
events: Ensure touchpad gesture events go through clutter

They otherwise fall through paths that enable bypass_clutter, this
is necessary so they can be picked by captured-event handlers
along the actor hierarchy.

Attachment 307345 [details] pushed as e648f2c - events: Ensure touchpad gesture events go through clutter