On Wed, Jun 25, 2014 at 6:16 AM, Pekka Paalanen <[email protected]> wrote:
> 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. > Good idea. Done. > > > > > - 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? > 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. > > > + 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
