GNOME Bugzilla – Bug 744104
Implement pointer motion, locks and confinemen
Last modified: 2016-02-16 11:09:19 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
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.
*** Bug 752321 has been marked as a duplicate of this bug. ***
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.
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.
Created attachment 307805 [details] [review] wayland: Add global to surface coordinate helper
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.
Created attachment 307807 [details] [review] native: Update to new constrain callback API
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.
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.
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.
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().
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.
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.
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.
Review of attachment 307810 [details] [review]: Nice catch. Push straight away.
This seems to have fallen off the table ? Can we land this now ?
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.
Created attachment 316700 [details] [review] wayland: Add global to surface coordinate helper
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.
Created attachment 316702 [details] [review] native: Update to new constrain callback API
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.
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.
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.
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.
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.
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 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.
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 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 on attachment 316705 [details] [review] MetaBorder: Use float constants and functions instead of double variants Totally makes sense.
Comment on attachment 316708 [details] [review] native: Don't wait for a new input event to wrap the pointer Indeed.
Comment on attachment 316706 [details] [review] wayland: Use the event coordinates when sending pointer motion events Totally.
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.
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.
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.
(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.
(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.
(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.
(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.
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.
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.
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
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.
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.
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.
Created attachment 319926 [details] [review] wayland: Add global to surface coordinate helper
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.
Created attachment 319929 [details] [review] native: Update to new constrain callback API
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.
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.
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.
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.
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.
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.
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 on attachment 319925 [details] [review] wayland: Implement support for wp_relative_pointer Looks good to me
Comment on attachment 319926 [details] [review] wayland: Add global to surface coordinate helper Ditto
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 on attachment 319929 [details] [review] native: Update to new constrain callback API Sure
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 on attachment 319931 [details] [review] wayland: Make the pending surface state a GObject Looks good!
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 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 on attachment 319934 [details] [review] wayland: Use the event coordinates when sending pointer motion events Indeed.
Comment on attachment 319936 [details] [review] native: Don't wait for a new input event to wrap the pointer Indeed
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?
(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.
(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.
(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?
(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.
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
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