[PATCH weston 1/3] Introduce pointer lock interface

[Pekka Paalanen]

On Wed, 17 Sep 2014 21:39:51 +0200
Jonas Ådahl <jadahl at gmail.com> wrote:

> Introduce a pointer lock interface and implementation. The interface
> consists of a global currently called _wl_pointer_lock. It is prefixed
> with an underscore (_) in order to not conflict with a potential
> official protocol in Wayland, and if moving it there, the prefixed
> should be removed.
> 
> The protocol works by exposing the global _wl_pointer_lock to the
> client, and when the client wants to lock the pointer, it creates a new
> wl_pointer object using the 'lock_pointer' request. A more detailed
> description of the protocol seen in pointer-lock.xml file.
> 
> The interface is based on the W3C pointer lock interface [0].
> 
> [0] http://www.w3.org/TR/pointerlock/
> 
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
> 
> The part of the implementation I'm not very happy with is the internal
> API change: the extra argument in the pointer lock motion callback used
> to determine if the motion event was a result of a relative motion or an
> absolute motion. In this implementation absolute events are simply
> ignored as they don't tend to behave in any sensible way. One could
> generate relative motion events from absolute ones as done from a
> touchpad, but one would need some kind of threshold as there are no
> "touch down" or "touch up" events.

Why would you ever want to turn absolute input to relative here?

Is there any use case where that would actually work, and you would not
e.g. hit the physical limits of an absolute input device (mouse-tool on
a drawing tablet?) while playing Quake?

Like discussed in IRC, to me it seems that an "absolute pointer device"
is a concept that is both not a good human interface and
considerably complicates the software design. Why would we not just
claim it doesn't exist here and move on?

Though, the tablet case is a good question, if you want to use a
tool on a tablet as a pointer device for general window interactions.
Do we intend to fake wl_pointer events from a tablet device at all?
Or would it be solved like the touchscreen issue was solved: never fake
to be a wl_pointer but require all apps to explicitly support this type
of input devices for even simple interaction?

I have hard time to imagine someone playing Quake with a tablet, and
here it is just an implementation issue, so I think it can be solved
later, too. After all, the main use case here is with essentially
relative input devices, like mice and touchpads.

> With backends where relative and absolute events are indistinguishable
> (X11, Wayland) is tricker, as there is no way to know whether the event
> is suitable or not. One could also just let the client deal with it
> making "detached" absolute motions just result in very long vectors.

I think for nested compositors, the compositor will just need to ask
for relative events from the parent display server. If that doesn't
work, don't advertise the pointer-lock feature, or end all pointer-locks
immediately.

An X11 backend would use whatever you do to get relative pointer
motion on X11, be that some XI2 feature or continuous warping to the
center of the window.

A Wayland backend would just use pointer-lock itself, to offer
pointer-lock functionality to its clients.


>  Makefile.am               |   7 +-
>  desktop-shell/exposay.c   |   2 +-
>  desktop-shell/shell.c     |  10 +-
>  protocol/pointer-lock.xml |  85 ++++++++++++++
>  src/compositor.c          |   4 +
>  src/compositor.h          |  21 +++-
>  src/input.c               | 277 +++++++++++++++++++++++++++++++++++++++++++++-
>  7 files changed, 394 insertions(+), 12 deletions(-)
>  create mode 100644 protocol/pointer-lock.xml
> 

> diff --git a/protocol/pointer-lock.xml b/protocol/pointer-lock.xml
> new file mode 100644
> index 0000000..e00ff80
> --- /dev/null
> +++ b/protocol/pointer-lock.xml
> @@ -0,0 +1,85 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="pointer_lock">
> +
> +  <copyright>
> +    Copyright © 2013      Intel Corporation
> +    Copyright © 2014      Jonas Ådahl
> +
> +    Permission to use, copy, modify, distribute, and sell this
> +    software and its documentation for any purpose is hereby granted
> +    without fee, provided that the above copyright notice appear in
> +    all copies and that both that copyright notice and this permission
> +    notice appear in supporting documentation, and that the name of
> +    the copyright holders not be used in advertising or publicity
> +    pertaining to distribution of the software without specific,
> +    written prior permission.  The copyright holders make no
> +    representations about the suitability of this software for any
> +    purpose.  It is provided "as is" without express or implied
> +    warranty.
> +
> +    THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> +    SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> +    FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> +    SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> +    WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN
> +    AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> +    ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF
> +    THIS SOFTWARE.
> +  </copyright>
> +
> +  <interface name="_wl_pointer_lock" version="1">
> +    <description summary="lock pointer to a surface">
> +      This interface enables clients to lock pointers to a surface in order
> +      to only receive relative pointer motion events.
> +    </description>
> +
> +    <request name="lock_pointer">
> +      <description summary="return pointer object">
> +        The lock_pointer request lets the client disable pointer
> +        motion and request relative motion events.
> +
> +        Relative motion events have the same dimension as regular
> +        absolute motion events on a non-transformed surface, meaning that
> +        a relative motion event (dx, dy) is equivalent to the transition
> +        from the absolute coordinate (x y) to (x + dx, y + dy).

It's a little strange to talk about transformations and absolute
coordinates in the protocol spec, when such things do not exist from
the client perspective. But I understand we need to say it somehow.

There was also the question if the relative motion should be with or
without effects like pointer acceleration. I seem to recall that the
concensus might have been to include pointer acceleration. At least it
would seem useful, so that relative motion matches the normal cursor
motion. But there are also use cases where such is not wanted I believe
(e.g. FPS games).

> +        If the surface where the pointer is locked is transformed in
> +        any way (for example shown as a preview, rotated, etc), the
> +        motion events of the locked pointer object are sent as if the
> +        surface was not transformed.

Btw. there is a "transformed" that clients do know about, which is not
the thing referred to here: output and buffer transformation.

Could we talk about motion on screen? That would include acceleration
etc. but nothing surface-specific. Ah, but then we have output
scale/transformation to confuse things.

Raw or input device coordinates OTOH sound more like without
acceleration and maybe not even in pixels.

> +        This request initializes the pointer lock and activates it in
> +        case the surface is active. If the surface isn't active when
> +        the server receives the request, the compositor will activate
> +        the pointer_lock when the surface is eventually activated. It's
> +        up to the compositor and its shell to decide when a surface is
> +        active.

Oh right, this is the reason to not include a serial.

> +        The lock_pointer request will create a new wl_pointer object.
> +        When the pointer_lock is activated, the original wl_pointer will
> +        send a leave event and the pointer_lock wl_pointer object
> +        will send an enter event. The enter event will contain the
> +        motion delta (0, 0). The wl_pointer object created with this

Any reason you changed the meaning of enter coordinates here?
The previous draft said they indicate the surface location where the
pointer-lock got activated.

If an app is drawing its own cursor, it would probably want to put it
where the normal cursor left, whether it was on this or some other
client's surface.

> +        request will always sends relative motion deltas, in contrast
> +        with a regular wl_pointer which sends surface local absolute
> +        coordinates.
> +
> +        The compositor can suspend the pointer_lock at any time, for
> +        example when switching to a different application (eg,
> +        alt-tab), if a notification pops up or when the screensaver
> +        starts.  When this happens the pointer lock wl_pointer will
> +        send a leave event. When or if the server activates the
> +        surface again, the client will receive an enter event again.
> +
> +        The client can break the pointer lock at any time by releasing
> +        the pointer lock wl_pointer using the wl_pointer.release request.

This requires wl_pointer version 3.

> +        The interface version of the locked wl_pointer object is the same
> +        as the original wl_pointer object.
> +      </description>
> +      <arg name="id" type="new_id" interface="wl_pointer"/>

As discussed elsewhere, we really should not add multiple
factory-interface trees for a single type (wl_pointer). It is already
constructed by wl_seat.get_pointer, and wl_seat is a global.

If you really want to re-use wl_pointer, then this request must be in
wl_seat. It could be behind a capability bit. Which is exactly how it
was in krh's draft.

> +      <arg name="surface" type="object" interface="wl_surface"/>
> +      <arg name="pointer" type="object" interface="wl_pointer"/>
> +    </request>

In another email it was suggested to specify, that the cursor gets
hidden and the pointer's absolute position will freeze. I think that's
good.

> +  </interface>
> +</protocol>

I recall some discussion from the earlier days, that a client should be
sending a "cursor is here now" requests to the compositor while the
pointer-lock is active, so that when the pointer-lock gets broken, the
real cursor could appear where the pointer-locked app-drawn cursor
left. This would be for cases where the app is actually using a cursor
that it draws itself, so it can implement e.g. confined motion. But I
don't recall the conclusion.


> diff --git a/src/compositor.h b/src/compositor.h
> index 9611dea..e33e341 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -242,7 +242,8 @@ struct weston_pointer_grab;
>  struct weston_pointer_grab_interface {
>  	void (*focus)(struct weston_pointer_grab *grab);
>  	void (*motion)(struct weston_pointer_grab *grab, uint32_t time,
> -		       wl_fixed_t x, wl_fixed_t y);
> +		       wl_fixed_t x, wl_fixed_t y,
> +		       int is_relative);

When reading this, I expected you'd pass relative values as x,y when
is_relative=true, but that's not the case below. You always pass
absolute coordinates.

Do we actually have a case, where the same handlers can handle both
relative and absolute motion?

Maybe just do as we did with the notify_*() functions, add a new entry
point for absolute... err, relative coords? That way we cannot mix them
up accidentally.

But it would still be kind of strange, because the grab handler funcs
should be choosing whether they use the relative or absolute
coordinates, and the relative coordinates should be "unmangled" i.e.
not added to an absolute position... yet, every grab handler should be
using absolute coords except the special case of pointer-lock.

How about just saving the relative values in struct weston_pointer, and
the pointer-lock handler just uses those instead of the x,y arguments?

>  	void (*button)(struct weston_pointer_grab *grab,
>  		       uint32_t time, uint32_t button, uint32_t state);
>  	void (*cancel)(struct weston_pointer_grab *grab);
> @@ -321,6 +322,7 @@ struct weston_pointer {
>  	struct wl_listener focus_resource_listener;
>  	struct wl_signal focus_signal;
>  	struct wl_signal motion_signal;
> +	struct wl_signal destroy_signal;
>  
>  	struct weston_view *sprite;
>  	struct wl_listener sprite_destroy_listener;
> @@ -663,6 +665,8 @@ struct weston_compositor {
>  
>  	int32_t kb_repeat_rate;
>  	int32_t kb_repeat_delay;
> +
> +	struct wl_global *pointer_lock;
>  };
>  
>  struct weston_buffer {
> @@ -899,6 +903,15 @@ struct weston_surface {
>  	 */
>  	struct wl_list subsurface_list; /* weston_subsurface::parent_link */
>  	struct wl_list subsurface_list_pending; /* ...::parent_link_pending */
> +
> +	struct {
> +		struct wl_resource *pointer_resource;
> +		struct weston_pointer_grab grab;
> +		struct weston_pointer *pointer;
> +		struct wl_listener pointer_destroy_listener;
> +		struct wl_listener keyboard_focus_listener;
> +		struct wl_listener surface_destroy_listener;
> +	} pointer_lock;
>  };
>  
>  struct weston_subsurface {
> @@ -1020,6 +1033,12 @@ void
>  notify_touch_frame(struct weston_seat *seat);
>  
>  void
> +init_pointer_lock(struct weston_compositor *compositor);
> +
> +void
> +destroy_pointer_lock(struct weston_compositor *compositor);
> +
> +void
>  weston_layer_entry_insert(struct weston_layer_entry *list,
>  			  struct weston_layer_entry *entry);
>  void


Thanks,
pq