GNOME Bugzilla – Bug 706655
wayland: implement pointer barriers
Last modified: 2015-01-19 10:29:04 UTC
Use the new Clutter hook to make sure the pointer never enters the dead area caused by the different monitor sizes. You don't realize how much X is doing for you until you lose it...
Created attachment 252842 [details] [review] wayland: constraint the pointer onto visible monitors when running on evdev
Review of attachment 252842 [details] [review]: Hm; I would do this differently, as the CRTC constraining code in X is fairly bad. If I have CRTCs like: ,----. | A | `--------. | | B | | | | | '- - - - - - -' and I move the cursor from A to B, then it will go through, since it hopped across the gap. It would be better if we could compute the union shape of the CRTCs: ,----. | | `--------. | | | | | '- - - - - - -' and set up those edges as hard barriers which the pointer can't pass through by any means. You don't have to use any existing barrier code to do this (though we do need to write a Wayland barrier at some point); as long as you ensure that you can't hop that gap even if you landed back in-bounds, we're fine. You can make the set of contour edges with a hack on top of pixman regions Alex Larsson suggested: pixman regions give you rectangles in "bands" of constant height wtih maximimally wide rects inside the bands, from left to right. The vertical edges of these rectangles give you the vertical edges of the contour (since they're maximally wide), but the horizontal edges are a bit more complex. When you step through a band, you need to step through the band above in parallel (if adjacent), and compute the horizontal edges where the rectangles don't overlap, with a simple cap at the bottom for the last region. I can write this code if you don't want to :)
If you feel like writing it, go ahead! To me, it seems a lot of complex code, and expensive region manipulation, just for the (hopefully) uncommon case of doing motion compression across a corner, but I understand it can make the future barrier implementation simpler.
(In reply to comment #3) > If you feel like writing it, go ahead! > > To me, it seems a lot of complex code, and expensive region manipulation, just > for the (hopefully) uncommon case of doing motion compression across a corner, It's very common on my multimonitor setup right now.
Giovannis patch is more or less what we do in weston too. Jasper, if I understand your suggestion correctly, you want to avoid the case where if you move diagonally across from point A to B we allow that cursor motion. To do that you'd have to intersect the line from A to B and see if it intersects with the boundary of the output union. But why is that interesting? And what would you clamp the new coordinate to? And of course I could get really annoying and point out that a line segment is a poor approximation of the actual pointer path (ie, what the user traces on the touchpad or with the mouse). You could take more points into account and get a better (higher order) approximation of the path and potentially allow or reject more "accurately". But seriously, I don't see any value in reject this in the first place. Certainly, with all the other things on the plate for mutter-wayland, it doesn't seem like a good case to obsess over right now. Giovannis patch looks good to commit to me.
I forgot to mention, this depends on clutter bug 706652.
Created attachment 254245 [details] [review] wayland: add support for pointer barriers Use the clutter pointer constrain callback and a lot of copypasted code from Xorg to implement reactive pointer barriers and pointer barrier events. Works with the message tray, but not with the hot corner, for some reason.
Created attachment 254446 [details] [review] wayland: add support for pointer barriers Use the clutter pointer constrain callback and a lot of copypasted code from Xorg to implement reactive pointer barriers and pointer barrier events. Found the problem, it was just a typo.
Comment on attachment 252842 [details] [review] wayland: constraint the pointer onto visible monitors when running on evdev Pushing the first half after Kristian's review. Attachment 252842 [details] pushed as a6dc454 - wayland: constraint the pointer onto visible monitors when running on evdev
Note: if we're not confident enough to introduce pointer barriers this late in the cycle, we can patch meta_display_supports_extended_barriers() to return false, and rely on the fallback code in gnome-shell.
(In reply to comment #10) > Note: if we're not confident enough to introduce pointer barriers this late in > the cycle, we can patch meta_display_supports_extended_barriers() to return > false, and rely on the fallback code in gnome-shell. Yeah, let's do that for now.
Ok, I went ahead and pushed the workaround to the wayland branch. Moving this off the 3.10 list.
We've done this for now. Pointer barriers are hella difficult and I'll write support for them later.
(In reply to comment #13) > We've done this for now. Pointer barriers are hella difficult and I'll write > support for them later. Keep in mind there is working code here. Not pretty code, but working (and not worse than the code in Xorg)
I wrote the code in Xorg. I know how it works and how painful it is to get right.
Created attachment 294481 [details] [review] barrier: Make X related hook more clearly named
Created attachment 294482 [details] [review] barrier: Move out X11 specific code to a separate file
Created attachment 294483 [details] [review] wayland: Implement support for pointer barriers Pointer barriers in Wayland is purely a server side feature and requires no protocol or client interaction. This patch implements the already used MetaBarrier GObject API in mutter when running as a Wayland display server. Only the native backend is supported so far.
Not sure what your plan is here Jasper, but I wrote some patches implementing the MetaBarrier GObject API that seems to work for the gnome-shell use cases when running on Wayland. It doesn't use the Xorg implementation, but is partly based on some other code I had laying around that I had written for weston.
Review of attachment 294483 [details] [review]: After discussing with Jasper, moving the implementation to backend/native as it is not really Wayland related.
Created attachment 294575 [details] [review] backends/native: Implement support for pointer barriers When running as a display server (i.e. as a Wayland compositor), pointer barriers are purely a server side feature and requires no protocol or client interaction. This patch implements the already used MetaBarrier GObject API in mutter when running using the native backend.
Created attachment 294576 [details] [review] backends/native: Use the new 'priv' pointer in MetaBackendNative Instead of using meta_backend_native_get_instance_private () just use the priv pointer we now have.
Created attachment 294653 [details] [review] barriers: Separate implementation from public API This patch removes the X11 specific code from MetaBarrier and creates an abstraction layer MetaBarrierImpl. The existing X11 implementation is moved to a new GObject MetaBarrierImplX11 implementing the abstract interface MetaBarrierImpl which is instantiated by MetaBarrier when supported. While at it, move it to backends/ and properly name the files.
Created attachment 294654 [details] [review] backends/native: Implement support for pointer barriers When running as a dispay server pointer barriers are a server side feature and requires no client interaction of any sort. This patch implements pointer barriers that can be used when running as a display server on the native backend. Running as a display server using the X11 backend is currently not supported.
Review of attachment 294482 [details] [review]: Previous API-Impl patch replaced by a slightly bigger refactoring patch.
Review of attachment 294576 [details] [review]: Went back to using ..._get_instance_private everywhere in the second refactoring, so marking this one as rejected as well.
*** Bug 743057 has been marked as a duplicate of this bug. ***
Attachment 294481 [details] pushed as 152b2da - barrier: Make X related hook more clearly named Attachment 294653 [details] pushed as 5f91a62 - barriers: Separate implementation from public API
Review of attachment 294654 [details] [review]: ::: src/backends/meta-barrier.c @@ +175,3 @@ #if defined(HAVE_XI23) if (META_IS_BACKEND_X11 (meta_get_backend ()) && !meta_is_wayland_compositor ()) You should probably remove this now. ::: src/backends/native/meta-barrier-native.c @@ +53,3 @@ + META_BARRIER_STATE_HELD, + META_BARRIER_STATE_RELEASE, + META_BARRIER_STATE_RELEASED, Explanations of what states mean what would be appreciated, perhaps talking about the state transitions, would be appreciated. Perhaps rename the "released" state to "left", as otherwise it can easily be confused with "release". @@ +71,3 @@ + +static guint +next_serial () next_serial (void) @@ +73,3 @@ +next_serial () +{ + return ++barrier_serial == 0 ? ++barrier_serial : barrier_serial; This is clever enough to need a comment. Or do it in multiple lines: ++barrier_serial; /* Make sure 0 is never a valid serial */ if (barrier_serial == 0) ++barrier_serial; return barrier_serial; @@ +126,3 @@ + Vector2 r = vector2_subtract(line1->b, line1->a); + Vector2 q = line2->a; + Vector2 s = vector2_subtract(line2->b, line2->a); Missing spaces before parens in this method. @@ +188,3 @@ +{ + MetaBarrierImplNativePrivate *priv = + meta_barrier_impl_native_get_instance_private (self); You don't need to wrap to multiple lines -- I'd prefer it on one line. @@ +217,3 @@ + if (is_barrier_blocking_directions (barrier, + META_BARRIER_DIRECTION_POSITIVE_Y)) + hit_box.a.y -= 2.0f; A comment about what the "hit box" is, the meaning of the value 2, and pointing to the X server implementation ( http://cgit.freedesktop.org/xorg/xserver/tree/Xi/xibarriers.c#n262 ) would be appreciated. @@ +432,3 @@ + break; + case META_BARRIER_STATE_ACTIVE: + abort (); /* Invalid state. */ g_assert_not_reached (); ::: src/core/display.c @@ +2923,3 @@ + return ((META_DISPLAY_HAS_XINPUT_23 (display) && + !meta_is_wayland_compositor()) || + META_IS_BACKEND_NATIVE (meta_get_backend ())); I'd do: if (META_IS_BACKEND_NATIVE (meta_get_backend ()) return TRUE; if (META_IS_BACKEND_X11 (meta_get_backend ()) return META_DISPLAY_HAS_XINPUT_23 (display);
Review of attachment 294654 [details] [review]: ::: src/backends/meta-barrier.c @@ +175,3 @@ #if defined(HAVE_XI23) if (META_IS_BACKEND_X11 (meta_get_backend ()) && !meta_is_wayland_compositor ()) Do we really want to use barriers when running in nested mode? This patch does nothing to improve barrier support on any !native backend.
(In reply to comment #30) > Do we really want to use barriers when running in nested mode? This patch does > nothing to improve barrier support on any !native backend. I suppose not.
Created attachment 294840 [details] [review] backends/native: Implement support for pointer barriers When running as a dispay server pointer barriers are a server side feature and requires no client interaction of any sort. This patch implements pointer barriers that can be used when running as a display server on the native backend. Running as a display server using the X11 backend is currently not supported. --- Note that this new version of this patch has more changes in it that what was needed to be addressed from the code review (i.e. I found some bugs).
Attachment 294840 [details] pushed as db384a6 - backends/native: Implement support for pointer barriers Thanks! Pushed with some minor fixes and changes.