[PATCH weston v3 3/3] Introduce wl_relative_pointer interface
Jonas Ådahl
jadahl at gmail.com
Wed Oct 7 17:41:36 PDT 2015
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.
>
> 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).
>
> > + <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.
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.
>
> Can the "parent" wl_pointer be safely released once the relative one is
> acquired?
Makes sense.
>
> > + </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?
>
> > + summary="32 least significant bits of a 64 bit timestamp with microsecond granularity"/>
> > + <arg name="dx_int" type="int"
> > + summary="integral part of the x component of the motion vector"/>
> > + <arg name="dx_frac" type="int"
> > + summary="fractional part of the x component of the motion vector"/>
> > + <arg name="dy_int" type="int"
> > + summary="integral part of the y component of the motion vector"/>
> > + <arg name="dy_frac" type="int"
> > + summary="fractional part of the y component of the motion vector"/>
> > + <arg name="dx_unaccel_int" type="int"
> > + summary="integral part of the x component of the unaccelerated motion vector"/>
> > + <arg name="dx_unaccel_frac" type="int"
> > + summary="fractional part of the x component of the unaccelerated motion vector"/>
> > + <arg name="dy_unaccel_int" type="int"
> > + summary="integral part of the y component of the unaccelerated motion vector"/>
> > + <arg name="dy_unaccel_frac" type="int"
> > + summary="fractional part of the y component of the unaccelerated motion vector"/>
> > + </event>
> > + </interface>
> > +
> > +</protocol>
> > diff --git a/src/compositor.c b/src/compositor.c
> > index ce1dacd..8ffa976 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -4503,6 +4503,9 @@ weston_compositor_create(struct wl_display *display, void *user_data)
> > ec, bind_presentation))
> > goto fail;
> >
> > + if (weston_input_init(ec) != 0)
> > + goto fail;
> > +
> > wl_list_init(&ec->view_list);
> > wl_list_init(&ec->plane_list);
> > wl_list_init(&ec->layer_list);
> > diff --git a/src/compositor.h b/src/compositor.h
> > index 20c1dd3..faece84 100644
> > --- a/src/compositor.h
> > +++ b/src/compositor.h
> > @@ -334,6 +334,7 @@ struct weston_pointer_client {
> > struct wl_list link;
> > struct wl_client *client;
> > struct wl_list pointer_resources;
> > + struct wl_list relative_pointer_resources;
> > };
> >
> > struct weston_pointer {
> > @@ -1603,6 +1604,9 @@ int
> > noop_renderer_init(struct weston_compositor *ec);
> >
> > int
> > +weston_input_init(struct weston_compositor *compositor);
> > +
> > +int
> > backend_init(struct weston_compositor *c,
> > int *argc, char *argv[],
> > struct weston_config *config);
> > diff --git a/src/input.c b/src/input.c
> > index 3338fdb..c19528d 100644
> > --- a/src/input.c
> > +++ b/src/input.c
> > @@ -36,7 +36,9 @@
> >
> > #include "shared/helpers.h"
> > #include "shared/os-compatibility.h"
> > +#include "shared/util.h"
> > #include "compositor.h"
> > +#include "protocol/relative-pointer-server-protocol.h"
> >
> > static void
> > empty_region(pixman_region32_t *region)
> > @@ -56,6 +58,7 @@ weston_pointer_client_create(struct wl_client *client)
> >
> > pointer_client->client = client;
> > wl_list_init(&pointer_client->pointer_resources);
> > + wl_list_init(&pointer_client->relative_pointer_resources);
> >
> > return pointer_client;
> > }
> > @@ -69,7 +72,9 @@ weston_pointer_client_destroy(struct weston_pointer_client *pointer_client)
> > static bool
> > weston_pointer_client_is_empty(struct weston_pointer_client *pointer_client)
> > {
> > - return wl_list_empty(&pointer_client->pointer_resources);
> > + return
> > + wl_list_empty(&pointer_client->pointer_resources) &&
> > + wl_list_empty(&pointer_client->relative_pointer_resources);
> > }
> >
> > static struct weston_pointer_client *
> > @@ -140,6 +145,49 @@ static void unbind_resource(struct wl_resource *resource)
> > }
> >
> > WL_EXPORT void
> > +weston_pointer_motion_to_abs(struct weston_pointer *pointer,
> > + struct weston_pointer_motion_event *event,
> > + wl_fixed_t *x, wl_fixed_t *y)
> > +{
> > + if (event->mask & WESTON_POINTER_MOTION_ABS) {
> > + *x = wl_fixed_from_double(event->x);
> > + *y = wl_fixed_from_double(event->y);
> > + } else if (event->mask & WESTON_POINTER_MOTION_REL) {
> > + *x = pointer->x + wl_fixed_from_double(event->dx);
> > + *y = pointer->y + wl_fixed_from_double(event->dy);
> > + } else {
> > + assert(!"invalid motion event");
> > + *x = *y = 0;
> > + }
> > +}
> > +
> > +static bool
> > +weston_pointer_motion_to_rel(struct weston_pointer *pointer,
> > + struct weston_pointer_motion_event *event,
> > + double *dx, double *dy,
> > + double *dx_unaccel, double *dy_unaccel)
> > +{
> > + if (event->mask & WESTON_POINTER_MOTION_REL &&
> > + event->mask & WESTON_POINTER_MOTION_REL_NOACCEL) {
> > + *dx = event->dx;
> > + *dy = event->dy;
> > + *dx_unaccel = event->dx_unaccel;
> > + *dy_unaccel = event->dy_unaccel;
> > + return true;
> > + } else if (event->mask & WESTON_POINTER_MOTION_REL) {
> > + *dx_unaccel = *dx = event->dx;
> > + *dy_unaccel = *dy = event->dy;
> > + return true;
> > + } else if (event->mask & WESTON_POINTER_MOTION_REL_NOACCEL) {
> > + *dx_unaccel = *dx = event->dx_unaccel;
> > + *dy_unaccel = *dy = event->dy_unaccel;
> > + return true;
> > + } else {
> > + return false;
> > + }
> > +}
> > +
> > +WL_EXPORT void
> > weston_seat_repick(struct weston_seat *seat)
> > {
> > const struct weston_pointer *pointer = seat->pointer;
> > @@ -255,6 +303,52 @@ default_grab_pointer_focus(struct weston_pointer_grab *grab)
> > }
> >
> > static void
> > +weston_pointer_send_relative_motion(struct weston_pointer *pointer,
> > + uint32_t time,
> > + struct weston_pointer_motion_event *event)
> > +{
> > + uint64_t time_usec;
> > + double dx, dy, dx_unaccel, dy_unaccel;
> > + int32_t dx_int, dx_frac;
> > + int32_t dy_int, dy_frac;
> > + int32_t dx_unaccel_int, dx_unaccel_frac;
> > + int32_t dy_unaccel_int, dy_unaccel_frac;
> > + struct wl_list *resource_list;
> > + struct wl_resource *resource;
> > +
> > + if (!pointer->focus_client)
> > + return;
> > +
> > + if (!weston_pointer_motion_to_rel(pointer, event,
> > + &dx, &dy,
> > + &dx_unaccel, &dy_unaccel))
> > + return;
> > +
> > + resource_list = &pointer->focus_client->relative_pointer_resources;
> > + time_usec = event->time_usec;
> > + if (time_usec == 0)
> > + time_usec = time * 1000ULL;
> > + 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/
>
> > + wl_double_fixed_from_double(dx_unaccel,
> > + &dx_unaccel_int,
> > + &dx_unaccel_frac);
> > + wl_double_fixed_from_double(dy_unaccel,
> > + &dy_unaccel_int,
> > + &dy_unaccel_frac);
> > + wl_resource_for_each(resource, resource_list) {
> > + _wl_relative_pointer_send_relative_motion(
> > + resource,
> > + (uint32_t) (time_usec >> 32),
> > + (uint32_t) time_usec,
> > + dx_int, dx_frac,
> > + dy_int, dy_frac,
> > + dx_unaccel_int, dx_unaccel_frac,
> > + dy_unaccel_int, dy_unaccel_frac);
> > + }
> > +}
> > +
> > +static void
> > default_grab_pointer_motion(struct weston_pointer_grab *grab, uint32_t time,
> > struct weston_pointer_motion_event *event)
> > {
> > @@ -281,6 +375,8 @@ default_grab_pointer_motion(struct weston_pointer_grab *grab, uint32_t time,
> > pointer->sx, pointer->sy);
> > }
> > }
> > +
> > + weston_pointer_send_relative_motion(pointer, time, event);
> > }
> >
> > static void
> > @@ -789,7 +885,6 @@ weston_pointer_set_focus(struct weston_pointer *pointer,
> > pointer->sx != sx || pointer->sy != sy)
> > refocus = 1;
> >
> > -
>
> Unrelated, but no big deal. :)
>
> > if (pointer->focus_client) {
> > focus_resource_list = &pointer->focus_client->pointer_resources;
> > if (!wl_list_empty(focus_resource_list)) {
> > @@ -1050,23 +1145,6 @@ weston_pointer_move_to(struct weston_pointer *pointer,
> > }
> >
> > WL_EXPORT void
> > -weston_pointer_motion_to_abs(struct weston_pointer *pointer,
> > - struct weston_pointer_motion_event *event,
> > - wl_fixed_t *x, wl_fixed_t *y)
> > -{
> > - if (event->mask & WESTON_POINTER_MOTION_ABS) {
> > - *x = wl_fixed_from_double(event->x);
> > - *y = wl_fixed_from_double(event->y);
> > - } else if (event->mask & WESTON_POINTER_MOTION_REL) {
> > - *x = pointer->x + wl_fixed_from_double(event->dx);
> > - *y = pointer->y + wl_fixed_from_double(event->dy);
> > - } else {
> > - assert(!"invalid motion event");
> > - *x = *y = 0;
> > - }
> > -}
> > -
> > -WL_EXPORT void
> > weston_pointer_move(struct weston_pointer *pointer,
> > struct weston_pointer_motion_event *event)
> > {
> > @@ -1932,12 +2010,17 @@ seat_get_pointer(struct wl_client *client, struct wl_resource *resource,
> > wl_client_post_no_memory(client);
> > return;
> > }
> > - wl_resource_set_implementation(cr, &pointer_interface, seat->pointer,
> > - unbind_pointer_client_resource);
> >
> > pointer_client = weston_pointer_ensure_pointer_client(pointer, client);
> > + if (!pointer_client) {
> > + wl_client_post_no_memory(client);
> > + return;
> > + }
> > +
> > wl_list_insert(&pointer_client->pointer_resources,
> > wl_resource_get_link(cr));
> > + wl_resource_set_implementation(cr, &pointer_interface, seat->pointer,
> > + unbind_pointer_client_resource);
>
> Is this an unrelated bug fix?
Seems to have leaked over from
http://patchwork.freedesktop.org/patch/55718/ . Thanks for spotting.
Thanks for taking a look,
Jonas
>
> >
> > if (seat->pointer->focus && seat->pointer->focus->surface->resource &&
> > wl_resource_get_client(seat->pointer->focus->surface->resource) == client) {
> > @@ -2124,6 +2207,67 @@ bind_seat(struct wl_client *client, void *data, uint32_t version, uint32_t id)
> > wl_seat_send_name(resource, seat->seat_name);
> > }
> >
> > +static void
> > +relative_pointer_release(struct wl_client *client,
> > + struct wl_resource *resource)
> > +{
> > + wl_resource_destroy(resource);
> > +}
> > +
> > +static const struct _wl_relative_pointer_interface relative_pointer_interface = {
> > + relative_pointer_release
> > +};
> > +
> > +static void
> > +relative_pointer_manager_get_relative_pointer(struct wl_client *client,
> > + struct wl_resource *resource,
> > + uint32_t id,
> > + struct wl_resource *pointer_resource)
> > +{
> > + struct weston_pointer *pointer =
> > + wl_resource_get_user_data(pointer_resource);
> > + struct weston_pointer_client *pointer_client;
> > + struct wl_resource *cr;
> > +
> > + cr = wl_resource_create(client, &_wl_relative_pointer_interface,
> > + wl_resource_get_version(resource), id);
> > + if (cr == NULL) {
> > + wl_client_post_no_memory(client);
> > + return;
> > + }
> > +
> > + pointer_client = weston_pointer_ensure_pointer_client(pointer, client);
> > + if (!pointer_client) {
> > + wl_client_post_no_memory(client);
> > + return;
> > + }
> > +
> > + wl_list_insert(&pointer_client->relative_pointer_resources,
> > + wl_resource_get_link(cr));
> > + wl_resource_set_implementation(cr, &relative_pointer_interface,
> > + pointer,
> > + unbind_pointer_client_resource);
> > +}
> > +
> > +static const struct _wl_relative_pointer_manager_interface relative_pointer_manager = {
> > + relative_pointer_manager_get_relative_pointer,
> > +};
> > +
> > +static void
> > +bind_relative_pointer_manager(struct wl_client *client, void *data,
> > + uint32_t version, uint32_t id)
> > +{
> > + struct weston_compositor *compositor = data;
> > + struct wl_resource *resource;
> > +
> > + resource = wl_resource_create(client,
> > + &_wl_relative_pointer_manager_interface,
> > + 1, id);
> > + wl_resource_set_implementation(resource, &relative_pointer_manager,
> > + compositor,
> > + NULL);
> > +}
> > +
> > #ifdef ENABLE_XKBCOMMON
> > int
> > weston_compositor_xkb_init(struct weston_compositor *ec,
> > @@ -2545,3 +2689,14 @@ weston_seat_release(struct weston_seat *seat)
> >
> > wl_signal_emit(&seat->destroy_signal, seat);
> > }
> > +
> > +int
> > +weston_input_init(struct weston_compositor *compositor)
> > +{
> > + if (!wl_global_create(compositor->wl_display,
> > + &_wl_relative_pointer_manager_interface, 1,
> > + compositor, bind_relative_pointer_manager))
> > + return -1;
> > +
> > + return 0;
> > +}
> >
>
More information about the wayland-devel
mailing list