[PATCH wayland-protocols v2 2/2] Introduce pointer locking and confinement protocol

[Daniel Stone]

Hi,
A couple of (belated) comments ...

On 3 December 2015 at 07:28, Jonas Ådahl <jadahl at gmail.com> wrote:
> +    <request name="lock_pointer">
> +      <description summary="lock pointer to a position">
> +       The lock_pointer request lets the client request to disable movements of
> +       the virtual pointer (i.e. the cursor), effectively locking the pointer
> +       to a position. This request may not take effect immediately; in the
> +       future, when the compositor deems implementation-specific constraints
> +       are satisfied, the pointer lock will be activated and the compositor
> +       sends a locked event.
> +
> +       The protocol provides no guarantee that the constraints are ever
> +       satisfied, and does not require the compositor to send an error if the
> +       constraints cannot ever be satisfied. It is thus possible to request a
> +       lock that will never activate.
> +
> +       There may not be another lock of any kind requested or active on the
> +        surface for the seat when requesting a lock. If there is, an error will
> +        be raised. See general pointer lock documentation for more details.
> +
> +       The intersection of the region passed with this request and the input
> +       region of the surface is used to determine where the pointer must be
> +       in order for the lock to activate. It is up to the compositor whether to
> +       warp the pointer or require some kind of user interaction for the lock
> +       to activate. If the region is null the surface input region is used.
> +
> +       A surface may receive pointer focus without the lock being activated.
> +
> +       The request will create a new object wp_locked_pointer which is used to
> +       interact with the lock as well as receive updates about its state. See
> +       the the description of wp_locked_pointer for further information.
> +
> +       Note that while a pointer is locked, the wl_pointer objects of the
> +       corresponding seat will not emit any wl_pointer.motion events, but
> +       relative motion events will still be emitted via wp_relative_pointer
> +       objects of the same seat. wl_pointer.axis and wl_pointer.button events
> +       are unaffected.
> +      </description>
> +
> +      <arg name="id" type="new_id" interface="zwp_locked_pointer_v1"/>
> +      <arg name="surface" type="object" interface="wl_surface"
> +          summary="surface to lock pointer to"/>
> +      <arg name="seat" type="object" interface="wl_seat"
> +          summary="seat where the pointer should be locked"/>
> +      <arg name="region" type="object" interface="wl_region" allow-null="true"
> +          summary="region of surface"/>
> +    </request>
> +
> +    <request name="confine_pointer">
> +      <description summary="confine pointer to a region">
> +       The confine_pointer request lets the client request to confine the
> +       pointer cursor to a given region. This request may not take effect
> +       immediately; in the future, when the compositor deems implementation-
> +       specific constraints are satisfied, the pointer confinement will be
> +       activated and the compositor sends a confined event.
> +
> +       The intersection of the region passed with this request and the input
> +       region of the surface is used to determine where the pointer must be
> +       in order for the confinement to activate. It is up to the compositor
> +       whether to warp the pointer or require some kind of user interaction for
> +       the confinement to activate. If the region is null the surface input
> +       region is used.
> +
> +       The request will create a new object wp_confined_pointer which is used
> +       to interact with the confinement as well as receive updates about its
> +       state. See the the description of wp_confined_pointer for further
> +       information.
> +      </description>
> +
> +      <arg name="id" type="new_id" interface="zwp_confined_pointer_v1"/>
> +      <arg name="surface" type="object" interface="wl_surface"
> +          summary="surface to lock pointer to"/>
> +      <arg name="seat" type="object" interface="wl_seat"
> +          summary="seat where the pointer should be locked"/>
> +      <arg name="region" type="object" interface="wl_region" allow-null="true"
> +          summary="region of surface"/>
> +    </request>
> +  </interface>

I almost wonder if we couldn't make peoples' lives easier by merging
locking and confinement into a single interface, adding a bool for
whether or not to allow pointer movement (confine) or not (lock). Or
perhaps, rather than a bool, we could add an allow-null
wp_relative_pointer argument instead: gives you lock if non-null, or
confine if null.

Also, for symmetry with wp_relative_pointer, should this take a
wl_pointer instead of wl_seat?

> +  <interface name="zwp_locked_pointer_v1" version="1">
> +    <description summary="receive relative pointer motion events">
> +      The wp_locked_pointer interface represents a locked pointer state.
> +
> +      While the lock of this object is active, the wl_pointer objects of the
> +      associated seat will not emit any wl_pointer.motion events.
> +
> +      This object will send the event 'locked' when the lock is activated.
> +      Whenever the lock is activated, it is guaranteed that the locked surface
> +      will already have received pointer focus and that the pointer will be
> +      within the region passed to the request creating this object.
> +
> +      To unlock the pointer, send the destroy request. This will also destroy
> +      the wp_locked_pointer object.
> +
> +      If the compositor decides to unlock the pointer the unlocked event is
> +      sent. The wp_locked_pointer object is at this point defunct and should be
> +      destroyed.

Egads. :( This is a bit hostile - what's the harm in allowing latent
objects to remain? To my mind, once a previously-locked/confined
wp_{locked,confined}_pointer object is unlocked, it is for all intents
and purposes the same as one which just hasn't yet been activated. It
also introduces an annoying race where if you recreate the object just
after you receive focus (not unlikely for transient loss of focus,
e.g. cancelled Alt-Tab or temporary notifications ... ?), then you'll
receive a different event stream to if you won the race.

There shouldn't be any need the event to prompt destruction anyway, if
you define the conditions under which a client must destroy more
clearly: i.e. when the wp_relative_pointer (for locks) or
wl_pointer/wl_seat (for confinement) is destroyed, or when the
wl_surface is destroyed.

> +      If the surface the lock was requested on is destroyed and the lock is not
> +      yet activated, the wp_locked_pointer object is now defunct and must be
> +      destroyed.

I think you can delete '... and the lock is not yet activated',
assuming that the intention here was just to make sure that people
process outstanding events before they destroy the object. If they've
deleted the surface, presumably any input events are going to be
discarded as being meaningless anyway.

> +    <request name="set_region">
> +      <description summary="set a new lock region">
> +       Set a new region used to lock the pointer.
> +
> +       The new lock region is double-buffered. The new lock region will
> +       only take effect when the associated surface gets its pending state
> +       applied. See wl_surface.commit for details.
> +
> +       The new region has no effect on a lock that has already been activated.
> +
> +       For details about the lock region, see wp_locked_pointer.
> +      </description>
> +
> +      <arg name="region" type="object" interface="wl_region" allow-null="true"
> +          summary="region of surface"/>
> +    </request>

If unlocked/unconfined didn't require destruction and recreation of
the object (as above), the wording of 'new region has no effect' would
become ambiguous here: it could mean 'this state is double-buffered on
lock/confine activation state and the update will be applied the next
time the region is unlocked', or 'any changes requested whilst a lock
is active will be discarded'. The latter reading makes it unreliable,
and the former makes implementing it more of a pain.

> +    <event name="locked">
> +      <description summary="lock activation event">
> +       Notification that the pointer lock of this seat's pointer is activated.
> +      </description>
> +    </event>
> +
> +    <event name="unlocked">
> +      <description summary="lock deactivation event">
> +       Notification that the pointer lock of seat's pointer is no longer
> +       active. This object is now defunct and should be destroyed.
> +      </description>
> +    </event>

I think these (and ditto for confinement) need clarification of their
relationship to wl_pointer::{enter,leave}. Are they still sent? What
about in corner cases such as destroying a currently-active
lock/confinement region, i.e. where focus is retained and ownership
thus passes to wl_pointer?

Other than that, looks pretty good to me! Should've reviewed this earlier ... :\

Cheers,
Daniel