[PATCH libinput 00/12] typesafe coordinate wrappers

[Peter Hutterer]

On Mon, Mar 16, 2015 at 12:40:20PM +0100, Hans de Goede wrote:
> 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 :)   (*)

yeah, the double vs int issue was what I tried to avoid but we can switch
this over now easier.

> 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.

ok, thanks heaps for the review, I've pushed this now (after a couple of
minor rebase changes). rest will go on top, that way it won't hold up the
other patches:

   eeac710..b2b5913  master -> master


> 
> 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.

yep, will do that once the touchpad filtering is sorted out

Cheers,
   Peter