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

[Jason Ekstrand]

On Wed, Jun 25, 2014 at 6:16 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> 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.
>

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 <pekka.paalanen at collabora.co.uk>
>
> Feel free to push if my comments are not disturbing. :-)
>
>
> Thanks,
> pq
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140625/664b4de5/attachment-0001.html>