Created attachment 322323 [details] [review]
MetaWaylandPointerConstraint: Fix effective region calculation

The when surface->input_region is NULL, it should be interpreted as the
whole surface region. If not, the effective input region is the
intersection of the buffer region and the input region set by
wl_surface.set_input_region. Add
meta_wayland_surface_calculate_input_region() that does this
calculation.

Created attachment 322324 [details] [review]
MetaWaylandPointerConstraint: Use NULL for infinite region

Instead of having a very large region represent an infinitely large
region, use NULL, and use the calculated input region from the
MetaWaylandSurface if the constraint region was not set.

Created attachment 322325 [details] [review]
MetaWaylandPointerConstraint: Make per surface state a quark

Make the per surface pointer constraint related state (list of
constraints on given surface) a quark managed from the pointer
constraints unit.

Created attachment 322326 [details] [review]
MetaWaylandPointerConstraint: Use own 'appears-focused' signal handler

Instead of having MetaWindowWayland having hooks into pointer
constraints subsystem, have the pointer constraints subsystem listen
for the signal itself and enable/disable itself.

Created attachment 322327 [details] [review]
MetaWaylandPointerConstraint: Unset is_enabled state when disabling

If we don't set the is_disabled state to FALSE when disabling,
re-enabling will fail.

Created attachment 322328 [details] [review]
MetaWaylandPointerConstraint: Require 'appears-focused' to enable

Instead of relying on the keyboard focus surface, use the
'appears-focused' state of the corresponding MetaWindow to determine if
a constraint should enable or not.

Created attachment 322329 [details] [review]
MetaWaylandPointerConstraint: Disable if 'appears-focused' false

Disable if the corresponding MetaWindow's 'appears-focused' state
changes to false.

Created attachment 322330 [details] [review]
MetaWaylandPointerConstraint: Maybe enable on window 'raise'

If a MetaWindow's 'appears-focused' state changed to true, but the
window did not have pointer focus, the constraint did not enable. Thus,
make it possible for the user to also click the window to enable it.

Review of attachment 322323 [details] [review]:

Looks right to me.

::: src/wayland/meta-wayland-pointer-constraints.c
@@ -239,2 @@
   region = meta_wayland_pointer_constraint_calculate_effective_region (constraint);
-  is_within = cairo_region_contains_point (constraint->region,

Oops :)

Comment on attachment 322324 [details] [review]
MetaWaylandPointerConstraint: Use NULL for infinite region

Nice to see this gone.

Comment on attachment 322325 [details] [review]
MetaWaylandPointerConstraint: Make per surface state a quark

Nice cleanup

Comment on attachment 322326 [details] [review]
MetaWaylandPointerConstraint: Use own 'appears-focused' signal handler

Makes sense

Review of attachment 322326 [details] [review]:

::: src/wayland/meta-wayland-pointer-constraints.c
@@ +97,3 @@
 
+static void
+meta_wayland_pointer_constraint_maybe_enable_for_window (MetaWindow *window);

Oh, I don't think I see the function itself being made static void. My a-c-n stands with this change.

Comment on attachment 322327 [details] [review]
MetaWaylandPointerConstraint: Unset is_enabled state when disabling

Indeed

Review of attachment 322328 [details] [review]:

Yeah, makes sense for consistency. Maybe just using g_return_if_fail() though?

Comment on attachment 322329 [details] [review]
MetaWaylandPointerConstraint: Disable if 'appears-focused' false

Looks good to me. Makes sense to check on the same thing that maybe triggers this.

Comment on attachment 322330 [details] [review]
MetaWaylandPointerConstraint: Maybe enable on window 'raise'

Sure.

(In reply to Carlos Garnacho from comment #16)
> Comment on attachment 322329 [details] [review] [review]
> MetaWaylandPointerConstraint: Disable if 'appears-focused' false
> 
> Looks good to me. Makes sense to check on the same thing that maybe triggers
> this.

Oh, the commit log also seemed broken to me, like a missing word replaced by \n or something.

(In reply to Carlos Garnacho from comment #15)
> Review of attachment 322328 [details] [review] [review]:
> 
> Yeah, makes sense for consistency. Maybe just using g_return_if_fail()
> though?

IIRC g_return_if_fail() is a macro that may expand to nothing if configured to do so. We still need to do the check since a client can request a constraint on a subsurface (which we won't support right now). I have another branch which introduces a meta_wayland_surface_get_toplevel() like function that I plan to use here though, which will fix that issue.

Thanks for the review! Will fix the other issues you noticed before pushing.

Attachment 322323 [details] pushed as 62ac9df - MetaWaylandPointerConstraint: Fix effective region calculation
Attachment 322324 [details] pushed as 4abfb29 - MetaWaylandPointerConstraint: Use NULL for infinite region
Attachment 322325 [details] pushed as 20908b9 - MetaWaylandPointerConstraint: Make per surface state a quark
Attachment 322326 [details] pushed as 6396974 - MetaWaylandPointerConstraint: Use own 'appears-focused' signal handler
Attachment 322327 [details] pushed as b04747b - MetaWaylandPointerConstraint: Unset is_enabled state when disabling
Attachment 322328 [details] pushed as 1c94d0e - MetaWaylandPointerConstraint: Require 'appears-focused' to enable
Attachment 322330 [details] pushed as d4b0c21 - MetaWaylandPointerConstraint: Maybe enable on window 'raise'

Comment on attachment 322329 [details] [review]
MetaWaylandPointerConstraint: Disable if 'appears-focused' false

This one has already landed.