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 762661 - wayland: Various pointer constraints fixes
wayland: Various pointer constraints fixes
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2016-02-25 03:54 UTC by Jonas Ådahl
Modified: 2016-09-06 07:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
MetaWaylandPointerConstraint: Fix effective region calculation (3.42 KB, patch)
2016-02-25 03:54 UTC, Jonas Ådahl
committed Details | Review
MetaWaylandPointerConstraint: Use NULL for infinite region (2.96 KB, patch)
2016-02-25 03:54 UTC, Jonas Ådahl
committed Details | Review
MetaWaylandPointerConstraint: Make per surface state a quark (11.38 KB, patch)
2016-02-25 03:54 UTC, Jonas Ådahl
committed Details | Review
MetaWaylandPointerConstraint: Use own 'appears-focused' signal handler (5.80 KB, patch)
2016-02-25 03:55 UTC, Jonas Ådahl
committed Details | Review
MetaWaylandPointerConstraint: Unset is_enabled state when disabling (1.15 KB, patch)
2016-02-25 03:55 UTC, Jonas Ådahl
committed Details | Review
MetaWaylandPointerConstraint: Require 'appears-focused' to enable (1.43 KB, patch)
2016-02-25 03:55 UTC, Jonas Ådahl
committed Details | Review
MetaWaylandPointerConstraint: Disable if 'appears-focused' false (3.14 KB, patch)
2016-02-25 03:55 UTC, Jonas Ådahl
committed Details | Review
MetaWaylandPointerConstraint: Maybe enable on window 'raise' (2.34 KB, patch)
2016-02-25 03:55 UTC, Jonas Ådahl
committed Details | Review

Description Jonas Ådahl 2016-02-25 03:54:42 UTC
These patches fixes various issues discovered while testing pointer constraint
clients and usages.
Comment 1 Jonas Ådahl 2016-02-25 03:54:46 UTC
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.
Comment 2 Jonas Ådahl 2016-02-25 03:54:52 UTC
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.
Comment 3 Jonas Ådahl 2016-02-25 03:54:57 UTC
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.
Comment 4 Jonas Ådahl 2016-02-25 03:55:02 UTC
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.
Comment 5 Jonas Ådahl 2016-02-25 03:55:07 UTC
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.
Comment 6 Jonas Ådahl 2016-02-25 03:55:13 UTC
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.
Comment 7 Jonas Ådahl 2016-02-25 03:55:18 UTC
Created attachment 322329 [details] [review]
MetaWaylandPointerConstraint: Disable if 'appears-focused' false

Disable if the corresponding MetaWindow's 'appears-focused' state
changes to false.
Comment 8 Jonas Ådahl 2016-02-25 03:55:23 UTC
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.
Comment 9 Carlos Garnacho 2016-02-26 00:15:40 UTC
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 10 Carlos Garnacho 2016-02-26 00:16:12 UTC
Comment on attachment 322324 [details] [review]
MetaWaylandPointerConstraint: Use NULL for infinite region

Nice to see this gone.
Comment 11 Carlos Garnacho 2016-02-26 00:17:10 UTC
Comment on attachment 322325 [details] [review]
MetaWaylandPointerConstraint: Make per surface state a quark

Nice cleanup
Comment 12 Carlos Garnacho 2016-02-26 00:19:29 UTC
Comment on attachment 322326 [details] [review]
MetaWaylandPointerConstraint: Use own 'appears-focused' signal handler

Makes sense
Comment 13 Carlos Garnacho 2016-02-26 00:22:29 UTC
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 14 Carlos Garnacho 2016-02-26 00:23:10 UTC
Comment on attachment 322327 [details] [review]
MetaWaylandPointerConstraint: Unset is_enabled state when disabling

Indeed
Comment 15 Carlos Garnacho 2016-02-26 00:25:33 UTC
Review of attachment 322328 [details] [review]:

Yeah, makes sense for consistency. Maybe just using g_return_if_fail() though?
Comment 16 Carlos Garnacho 2016-02-26 00:27:38 UTC
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 17 Carlos Garnacho 2016-02-26 00:28:01 UTC
Comment on attachment 322330 [details] [review]
MetaWaylandPointerConstraint: Maybe enable on window 'raise'

Sure.
Comment 18 Carlos Garnacho 2016-02-26 00:29:34 UTC
(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.
Comment 19 Jonas Ådahl 2016-02-26 02:19:07 UTC
(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.
Comment 20 Jonas Ådahl 2016-03-09 06:34:26 UTC
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 21 Jonas Ådahl 2016-09-06 07:02:24 UTC
Comment on attachment 322329 [details] [review]
MetaWaylandPointerConstraint: Disable if 'appears-focused' false

This one has already landed.