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

[Peter Hutterer]

On Fri, Jan 01, 2016 at 04:54:14PM +0000, Daniel Stone wrote:
> 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.

you get a much less expressive API, less checking, etc. for a fairly minimal
benefit. Furthermore, any extension to *either/or* the lock/confine
interface would need to handle the appropriate NULL cases for the other
interface. Right now we can easily add a locking-specific request without
messing up the logic of the confinement, and vice versa.

I don't think merging the two is a good long-term strategy.

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

you'd be restricting locking and confinement to a pointer device, with a
wl_seat you can confine and lock devices that are not wl_pointer devices,
e.g. a wl_tablet tool in relative mode.

the "pointer" vs "wl_pointer" ambiguity isn't helping here, I admit.

> > +  <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. 

what happens whe the wl_pointer goes away on the wl_seat? does the client
expect the wp_locked_pointer object to hang around and reattach to the new
pointer should it come back on the seat?
see the mess with documenting the capabilities and lifetime of wl_pointer
objects, forcing this object to be dead avoids all that for very little
cost.

Cheers,
   Peter

> 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