[PATCH v2 xf86-input-libinput] Add tablet tool area ratio property

[Peter Hutterer]

On Fri, Jan 06, 2017 at 11:49:52AM -0800, Jason Gerecke wrote:
> On 01/03/2017 08:38 PM, Peter Hutterer wrote:
> > By default, the X server maps the tablet axes to the available screen area.
> > When a tablet is mapped to the screen but has a different aspect ratio than
> > the screen, input data is skewed. Expose an area ratio property to map the
> > a subsection of the available tablet area into the desired ratio.
> > 
> > Differences to the wacom driver: there the x/y min/max values must be
> > specified manually and in device coordinates. For this driver we merely
> > provide the area ratio (e.g. 4:3) and let the driver work out the rest.
> > 
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > Changes to v1:
> > - link up the tools so the ratio applies to all tools
> > - only init the area property for non-builtin tablets, for the built-in ones
> >   we don't use the area but the calibration matrix in case they're off
> > 
> >  include/libinput-properties.h |   3 +
> >  man/libinput.man              |  30 +++++++
> >  src/xf86libinput.c            | 188 ++++++++++++++++++++++++++++++++++++++++--
> >  3 files changed, 215 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/libinput-properties.h b/include/libinput-properties.h
> > index f76500f..a701316 100644
> > --- a/include/libinput-properties.h
> > +++ b/include/libinput-properties.h
[...]
> > @@ -418,6 +425,29 @@ xf86libinput_set_pressurecurve(struct xf86libinput *driver_data,
> >  			    driver_data->pressurecurve.sz);
> >  }
> >  
> > +static inline void
> > +xf86libinput_set_area_ratio(struct xf86libinput *driver_data,
> > +			    const struct ratio *ratio)
> > +{
> > +	double f;
> > +	double w, h;
> > +
> > +	if (libinput_device_get_size(driver_data->shared_device->device, &w, &h) != 0)
> > +		return;
> > +
> > +	f = 1.0 * (ratio->x * h)/(ratio->y * w);
> > +
> > +	if (f <= 1.0) {
> > +		driver_data->area_scale_factor.x = 1.0/f;
> > +		driver_data->area_scale_factor.y = 1.0;
> > +	} else {
> > +		driver_data->area_scale_factor.x = 1.0;
> > +		driver_data->area_scale_factor.y = f;
> > +	}
> > +
> > +	driver_data->options.area = *ratio;
> > +}
> 
> This function sets my Spidey-sense tingling. I don't like how
> ratio={0,0} is treated like any other valid value, causing the function
> to store an NaN in area_scale_factor. I want to say that we should
> explicitly handle the {0,0} case and set area_scale_factor to {1.0,1.0}
> so that other functions don't have to tiptoe around bogus values that
> may appear. However, area_scale_factor doesn't always have a valid value
> anyway (its initial value is {0,0} until a ratio is set) so maybe it
> doesn't really matter.

the second part is intentional: if there's some bug in the setup code, scale
factor means the pointer won't move, so it's immediately obvious. But you're
right, the NaN bit here isn't acceptable, I've amended this in v3.

[...]

> > @@ -4078,6 +4176,62 @@ LibinputSetPropertyPressureCurve(DeviceIntPtr dev,
> >  	return Success;
> >  }
> >  
> > +static inline int
> > +LibinputSetPropertyAreaRatio(DeviceIntPtr dev,
> > +			     Atom atom,
> > +			     XIPropertyValuePtr val,
> > +			     BOOL checkonly)
> > +{
> > +	InputInfoPtr pInfo = dev->public.devicePrivate;
> > +	struct xf86libinput *driver_data = pInfo->private;
> > +	uint32_t *vals;
> > +	struct ratio area = { 0, 0 };
> > +
> > +	if (val->format != 32 || val->size != 2 || val->type != XA_CARDINAL)
> > +		return BadMatch;
> 
> I was tripped up by this in testing so I might as dig out my newbie hat
> and ask... Why XA_CARDINAL? What's the difference between XA_CARDINAL
> and XA_INTEGER? Are cardinals related to the CARD{n} types, or is that
> just a naming coincidence? Is there any way to set cardinal properties
> with xinput set-prop?

32-bit XA_CARDINAL == uint32_t
32-bit XA_INTEGER == int32_t

that's all there is to it. We don't use XA_CARDINAL consistently in the
various drivers (not even within each driver, sorry). But the way the
properties work no-one ever really cares about the type anyway, everyone
just fetches the property and overwrites the values. that's what xinput
set-prop does too, it doesn't care about the type.

There's one other property in this driver that already uses it (the scroll
button) and it's documented as CARD8. So yeah, not ideal, but sticking to
the expected data type does make sense. All the others are integers because
they're either numbers or BOOL which is an integer.

Thanks for the review, much appreciated.

Cheers,
   Peter