[PATCH weston v3 3/3] Introduce wl_relative_pointer interface

Jonas Ådahl jadahl at gmail.com
Wed Oct 14 20:56:38 PDT 2015


On Thu, Oct 08, 2015 at 12:15:10PM -0500, Derek Foreman wrote:
> On 07/10/15 07:41 PM, Jonas Ådahl wrote:
> > On Wed, Oct 07, 2015 at 01:32:35PM -0500, Derek Foreman wrote:
> >> On 29/07/15 01:39 AM, Jonas Ådahl wrote:
> >>> A wl_relative_pointer object is an extension to the wl_pointer interface
> >>> only used for emitting relative pointer events. It will only emit events
> >>> when the parent pointer has focus.
> >>>
> >>> To get a relative pointer object, use the get_relative_pointer request
> >>> of the global wl_relative_pointer_manager object. When stabilizing it
> >>> might make more sense to just add it to wl_seat instead of having a
> >>> single use global interface.
> >>>
> >>> All interface names are currently prefixed with underscore in order to
> >>> avoid any future conflicts with stable protocol.
> >>>
> >>> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> >>
> >> Some comments below, and this series doesn't merge anymore - do you have
> >> time to rebase it?
> > 
> > Yes, but I've been avoiding reposting until some amount of review
> > progress has been made so there won't be too many versions on the list.
> 
> Seems reasonable.
> 
> >>
> >> As far as I'm concerned, the first 2 patches in the series are good to
> >> land (once they compile again...)
> >>
> >> This one's
> >> Acked-by: Derek Foreman <derekf at osg.samsung.com>
> >>
> >> Would really like to see Daniel's review, time permitting.
> >>
> >> I think it would be nice to land this in the same release cycle (ie:
> >> this one) as pointer confinement, because I think the two features
> >> really go hand in hand.
> > 
> > Indeed. Relative pointer events are quite useless if you loose focus
> > almost immediately
> >
> >>
> >>> ---
> >>>
> >>> Changes since v2:
> >>>
> >>> Time stamps are now 64 bit and in microseconds. Since Wayland can't
> >>> represent 64 bit unsigned integers, it is split into two 32 bit unsigned
> >>> integers, one for each half.
> >>>
> >>> The other change is that there is no more manual resource moving around
> >>> as focus resources are now tracked with weston_pointer_client.
> >>>
> >>>
> >>>  Makefile.am                   |   7 +-
> >>>  protocol/relative-pointer.xml | 129 +++++++++++++++++++++++++++
> >>>  src/compositor.c              |   3 +
> >>>  src/compositor.h              |   4 +
> >>>  src/input.c                   | 197 +++++++++++++++++++++++++++++++++++++-----
> >>>  5 files changed, 317 insertions(+), 23 deletions(-)
> >>>  create mode 100644 protocol/relative-pointer.xml
> >>>
> >>> diff --git a/Makefile.am b/Makefile.am
> >>> index 52c736c..af17e7e 100644
> >>> --- a/Makefile.am
> >>> +++ b/Makefile.am
> >>> @@ -109,7 +109,9 @@ nodist_weston_SOURCES =					\
> >>>  	protocol/presentation_timing-protocol.c		\
> >>>  	protocol/presentation_timing-server-protocol.h	\
> >>>  	protocol/scaler-protocol.c			\
> >>> -	protocol/scaler-server-protocol.h
> >>> +	protocol/scaler-server-protocol.h		\
> >>> +	protocol/relative-pointer-protocol.c		\
> >>> +	protocol/relative-pointer-server-protocol.h
> >>>  
> >>>  BUILT_SOURCES += $(nodist_weston_SOURCES)
> >>>  
> >>> @@ -1316,7 +1318,8 @@ EXTRA_DIST +=					\
> >>>  	protocol/presentation_timing.xml	\
> >>>  	protocol/scaler.xml			\
> >>>  	protocol/ivi-application.xml		\
> >>> -	protocol/ivi-hmi-controller.xml
> >>> +	protocol/ivi-hmi-controller.xml		\
> >>> +	protocol/relative-pointer.xml
> >>>  
> >>>  #
> >>>  # manual test modules in tests subdirectory
> >>> diff --git a/protocol/relative-pointer.xml b/protocol/relative-pointer.xml
> >>> new file mode 100644
> >>> index 0000000..dd993e4
> >>> --- /dev/null
> >>> +++ b/protocol/relative-pointer.xml
> >>> @@ -0,0 +1,129 @@
> >>> +<?xml version="1.0" encoding="UTF-8"?>
> >>> +<protocol name="relative_pointer">
> >>> +
> >>> +  <copyright>
> >>> +    Copyright © 2014      Jonas Ådahl
> >>> +    Copyright © 2015      Red Hat Inc.
> >>> +
> >>> +    Permission is hereby granted, free of charge, to any person obtaining a
> >>> +    copy of this software and associated documentation files (the "Software"),
> >>> +    to deal in the Software without restriction, including without limitation
> >>> +    the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >>> +    and/or sell copies of the Software, and to permit persons to whom the
> >>> +    Software is furnished to do so, subject to the following conditions:
> >>> +
> >>> +    The above copyright notice and this permission notice (including the next
> >>> +    paragraph) shall be included in all copies or substantial portions of the
> >>> +    Software.
> >>> +
> >>> +    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >>> +    IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >>> +    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> >>> +    THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >>> +    LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> >>> +    FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> >>> +    DEALINGS IN THE SOFTWARE.
> >>> +  </copyright>
> >>> +
> >>> +  <interface name="_wl_relative_pointer_manager" version="1">
> >>
> >> Should there be a comment somewhere in here that this should probably be
> >> part of wl_seat?
> > 
> > Should it? It can of course; initially I considered it to be the one
> > true path, but there is no reason why we it'd be better (except being lazy
> > binding globals).
> 
> Ah, you stated in the commit log that this was the ultimate intention,
> so I didn't want that information lost along the way.  I didn't realize
> this changed.

That was the original idea, but I've come to change my mind, I think. I
think that for these types of additions it makes sense to make them as
extensions so we can leave them out of wayland.xml.

> 
> >>
> >>> +    <description summary="get relative pointer objects">
> >>> +      A global interface used for getting the relative pointer object for a
> >>> +      given seat.
> >>> +
> >>> +      Warning! The protocol described in this file is experimental. Each version
> >>> +      of this protocol should be considered incompatible with any other version,
> >>> +      and a client binding to a version different to the one advertised will be
> >>> +      terminated. Once the protocol is declared stable, backward compatibility
> >>> +      is guaranteed, the '_' prefix will be removed from the name and the
> >>> +      version will be reset to 1.
> >>> +    </description>
> >>> +
> >>> +    <request name="get_relative_pointer">
> >>> +      <description summary="get a relative pointer object">
> >>> +        Create a relative pointer interface given a wl_pointer object. See
> >>> +        the wl_relative_pointer interface for more details.
> >>> +      </description>
> >>> +
> >>> +      <arg name="id" type="new_id" interface="_wl_relative_pointer"/>
> >>> +      <arg name="pointer" type="object" interface="wl_pointer"/>
> >>
> >> Really this is the only reason I acked instead of r-b, and it's probably
> >> just due to my own misunderstandings, but:
> >>
> >> Why are we passing a pointer object here instead of the seat?  If a game
> >> wants relative pointer data it'll likely not want absolute pointer data,
> >> so why does it have to bind an absolute pointer (and receive events on
> >> it?) before it can get the relative stuff it's actually going to use?
> > 
> > Initially this request took a seat, since then it has been more
> > considered an extension to the pointer object (sharing its focus, etc)
> > than its own seat device. It was brought up during an early review (on
> > phabricator, so you might not have seen it) that it might be a good idea
> > to make the extension-ness more obvious.
> 
> I can't find any of this in phabricator - can you give me a url?  Maybe
> I'm just doing a poor search...
> 
> > An besides that, the client will probably want to know the pointer
> > focus, it might want to set the cursor and it will probably like to know
> > about clicks etc.
> 
> But if it's a 1000hz gaming mouse the compositor will generate 1000
> events per second for the client to discard?
> 
> Why not just allow the pointer to be bound as either/both serial and
> absolute, and have each be a fully functional interface?
> 
> Perhaps I should read what's in phabricator before I continue to
> comment, though.

Hmm. I can't find it any more. It was in the fdo phabricator task
<https://phabricator.freedesktop.org/T1> but all those comments are no
longer there. I have no idea why.

> 
> >>
> >> Can the "parent" wl_pointer be safely released once the relative one is
> >> acquired?
> > 
> > Makes sense.
> 
> (but only if you don't need clicks and focus...)
> 
> >>
> >>> +    </request>
> >>> +  </interface>
> >>> +
> >>> +  <interface name="_wl_relative_pointer" version="1">
> >>> +    <description summary="relative pointer object">
> >>> +      A wl_relative_pointer object is an extension to the wl_pointer interface
> >>> +      used for emitting relative pointer events. It shares the same focus as
> >>> +      wl_pointer objects of the same seat and will only emit events when it
> >>> +      has focus.
> >>> +    </description>
> >>> +
> >>> +    <request name="release" type="destructor">
> >>> +      <description summary="release the relative pointer object"/>
> >>> +    </request>
> >>> +
> >>> +    <event name="relative_motion">
> >>> +      <description summary="relative pointer motion">
> >>> +        Relative x/y pointer motion in from the pointer of the seat associated
> >>> +        with this object.
> >>> +
> >>> +        A relative motion is in the same dimension as regular wl_pointer motion
> >>> +        events, except they do not represent an absolute position. For example,
> >>> +        moving a pointer from (x, y) to (x', y') would have the equivalent
> >>> +        relative motion (x' - x, y' - y). If a pointer motion caused the
> >>> +        absolute pointer position to be clipped by for example the edge of the
> >>> +        monitor, the relative motion is unaffected by the clipping and will
> >>> +        represent the unclipped motion.
> >>> +
> >>> +        This event also contains non-accelerated motion deltas. The
> >>> +        non-accelerated delta is, when applicable, the regular pointer motion
> >>> +        delta as it was before having applied motion acceleration
> >>> +        transformations. The compositor will have applied the same processing
> >>> +        (such as normalization) meaning the events will have roughly the same
> >>> +        magnitude as accelerated motion events.
> >>> +
> >>> +        Note that the non-accelerated delta does not represent 'raw' events as
> >>> +        they were read from some device. Pointer motion acceleration is device-
> >>> +        and configuration-specific and non-accelerated deltas and accelerated
> >>> +        deltas may have the same value on some devices.
> >>> +
> >>> +        Relative motions are not coupled to wl_pointer.motion events, and can
> >>> +        be sent in combination with such events, but also independently. There
> >>> +        may also be scenarious where wl_pointer.motion is sent, but there is no
> >>> +        relative motion. The order of an absolute and relative motion event
> >>> +        originating from the same physical motion is not guaranteed.
> >>> +
> >>> +        The motion vectors are encoded as double fixed point values.
> >>> +
> >>> +        A double fixed point value is a 64 bit data type encoded as two separate
> >>> +        signed 32 bit integers. The integral part of the value is stored in one
> >>> +        of the integers and the fractional part in the other.
> >>> +
> >>> +        If the client needs button events, it can receive them from a wl_pointer
> >>> +        object of the same seat that the wl_relative_pointer object is
> >>> +        associated with.
> >>> +      </description>
> >>> +
> >>> +      <arg name="utime_most" type="uint"
> >>> +           summary="32 most significant bits of a 64 bit timestamp with microsecond granularity"/>
> >>> +      <arg name="utime_least" type="uint"
> >>
> >> The presentation timing extension also splits a 64-bit value up like
> >> this, but there it's "tv_sec_hi" and "tv_sec_lo"
> >>
> >> Personally (and deep inside I realize this is pedantic bike shed
> >> painting...) I'd like to see consistency in how we name split 64 bit
> >> types...
> > 
> > True. My naming comes from "most/least significant bit", and I guess
> > "high/low order bit" is just as correct. Any preference?
> 
> I guess hi/lo is less to type, and already in use.  (I don't
> particularly LIKE it, mind you.)

Good points.

> >>> +	wl_double_fixed_from_double(dx, &dx_int, &dx_frac);
> >>> +	wl_double_fixed_from_double(dy, &dy_int, &dy_frac);
> >>
> >> Do you have a patch somewhere for wl_double_fixed_from_double?
> > 
> > http://patchwork.freedesktop.org/patch/49211/
> 
> Ah, yes, hung up indefinitely in the pointer confinement review. :/
> 
> It would be nice to unstick some of the preamble bits that require less
> fisticuffs than the protocol bits...

I wonder if we should put "wl_double_fixed" in wayland/ and declare that
an "official mutli part type" thing so we don't have to reimplement the
awkward from/to functions all over the place. Maybe even
a wl_double_fixed_t type as was suggested at an earlier point?


Jonas


More information about the wayland-devel mailing list