[PATCH] data-device: Clean up the logic in start_drag

[Pekka Paalanen]

On Tue, 24 Jun 2014 21:23:53 -0700
Jason Ekstrand <jason at jlekstrand.net> wrote:

> Previoiusly, we had a mess of logic that was repeated with one of the
> repeats negated.  Not only was this unnecisaraly confusing, but it
> segfaulted and one of the negations was wrong.  This cleans the whole mess
> up and should fix bug #79725.
> ---
>  src/data-device.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/src/data-device.c b/src/data-device.c
> index 6a81bc8..092eb0c 100644
> --- a/src/data-device.c
> +++ b/src/data-device.c
> @@ -648,15 +648,22 @@ data_device_start_drag(struct wl_client *client, struct wl_resource *resource,
>  	struct weston_seat *seat = wl_resource_get_user_data(resource);
>  	struct weston_data_source *source = NULL;
>  	struct weston_surface *icon = NULL;
> +	int is_pointer_grab, is_touch_grab;
>  	int32_t ret = 0;

Adding a temporary variable for
wl_resource_get_user_data(origin_resource) would make the lines below much shorter.

>  
> -	if ((seat->pointer->button_count == 0 ||
> -	    seat->pointer->grab_serial != serial ||
> -	    !seat->pointer->focus ||
> -	    seat->pointer->focus->surface != wl_resource_get_user_data(origin_resource)) &&
> -		(seat->touch->grab_serial != serial ||
> -		!seat->touch->focus ||
> -		seat->touch->focus->surface != wl_resource_get_user_data(origin_resource)))
> +	is_pointer_grab = seat->pointer &&
> +			  seat->pointer->button_count == 1 &&

The negation of seat->pointer->button_count == 0 would be to
compare !=0 or >0. Is ==1 always correct?
Can't we start a drag with two buttons down?

> +			  seat->pointer->grab_serial == serial &&
> +			  seat->pointer->focus &&
> +			  seat->pointer->focus->surface == wl_resource_get_user_data(origin_resource);
> +
> +	is_touch_grab = seat->touch &&
> +			seat->touch->num_tp == 1 &&

Is it valid to assume the number of touchpoints must be one if it is
not zero?

> +			seat->touch->grab_serial == serial &&
> +			seat->touch->focus &&
> +			seat->touch->focus->surface == wl_resource_get_user_data(origin_resource);
> +
> +	if (!is_pointer_grab && !is_touch_grab)
>  		return;

I see the num_tp check is new here, but likely good to have.

>  
>  	/* FIXME: Check that the data source type array isn't empty. */
> @@ -672,14 +679,9 @@ data_device_start_drag(struct wl_client *client, struct wl_resource *resource,
>  		return;
>  	}
>  
> -	if (seat->pointer->button_count == 1 &&

Ok, so the button_count == 1 came from here. Should be fine then.

> -		seat->pointer->grab_serial == serial &&
> -		seat->pointer->focus &&
> -		seat->pointer->focus->surface == wl_resource_get_user_data(origin_resource))
> +	if (is_pointer_grab)
>  		ret = weston_pointer_start_drag(seat->pointer, source, icon, client);
> -	else if (seat->touch->grab_serial != serial ||

Ha, yes, this is the wrong negation you mentioned.

> -		seat->touch->focus ||
> -		seat->touch->focus->surface != wl_resource_get_user_data(origin_resource))
> +	else if (is_touch_grab)
>  		ret = weston_touch_start_drag(seat->touch, source, icon, client);

Looks like num_tp check was not here originally either. num_tp==0
certainly does not make sense AFAIU, so checking it is good, but one
vs. many is a question.

>  
>  	if (ret < 0)

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

Feel free to push if my comments are not disturbing. :-)


Thanks,
pq