On Tue, 24 Jun 2014 21:23:53 -0700 Jason Ekstrand <[email protected]> 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 <[email protected]> Feel free to push if my comments are not disturbing. :-) Thanks, pq _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
