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 <hdego...@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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel