[PATCH libinput 4/4] touchpad: change manual calculations of dimensions to helper functions
Peter Hutterer
peter.hutterer at who-t.net
Mon Jul 18 00:36:53 UTC 2016
On Fri, Jul 15, 2016 at 10:58:06AM +0200, Hans de Goede wrote:
> Hi,
>
> Patch 1-3 look good to me and are:
>
> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
>
> This one needs some work, see comments online.
>
> As mentioned before I'm going on vacation, so this will likely
> be me last wayland-devel mail for the coming 2 weeks.
>
> On 15-07-16 09:50, Peter Hutterer wrote:
> > Wherever we use an absolute size in mm on the touchpad, switch to the new
> > helper functions. In a few cases we only need one coordinate so just leave the
> > other one as 0 in those cases.
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > src/evdev-mt-touchpad-buttons.c | 80 +++++++++++++++++++------------------
> > src/evdev-mt-touchpad-edge-scroll.c | 17 ++++----
> > src/evdev-mt-touchpad.c | 33 ++++++++++-----
> > 3 files changed, 72 insertions(+), 58 deletions(-)
> >
> > diff --git a/src/evdev-mt-touchpad-buttons.c b/src/evdev-mt-touchpad-buttons.c
> > index 85a355f..811d121 100644
> > --- a/src/evdev-mt-touchpad-buttons.c
> > +++ b/src/evdev-mt-touchpad-buttons.c
> > @@ -541,33 +541,29 @@ static void
> > tp_init_softbuttons(struct tp_dispatch *tp,
> > struct evdev_device *device)
> > {
> > - int width, height;
> > - const struct input_absinfo *absinfo_x, *absinfo_y;
> > - struct device_coords offset;
> > - int xres, yres;
> > + double width, height;
> > + struct device_coords edges;
> > int mb_le, mb_re; /* middle button left/right edge */
> > + struct phys_coords mm = { 0.0, 0.0 };
> >
> > - absinfo_x = device->abs.absinfo_x;
> > - absinfo_y = device->abs.absinfo_y;
> > -
> > - offset.x = absinfo_x->minimum,
> > - offset.y = absinfo_y->minimum,
> > - xres = absinfo_x->resolution;
> > - yres = absinfo_y->resolution;
> > - width = device->abs.dimensions.x;
> > - height = device->abs.dimensions.y;
> > + evdev_device_get_size(device, &width, &height);
> >
> > /* button height: 10mm or 15% or the touchpad height,
> > whichever is smaller */
> > - if ((height * 0.15)/yres > 10) {
> > - tp->buttons.bottom_area.top_edge =
> > - absinfo_y->maximum - 10 * yres;
> > + if (height * 0.15 > 10) {
> > + mm.y = height - 10;
> > + edges = evdev_device_mm_to_units(device, &mm);
>
> This evdev_device_mm_to_units() call seems redundant.
>
> > } else {
> > - tp->buttons.bottom_area.top_edge = height * .85 + offset.y;
> > + mm.y = height * 0.85;
> > + edges = evdev_device_mm_to_units(device, &mm);
>
> Idem.
whoops, indeed.
>
> > }
> >
> > + mm.x = width * 0.5;
> > + edges = evdev_device_mm_to_units(device, &mm);
>
> This will override the result of the previous call.
yes, but it doesn't matter - the condition above sets x, here we set y.
dropping the calls above makes this more obvious, so I did that and left
this one in place.
>
> > + tp->buttons.bottom_area.top_edge = edges.y;
> > + tp->buttons.bottom_area.rightbutton_left_edge = edges.x;
> > +
> > tp->buttons.bottom_area.middlebutton_left_edge = INT_MAX;
> > - tp->buttons.bottom_area.rightbutton_left_edge = width/2 + offset.x;
> >
> > /* if middlebutton emulation is enabled, don't init a software area */
> > if (device->middlebutton.want_enabled)
> > @@ -584,14 +580,21 @@ tp_init_softbuttons(struct tp_dispatch *tp,
> > * All Dell touchpads appear to have a middle marker.
> > */
> > if (tp->device->model_flags & EVDEV_MODEL_DELL_TOUCHPAD) {
> > - const int MIDDLE_BUTTON_WIDTH = 10; /* mm */
> > - int half_width = MIDDLE_BUTTON_WIDTH/2 * xres; /* units */
> > + mm.x = width/2 - 5; /* 10mm wide */
> > + edges = evdev_device_mm_to_units(device, &mm);
> > + mb_le = edges.x;
> >
> > - mb_le = offset.x + width/2 - half_width;
> > - mb_re = offset.x + width/2 + half_width;
> > + mm.x = width/2 + 5; /* 10mm wide */
> > + edges = evdev_device_mm_to_units(device, &mm);
> > + mb_le = edges.y;
>
> This looks wrong, you're not setting mb_re and you're using
> edges.y here. I guess this is missing a test case in the test
> suite ?
yeah, I thought at first we can't have test devices that require dmi
matching but really, we can assign any property we want :)
and we have the XPS13 touchpad already, so I'll just do that.
(also: fixed locally)
Cheers,
Peter
> > } else {
> > - mb_le = offset.x + width * 0.375;
> > - mb_re = offset.x + width * 0.625;
> > + mm.x = width * 0.375;
> > + edges = evdev_device_mm_to_units(device, &mm);
> > + mb_le = edges.x;
> > +
> > + mm.x = width * 0.625;
> > + edges = evdev_device_mm_to_units(device, &mm);
> > + mb_re = edges.x;
> > }
> >
> > tp->buttons.bottom_area.middlebutton_left_edge = mb_le;
> > @@ -603,18 +606,7 @@ tp_init_top_softbuttons(struct tp_dispatch *tp,
> > struct evdev_device *device,
> > double topbutton_size_mult)
> > {
> > - int width;
> > - const struct input_absinfo *absinfo_x, *absinfo_y;
> > - struct device_coords offset;
> > - int yres;
> > -
> > - absinfo_x = device->abs.absinfo_x;
> > - absinfo_y = device->abs.absinfo_y;
> > -
> > - offset.x = absinfo_x->minimum,
> > - offset.y = absinfo_y->minimum;
> > - yres = absinfo_y->resolution;
> > - width = device->abs.dimensions.x;
> > + struct device_coords edges;
> >
> > if (tp->buttons.has_topbuttons) {
> > /* T440s has the top button line 5mm from the top, event
> > @@ -622,10 +614,20 @@ tp_init_top_softbuttons(struct tp_dispatch *tp,
> > top - which maps to 15%. We allow the caller to enlarge the
> > area using a multiplier for the touchpad disabled case. */
> > double topsize_mm = 10 * topbutton_size_mult;
> > + struct phys_coords mm;
> > + double width, height;
> >
> > - tp->buttons.top_area.bottom_edge = offset.y + topsize_mm * yres;
> > - tp->buttons.top_area.rightbutton_left_edge = width * .58 + offset.x;
> > - tp->buttons.top_area.leftbutton_right_edge = width * .42 + offset.x;
> > + evdev_device_get_size(device, &width, &height);
> > +
> > + mm.x = width * 0.58;
> > + mm.y = topsize_mm;
> > + edges = evdev_device_mm_to_units(device, &mm);
> > + tp->buttons.top_area.bottom_edge = edges.y;
> > + tp->buttons.top_area.rightbutton_left_edge = edges.x;
> > +
> > + mm.x = width * 0.42;
> > + edges = evdev_device_mm_to_units(device, &mm);
> > + tp->buttons.top_area.leftbutton_right_edge = edges.x;
> > } else {
> > tp->buttons.top_area.bottom_edge = INT_MIN;
> > }
> > diff --git a/src/evdev-mt-touchpad-edge-scroll.c b/src/evdev-mt-touchpad-edge-scroll.c
> > index 610744a..7675885 100644
> > --- a/src/evdev-mt-touchpad-edge-scroll.c
> > +++ b/src/evdev-mt-touchpad-edge-scroll.c
> > @@ -285,28 +285,29 @@ int
> > tp_edge_scroll_init(struct tp_dispatch *tp, struct evdev_device *device)
> > {
> > struct tp_touch *t;
> > - int edge_width, edge_height;
> > double width, height;
> > bool want_horiz_scroll = true;
> > + struct device_coords edges;
> > + struct phys_coords mm = { 0.0, 0.0 };
> >
> > + evdev_device_get_size(device, &width, &height);
> > /* Touchpads smaller than 50mm are not tall enough to have a
> > horizontal scroll area, it takes too much space away. But
> > clickpads have enough space here anyway because of the
> > software button area (and all these tiny clickpads were built
> > when software buttons were a thing, e.g. Lenovo *20 series)
> > */
> > - if (!tp->buttons.is_clickpad) {
> > - evdev_device_get_size(device, &width, &height);
> > + if (!tp->buttons.is_clickpad)
> > want_horiz_scroll = (height >= 50);
> > - }
> >
> > /* 7mm edge size */
> > - edge_width = device->abs.absinfo_x->resolution * 7;
> > - edge_height = device->abs.absinfo_y->resolution * 7;
> > + mm.x = width - 7;
> > + mm.y = height - 7;
> > + edges = evdev_device_mm_to_units(device, &mm);
> >
> > - tp->scroll.right_edge = device->abs.absinfo_x->maximum - edge_width;
> > + tp->scroll.right_edge = edges.x;
> > if (want_horiz_scroll)
> > - tp->scroll.bottom_edge = device->abs.absinfo_y->maximum - edge_height;
> > + tp->scroll.bottom_edge = edges.y;
> > else
> > tp->scroll.bottom_edge = INT_MAX;
> >
> > diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> > index a7b5a87..eff09f9 100644
> > --- a/src/evdev-mt-touchpad.c
> > +++ b/src/evdev-mt-touchpad.c
> > @@ -1993,25 +1993,32 @@ static int
> > tp_init_palmdetect(struct tp_dispatch *tp,
> > struct evdev_device *device)
> > {
> > - int width;
> > + double width, height;
> > + struct phys_coords mm = { 0.0, 0.0 };
> > + struct device_coords edges;
> >
> > tp->palm.right_edge = INT_MAX;
> > tp->palm.left_edge = INT_MIN;
> >
> > - width = device->abs.dimensions.x;
> > -
> > /* Wacom doesn't have internal touchpads */
> > if (device->model_flags & EVDEV_MODEL_WACOM_TOUCHPAD)
> > return 0;
> >
> > + evdev_device_get_size(device, &width, &height);
> > +
> > /* Enable palm detection on touchpads >= 70 mm. Anything smaller
> > probably won't need it, until we find out it does */
> > - if (width/device->abs.absinfo_x->resolution < 70)
> > + if (width < 70.0)
> > return 0;
> >
> > /* palm edges are 5% of the width on each side */
> > - tp->palm.right_edge = device->abs.absinfo_x->maximum - width * 0.05;
> > - tp->palm.left_edge = device->abs.absinfo_x->minimum + width * 0.05;
> > + mm.x = width * 0.05;
> > + edges = evdev_device_mm_to_units(device, &mm);
> > + tp->palm.left_edge = edges.x;
> > +
> > + mm.x = width * 0.95;
> > + edges = evdev_device_mm_to_units(device, &mm);
> > + tp->palm.right_edge = edges.x;
> >
> > tp->palm.monitor_trackpoint = true;
> >
> > @@ -2038,8 +2045,9 @@ tp_init_thumb(struct tp_dispatch *tp)
> > struct evdev_device *device = tp->device;
> > const struct input_absinfo *abs;
> > double w = 0.0, h = 0.0;
> > + struct device_coords edges;
> > + struct phys_coords mm = { 0.0, 0.0 };
> > int xres, yres;
> > - int ymax;
> > double threshold;
> >
> > if (!tp->buttons.is_clickpad)
> > @@ -2057,10 +2065,13 @@ tp_init_thumb(struct tp_dispatch *tp)
> >
> > /* detect thumbs by pressure in the bottom 15mm, detect thumbs by
> > * lingering in the bottom 8mm */
> > - ymax = tp->device->abs.absinfo_y->maximum;
> > - yres = tp->device->abs.absinfo_y->resolution;
> > - tp->thumb.upper_thumb_line = ymax - yres * 15;
> > - tp->thumb.lower_thumb_line = ymax - yres * 8;
> > + mm.y = h * 0.85;
> > + edges = evdev_device_mm_to_units(device, &mm);
> > + tp->thumb.upper_thumb_line = edges.y;
> > +
> > + mm.y = h * 0.92;
> > + edges = evdev_device_mm_to_units(device, &mm);
> > + tp->thumb.lower_thumb_line = edges.y;
> >
> > abs = libevdev_get_abs_info(device->evdev, ABS_MT_PRESSURE);
> > if (!abs)
> >
>
> Regards,
>
> Hans
More information about the wayland-devel
mailing list