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 744104 - Implement pointer motion, locks and confinemen
Implement pointer motion, locks and confinemen
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
: 752321 (view as bug list)
Depends on:
Blocks: WaylandRelated
 
 
Reported: 2015-02-06 17:05 UTC by Armin K.
Modified: 2016-02-16 11:09 UTC
See Also:
GNOME target: 3.20
GNOME version: ---


Attachments
MetaWaylandPointer: Put client resources in its own struct (13.69 KB, patch)
2015-07-21 09:02 UTC, Jonas Ådahl
none Details | Review
wayland: Implement support for wl_relative_pointer (21.11 KB, patch)
2015-07-21 09:02 UTC, Jonas Ådahl
none Details | Review
wayland: Add global to surface coordinate helper (3.25 KB, patch)
2015-07-21 09:03 UTC, Jonas Ådahl
none Details | Review
Move out generic math parts out of the native barrier implementation (19.91 KB, patch)
2015-07-21 09:03 UTC, Jonas Ådahl
none Details | Review
native: Update to new constrain callback API (1.30 KB, patch)
2015-07-21 09:03 UTC, Jonas Ådahl
none Details | Review
wayland: Add "painting" signal to surface actor (3.32 KB, patch)
2015-07-21 09:03 UTC, Jonas Ådahl
none Details | Review
Implement support for the _wl_pointer_lock protocol (75.53 KB, patch)
2015-07-21 09:03 UTC, Jonas Ådahl
none Details | Review
MetaBorder: Use float constants and functions instead of double variants (927 bytes, patch)
2015-07-21 09:03 UTC, Jonas Ådahl
none Details | Review
wayland: Calculate surface local coordinates more accurately (1.61 KB, patch)
2015-07-21 09:03 UTC, Jonas Ådahl
none Details | Review
wayland: Use the event coordinates when sending pointer motion events (1.85 KB, patch)
2015-07-21 09:03 UTC, Jonas Ådahl
none Details | Review
MetaPointerConfinementWayland: Support non-rectangular confinement regions (21.81 KB, patch)
2015-07-21 09:03 UTC, Jonas Ådahl
none Details | Review
native: Don't wait for a new input event to wrap the pointer (1.64 KB, patch)
2015-07-21 09:03 UTC, Jonas Ådahl
none Details | Review
wayland: Implement support for wp_relative_pointer (15.25 KB, patch)
2015-12-03 08:06 UTC, Jonas Ådahl
none Details | Review
wayland: Add global to surface coordinate helper (3.37 KB, patch)
2015-12-03 08:06 UTC, Jonas Ådahl
none Details | Review
Move out generic math parts out of the native barrier implementation (19.91 KB, patch)
2015-12-03 08:06 UTC, Jonas Ådahl
none Details | Review
native: Update to new constrain callback API (1.30 KB, patch)
2015-12-03 08:07 UTC, Jonas Ådahl
none Details | Review
wayland: Add "painting" signal to surface actor (2.94 KB, patch)
2015-12-03 08:07 UTC, Jonas Ådahl
none Details | Review
Implement support for the wp_pointer_constraints protocol (63.76 KB, patch)
2015-12-03 08:07 UTC, Jonas Ådahl
none Details | Review
MetaBorder: Use float constants and functions instead of double variants (927 bytes, patch)
2015-12-03 08:07 UTC, Jonas Ådahl
none Details | Review
wayland: Use the event coordinates when sending pointer motion events (1.85 KB, patch)
2015-12-03 08:07 UTC, Jonas Ådahl
none Details | Review
MetaPointerConfinementWayland: Support non-rectangular confinement regions (22.76 KB, patch)
2015-12-03 08:07 UTC, Jonas Ådahl
none Details | Review
native: Don't wait for a new input event to wrap the pointer (1.63 KB, patch)
2015-12-03 08:07 UTC, Jonas Ådahl
none Details | Review
wayland: Implement support for wp_relative_pointer (15.30 KB, patch)
2015-12-14 13:49 UTC, Jonas Ådahl
none Details | Review
Move out generic math parts out of the native barrier implementation (21.55 KB, patch)
2015-12-14 13:53 UTC, Jonas Ådahl
none Details | Review
wayland: Add "painting" signal to surface actor (3.20 KB, patch)
2015-12-14 14:00 UTC, Jonas Ådahl
none Details | Review
wayland: Make the pending surface state a GObject (10.80 KB, patch)
2015-12-14 14:04 UTC, Jonas Ådahl
none Details | Review
Implement support for the wp_pointer_constraints protocol (64.27 KB, patch)
2015-12-14 14:12 UTC, Jonas Ådahl
none Details | Review
wayland: Implement support for wp_relative_pointer (15.56 KB, patch)
2016-01-28 13:42 UTC, Jonas Ådahl
committed Details | Review
wayland: Add global to surface coordinate helper (3.37 KB, patch)
2016-01-28 13:42 UTC, Jonas Ådahl
committed Details | Review
Move out generic math parts out of the native barrier implementation (21.55 KB, patch)
2016-01-28 13:42 UTC, Jonas Ådahl
committed Details | Review
native: Update to new constrain callback API (1.30 KB, patch)
2016-01-28 13:42 UTC, Jonas Ådahl
committed Details | Review
wayland: Add "painting" signal to surface actor (3.20 KB, patch)
2016-01-28 13:42 UTC, Jonas Ådahl
committed Details | Review
wayland: Make the pending surface state a GObject (10.80 KB, patch)
2016-01-28 13:42 UTC, Jonas Ådahl
committed Details | Review
Implement support for the wp_pointer_constraints protocol (66.19 KB, patch)
2016-01-28 13:43 UTC, Jonas Ådahl
committed Details | Review
MetaBorder: Use float constants and functions instead of double variants (951 bytes, patch)
2016-01-28 13:43 UTC, Jonas Ådahl
committed Details | Review
wayland: Use the event coordinates when sending pointer motion events (1.85 KB, patch)
2016-01-28 13:43 UTC, Jonas Ådahl
committed Details | Review
MetaPointerConfinementWayland: Support non-rectangular confinement regions (22.90 KB, patch)
2016-01-28 13:43 UTC, Jonas Ådahl
committed Details | Review
native: Don't wait for a new input event to wrap the pointer (1.63 KB, patch)
2016-01-28 13:43 UTC, Jonas Ådahl
committed Details | Review

Description Armin K. 2015-02-06 17:05:37 UTC
Jonas Ådahl implemented this functionality for Weston, but it didn't seem to get merged. But, without this functionality, playing FPS games that run natively on wayland, such as SDL2-based xonotic which runs natively on Wayland via SDL2 Wayland backend, is impossible due to mouse issues. It's mostly fixed when using his patches for Weston and SDL2, so it's a nice to have feature.

Leaving this here as a feature request for some time in the future (possibly too late to get it into shape for 3.16)

http://lists.freedesktop.org/archives/wayland-devel/2014-December/018653.html
Comment 1 Jonas Ådahl 2015-02-07 02:05:24 UTC
Yes, this will be implemented in mutter as well. Some version will be merged into weston as well, but there are still some protocol discussion to take place.
Comment 2 Jonas Ådahl 2015-07-13 12:27:27 UTC
*** Bug 752321 has been marked as a duplicate of this bug. ***
Comment 3 Jonas Ådahl 2015-07-21 09:02:52 UTC
Created attachment 307803 [details] [review]
MetaWaylandPointer: Put client resources in its own struct

Instead of moving around all the bound pointer resources for a client
when changing focus, keep all the resources bound by a client in a per
client struct, and track the focus by having a pointer to the current
active pointer client struct instance.

This will simplify having wl_pointer extensinos sharing the pointer
focus of the wl_pointer by only having to add them to the pointer
client.
Comment 4 Jonas Ådahl 2015-07-21 09:02:58 UTC
Created attachment 307804 [details] [review]
wayland: Implement support for wl_relative_pointer

Add support for sending relative pointer motion deltas to clients who
request such events by creating wl_relative_pointer objects via
wl_relative_pointer_manager.

This currently implements the '_' prefix unstable protocol, and
has its own copy of the protocol XML. The protocol XML and the prefix
will be removed once the protocol is stabalized.
Comment 5 Jonas Ådahl 2015-07-21 09:03:04 UTC
Created attachment 307805 [details] [review]
wayland: Add global to surface coordinate helper
Comment 6 Jonas Ådahl 2015-07-21 09:03:09 UTC
Created attachment 307806 [details] [review]
Move out generic math parts out of the native barrier implementation

In order to reuse some vector math for pointer confinement, move out
those parts to its own file, introducing the types old types "Vector2"
and "Line2" outside of meta-barrier-native.c, as well as introducing
MetaBorder which is a line, with a blocking direction.
Comment 7 Jonas Ådahl 2015-07-21 09:03:15 UTC
Created attachment 307807 [details] [review]
native: Update to new constrain callback API
Comment 8 Jonas Ådahl 2015-07-21 09:03:22 UTC
Created attachment 307808 [details] [review]
wayland: Add "painting" signal to surface actor

Make MetaWaylandSurface a listener and move output state updating to
the handlen function.
Comment 9 Jonas Ådahl 2015-07-21 09:03:28 UTC
Created attachment 307809 [details] [review]
Implement support for the _wl_pointer_lock protocol

The _wl_pointer_lock protocol is an unstable not backward compatible
version of the future wl_pointer_lock protocol which enables clients
to manipulate the behavior of the pointer cursor associated with a seat.

It currently supports locking the pointer to a static position, and
confining the pointer to a given region.

Currently locking is fully implemented, and confining is implemented for
rectangular confinement regions.

What else is lacking is less troublesome semantics for enabling the lock
or confinement; currently the only requirement implemented is that the
window that appears focused is the one that may aquire the lock.

This means that a pointer could be 'stolen' by creating a new window that
receives active focus, or when using focus-follows-mouse, a pointer
passes a window that has requested a lock. This semantics can be changed
and the protocol itself allows any semantics as seems fit.
Comment 10 Jonas Ådahl 2015-07-21 09:03:34 UTC
Created attachment 307810 [details] [review]
MetaBorder: Use float constants and functions instead of double variants

We calculate with floats, so lets use that type throughout.
Comment 11 Jonas Ådahl 2015-07-21 09:03:41 UTC
Created attachment 307811 [details] [review]
wayland: Calculate surface local coordinates more accurately

clutter_actor_transform_stage_point is not precis enough; it may be
almost a pixel off. This is not good enough when clamping confined
pointer motions since after transforming inaccurately the position may
end up on the wrong side of the border.

To get more accurate coordinates, use the newly introduced coordinate
transform function
clutter_actor_apply_inverse_relative_transform_to_point().
Comment 12 Jonas Ådahl 2015-07-21 09:03:47 UTC
Created attachment 307812 [details] [review]
wayland: Use the event coordinates when sending pointer motion events

The x/y coordinates of the ClutterInputDevice were not the ones which was
the result of this event but whatever event was queued the last. The
correct coordinates can, however, be found in the event itself, so lets
use those.
Comment 13 Jonas Ådahl 2015-07-21 09:03:53 UTC
Created attachment 307813 [details] [review]
MetaPointerConfinementWayland: Support non-rectangular confinement regions

This patch adds support for confinement regions that are more complex
than a single rectangle. It relies on details about cairo regions not
explicitly in the API in order to generate the outer border of the
region.
Comment 14 Jonas Ådahl 2015-07-21 09:03:58 UTC
Created attachment 307814 [details] [review]
native: Don't wait for a new input event to wrap the pointer

If we rely on getting back an input event with the warped pointer
coordinates, we might draw a frame with the old coordinates if we warp
during the paint phase. Avoid that by moving the cursor immediately.
Comment 15 Jasper St. Pierre (not reading bugmail) 2015-07-21 15:44:37 UTC
Review of attachment 307810 [details] [review]:

Nice catch. Push straight away.
Comment 16 Matthias Clasen 2015-11-24 19:16:44 UTC
This seems to have fallen off the table ? Can we land this now ?
Comment 17 Jonas Ådahl 2015-12-03 08:06:46 UTC
Created attachment 316699 [details] [review]
wayland: Implement support for wp_relative_pointer

Add support for sending relative pointer motion deltas to clients who
request such events by creating wp_relative_pointer objects via
wp_relative_pointer_manager.

This currently implements the unstable version 1 from wayland-protocols.
Comment 18 Jonas Ådahl 2015-12-03 08:06:53 UTC
Created attachment 316700 [details] [review]
wayland: Add global to surface coordinate helper
Comment 19 Jonas Ådahl 2015-12-03 08:06:58 UTC
Created attachment 316701 [details] [review]
Move out generic math parts out of the native barrier implementation

In order to reuse some vector math for pointer confinement, move out
those parts to its own file, introducing the types old types "Vector2"
and "Line2" outside of meta-barrier-native.c, as well as introducing
MetaBorder which is a line, with a blocking direction.
Comment 20 Jonas Ådahl 2015-12-03 08:07:04 UTC
Created attachment 316702 [details] [review]
native: Update to new constrain callback API
Comment 21 Jonas Ådahl 2015-12-03 08:07:09 UTC
Created attachment 316703 [details] [review]
wayland: Add "painting" signal to surface actor

Make MetaWaylandSurface a listener and move output state updating to
the handlen function.
Comment 22 Jonas Ådahl 2015-12-03 08:07:15 UTC
Created attachment 316704 [details] [review]
Implement support for the wp_pointer_constraints protocol

The wp_pointer_constraints protocol is a protocol which enables clients
to manipulate the behavior of the pointer cursor associated with a seat.

Currently available constraints are locking the pointer to a static
position, and confining the pointer to a given region.

Currently locking is fully implemented, and confining is implemented for
rectangular confinement regions.

What else is lacking is less troublesome semantics for enabling the lock
or confinement; currently the only requirement implemented is that the
window that appears focused is the one that may aquire the lock.

This means that a pointer could be 'stolen' by creating a new window that
receives active focus, or when using focus-follows-mouse, a pointer
passes a window that has requested a lock. This semantics can be changed
and the protocol itself allows any semantics as seems fit.
Comment 23 Jonas Ådahl 2015-12-03 08:07:21 UTC
Created attachment 316705 [details] [review]
MetaBorder: Use float constants and functions instead of double variants

We calculate with floats, so lets use that type throughout.
Comment 24 Jonas Ådahl 2015-12-03 08:07:26 UTC
Created attachment 316706 [details] [review]
wayland: Use the event coordinates when sending pointer motion events

The x/y coordinates of the ClutterInputDevice were not the ones which was
the result of this event but whatever event was queued the last. The
correct coordinates can, however, be found in the event itself, so lets
use those.
Comment 25 Jonas Ådahl 2015-12-03 08:07:33 UTC
Created attachment 316707 [details] [review]
MetaPointerConfinementWayland: Support non-rectangular confinement regions

This patch adds support for confinement regions that are more complex
than a single rectangle. It relies on details about cairo regions not
explicitly in the API in order to generate the outer border of the
region.
Comment 26 Jonas Ådahl 2015-12-03 08:07:39 UTC
Created attachment 316708 [details] [review]
native: Don't wait for a new input event to wrap the pointer

If we rely on getting back an input event with the warped pointer
coordinates, we might draw a frame with the old coordinates if we warp
during the paint phase. Avoid that by moving the cursor immediately.
Comment 27 Carlos Garnacho 2015-12-09 17:26:23 UTC
Comment on attachment 316700 [details] [review]
wayland: Add global to surface coordinate helper

Looks good, there's places in meta-wayland-data-device.c and meta-wayland-touch.c that can benefit of this helper too.
Comment 28 Carlos Garnacho 2015-12-09 17:27:36 UTC
Review of attachment 316701 [details] [review]:

This one makes sense, setting as reviewed mainly because of the minor refactor bikeshedding below, but I think this one could be pushed before pointer confinement itself when these are sorted out.

::: src/core/meta-border.h
@@ +85,3 @@
+    .y = c * a.y,
+  };
+}

Do we need all these inline functions in the header? it seems to me like we only use a few of those functions in meta-pointer-confinement-wayland.c later, so perhaps we should add some further api to wrap the little usages there?

Also, I'm not too thrilled with polluting non "Meta" prefixed namespaces, slightly indifferent here because this is all private, but... s/Vector2/ClutterPoint/ comes to mind here too.

@@ +103,3 @@
+
+void
+meta_border_set_allows_directions (MetaBorder *border, unsigned int directions);

Looks like it should use MetaBorderMotionDirection? Also, one line per argument :)
Comment 29 Carlos Garnacho 2015-12-09 17:28:07 UTC
Comment on attachment 316703 [details] [review]
wayland: Add "painting" signal to surface actor

Makes sense :). A minor nitpick in the commit log: you don't seem to be actually moving anything, I would suggest "ensure the outputs are updated before repaint".
Comment 30 Carlos Garnacho 2015-12-09 17:28:29 UTC
Comment on attachment 316705 [details] [review]
MetaBorder: Use float constants and functions instead of double variants

Totally makes sense.
Comment 31 Carlos Garnacho 2015-12-09 17:30:18 UTC
Comment on attachment 316708 [details] [review]
native: Don't wait for a new input event to wrap the pointer

Indeed.
Comment 32 Carlos Garnacho 2015-12-09 17:33:51 UTC
Comment on attachment 316706 [details] [review]
wayland: Use the event coordinates when sending pointer motion events

Totally.
Comment 33 Carlos Garnacho 2015-12-09 17:39:04 UTC
Comment on attachment 316702 [details] [review]
native: Update to new constrain callback API

Makes sense to have this in a separate commit, perhaps explain the clutter API break in the commit log. Looks fine to me otherwise, feel free to assume a-c-n from me when the Clutter patches are in.
Comment 34 Carlos Garnacho 2015-12-09 17:49:57 UTC
Review of attachment 316699 [details] [review]:

Looks good! Just a couple minor nitpicks.

::: src/wayland/meta-wayland-pointer.c
@@ +286,3 @@
+  wl_resource_for_each (resource,
+                        &pointer->focus_client->relative_pointer_resources)
+    zwp_relative_pointer_v1_send_relative_motion (resource,

Better to have enclosing {} here.

@@ +1111,3 @@
+meta_wayland_relative_pointer_init (MetaWaylandCompositor *compositor)
+{
+  if (!wl_global_create (compositor->wayland_display,

I know caring about nested compositors here is moot, but perhaps we should only create the global when we know the backend can provide relative motion? I guess it's fine to create the global and then send nothing to clients, although that's potentially misleading.
Comment 35 Carlos Garnacho 2015-12-09 18:23:42 UTC
Review of attachment 316704 [details] [review]:

::: src/wayland/meta-wayland-pointer-constraints.c
@@ +79,3 @@
+
+static cairo_region_t *
+create_infinite_constraint_region (void)

Can't we encode somehow in meta-wayland-pointer-constraints.c that a NULL region means "unrestricted"? INT_MAX is a big enough arbitrary limit, but still arbitrary :)

@@ +224,3 @@
+  gboolean is_within;
+
+  region = meta_wayland_pointer_constraint_calculate_effective_region (constraint);

Could it make sense to cache this? I guess would require invalidating all the MetaWaylandPointerContraint when the surface input shape changes.

::: src/wayland/meta-wayland-pointer.c
@@ +52,2 @@
 #include "meta-wayland-pointer.h"
+#include "meta-wayland-pointer-constraints.h"

Looks like this include is not needed :)

::: src/wayland/meta-wayland-surface.c
@@ +2507,3 @@
+  ClutterVertex v = { 0 };
+
+  clutter_actor_apply_relative_transform_to_point (actor, NULL, &sv, &v);

Can't we use clutter_actor_get_transformed_position() here?

::: src/wayland/meta-wayland-surface.h
@@ +95,3 @@
+typedef struct _MetaWaylandPendingStateExtra MetaWaylandPendingStateExtra;
+
+typedef void (*MetaWaylandPendingExtraCommitFun) (

functions with typedef are usually suffixed "Func"

@@ +98,3 @@
+  MetaWaylandPendingStateExtra *extra,
+  MetaWaylandSurface *surface);
+typedef void (*MetaWaylandPendingExtraFreeFun) (

Same here, also, why not GDestroyNotify?

@@ +101,3 @@
+  MetaWaylandPendingStateExtra *extra);
+
+struct _MetaWaylandPendingStateExtra

Hmm, "Extra" doesn't tell a lot... If the struct contains just vfuncs, IMO should be suffixed "Vtable", "Listeners", "Notify" or somesuch.
Comment 36 Jonas Ådahl 2015-12-13 02:58:52 UTC
(In reply to Carlos Garnacho from comment #28)
> Review of attachment 316701 [details] [review] [review]:
> 
> This one makes sense, setting as reviewed mainly because of the minor
> refactor bikeshedding below, but I think this one could be pushed before
> pointer confinement itself when these are sorted out.
> 
> ::: src/core/meta-border.h
> @@ +85,3 @@
> +    .y = c * a.y,
> +  };
> +}
> 
> Do we need all these inline functions in the header? it seems to me like we
> only use a few of those functions in meta-pointer-confinement-wayland.c
> later, so perhaps we should add some further api to wrap the little usages
> there?

Hmm, seems to only ever be used in meta-border so makes sense to keep it in the .c file indeed.

> 
> Also, I'm not too thrilled with polluting non "Meta" prefixed namespaces,
> slightly indifferent here because this is all private, but...
> s/Vector2/ClutterPoint/ comes to mind here too.

Do you mean you'd prefer MetaLine and MetaVector, even if its private to a .c file? I guess I could do that.

> 
> @@ +103,3 @@
> +
> +void
> +meta_border_set_allows_directions (MetaBorder *border, unsigned int
> directions);
> 
> Looks like it should use MetaBorderMotionDirection? Also, one line per
> argument :)

'directions' is a bitmask so it should not have the enum type.
Comment 37 Jonas Ådahl 2015-12-13 03:06:04 UTC
(In reply to Carlos Garnacho from comment #35)
> Review of attachment 316704 [details] [review] [review]:
> 
> ::: src/wayland/meta-wayland-pointer-constraints.c
> @@ +79,3 @@
> +
> +static cairo_region_t *
> +create_infinite_constraint_region (void)
> 
> Can't we encode somehow in meta-wayland-pointer-constraints.c that a NULL
> region means "unrestricted"? INT_MAX is a big enough arbitrary limit, but
> still arbitrary :)

True. I'll check how it'd look code wise to do this.

> 
> @@ +224,3 @@
> +  gboolean is_within;
> +
> +  region = meta_wayland_pointer_constraint_calculate_effective_region
> (constraint);
> 
> Could it make sense to cache this? I guess would require invalidating all
> the MetaWaylandPointerContraint when the surface input shape changes.

Indeed it would. I wanted to make it work first, and do optimizations later, which is why its not there now. I plan to do it as a follow up.

> 
> ::: src/wayland/meta-wayland-pointer.c
> @@ +52,2 @@
>  #include "meta-wayland-pointer.h"
> +#include "meta-wayland-pointer-constraints.h"
> 
> Looks like this include is not needed :)
> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +2507,3 @@
> +  ClutterVertex v = { 0 };
> +
> +  clutter_actor_apply_relative_transform_to_point (actor, NULL, &sv, &v);
> 
> Can't we use clutter_actor_get_transformed_position() here?

Good point.

> 
> ::: src/wayland/meta-wayland-surface.h
> @@ +95,3 @@
> +typedef struct _MetaWaylandPendingStateExtra MetaWaylandPendingStateExtra;
> +
> +typedef void (*MetaWaylandPendingExtraCommitFun) (
> 
> functions with typedef are usually suffixed "Func"
> 
> @@ +98,3 @@
> +  MetaWaylandPendingStateExtra *extra,
> +  MetaWaylandSurface *surface);
> +typedef void (*MetaWaylandPendingExtraFreeFun) (
> 
> Same here, also, why not GDestroyNotify?

No reason. Will change it.

> 
> @@ +101,3 @@
> +  MetaWaylandPendingStateExtra *extra);
> +
> +struct _MetaWaylandPendingStateExtra
> 
> Hmm, "Extra" doesn't tell a lot... If the struct contains just vfuncs, IMO
> should be suffixed "Vtable", "Listeners", "Notify" or somesuch.

I don't really like "Listeners" or "Notify", and Vtable is just what it is. I'd like a name that makes it obvious that it's a state that is an extension to the pending state.
Comment 38 Carlos Garnacho 2015-12-13 12:47:09 UTC
(In reply to Jonas Ådahl from comment #36)
> (In reply to Carlos Garnacho from comment #28)
> > Review of attachment 316701 [details] [review] [review] [review]:
> > 
> > This one makes sense, setting as reviewed mainly because of the minor
> > refactor bikeshedding below, but I think this one could be pushed before
> > pointer confinement itself when these are sorted out.
> > 
> > ::: src/core/meta-border.h
> > @@ +85,3 @@
> > +    .y = c * a.y,
> > +  };
> > +}
> > 
> > Do we need all these inline functions in the header? it seems to me like we
> > only use a few of those functions in meta-pointer-confinement-wayland.c
> > later, so perhaps we should add some further api to wrap the little usages
> > there?
> 
> Hmm, seems to only ever be used in meta-border so makes sense to keep it in
> the .c file indeed.
> 
> > 
> > Also, I'm not too thrilled with polluting non "Meta" prefixed namespaces,
> > slightly indifferent here because this is all private, but...
> > s/Vector2/ClutterPoint/ comes to mind here too.
> 
> Do you mean you'd prefer MetaLine and MetaVector, even if its private to a
> .c file? I guess I could do that.

Yeah I meant that, it looks like you use the MetaBorder fields in the public (eg. as storage of the border data in MetaBarrier), so the structs definitions themselves look like should stay in the header anyway.

> 
> > 
> > @@ +103,3 @@
> > +
> > +void
> > +meta_border_set_allows_directions (MetaBorder *border, unsigned int
> > directions);
> > 
> > Looks like it should use MetaBorderMotionDirection? Also, one line per
> > argument :)
> 
> 'directions' is a bitmask so it should not have the enum type.

Fair enough, however git grep "Flags \+[a-z]*[),]" seems to show a few usages of flag typedefs as function arguments on mutter, and I'd dare say that also seems the standard on glib/gtk+.

(In reply to Jonas Ådahl from comment #37)
> (In reply to Carlos Garnacho from comment #35)
> > Review of attachment 316704 [details] [review] [review] [review]:
> > 
> > ::: src/wayland/meta-wayland-pointer-constraints.c
> > @@ +79,3 @@
> > +
> > +static cairo_region_t *
> > +create_infinite_constraint_region (void)
> > 
> > Can't we encode somehow in meta-wayland-pointer-constraints.c that a NULL
> > region means "unrestricted"? INT_MAX is a big enough arbitrary limit, but
> > still arbitrary :)
> 
> True. I'll check how it'd look code wise to do this.
> 
> > 
> > @@ +224,3 @@
> > +  gboolean is_within;
> > +
> > +  region = meta_wayland_pointer_constraint_calculate_effective_region
> > (constraint);
> > 
> > Could it make sense to cache this? I guess would require invalidating all
> > the MetaWaylandPointerContraint when the surface input shape changes.
> 
> Indeed it would. I wanted to make it work first, and do optimizations later,
> which is why its not there now. I plan to do it as a follow up.

Aha sure, it just struck me as a sort of frequent path.

> 
> > 
> > ::: src/wayland/meta-wayland-pointer.c
> > @@ +52,2 @@
> >  #include "meta-wayland-pointer.h"
> > +#include "meta-wayland-pointer-constraints.h"
> > 
> > Looks like this include is not needed :)
> > 
> > ::: src/wayland/meta-wayland-surface.c
> > @@ +2507,3 @@
> > +  ClutterVertex v = { 0 };
> > +
> > +  clutter_actor_apply_relative_transform_to_point (actor, NULL, &sv, &v);
> > 
> > Can't we use clutter_actor_get_transformed_position() here?
> 
> Good point.
> 
> > 
> > ::: src/wayland/meta-wayland-surface.h
> > @@ +95,3 @@
> > +typedef struct _MetaWaylandPendingStateExtra MetaWaylandPendingStateExtra;
> > +
> > +typedef void (*MetaWaylandPendingExtraCommitFun) (
> > 
> > functions with typedef are usually suffixed "Func"
> > 
> > @@ +98,3 @@
> > +  MetaWaylandPendingStateExtra *extra,
> > +  MetaWaylandSurface *surface);
> > +typedef void (*MetaWaylandPendingExtraFreeFun) (
> > 
> > Same here, also, why not GDestroyNotify?
> 
> No reason. Will change it.
> 
> > 
> > @@ +101,3 @@
> > +  MetaWaylandPendingStateExtra *extra);
> > +
> > +struct _MetaWaylandPendingStateExtra
> > 
> > Hmm, "Extra" doesn't tell a lot... If the struct contains just vfuncs, IMO
> > should be suffixed "Vtable", "Listeners", "Notify" or somesuch.
> 
> I don't really like "Listeners" or "Notify", and Vtable is just what it is.
> I'd like a name that makes it obvious that it's a state that is an extension
> to the pending state.

Hmm, I think it's worth having MetaWaylandPendingState be a GObject, so we add an "applied" signal and attach any extra info with g_object_set_qdata_full() for the PendingState lifetime.
Comment 39 Jonas Ådahl 2015-12-14 08:46:10 UTC
(In reply to Carlos Garnacho from comment #38)
> (In reply to Jonas Ådahl from comment #36) 
> > > 
> > > @@ +101,3 @@
> > > +  MetaWaylandPendingStateExtra *extra);
> > > +
> > > +struct _MetaWaylandPendingStateExtra
> > > 
> > > Hmm, "Extra" doesn't tell a lot... If the struct contains just vfuncs, IMO
> > > should be suffixed "Vtable", "Listeners", "Notify" or somesuch.
> > 
> > I don't really like "Listeners" or "Notify", and Vtable is just what it is.
> > I'd like a name that makes it obvious that it's a state that is an extension
> > to the pending state.
> 
> Hmm, I think it's worth having MetaWaylandPendingState be a GObject, so we
> add an "applied" signal and attach any extra info with
> g_object_set_qdata_full() for the PendingState lifetime.

I suppose we can do that. It'll require some some non-trivial changes since we rely on the state is never allocated separately, but might as well be worth in in the long run.
Comment 40 Jonas Ådahl 2015-12-14 13:49:20 UTC
Created attachment 317364 [details] [review]
wayland: Implement support for wp_relative_pointer

Add support for sending relative pointer motion deltas to clients who
request such events by creating wp_relative_pointer objects via
wp_relative_pointer_manager.

This currently implements the unstable version 1 from wayland-protocols.
Comment 41 Jonas Ådahl 2015-12-14 13:53:07 UTC
Created attachment 317365 [details] [review]
Move out generic math parts out of the native barrier implementation

In order to reuse some vector math for pointer confinement, move out
those parts to its own file, introducing the types old types
"MetaVector2" and "MetaLine2" outside of meta-barrier-native.c, as well
as introducing MetaBorder which is a line, with a blocking direction.

----

Changes:
The new types are now Meta prefixed, and a couple of functions were moved to the meta-border.c file.
Comment 42 Jonas Ådahl 2015-12-14 14:00:34 UTC
Created attachment 317368 [details] [review]
wayland: Add "painting" signal to surface actor

Make MetaWaylandSurface a listener and move output state updating to
the handler function.

---

Changes: Actually move the output updating
Comment 43 Jonas Ådahl 2015-12-14 14:04:40 UTC
Created attachment 317369 [details] [review]
wayland: Make the pending surface state a GObject

Making the pending state an GObject makes it easier to extend it with
additional optional state without putting everything inside one big
struct.

--

This is a new commit that turnes the pending state into a GObject. It makes the state move a bit more verbose, but since its now actually spelled out, I spotted an error: we replace the cached subsurface state when we should actually accumulate it (buffer damage, buffer_set, ...). Its out of scope for this series so I'll fix it separately.
Comment 44 Jonas Ådahl 2015-12-14 14:12:18 UTC
Created attachment 317370 [details] [review]
Implement support for the wp_pointer_constraints protocol

The wp_pointer_constraints protocol is a protocol which enables clients
to manipulate the behavior of the pointer cursor associated with a seat.

Currently available constraints are locking the pointer to a static
position, and confining the pointer to a given region.

Currently locking is fully implemented, and confining is implemented for
rectangular confinement regions.

What else is lacking is less troublesome semantics for enabling the lock
or confinement; currently the only requirement implemented is that the
window that appears focused is the one that may aquire the lock.

This means that a pointer could be 'stolen' by creating a new window that
receives active focus, or when using focus-follows-mouse, a pointer
passes a window that has requested a lock. This semantics can be changed
and the protocol itself allows any semantics as seems fit.

-----

Changes:

This new version completely replaces the pending state tracking. Instead of having the MetaWaylandSurface track state-per-seat, it's now done internally in meta-wayland-pointer-constraints.c which just stores one pending state container per pending surface state (each carrying the state associated with the constraint i.e. seat).

I didn't special case NULL regions, mostly because it'd make the mathematics more confusing. If you insist, I can still do it, but didn't really see it as much of an improvements.

I didn't change to using clutter_actor_get_transformed_position() because it's a point that is transformed, not the actor position that is retrieved.
Comment 45 Jonas Ådahl 2016-01-28 13:42:15 UTC
Created attachment 319925 [details] [review]
wayland: Implement support for wp_relative_pointer

Add support for sending relative pointer motion deltas to clients who
request such events by creating wp_relative_pointer objects via
wp_relative_pointer_manager.

This currently implements the unstable version 1 from wayland-protocols.
Comment 46 Jonas Ådahl 2016-01-28 13:42:23 UTC
Created attachment 319926 [details] [review]
wayland: Add global to surface coordinate helper
Comment 47 Jonas Ådahl 2016-01-28 13:42:31 UTC
Created attachment 319927 [details] [review]
Move out generic math parts out of the native barrier implementation

In order to reuse some vector math for pointer confinement, move out
those parts to its own file, introducing the types old types
"MetaVector2" and "MetaLine2" outside of meta-barrier-native.c, as well
as introducing MetaBorder which is a line, with a blocking direction.
Comment 48 Jonas Ådahl 2016-01-28 13:42:41 UTC
Created attachment 319929 [details] [review]
native: Update to new constrain callback API
Comment 49 Jonas Ådahl 2016-01-28 13:42:49 UTC
Created attachment 319930 [details] [review]
wayland: Add "painting" signal to surface actor

Make MetaWaylandSurface a listener and move output state updating to
the handler function.
Comment 50 Jonas Ådahl 2016-01-28 13:42:58 UTC
Created attachment 319931 [details] [review]
wayland: Make the pending surface state a GObject

Making the pending state an GObject makes it easier to extend it with
additional optional state without putting everything inside one big
struct.
Comment 51 Jonas Ådahl 2016-01-28 13:43:07 UTC
Created attachment 319932 [details] [review]
Implement support for the wp_pointer_constraints protocol

The wp_pointer_constraints protocol is a protocol which enables clients
to manipulate the behavior of the pointer cursor associated with a seat.

Currently available constraints are locking the pointer to a static
position, and confining the pointer to a given region.

Currently locking is fully implemented, and confining is implemented for
rectangular confinement regions.

What else is lacking is less troublesome semantics for enabling the lock
or confinement; currently the only requirement implemented is that the
window that appears focused is the one that may aquire the lock.

This means that a pointer could be 'stolen' by creating a new window that
receives active focus, or when using focus-follows-mouse, a pointer
passes a window that has requested a lock. This semantics can be changed
and the protocol itself allows any semantics as seems fit.
Comment 52 Jonas Ådahl 2016-01-28 13:43:15 UTC
Created attachment 319933 [details] [review]
MetaBorder: Use float constants and functions instead of double variants

We calculate with floats, so lets use that type throughout.
Comment 53 Jonas Ådahl 2016-01-28 13:43:25 UTC
Created attachment 319934 [details] [review]
wayland: Use the event coordinates when sending pointer motion events

The x/y coordinates of the ClutterInputDevice were not the ones which was
the result of this event but whatever event was queued the last. The
correct coordinates can, however, be found in the event itself, so lets
use those.
Comment 54 Jonas Ådahl 2016-01-28 13:43:32 UTC
Created attachment 319935 [details] [review]
MetaPointerConfinementWayland: Support non-rectangular confinement regions

This patch adds support for confinement regions that are more complex
than a single rectangle. It relies on details about cairo regions not
explicitly in the API in order to generate the outer border of the
region.
Comment 55 Jonas Ådahl 2016-01-28 13:43:41 UTC
Created attachment 319936 [details] [review]
native: Don't wait for a new input event to wrap the pointer

If we rely on getting back an input event with the warped pointer
coordinates, we might draw a frame with the old coordinates if we warp
during the paint phase. Avoid that by moving the cursor immediately.
Comment 56 Carlos Garnacho 2016-01-28 23:12:45 UTC
Comment on attachment 319925 [details] [review]
wayland: Implement support for wp_relative_pointer

Looks good to me
Comment 57 Carlos Garnacho 2016-01-28 23:12:58 UTC
Comment on attachment 319926 [details] [review]
wayland: Add global to surface coordinate helper

Ditto
Comment 58 Carlos Garnacho 2016-01-28 23:13:10 UTC
Review of attachment 319927 [details] [review]:

Indeed. Not a fan of kernel like struct initializations, but I realize you didn't introduce this, won't bikeshed over it.
Comment 59 Carlos Garnacho 2016-01-28 23:13:22 UTC
Comment on attachment 319929 [details] [review]
native: Update to new constrain callback API

Sure
Comment 60 Carlos Garnacho 2016-01-28 23:13:36 UTC
Comment on attachment 319930 [details] [review]
wayland: Add "painting" signal to surface actor

Thinking twice about it, I think you could just do:

g_signal_connect_object (surface->surface_actor,
                         "paint",
                         G_CALLBACK (meta_wayland_surface_update_outputs),
                         surface,
                         G_CONNECT_AFTER | G_CONNECT_SWAPPED)

and avoid creating/emitting the signal altogether. We're after all tracking something we already have a signal for.
Comment 61 Carlos Garnacho 2016-01-28 23:13:45 UTC
Comment on attachment 319931 [details] [review]
wayland: Make the pending surface state a GObject

Looks good!
Comment 62 Carlos Garnacho 2016-01-28 23:14:25 UTC
Review of attachment 319932 [details] [review]:

Looks good to me, just a couple minor comments. Deem this a-c-n after fixing those.

::: src/wayland/meta-pointer-lock-wayland.c
@@ +46,3 @@
+                                     float                  prev_y,
+                                     float                  *x,
+                                     float                  *y)

Nit: bad indentation

::: src/wayland/meta-wayland-pointer-constraints.c
@@ +92,3 @@
+                                          .y = -INT_MAX / 2,
+                                          .width = INT_MAX,
+                                          .height = INT_MAX,

I'd rather if you'd use INT_MIN instead of -INT_MAX here, less time will be spent in thinking whether there's underflow or not.

Again not a fan of kernel like struct initialization, but...
Comment 63 Carlos Garnacho 2016-01-28 23:14:41 UTC
Comment on attachment 319933 [details] [review]
MetaBorder: Use float constants and functions instead of double variants

I think this patch could just be merged with the one adding meta-border.c? Looks fine to me nonetheless.
Comment 64 Carlos Garnacho 2016-01-28 23:15:14 UTC
Comment on attachment 319934 [details] [review]
wayland: Use the event coordinates when sending pointer motion events

Indeed.
Comment 65 Carlos Garnacho 2016-01-28 23:15:24 UTC
Comment on attachment 319936 [details] [review]
native: Don't wait for a new input event to wrap the pointer

Indeed
Comment 66 Carlos Garnacho 2016-01-28 23:15:55 UTC
Comment on attachment 319935 [details] [review]
MetaPointerConfinementWayland: Support non-rectangular confinement regions

Still trying to fully grasp the FP logic. I would be certainly less uneasy about this one if we didn't rely on cairo_region_t implementation details.

Is this something the protocol mandates? or just for full correctness?
Comment 67 Jonas Ådahl 2016-01-29 00:00:17 UTC
(In reply to Carlos Garnacho from comment #66)
> Comment on attachment 319935 [details] [review] [review]
> MetaPointerConfinementWayland: Support non-rectangular confinement regions
> 
> Still trying to fully grasp the FP logic. I would be certainly less uneasy
> about this one if we didn't rely on cairo_region_t implementation details.
> 
> Is this something the protocol mandates? or just for full correctness?

Yes, this is mandated by the proticol by the fact that input and lock regions may be nonrectangular. It'd be nice of course to not rely on the fact that rect bands is only an implementation detail but an API promise. Could it be made into a promise maybe? I have no other ideas how to do these things any other way except duplicate all of the region code in mutter.
Comment 68 Carlos Garnacho 2016-01-29 00:16:30 UTC
(In reply to Jonas Ådahl from comment #67)
> (In reply to Carlos Garnacho from comment #66)
> > Comment on attachment 319935 [details] [review] [review] [review]
> > MetaPointerConfinementWayland: Support non-rectangular confinement regions
> > 
> > Still trying to fully grasp the FP logic. I would be certainly less uneasy
> > about this one if we didn't rely on cairo_region_t implementation details.
> > 
> > Is this something the protocol mandates? or just for full correctness?
> 
> Yes, this is mandated by the proticol by the fact that input and lock
> regions may be nonrectangular. It'd be nice of course to not rely on the
> fact that rect bands is only an implementation detail but an API promise.
> Could it be made into a promise maybe? I have no other ideas how to do these
> things any other way except duplicate all of the region code in mutter.

Maybe we could "just" attempt to reorder/transform the rectangles so they match with our expectancies here, even if we know cairo_region_t will behave for us, so that check should mostly be a O(n) no-op for the common case.

Although that's just my opinion, if we eg. get a word from Owen that this is something we can rely on, I'm fine with it.
Comment 69 Jonas Ådahl 2016-01-29 06:29:05 UTC
(In reply to Carlos Garnacho from comment #60)
> Comment on attachment 319930 [details] [review] [review]
> wayland: Add "painting" signal to surface actor
> 
> Thinking twice about it, I think you could just do:
> 
> g_signal_connect_object (surface->surface_actor,
>                          "paint",
>                          G_CALLBACK (meta_wayland_surface_update_outputs),
>                          surface,
>                          G_CONNECT_AFTER | G_CONNECT_SWAPPED)
> 
> and avoid creating/emitting the signal altogether. We're after all tracking
> something we already have a signal for.

The "paint" signal is deprecated since 1.12.

(In reply to Carlos Garnacho from comment #68)
> (In reply to Jonas Ådahl from comment #67)
> > (In reply to Carlos Garnacho from comment #66)
> > > Comment on attachment 319935 [details] [review] [review] [review] [review]
> > > MetaPointerConfinementWayland: Support non-rectangular confinement regions
> > > 
> > > Still trying to fully grasp the FP logic. I would be certainly less uneasy
> > > about this one if we didn't rely on cairo_region_t implementation details.
> > > 
> > > Is this something the protocol mandates? or just for full correctness?
> > 
> > Yes, this is mandated by the proticol by the fact that input and lock
> > regions may be nonrectangular. It'd be nice of course to not rely on the
> > fact that rect bands is only an implementation detail but an API promise.
> > Could it be made into a promise maybe? I have no other ideas how to do these
> > things any other way except duplicate all of the region code in mutter.
> 
> Maybe we could "just" attempt to reorder/transform the rectangles so they
> match with our expectancies here, even if we know cairo_region_t will behave
> for us, so that check should mostly be a O(n) no-op for the common case.

I guess we could add some sanity checking, and just bail cairo changed the internal rect structuring under our feet.

> 
> Although that's just my opinion, if we eg. get a word from Owen that this is
> something we can rely on, I'm fine with it.

Owen, can we rely on the rects provided by cairo to be in non-overlapping bands where rects in every band don't ever touch each other?
Comment 70 Owen Taylor 2016-02-01 15:41:34 UTC
(In reply to Jonas Ådahl from comment #69)
> (In reply to Carlos Garnacho from comment #60)
> > Comment on attachment 319930 [details] [review] [review] [review]
> > wayland: Add "painting" signal to surface actor
> > 
> > Thinking twice about it, I think you could just do:
> > 
> > g_signal_connect_object (surface->surface_actor,
> >                          "paint",
> >                          G_CALLBACK (meta_wayland_surface_update_outputs),
> >                          surface,
> >                          G_CONNECT_AFTER | G_CONNECT_SWAPPED)
> > 
> > and avoid creating/emitting the signal altogether. We're after all tracking
> > something we already have a signal for.
> 
> The "paint" signal is deprecated since 1.12.

Whether or not it's deprecated (not to relevant given the slow movement of clutter), it's a bad idea to connect to "paint", since connecting to paint disables culling in clutter - so this could suddenly make Mutter much slower.

> > Although that's just my opinion, if we eg. get a word from Owen that this is
> > something we can rely on, I'm fine with it.
> 
> Owen, can we rely on the rects provided by cairo to be in non-overlapping
> bands where rects in every band don't ever touch each other?

I'm not sure what you mean by "touch" each other ,but the "XY" banding nature of pixman regions is inherent to their imlpementation, and has been that way for ~25 years (code comes from X). Since they work quite well and are internally super-complex, it's sort of inconceivable to me that it would change; at most I'd add an sertion that fails cleanly if thigns change, but even that seems like a waste of code.
Comment 71 Jasper St. Pierre (not reading bugmail) 2016-02-01 17:53:40 UTC
Yeah, the YX banding approach is central to how regions work! I have a bit of elaboration about it:

http://magcius.github.io/xplain/article/regions.html
Comment 72 Jonas Ådahl 2016-02-16 11:08:14 UTC
Attachment 319925 [details] pushed as 5b0eabe - wayland: Implement support for wp_relative_pointer
Attachment 319926 [details] pushed as bc8ec2d - wayland: Add global to surface coordinate helper
Attachment 319927 [details] pushed as f0f638d - Move out generic math parts out of the native barrier implementation
Attachment 319929 [details] pushed as 5256440 - native: Update to new constrain callback API
Attachment 319930 [details] pushed as 020ae58 - wayland: Add "painting" signal to surface actor
Attachment 319931 [details] pushed as e2efc85 - wayland: Make the pending surface state a GObject
Attachment 319932 [details] pushed as 495c894 - Implement support for the wp_pointer_constraints protocol
Attachment 319933 [details] pushed as bc1dd1c - MetaBorder: Use float constants and functions instead of double variants
Attachment 319934 [details] pushed as bc47b19 - wayland: Use the event coordinates when sending pointer motion events
Attachment 319935 [details] pushed as a70a2c3 - MetaPointerConfinementWayland: Support non-rectangular confinement regions
Attachment 319936 [details] pushed as 9611661 - native: Don't wait for a new input event to wrap the pointer