[RFC libinput 2/2] touchpad: Implement pinch gesture support (wip)

[Peter Hutterer]

On Mon, Mar 16, 2015 at 02:58:02PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12-03-15 09:23, Peter Hutterer wrote:
> >On Wed, Mar 11, 2015 at 03:20:55PM +0100, Hans de Goede wrote:
> >>Implement touchpad pinch (and rotate) gesture support.
> >>
> >>WIP: TODO: fix testsuite.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >
> >looks good, almost all comments are just related to coding style/shuffling
> >things around.
> >
> >fwiw, it'd be good adding this to the event-gui test program as well, this
> >should make visual debugging a lot easier.
> 
> Hmm, I somehow missed this mail when sending the first non RFC version
> of this patch, so not all review comments are addressed there.

[...]

> >>+		if (hypot(dx, dy) > TP_MM_TO_DPI_NORMALIZED(20)) {
> >>+			tp->gesture.twofinger_state = GESTURE_2FG_STATE_PINCH;
> >>+			tp_gesture_get_pinch_info(tp,
> >>+						  &tp->gesture.distance,
> >>+						  &tp->gesture.angle,
> >>+						  &tp->gesture.center_x,
> >>+						  &tp->gesture.center_y);
> >>+			break;
> >>+		}
> >>+
> >>+		/* Elif fingers have been close together for a while, scroll */
> >>+		if (time > (tp->gesture.initial_time + DEFAULT_GESTURE_2FG_SCROLL_TIMEOUT)) {
> >>+			tp->gesture.twofinger_state = GESTURE_2FG_STATE_SCROLL;
> >>+			break;
> >>+		}
> >>+
> >>+		/* Else wait for both fingers to have moved */
> >>+		dir0 = tp_gesture_get_direction(tp, 0);
> >>+		dir1 = tp_gesture_get_direction(tp, 1);
> >>+		if (dir0 == UNDEFINED_DIRECTION || dir1 == UNDEFINED_DIRECTION)
> >>+			break;
> >>+
> >>+		/* If both touches are moving in the same direction assume scroll */
> >>+		if (((dir0 | (dir0 >> 1)) & dir1) ||
> >>+		    ((dir1 | (dir1 >> 1)) & dir0) ||
> >
> >I take it this is checking for the same octant? vector_get_direction()
> >already returns at least two octants, so a simple & should be enough here.
> 
> In some cases (semi-mt touchpads) I've seen one finger move e.g. N/NE and the other
> W/NW so this is deliberate.

drop a comment in so we have this explicitly listed please.
 
> >also, if
> >you're just shifting to the left you're more lenient in a clockwise direction of
> >octant overlap.
> 
> In the second statement dir1 and dir0 are swapped, so the above statement from
> you is false unless I'm misunderstanding things.

yeah, true, sorry about that.

> 
> >
> >>+		    ((dir0 & 0x80) && (dir1 & 0x01)) ||
> >>+		    ((dir1 & 0x80) && (dir0 & 0x01))) {
> >
> >pls use the N, NE, etc. enum values for this
> 
> This is to check for rolover the direction is not really relevant which is
> relevant is that bit numbers 0 and 7 are checked which us much clearer to
> see with raw hex.

here too, a comment would be nice.

Cheers,
   Peter