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

[Peter Hutterer]

On Tue, Jan 05, 2016 at 09:21:27AM +0800, Jonas Ådahl wrote:
> On Mon, Jan 04, 2016 at 02:21:37PM +1000, Peter Hutterer wrote:
> > 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.
> 
> Then again... how would a single "lock" request deal with multiple
> tablet pointers? Seems we would need a request per device, i.e.
> lock_pointer(wl_pointer) and a lock_tablet_pointer(wp_tablet). So maybe
> its best to make lock/confine take a wl_pointer to begin with.

I was about to make an argument against this and then I realised that the
reasoning here is the same as a few paragraphs above where we both agree
that separate interfaces is better. So now I'm out of arguments, wl_pointer
it is :)

Cheers,
   Peter


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