Created attachment 281508 [details] [review]
wayland: Fix infinite loop in touch_handle_cancel_event()

Created attachment 281509 [details] [review]
wayland: Rename touch_handle_cancel_event() to meta_wayland_touch_cancel()

This will serve as a generic call to issue touch cancellation on clients,
useful for the gesture tracker support.

Created attachment 281510 [details] [review]
window: Use event data getters in event handling code

This makes these functions more independent wrt touch vs pointer events

Created attachment 281511 [details] [review]
wayland: Add meta_wayland_touch_get_n_current_touches()

Created attachment 281512 [details] [review]
display: Add meta_display_is_pointer_emulating_sequence()

This function tells the obvious on X11, and implements a similar mechanism
on wayland to determine the "pointer emulating" sequence, or one to stick
with when implementing single-touch behavior.

Created attachment 281513 [details] [review]
wayland: set the current serial on touch down/up notifications

Do just like pointer button events, and do not bump the serial, so serials
match on window drag/move requests triggered from touch events.

Created attachment 281514 [details] [review]
window: Implement single-touch window dragging

On X11 this works because only emulated pointer events are listened for. On
wayland, the single touch behavior must be enforced in touch events, ignoring
every other sequence.

Created attachment 281515 [details] [review]
wayland: Handle window drags for touch events

The grabbing state is now checked for both pointer/touch devices
within the seat, and the grab start coordinates returned by
meta_wayland_seat_can_grab_surface().

Created attachment 281516 [details] [review]
wayland: Show/hide the pointer cursor on touch/pointer activity

Created attachment 281517 [details] [review]
display: cancel wayland client touches when the compositor is grabbed

When a compositor grab begins, clients will stop receiving events, so any
ongoing sequence at that time must be cancelled.

Created attachment 281518 [details] [review]
display: Implement gesture-induced touch cancellation for wayland

On wayland, touches are initially both handled by the compositor and sent
to clients, proceeding to cancellation on clients only after the compositor
claims the sequence for itself. Implement the cancellation detail through
MetaGestureTracker::state-changed.

Created attachment 281519 [details] [review]
events: Simplify gesture event management

If a sequence has no accepted state, the gesture tracker will let the event
go through, so it can update grabs and clients. Whenever the gesture tracker
accepts the state, the event will only be run on the stage actor, so only
gestures are triggered at that point.

Created attachment 281520 [details] [review]
events: Do not swallow touch events on windows

Those might eventually trigger a gesture into recognition.

Review of attachment 281508 [details] [review]:

::: src/wayland/meta-wayland-touch.c
@@ +453,1 @@
   surfaces = s = touch_get_surfaces (touch, FALSE);

Remove the "s = " now.

Review of attachment 281512 [details] [review]:

::: src/core/events.c
@@ +111,3 @@
+      display->pointer_emulating_sequence == sequence)
+    display->pointer_emulating_sequence = NULL;
+  else if (event->type == CLUTTER_TOUCH_BEGIN)

Can you reverse this so that begin is before end?

    if (event->type == CLUTTER_TOUCH_BEGIN && display->pointer_emulating_sequence == NULL)
      ...
    else if (event->type == CLUTTER_TOUCH_END && display->pointer_emulating_sequence == sequence)
      ...

@@ +113,3 @@
+  else if (event->type == CLUTTER_TOUCH_BEGIN)
+    {
+      if (!clutter_event_is_pointer_emulated (event))

Can you split this logic out?

  static gboolean
  sequence_is_pointer_emulated (const ClutterEvent *event)
  {
    if (clutter_event_is_pointer_emulated (event))
      return TRUE;

    /* Under Wayland, our Clutter's input backend doesn't track 
     * pointer-emulating sequences, so do it ourselves. */
    ...
  }

@@ +121,3 @@
+              compositor = meta_wayland_compositor_get_default ();
+
+              if (meta_wayland_touch_get_n_current_touches (&compositor->seat->touch) != 0)

I think this is a bit conflated here. Again, I always have a clean separation here between the "backend" which is Clutter's input backend, and the "frontend" like MetaWayland.

There are certain cases like during grab ops where we don't send touches to Wayland clients, so using its view of the world about whether there are ongoing touches during this seems incorrect.

Clutter doesn't want to have a concept of a pointer-emulating touch in its input backend, which makes things difficult for us, so we have to record the information *somewhere*, but I think the front-end is the wrong place.

For now, just put it in MetaDisplay, much like you're doing for pointer_emulating_sequence.

@@ +139,3 @@
   MetaGestureTracker *tracker;
 
+  if (!display->pointer_emulating_sequence)

What's going to end an ongoing sequence with this check? Should we just call this unconditionally?

Review of attachment 281511 [details] [review]:

See previous review.

Review of attachment 281513 [details] [review]:

OK.

Review of attachment 281514 [details] [review]:

::: src/core/window.c
@@ +6143,3 @@
       return TRUE;
 
+    case CLUTTER_TOUCH_END:

...

Where's CLUTTER_TOUCH_BEGIN ?

Review of attachment 281516 [details] [review]:

Hm. I'd say if we want to do this in mutter, we should do this in mutter for both X11 and Wayland, and remove the code in g-s-d that does this currently. So, put this in core/events.c, not the front-end stuff. That's wrong anyway.

Review of attachment 281517 [details] [review]:

This doesn't work for compositor grabs proper, since those are in meta_compositor_begin_modal_for_plugin, I believe it's called?

I wonder if it makes sense to drop touch events whenever the pointer / keyboard focus is properly dropped. I hate to have "meta_wayland_sync_input_focus"-like functions around where we just sort of call them everywhere. If it's difficult to do properly, just adding the code there is fine.

Review of attachment 281509 [details] [review]:

OK.

Review of attachment 281518 [details] [review]:

OK.

Review of attachment 281519 [details] [review]:

::: src/core/meta-gesture-tracker.c
@@ +492,3 @@
+    {
+      clutter_actor_event (CLUTTER_ACTOR (clutter_event_get_stage (event)),
+                           event, TRUE);

This is technically wrong. The actor in the event that we pass to clutter_actor_event needs to be the same as the actor here.

This is also particularly gross. I'm not going to accept this until you give a better rationale for *why* you're doing this, and putting a comment above here.

Review of attachment 281520 [details] [review]:

... but we already call meta_gesture_tracker_handle_event above here? What's going on, and what is this fixing?

Review of attachment 281510 [details] [review]:

OK. You might want to expand the commit message a bit and say "we're going to handle touch events in here very soon", rather than just leaving it up in the air.

Review of attachment 281515 [details] [review]:

::: src/wayland/meta-wayland-seat.c
@@ +322,2 @@
 gboolean
 meta_wayland_seat_can_grab_surface (MetaWaylandSeat    *seat,

If we want to expand this interface, I think we should rename it. I can't think of any better names, though :(

@@ +329,3 @@
+  ClutterEventSequence *sequence;
+
+  sequence = meta_wayland_touch_find_grab_sequence (&seat->touch, surface, serial);

Is there any guarantee that seat->touch is initialized, after what we just landed? Can we make sure that this doesn't segfault anywhere if the user doesn't have a touchscreen device?

::: src/wayland/meta-wayland-touch.c
@@ +578,3 @@
+
+gboolean
+meta_wayland_touch_get_coords (MetaWaylandTouch     *touch,

This should probably be "get_press_coords" to mirror the ClutterGestureAction API.

Comment on attachment 281508 [details] [review]
wayland: Fix infinite loop in touch_handle_cancel_event()

Pushed after fixing the unnecessary assignation

Created attachment 281588 [details] [review]
gesture-tracker: Add meta_gesture_tracker_get_n_current_touches()

Due to the way the MetaGestureTracker processes every touch event, this
will tell as closely to Clutter as possible the current number of touches
happening on the stage.

Even though, this is subject to windowing behavior, on X11, rejected touches
will be soon followed by a XI_TouchEnd event, so the compositor will stop
seeing touch sequences that are still operating on clients. On wayland, touch
sequences are processed by the compositor during all their lifetime, so these
will stay on the MetaGestureTracker with META_SEQUENCE_PENDING_END state, yet
still tracked.

Created attachment 281589 [details] [review]
display: Add meta_display_is_pointer_emulating_sequence()

This function tells the obvious on X11, and implements a similar mechanism
on wayland to determine the "pointer emulating" sequence, or one to stick
with when implementing single-touch behavior.

Created attachment 281590 [details] [review]
window: Implement single-touch window dragging

On X11 this works because only emulated pointer events are listened for. On
wayland, the single touch behavior must be enforced in touch events, ignoring
every other sequence.

Created attachment 281591 [details] [review]
wayland: Clear hashtable pointers on meta_wayland_touch_release()

Just in case they are poked while no touch interface is available;

Created attachment 281592 [details] [review]
wayland: Handle window drags for touch events

The grabbing state is now checked for both pointer/touch devices
within the seat, and the grab start coordinates returned by
meta_wayland_seat_can_grab_surface().

Created attachment 281593 [details] [review]
wayland: Show/hide the pointer cursor on touch/pointer activity

Created attachment 281594 [details] [review]
display: cancel wayland client touches when the compositor is grabbed

When a compositor grab begins, clients will stop receiving events, so any
ongoing sequence at that time must be cancelled.

Created attachment 281595 [details] [review]
events: Simplify gesture event management

MetaGestureTracker has been separating the "did I handle an event?" and the
"should the event be filtered out?" questions, merge this and make
handle_event() reply to "should the event be only handled by me?".

If a sequence wasn't accepted yet by the gesture tracker, the event will
go through (eg. not handled exclusively by the gesture tracker) and it'll
still be processed by Clutter, triggering gesture actions, and maybe
changing the sequence into other state.

Created attachment 281596 [details] [review]
events: Do not swallow touch events on windows

Those might eventually trigger a gesture into recognition.

(In reply to comment #15)
> Review of attachment 281512 [details] [review]:
> @@ +113,3 @@
> +  else if (event->type == CLUTTER_TOUCH_BEGIN)
> +    {
> +      if (!clutter_event_is_pointer_emulated (event))
> 
> Can you split this logic out?

Separated that into a separate function

> @@ +121,3 @@
> +              compositor = meta_wayland_compositor_get_default ();
> +
> +              if (meta_wayland_touch_get_n_current_touches
> (&compositor->seat->touch) != 0)
> 
> I think this is a bit conflated here. Again, I always have a clean separation
> here between the "backend" which is Clutter's input backend, and the "frontend"
> like MetaWayland.
> 
> There are certain cases like during grab ops where we don't send touches to
> Wayland clients, so using its view of the world about whether there are ongoing
> touches during this seems incorrect.
> 
> Clutter doesn't want to have a concept of a pointer-emulating touch in its
> input backend, which makes things difficult for us, so we have to record the
> information *somewhere*, but I think the front-end is the wrong place.
> 
> For now, just put it in MetaDisplay, much like you're doing for
> pointer_emulating_sequence.

I see your point, I've done a new patch using MetaGestureTracker (which is quite close to MetaDisplay), I didn't fall in that sequences will stay there for all their lifetime on wayland, unlike in x11 where you get a XI_TouchEnd shortly after the XI_RejectTouch.

> 
> @@ +139,3 @@
>    MetaGestureTracker *tracker;
> 
> +  if (!display->pointer_emulating_sequence)
> 
> What's going to end an ongoing sequence with this check? Should we just call
> this unconditionally?

Right, I changed that so the update function is called unconditionally.



(In reply to comment #18)
> Review of attachment 281514 [details] [review]:
> 
> ::: src/core/window.c
> @@ +6143,3 @@
>        return TRUE;
> 
> +    case CLUTTER_TOUCH_END:
> 
> ...
> 
> Where's CLUTTER_TOUCH_BEGIN ?

CLUTTER_TOUCH_BEGIN should only happen on moves/resizes triggered from keybindings/menu, a behavior similar to CLUTTER_BUTTON_PRESS wouldn't be too desirable as you'd just warp the window under the finger and undo the grab.

I changed the patch to deal with CLUTTER_TOUCH_BEGIN just like CLUTTER_TOUCH_UPDATE

(In reply to comment #19)
> Review of attachment 281516 [details] [review]:
> 
> Hm. I'd say if we want to do this in mutter, we should do this in mutter for
> both X11 and Wayland, and remove the code in g-s-d that does this currently.
> So, put this in core/events.c, not the front-end stuff. That's wrong anyway.

Ups, updated this patch but there are actually no changes here, should be put aside and dealt with as a separate bug I guess.

(In reply to comment #20)
> Review of attachment 281517 [details] [review]:
> 
> This doesn't work for compositor grabs proper, since those are in
> meta_compositor_begin_modal_for_plugin, I believe it's called?
> 
> I wonder if it makes sense to drop touch events whenever the pointer / keyboard
> focus is properly dropped. I hate to have "meta_wayland_sync_input_focus"-like
> functions around where we just sort of call them everywhere. If it's difficult
> to do properly, just adding the code there is fine.

Right, looks like a proper place to be called seeing where sync_wayland_input_focus() is called, I've updated the patch.(In reply to comment #23)
> Review of attachment 281519 [details] [review]:
> 
> ::: src/core/meta-gesture-tracker.c
> @@ +492,3 @@
> +    {
> +      clutter_actor_event (CLUTTER_ACTOR (clutter_event_get_stage (event)),
> +                           event, TRUE);
> 
> This is technically wrong. The actor in the event that we pass to
> clutter_actor_event needs to be the same as the actor here.
> 
> This is also particularly gross. I'm not going to accept this until you give a
> better rationale for *why* you're doing this, and putting a comment above here.

As discussed on IRC, this is the closest I can get to a "grab broken" behavior when a gesture is recognized, I've
added more verbose comments and commit log.

(In reply to comment #24)
> Review of attachment 281520 [details] [review]:
> 
> ... but we already call meta_gesture_tracker_handle_event above here? What's
> going on, and what is this fixing?

This is actually a subproduct of the previous patch, touch events that aren't yet accepted are let through by the gesture tracker, so they can trigger window dragging, clients, and maybe gestures too. So touch events over windows may still need clutter processing in order for the gestures to see these.

Review of attachment 281589 [details] [review]:

::: src/core/events.c
@@ +114,3 @@
+    return TRUE;
+
+  /* On wayland there is no concept of pointer emulating sequence,

"When using Clutter's native input backend"

@@ +122,3 @@
+   * to another sequence until the next first touch on an idle touchscreen.
+   */
+  if (meta_is_wayland_compositor ())

I think this is going to be wrong if we're running under nested, since we'll get proper X11 events there. This should be:

   MetaBackend *backend = meta_get_backend ();
   if (META_IS_BACKEND_NATIVE (backend))
     {
     }

(In reply to comment #37)
> CLUTTER_TOUCH_BEGIN should only happen on moves/resizes triggered from
> keybindings/menu, a behavior similar to CLUTTER_BUTTON_PRESS wouldn't be too
> desirable as you'd just warp the window under the finger and undo the grab.
> 
> I changed the patch to deal with CLUTTER_TOUCH_BEGIN just like
> CLUTTER_TOUCH_UPDATE

Oh, right, a touch begin will start the grab. Feel free to remove it again, sorry.

Review of attachment 281590 [details] [review]:

Old patch is fine too. Your choice on whether you want to keep the CLUTTER_TOUCH_BEGIN or not.

Review of attachment 281591 [details] [review]:

::: src/wayland/meta-wayland-touch.c
@@ +531,3 @@
   g_hash_table_unref (touch->touch_surfaces);
   g_hash_table_unref (touch->touches);
+  touch->touch_surfaces = touch->touches = NULL;

Can you do this as:

   g_hash_table_unref (touch->touch_surfaces);
   touch->touch_surfaces = NULL;

   g_hash_table_unref (touch->touches);
   touch->touches = NULL;

Review of attachment 281593 [details] [review]:

Still a no. Put this in core/events.c and remove the code from g-s-d.

Review of attachment 281594 [details] [review]:

OK.

Review of attachment 281588 [details] [review]:

OK.

Review of attachment 281592 [details] [review]:

Let's rename this to meta_wayland_seat_get_grab_info, as we discussed.

Review of attachment 281594 [details] [review]:

Actually, no. There's no guarantee the focus changed. sync_wayland_input_focus is supposed to be idempotent if nothing changed. Perhaps meta_wayland_compositor_set_input_focus is a better fit for this.

Review of attachment 281595 [details] [review]:

Good, with a comment fix.

::: src/core/meta-gesture-tracker.c
@@ +490,3 @@
 
+  /* As soon as a sequence is accepted, the gesture tracker will on one
+   * hand ensure that only the stage (where shell-level gestures are tracked

"on one hand" grammar is a bit weird. What about:

  /* As soon as a sequence is accepted, we replay it to
   * the stage as a captured event, and make sure it's never
   * propagated anywhere else. Since ClutterGestureAction does
   * all its event handling from a captured-event handler on
   * the stage, this effectively acts as a "sequence grab" on
   * gesture actions.
   */

Attachment 281588 [details] pushed as 177ec27 - gesture-tracker: Add meta_gesture_tracker_get_n_current_touches()
Attachment 281589 [details] pushed as 41fdc4a - display: Add meta_display_is_pointer_emulating_sequence()
Attachment 281590 [details] pushed as f28f5dc - window: Implement single-touch window dragging
Attachment 281591 [details] pushed as baadb75 - wayland: Clear hashtable pointers on meta_wayland_touch_release()
Attachment 281592 [details] pushed as 930361b - wayland: Handle window drags for touch events
Attachment 281595 [details] pushed as 63c7591 - events: Simplify gesture event management

Created attachment 281615 [details] [review]
display: cancel wayland client touches when the compositor is grabbed

When a compositor grab begins, clients will stop receiving events, so any
ongoing sequence at that time must be cancelled.

Review of attachment 281615 [details] [review]:

Looks good to me.

Review of attachment 281596 [details] [review]:

OK.

Attachment 281596 [details] pushed as 62e0c42 - events: Do not swallow touch events on windows
Attachment 281615 [details] pushed as 70aee2d - display: cancel wayland client touches when the compositor is grabbed