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

[Hans de Goede]

Hi,

On 17-03-15 06:35, Peter Hutterer wrote:
> 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.

Comments added to for both (in my personal tree).

Regards,

Hans