<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jun 25, 2014 at 6:16 AM, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On Tue, 24 Jun 2014 21:23:53 -0700<br>
Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br>
<br>
> Previoiusly, we had a mess of logic that was repeated with one of the<br>
> repeats negated. Not only was this unnecisaraly confusing, but it<br>
> segfaulted and one of the negations was wrong. This cleans the whole mess<br>
> up and should fix bug #79725.<br>
> ---<br>
> src/data-device.c | 30 ++++++++++++++++--------------<br>
> 1 file changed, 16 insertions(+), 14 deletions(-)<br>
><br>
> diff --git a/src/data-device.c b/src/data-device.c<br>
> index 6a81bc8..092eb0c 100644<br>
> --- a/src/data-device.c<br>
> +++ b/src/data-device.c<br>
> @@ -648,15 +648,22 @@ data_device_start_drag(struct wl_client *client, struct wl_resource *resource,<br>
> struct weston_seat *seat = wl_resource_get_user_data(resource);<br>
> struct weston_data_source *source = NULL;<br>
> struct weston_surface *icon = NULL;<br>
> + int is_pointer_grab, is_touch_grab;<br>
> int32_t ret = 0;<br>
<br>
</div>Adding a temporary variable for<br>
wl_resource_get_user_data(origin_resource) would make the lines below much shorter.<br></blockquote><div><br></div><div>Good idea. Done.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
><br>
> - if ((seat->pointer->button_count == 0 ||<br>
> - seat->pointer->grab_serial != serial ||<br>
> - !seat->pointer->focus ||<br>
> - seat->pointer->focus->surface != wl_resource_get_user_data(origin_resource)) &&<br>
> - (seat->touch->grab_serial != serial ||<br>
> - !seat->touch->focus ||<br>
> - seat->touch->focus->surface != wl_resource_get_user_data(origin_resource)))<br>
> + is_pointer_grab = seat->pointer &&<br>
> + seat->pointer->button_count == 1 &&<br>
<br>
</div>The negation of seat->pointer->button_count == 0 would be to<br>
compare !=0 or >0. Is ==1 always correct?<br>
Can't we start a drag with two buttons down?<br></blockquote><div><br></div><div>Let's keep with one for now. Multitouch is hard to get right and I have yet to see a particularly good framework for handling it in a non-fullscreen scenario. Also, I don't think weston allows for per-finger dragging right now, I think it has to be per wl_touch.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
> + seat->pointer->grab_serial == serial &&<br>
> + seat->pointer->focus &&<br>
> + seat->pointer->focus->surface == wl_resource_get_user_data(origin_resource);<br>
> +<br>
> + is_touch_grab = seat->touch &&<br>
> + seat->touch->num_tp == 1 &&<br>
<br>
</div>Is it valid to assume the number of touchpoints must be one if it is<br>
not zero?<br>
<div class=""><br>
> + seat->touch->grab_serial == serial &&<br>
> + seat->touch->focus &&<br>
> + seat->touch->focus->surface == wl_resource_get_user_data(origin_resource);<br>
> +<br>
> + if (!is_pointer_grab && !is_touch_grab)<br>
> return;<br>
<br>
</div>I see the num_tp check is new here, but likely good to have.<br>
<div class=""><br>
><br>
> /* FIXME: Check that the data source type array isn't empty. */<br>
> @@ -672,14 +679,9 @@ data_device_start_drag(struct wl_client *client, struct wl_resource *resource,<br>
> return;<br>
> }<br>
><br>
> - if (seat->pointer->button_count == 1 &&<br>
<br>
</div>Ok, so the button_count == 1 came from here. Should be fine then.<br>
<div class=""><br>
> - seat->pointer->grab_serial == serial &&<br>
> - seat->pointer->focus &&<br>
> - seat->pointer->focus->surface == wl_resource_get_user_data(origin_resource))<br>
> + if (is_pointer_grab)<br>
> ret = weston_pointer_start_drag(seat->pointer, source, icon, client);<br>
> - else if (seat->touch->grab_serial != serial ||<br>
<br>
</div>Ha, yes, this is the wrong negation you mentioned.<br>
<div class=""><br>
> - seat->touch->focus ||<br>
> - seat->touch->focus->surface != wl_resource_get_user_data(origin_resource))<br>
> + else if (is_touch_grab)<br>
> ret = weston_touch_start_drag(seat->touch, source, icon, client);<br>
<br>
</div>Looks like num_tp check was not here originally either. num_tp==0<br>
certainly does not make sense AFAIU, so checking it is good, but one<br>
vs. many is a question.<br>
<br>
><br>
> if (ret < 0)<br>
<br>
Reviewed-by: Pekka Paalanen <<a href="mailto:pekka.paalanen@collabora.co.uk">pekka.paalanen@collabora.co.uk</a>><br>
<br>
Feel free to push if my comments are not disturbing. :-)<br>
<br>
<br>
Thanks,<br>
pq<br>
</blockquote></div><br></div></div>