[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