[PATCH libinput 00/12] typesafe coordinate wrappers

[Hans de Goede]

Hi Peter,

On 12-03-15 09:36, Peter Hutterer wrote:
>
> libinput has two types of coordinates - device coordinates and coordinates
> normalized into the 1000 dpi default. we generally use int/double for those
> two, but it's not always clear or obvious which type of coordinates we're
> dealing with. So there's a risk of mixing them up and we may not notice for
> a while (see 38ee for example where we used a threshold in device-specific
> coordinates).
>
> typedefs help with readability but the compiler won't catch it. this
> patchset uses two different structs for the x/y pairs (which is what we're
> mostly concerned with). It reads a bit clunkier but we get the benefit of
> immediately knowing what coordinates we're dealing with.
>
> I'm somewhat in two minds about it, I like the type-safety, not a fan of the
> extra clunkyness. Any suggestions for improvements appreciated.

Overall I like this, we're dealing with x,y pairs almost everywhere, so it
makes a lot of sense to put them in a struct together, once we pass that
bridge (putting them in a struct), then having separate struts for device
coordinates vs normalized coordinates makes sense.

I believe that part of the clunkyness comes from there still being separate
x y values in various places (other then the public api), e.g.
normalize_delta and tp_normalize_delta still take separate dx / dy values,
where they should IMHO take a struct device_coords as the deltas are in
device_coords.

That combined with adding a device_coords_substract function would allow
e.g. :

	case EDGE_SCROLL_TOUCH_STATE_EDGE_NEW:
		initial_dx = t->point.x - t->scroll.initial.x;
		initial_dy = t->point.y - t->scroll.initial.y;
		tp_normalize_delta(tp,
			initial_dx,
			initial_dy,
			&normalized);

To be rewritten to something like this:

		normalized = tp_normalize_delta(device_coords_substract(t->point, t->scroll.initial));

I know that device_coords are integers, and the deltas in some cases are
doubles, but that is an implementation detail :)   (*)
What I'm trying to say is that I believe the clunkyness in part comes from
not going to putting x,y pairs in structs all the way, as the above example
shows, once we do go all the way the clunkyness disappears, at least that
is what I believe.

Note this patch-set is a good start and we could start with merging
it as is.

The entire series looks good to me and is:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

Regards,

Hans


*) There are 2 ways around this:
1) Store device coords as doubles to, since we some time average them
(e.g. for deltas in various places)
2) Use a separate delta type

Both probably have merits and we need to see which one works best in practice.

p.s. Another candidate for taking a pair instead of separate values is the
filter / accel code.