[PATCH libinput 1/2] Replace output screen size callback with transform helpers
Jonas Ådahl
jadahl at gmail.com
Wed Jan 29 23:38:02 PST 2014
On Thu, Jan 30, 2014 at 01:02:15PM +1000, Peter Hutterer wrote:
> On Wed, Jan 29, 2014 at 09:33:11PM +0100, Jonas Ådahl wrote:
> > Instead of automatically transforming absolute coordinates of touch and
> > pointer events to screen coordinates, the user now uses the corresponding
> > transform helper function. This means the coordinates returned by
> > libinput_event_pointer_get_absolute_x(),
> > libinput_event_pointer_get_absolute_y(), libinput_touch_get_x() and
> > libinput_touch_get_y() has changed from being in output screen coordinate
> > space to being in device specific coordinate space.
> >
> > For example, where one before would call libinput_event_touch_get_x(event),
> > one now calls libinput_event_touch_get_x_transformed(event, output_width).
> >
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> > src/evdev.c | 54 ++++++++++--------------
> > src/evdev.h | 10 +++++
> > src/libinput.c | 44 ++++++++++++++++++++
> > src/libinput.h | 128 +++++++++++++++++++++++++++++++++++++++++++++++++--------
> > test/litest.c | 11 -----
> > 5 files changed, 186 insertions(+), 61 deletions(-)
> >
> > diff --git a/src/evdev.c b/src/evdev.c
> > index 46bd35a..cb83a1f 100644
> > --- a/src/evdev.c
> > +++ b/src/evdev.c
> > @@ -86,6 +86,24 @@ transform_absolute(struct evdev_device *device, int32_t *x, int32_t *y)
> > }
> > }
> >
> > +li_fixed_t
> > +evdev_device_transform_x(struct evdev_device *device,
> > + li_fixed_t x,
> > + uint32_t width)
> > +{
> > + return (x - device->abs.min_x) * width /
> > + (device->abs.max_x - device->abs.min_x);
> > +}
> > +
> > +li_fixed_t
> > +evdev_device_transform_y(struct evdev_device *device,
> > + li_fixed_t y,
> > + uint32_t height)
> > +{
> > + return (y - device->abs.min_y) * height /
> > + (device->abs.max_y - device->abs.min_y);
>
> you're mixing coordinate systems here, x and y are in fixed_t but
> abs.min/max is in normal integers. that breaks if you have a non-zero min.
> You'll need to convert the rest to li_fixed_t too if you want to keep the
> integer division.
Yea, missed the wl_fixed_from_int here (and in _x), and they were all 0
so didn't notice it either. For multiplication, one of the factors cannot be
li_fixed_t though. Same goes for division where the denominator needs to
be a normal int even if the numerator is li_fixed_t.
>
> also, should we add a non-zero min for width and height to scale to a screen
> not the top/left-most? The compositor can just add it afterwards, but
> it would have to convert to fixed_t as well:
>
> x = libinput_event_touch_get_x_transformed(event, screen_width);
> x += li_fixed_from_int(screen_offset);
>
> which is more error prone than something like:
>
> x = libinput_event_touch_get_x_transformed(event, screen_offset_x, screen_width);
>
That transform wouldn't be enough. We'd have to rotate etc as well. See
http://cgit.freedesktop.org/wayland/weston/tree/src/compositor.c#n3408 .
Given that, I think its easiest to let libinput do the device specific
transform (device coords -> output coords) and then have the user
translate, and do other transformations.
>
> also, is it likely that the caller always has the screen dimensions handy
> when it comes to processing events? or would an config-style approach work
> better:
Where I implemented this (weston), this is indeed the case. See
https://github.com/jadahl/weston/commit/9230416e82929dd2a545b176934e38538ea19e11
>
> libinput_device_set_output_dimensions(device, xoff, yoff, width, height);
> ...
> x = libinput_event_touch_get_x_transformed(event);
> y = libinput_event_touch_get_y_transformed(event);
I don't think we gain anything by duplicating the state in libinput, as
the user most likely will always have it. Instead we'd have to add extra
hooks in the user's code to update the output dimensions, that would be
wrong anyway if an output gets disconnected, and then we'd need to have
a unset_output_dimensions() or set to 0, 0, 0, 0, which is a bit
awkward.
>
> I also suspect that the device-specific dimensions will be rather useless if
> we don't have a call to get the min/max from each device. which I should be
> focusing on real soon :)
Yea, I didn't add them because I wouldn't use them anywhere yet. I did
consider just removing _get_x/y() but didn't.
>
> Cheers,
> Peter
>
> > +}
> > +
> > static void
> > evdev_flush_pending_event(struct evdev_device *device, uint32_t time)
> > {
> > @@ -242,16 +260,6 @@ evdev_process_touch(struct evdev_device *device,
> > struct input_event *e,
> > uint32_t time)
> > {
> > - struct libinput *libinput = device->base.seat->libinput;
> > - int screen_width;
> > - int screen_height;
> > -
> > - libinput->interface->get_current_screen_dimensions(
> > - &device->base,
> > - &screen_width,
> > - &screen_height,
> > - libinput->user_data);
> > -
> > switch (e->code) {
> > case ABS_MT_SLOT:
> > evdev_flush_pending_event(device, time);
> > @@ -267,16 +275,12 @@ evdev_process_touch(struct evdev_device *device,
> > device->pending_event = EVDEV_ABSOLUTE_MT_UP;
> > break;
> > case ABS_MT_POSITION_X:
> > - device->mt.slots[device->mt.slot].x =
> > - (e->value - device->abs.min_x) * screen_width /
> > - (device->abs.max_x - device->abs.min_x);
> > + device->mt.slots[device->mt.slot].x = e->value;
> > if (device->pending_event == EVDEV_NONE)
> > device->pending_event = EVDEV_ABSOLUTE_MT_MOTION;
> > break;
> > case ABS_MT_POSITION_Y:
> > - device->mt.slots[device->mt.slot].y =
> > - (e->value - device->abs.min_y) * screen_height /
> > - (device->abs.max_y - device->abs.min_y);
> > + device->mt.slots[device->mt.slot].y = e->value;
> > if (device->pending_event == EVDEV_NONE)
> > device->pending_event = EVDEV_ABSOLUTE_MT_MOTION;
> > break;
> > @@ -287,28 +291,14 @@ static inline void
> > evdev_process_absolute_motion(struct evdev_device *device,
> > struct input_event *e)
> > {
> > - struct libinput *libinput = device->base.seat->libinput;
> > - int screen_width;
> > - int screen_height;
> > -
> > - libinput->interface->get_current_screen_dimensions(
> > - &device->base,
> > - &screen_width,
> > - &screen_height,
> > - libinput->user_data);
> > -
> > switch (e->code) {
> > case ABS_X:
> > - device->abs.x =
> > - (e->value - device->abs.min_x) * screen_width /
> > - (device->abs.max_x - device->abs.min_x);
> > + device->abs.x = e->value;
> > if (device->pending_event == EVDEV_NONE)
> > device->pending_event = EVDEV_ABSOLUTE_MOTION;
> > break;
> > case ABS_Y:
> > - device->abs.y =
> > - (e->value - device->abs.min_y) * screen_height /
> > - (device->abs.max_y - device->abs.min_y);
> > + device->abs.y = e->value;
> > if (device->pending_event == EVDEV_NONE)
> > device->pending_event = EVDEV_ABSOLUTE_MOTION;
> > break;
> > diff --git a/src/evdev.h b/src/evdev.h
> > index 58ae552..37c32e5 100644
> > --- a/src/evdev.h
> > +++ b/src/evdev.h
> > @@ -146,6 +146,16 @@ int
> > evdev_device_has_capability(struct evdev_device *device,
> > enum libinput_device_capability capability);
> >
> > +li_fixed_t
> > +evdev_device_transform_x(struct evdev_device *device,
> > + li_fixed_t x,
> > + uint32_t width);
> > +
> > +li_fixed_t
> > +evdev_device_transform_y(struct evdev_device *device,
> > + li_fixed_t y,
> > + uint32_t height);
> > +
> > void
> > evdev_device_remove(struct evdev_device *device);
> >
> > diff --git a/src/libinput.c b/src/libinput.c
> > index a77f165..cfce2c5 100644
> > --- a/src/libinput.c
> > +++ b/src/libinput.c
> > @@ -245,6 +245,28 @@ libinput_event_pointer_get_absolute_y(
> > return event->y;
> > }
> >
> > +LIBINPUT_EXPORT li_fixed_t
> > +libinput_event_pointer_get_absolute_x_transformed(
> > + struct libinput_event_pointer *event,
> > + uint32_t width)
> > +{
> > + struct evdev_device *device =
> > + (struct evdev_device *) event->base.device;
> > +
> > + return evdev_device_transform_x(device, event->x, width);
> > +}
> > +
> > +LIBINPUT_EXPORT li_fixed_t
> > +libinput_event_pointer_get_absolute_y_transformed(
> > + struct libinput_event_pointer *event,
> > + uint32_t height)
> > +{
> > + struct evdev_device *device =
> > + (struct evdev_device *) event->base.device;
> > +
> > + return evdev_device_transform_y(device, event->y, height);
> > +}
> > +
> > LIBINPUT_EXPORT uint32_t
> > libinput_event_pointer_get_button(
> > struct libinput_event_pointer *event)
> > @@ -295,6 +317,28 @@ libinput_event_touch_get_x(
> > }
> >
> > LIBINPUT_EXPORT li_fixed_t
> > +libinput_event_touch_get_x_transformed(
> > + struct libinput_event_touch *event,
> > + uint32_t width)
> > +{
> > + struct evdev_device *device =
> > + (struct evdev_device *) event->base.device;
> > +
> > + return evdev_device_transform_x(device, event->x, width);
> > +}
> > +
> > +LIBINPUT_EXPORT li_fixed_t
> > +libinput_event_touch_get_y_transformed(
> > + struct libinput_event_touch *event,
> > + uint32_t height)
> > +{
> > + struct evdev_device *device =
> > + (struct evdev_device *) event->base.device;
> > +
> > + return evdev_device_transform_y(device, event->y, height);
> > +}
> > +
> > +LIBINPUT_EXPORT li_fixed_t
> > libinput_event_touch_get_y(
> > struct libinput_event_touch *event)
> > {
> > diff --git a/src/libinput.h b/src/libinput.h
> > index ddaeb73..8d347b9 100644
> > --- a/src/libinput.h
> > +++ b/src/libinput.h
> > @@ -395,16 +395,19 @@ libinput_event_pointer_get_dy(
> > /**
> > * @ingroup event_pointer
> > *
> > - * Return the absolute x coordinate of the device, scaled to screen
> > - * coordinates.
> > - * The axes' positive direction is device-specific. For pointer events that
> > - * are not of type LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, this function
> > - * returns 0.
> > + * Return the current absolute x coordinate of the pointer event.
> > + *
> > + * The coordinate is in a device specific coordinate space; to get the
> > + * corresponding output screen coordinate, use
> > + * libinput_event_pointer_get_x_transformed().
> > + *
> > + * For pointer events that are not of type
> > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, this function returns 0.
> > *
> > * @note It is an application bug to call this function for events other than
> > * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE.
> > *
> > - * @return the current absolute x coordinate scaled to screen coordinates.
> > + * @return the current absolute x coordinate
> > */
> > li_fixed_t
> > libinput_event_pointer_get_absolute_x(
> > @@ -413,15 +416,19 @@ libinput_event_pointer_get_absolute_x(
> > /**
> > * @ingroup event_pointer
> > *
> > - * Return the absolute y coordinate of the device, scaled to screen coordinates.
> > - * The axes' positive direction is device-specific. For pointer events that
> > - * are not of type LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, this function
> > - * returns 0.
> > + * Return the current absolute y coordinate of the pointer event.
> > + *
> > + * The coordinate is in a device specific coordinate space; to get the
> > + * corresponding output screen coordinate, use
> > + * libinput_event_pointer_get_y_transformed().
> > + *
> > + * For pointer events that are not of type
> > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, this function returns 0.
> > *
> > * @note It is an application bug to call this function for events other than
> > * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE.
> > *
> > - * @return the current absolute y coordinate scaled to screen coordinates.
> > + * @return the current absolute y coordinate
> > */
> > li_fixed_t
> > libinput_event_pointer_get_absolute_y(
> > @@ -430,6 +437,50 @@ libinput_event_pointer_get_absolute_y(
> > /**
> > * @ingroup event_pointer
> > *
> > + * Return the current absolute x coordinate of the pointer event, transformed to
> > + * screen coordinates.
> > + *
> > + * For pointer events that are not of type
> > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, the return value of this function is
> > + * undefined.
> > + *
> > + * @note It is an application bug to call this function for events other than
> > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE.
> > + *
> > + * @param event The libinput pointer event
> > + * @param width The current output screen width
> > + * @return the current absolute x coordinate transformed to a screen coordinate
> > + */
> > +li_fixed_t
> > +libinput_event_pointer_get_absolute_x_transformed(
> > + struct libinput_event_pointer *event,
> > + uint32_t width);
> > +
> > +/**
> > + * @ingroup event_pointer
> > + *
> > + * Return the current absolute y coordinate of the pointer event, transformed to
> > + * screen coordinates.
> > + *
> > + * For pointer events that are not of type
> > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE, the return value of this function is
> > + * undefined.
> > + *
> > + * @note It is an application bug to call this function for events other than
> > + * LIBINPUT_EVENT_POINTER_MOTION_ABSOLUTE.
> > + *
> > + * @param event The libinput pointer event
> > + * @param height The current output screen height
> > + * @return the current absolute y coordinate transformed to a screen coordinate
> > + */
> > +li_fixed_t
> > +libinput_event_pointer_get_absolute_y_transformed(
> > + struct libinput_event_pointer *event,
> > + uint32_t height);
> > +
> > +/**
> > + * @ingroup event_pointer
> > + *
> > * Return the button that triggered this event.
> > * For pointer events that are not of type LIBINPUT_EVENT_POINTER_BUTTON,
> > * this function returns 0.
> > @@ -531,9 +582,16 @@ libinput_event_touch_get_slot(
> > /**
> > * @ingroup event_touch
> > *
> > + * Return the current absolute x coordinate of the touch event.
> > + *
> > + * The coordinate is in a device specific coordinate space; to get the
> > + * corresponding output screen coordinate, use
> > + * libinput_event_touch_get_x_transformed().
> > + *
> > * @note this function should not be called for LIBINPUT_EVENT_TOUCH_FRAME.
> > *
> > - * @return the absolute X coordinate on this touch device, scaled to screen coordinates.
> > + * @param event The libinput touch event
> > + * @return the current absolute x coordinate
> > */
> > li_fixed_t
> > libinput_event_touch_get_x(
> > @@ -542,9 +600,16 @@ libinput_event_touch_get_x(
> > /**
> > * @ingroup event_touch
> > *
> > + * Return the current absolute y coordinate of the touch event.
> > + *
> > + * The coordinate is in a device specific coordinate space; to get the
> > + * corresponding output screen coordinate, use
> > + * libinput_event_touch_get_y_transformed().
> > + *
> > * @note this function should not be called for LIBINPUT_EVENT_TOUCH_FRAME.
> > *
> > - * @return the absolute X coordinate on this touch device, scaled to screen coordinates.
> > + * @param event The libinput touch event
> > + * @return the current absolute y coordinate
> > */
> > li_fixed_t
> > libinput_event_touch_get_y(
> > @@ -553,6 +618,38 @@ libinput_event_touch_get_y(
> > /**
> > * @ingroup event_touch
> > *
> > + * Return the current absolute x coordinate of the touch event, transformed to
> > + * screen coordinates.
> > + *
> > + * @note this function should not be called for LIBINPUT_EVENT_TOUCH_FRAME.
> > + *
> > + * @param event The libinput touch event
> > + * @param width The current output screen width
> > + * @return the current absolute x coordinate transformed to a screen coordinate
> > + */
> > +li_fixed_t
> > +libinput_event_touch_get_x_transformed(struct libinput_event_touch *event,
> > + uint32_t width);
> > +
> > +/**
> > + * @ingroup event_touch
> > + *
> > + * Return the current absolute y coordinate of the touch event, transformed to
> > + * screen coordinates.
> > + *
> > + * @note this function should not be called for LIBINPUT_EVENT_TOUCH_FRAME.
> > + *
> > + * @param event The libinput touch event
> > + * @param height The current output screen height
> > + * @return the current absolute y coordinate transformed to a screen coordinate
> > + */
> > +li_fixed_t
> > +libinput_event_touch_get_y_transformed(struct libinput_event_touch *event,
> > + uint32_t height);
> > +
> > +/**
> > + * @ingroup event_touch
> > + *
> > * @note this function should not be called for LIBINPUT_EVENT_TOUCH_FRAME.
> > *
> > * @return the type of touch that occured on the device
> > @@ -586,11 +683,6 @@ struct libinput_interface {
> > * libinput_create_from_udev()
> > */
> > void (*close_restricted)(int fd, void *user_data);
> > -
> > - void (*get_current_screen_dimensions)(struct libinput_device *device,
> > - int *width,
> > - int *height,
> > - void *user_data);
> > };
> >
> > /**
> > diff --git a/test/litest.c b/test/litest.c
> > index 5235e3c..216e1a0 100644
> > --- a/test/litest.c
> > +++ b/test/litest.c
> > @@ -318,20 +318,9 @@ close_restricted(int fd, void *userdata)
> > close(fd);
> > }
> >
> > -static void
> > -get_current_screen_dimensions(struct libinput_device *device,
> > - int *width,
> > - int *height,
> > - void *user_data)
> > -{
> > - *width = 1024;
> > - *height = 768;
> > -}
> > -
> > const struct libinput_interface interface = {
> > .open_restricted = open_restricted,
> > .close_restricted = close_restricted,
> > - .get_current_screen_dimensions = get_current_screen_dimensions,
> > };
> >
> > struct litest_device *
> > --
> > 1.8.3.2
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >
More information about the wayland-devel
mailing list