Merge lp:~ilidrissi.amine/gesturetest/621600-no-args into lp:gesturetest

[Mohamed Amine Ilidrissi]

[Henrik Rydberg (rydberg)]

Looks ok to me.

review: Approve

[Chase Douglas (chasedouglas)]

I'm sorry I haven't gotten back to this sooner, but I'm against this change. gesturetest is not a demo program that you run to see things fly across your screen when you perform gestures :). It's a test tool that must be used in specific ways to get the results you want to see.

For example, if you are interested in testing rotation gestures, you should subscribe only to them and not to other gestures. As soon as you start to listen for other gestures things get very confusing. It can be hard to understand why you might be receiving drag gestures when you perform a pivot rotation, and subscribing to all gestures by default only exacerbates the issue. Essentially, I don't want people to run gesturetest and make decisions based on poor testing scenarios.

review: Disapprove

[Mohamed Amine Ilidrissi (ilidrissi.amine)]

So, for example, if a program listens to 2 different gestures at the same time (if it's waiting for the user to do something for example), those 2 gestures can collide, confusing the user? If so, shouldn't that be fixed?
(Just asking ; I don't know anything about gestures :p)

[Chase Douglas (chasedouglas)]

I'll give an example that caused us issues. If you want to perform a rotation where one finger pivots around another, you will receive both rotation and drag events. The reason is that gestures are performed about a point in space. Our gesture system uses the center (average) of all the touch points as this "focus" point. So if you perform a pivot rotation, you'll see the focus point being rotated, but you'll also see it being dragged. This is the correct behavior, but it isn't obvious.

We had a bug filed against our recognizer where the user didn't expect to see drag events when performing a pivot rotation. What they should have done is subscribed to just the rotate gesture events, since all they cared about was the rotation. I don't want to lead people to assume our stack is broken because they ran gesturetest without any arguments and saw behavior they didn't understand. Thus, I want to keep the functionality as is, potentially requiring the user to read the man page to understand what gesturetest is really doing.

[Mohamed Amine Ilidrissi (ilidrissi.amine)]

> I'll give an example that caused us issues. If you want to perform a rotation
> where one finger pivots around another, you will receive both rotation and
> drag events. The reason is that gestures are performed about a point in space.
> Our gesture system uses the center (average) of all the touch points as this
> "focus" point. So if you perform a pivot rotation, you'll see the focus point
> being rotated, but you'll also see it being dragged. This is the correct
> behavior, but it isn't obvious.
>
> We had a bug filed against our recognizer where the user didn't expect to see
> drag events when performing a pivot rotation. What they should have done is
> subscribed to just the rotate gesture events, since all they cared about was
> the rotation. I don't want to lead people to assume our stack is broken
> because they ran gesturetest without any arguments and saw behavior they
> didn't understand. Thus, I want to keep the functionality as is, potentially
> requiring the user to read the man page to understand what gesturetest is
> really doing.

Would an "--all-gestures" option do the trick? That way, the user knows that he is testing all the gestures at once.

[Henrik Rydberg (rydberg)]

I think the reasons for not including this change are really small. What you gain is the ability to see that *something* comes through in an easy way. For most people, that is that only thing they care about.

Regarding the completely different matter of rotation pivot, that bug is still valid, and the reasons for not confusing the center of the contacts with the pivot are still there. In the future, we will most likely see no drag events as one performs a rotation around a single finger, even if both types of events are listened to.

[Chase Douglas (chasedouglas)]

I guess my underlying question is why we need this functionality. It seems to me that what we really need is a demo to show that gestures are working. Something graphical that you can manipulate. Gesturetest is not intended to show an end user that gestures are working properly. It is merely a developer's tool. This feels like trying to fit a round peg into a square hole.

Also, note that gesturetest will be dropped after Maverick anyways since gesture recognition will be performed client side instead of inside X. Geistest really is the utility you are after :).

[Henrik Rydberg (rydberg)]

Geistest is one layer further away from the source, and there are reasons to be able to test the presence of events on several checkpoints, this one included. And geistest would not be any easier to use with the same line of reasoning.

[Chase Douglas (chasedouglas)]

I still disagree as I think this may lead to sloppy coding. For example, adding 0 as a window parameter to gesturetest to listen on all windows has already led to an improper implementation of gestures in Unity. Unity currently listens for window specific gestures on all windows, including root, and uses the focus point to choose the correct window for manipulation. This is plain wrong, and only occurred because the gesturetest code had a path for doing this (gesturetest code was basically copied and pasted into Unity). Thus far, 1/1 gesture projects have used gesturetest and subsequently been written improperly :).

That said, I'm apparently in the minority and this isn't a huge deal. I'll merge this change in when I have a chance.

review: Approve

[Mohamed Amine Ilidrissi (ilidrissi.amine)]

> I still disagree as I think this may lead to sloppy coding. For example,
> adding 0 as a window parameter to gesturetest to listen on all windows has
> already led to an improper implementation of gestures in Unity. Unity
> currently listens for window specific gestures on all windows, including root,
> and uses the focus point to choose the correct window for manipulation. This
> is plain wrong, and only occurred because the gesturetest code had a path for
> doing this (gesturetest code was basically copied and pasted into Unity). Thus
> far, 1/1 gesture projects have used gesturetest and subsequently been written
> improperly :).
>
> That said, I'm apparently in the minority and this isn't a huge deal. I'll
> merge this change in when I have a chance.

Your opinion still counts, though. You shouldn't approve this just because everyone else is okay with it. That said, thanks :)

[Chase Douglas (chasedouglas)]

Gesturetest is dead, so I'm rejecting this proposal.

Unmerged revisions

12. By Mohamed Amine Ilidrissi on 2010-09-25

Fixed the code.

11. By Rafi on 2010-08-21

Default args to 0 0 0xffffffffffffffff if no args are specified on the command
line. This should show all gestures that aren't caught by an application
(and fall through to the root window).

=== modified file 'src/main.c'
--- src/main.c	2010-08-12 09:43:29 +0000
+++ src/main.c	2010-09-25 18:22:39 +0000
@@ -40,42 +40,54 @@
 		      uint32_t **mask) {
 	unsigned long long int tmp;
 	char *end;
-        int i;
-
-	if (argc != 4) {
-		usage(argv[0]);
-		return -1;
-	}
-
-	tmp = strtoul(argv[1], &end, 0);
-	if (*end != '\0' || tmp > UINT_MAX) {
-		usage(argv[0]);
-		return -1;
-	}
-	*window = (xcb_window_t)tmp;
-
-	tmp = strtoul(argv[2], &end, 0);
-	if (*end != '\0' || tmp > USHRT_MAX) {
-		usage(argv[0]);
-		return -1;
-	}
-	*device_id = (uint16_t)tmp;
-
-	tmp = strtoull(argv[3], &end, 0);
-	if (*end != '\0') {
-		usage(argv[0]);
-		return -1;
-	}
-	*mask_len = (tmp > UINT_MAX) ? 2 : 1;
-	
-	*mask = malloc(sizeof(uint32_t) * *mask_len);
-	if (!*mask) {
-		perror("Error: Failed to allocate event mask");
-		return -1;
-	}
-
-	for (i = 0; i < *mask_len; i++)
-		(*mask)[i] = tmp >> (32 * i);
+    int i;
+
+	switch(argc) {
+		case 1:
+			*window = (xcb_window_t)0;
+			*device_id = (uint16_t)0;
+			*mask_len = 2;
+			*mask = malloc(sizeof(uint32_t) * *mask_len);
+			(*mask)[0] = 0xffffffff;
+			(*mask)[1] = 0xffffffff;
+			break;
+		case 4:
+			tmp = strtoul(argv[1], &end, 0);
+			if (*end != '\0' || tmp > UINT_MAX) {
+				usage(argv[0]);
+				return -1;
+			}
+			*window = (xcb_window_t)tmp;
+
+			tmp = strtoul(argv[2], &end, 0);
+			if (*end != '\0' || tmp > USHRT_MAX) {
+				usage(argv[0]);
+				return -1;
+			}
+			*device_id = (uint16_t)tmp;
+
+			tmp = strtoull(argv[3], &end, 0);
+			if (*end != '\0') {
+				usage(argv[0]);
+				return -1;
+			}
+			*mask_len = (tmp > UINT_MAX) ? 2 : 1;
+			
+			*mask = malloc(sizeof(uint32_t) * *mask_len);
+			if (!*mask) {
+				perror("Error: Failed to allocate event mask");
+				return -1;
+			}
+
+			for (i = 0; i < *mask_len; i++)
+				(*mask)[i] = tmp >> (32 * i);
+
+			break;
+		default:
+				usage(argv[0]);
+				return -1;
+	}
+
 
 	return 0;
 }