On Wed, 20 Aug 2014 11:27:10 +0200 Jonny Lamb <jonny.l...@collabora.co.uk> wrote:
> --- > desktop-shell/shell.c | 155 > ++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 143 insertions(+), 12 deletions(-) > > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c > index ec72287..db7841a 100644 > --- a/desktop-shell/shell.c > +++ b/desktop-shell/shell.c > @@ -223,9 +223,11 @@ struct shell_seat { > > struct { > struct weston_pointer_grab grab; > + struct weston_touch_grab touch_grab; > struct wl_list surfaces_list; > struct wl_client *client; > int32_t initial_up; > + enum { POINTER, TOUCH } type; > } popup_grab; > }; > > @@ -321,6 +323,8 @@ get_default_view(struct weston_surface *surface) > > static void > popup_grab_end(struct weston_pointer *pointer); > +static void > +touch_popup_grab_end(struct weston_touch *touch); > > static void > shell_grab_start(struct shell_grab *grab, > @@ -332,6 +336,8 @@ shell_grab_start(struct shell_grab *grab, > struct desktop_shell *shell = shsurf->shell; > > popup_grab_end(pointer); > + if (pointer->seat->touch) > + touch_popup_grab_end(pointer->seat->touch); > > grab->grab.interface = interface; > grab->shsurf = shsurf; > @@ -435,6 +441,7 @@ shell_touch_grab_start(struct shell_touch_grab *grab, > { > struct desktop_shell *shell = shsurf->shell; > > + touch_popup_grab_end(touch); > if (touch->seat->pointer) > popup_grab_end(touch->seat->pointer); > > @@ -3040,6 +3047,81 @@ static const struct weston_pointer_grab_interface > popup_grab_interface = { > }; > > static void > +touch_popup_grab_down(struct weston_touch_grab *grab, uint32_t time, > + int touch_id, wl_fixed_t sx, wl_fixed_t sy) > +{ > + struct wl_resource *resource; > + struct shell_seat *shseat = > + container_of(grab, struct shell_seat, popup_grab.touch_grab); > + struct wl_display *display = shseat->seat->compositor->wl_display; > + uint32_t serial; > + struct wl_list *resource_list; > + > + resource_list = &grab->touch->focus_resource_list; > + if (!wl_list_empty(resource_list)) { > + serial = wl_display_get_serial(display); > + wl_resource_for_each(resource, resource_list) { > + wl_touch_send_down(resource, serial, time, > + grab->touch->focus->surface->resource, > + touch_id, sx, sy); > + } > + } > +} > + > +static void > +touch_popup_grab_up(struct weston_touch_grab *grab, uint32_t time, int > touch_id) > +{ > + struct wl_resource *resource; > + struct shell_seat *shseat = > + container_of(grab, struct shell_seat, popup_grab.touch_grab); > + struct wl_display *display = shseat->seat->compositor->wl_display; > + uint32_t serial; > + struct wl_list *resource_list; > + > + resource_list = &grab->touch->focus_resource_list; > + if (!wl_list_empty(resource_list)) { > + serial = wl_display_get_serial(display); Not really necessary to check wl_list_empty here, since get_serial() does not bump the serial. It only fetches it. Hmm, is that right to not bump the serial? I suppose, since you are not storing a new serial anywhere to be checked against... > + wl_resource_for_each(resource, resource_list) { > + wl_touch_send_up(resource, serial, time, touch_id); > + } > + } > +} > + > +static void > +touch_popup_grab_motion(struct weston_touch_grab *grab, uint32_t time, > + int touch_id, wl_fixed_t sx, wl_fixed_t sy) > +{ > + struct wl_resource *resource; > + struct wl_list *resource_list; > + > + resource_list = &grab->touch->focus_resource_list; > + if (!wl_list_empty(resource_list)) { Not necessary to check wl_list_empty(). > + wl_resource_for_each(resource, resource_list) { > + wl_touch_send_motion(resource, time, touch_id, sx, sy); > + } > + } > +} > + > +static void > +touch_popup_grab_frame(struct weston_touch_grab *grab) > +{ > +} > + > +static void > +touch_popup_grab_cancel(struct weston_touch_grab *grab) > +{ > + touch_popup_grab_end(grab->touch); > +} > + > +static const struct weston_touch_grab_interface touch_popup_grab_interface = > { > + touch_popup_grab_down, > + touch_popup_grab_up, > + touch_popup_grab_motion, > + touch_popup_grab_frame, > + touch_popup_grab_cancel, > +}; > + > +static void > shell_surface_send_popup_done(struct shell_surface *shsurf) > { > if (shell_surface_is_wl_shell_surface(shsurf)) > @@ -3078,21 +3160,59 @@ popup_grab_end(struct weston_pointer *pointer) > } > > static void > -add_popup_grab(struct shell_surface *shsurf, struct shell_seat *shseat) > +touch_popup_grab_end(struct weston_touch *touch) > +{ > + struct weston_touch_grab *grab = touch->grab; > + struct shell_seat *shseat = > + container_of(grab, struct shell_seat, popup_grab.touch_grab); > + struct shell_surface *shsurf; > + struct shell_surface *prev = NULL; > + > + if (touch->grab->interface == &touch_popup_grab_interface) { > + weston_touch_end_grab(grab->touch); > + shseat->popup_grab.client = NULL; > + shseat->popup_grab.touch_grab.interface = NULL; > + assert(!wl_list_empty(&shseat->popup_grab.surfaces_list)); > + /* Send the popup_done event to all the popups open */ > + wl_list_for_each(shsurf, &shseat->popup_grab.surfaces_list, > popup.grab_link) { > + shell_surface_send_popup_done(shsurf); > + shsurf->popup.shseat = NULL; > + if (prev) { > + wl_list_init(&prev->popup.grab_link); > + } > + prev = shsurf; > + } > + wl_list_init(&prev->popup.grab_link); > + wl_list_init(&shseat->popup_grab.surfaces_list); > + } > +} > + > +static void > +add_popup_grab(struct shell_surface *shsurf, struct shell_seat *shseat, > int32_t type) > { > struct weston_seat *seat = shseat->seat; > > if (wl_list_empty(&shseat->popup_grab.surfaces_list)) { > + shseat->popup_grab.type = type; > shseat->popup_grab.client = > wl_resource_get_client(shsurf->resource); > - shseat->popup_grab.grab.interface = &popup_grab_interface; > - /* We must make sure here that this popup was opened after > - * a mouse press, and not just by moving around with other > - * popups already open. */ > - if (shseat->seat->pointer->button_count > 0) > - shseat->popup_grab.initial_up = 0; > + > + if (type == POINTER) { > + shseat->popup_grab.grab.interface = > &popup_grab_interface; > + /* We must make sure here that this popup was opened > after > + * a mouse press, and not just by moving around with > other > + * popups already open. */ > + if (shseat->seat->pointer->button_count > 0) > + shseat->popup_grab.initial_up = 0; Would this same logic not apply to touch as well? I mean, down-up to open a menu, another down-up to pick an item maybe popping up a sub-menu; vs. down to open a menu, slide over to an item to pop a sub-menu, slide... and first & final up for selection. Is touch ever used like that? > + } else if (type == TOUCH) { > + shseat->popup_grab.touch_grab.interface = > &touch_popup_grab_interface; > + } > > wl_list_insert(&shseat->popup_grab.surfaces_list, > &shsurf->popup.grab_link); > - weston_pointer_start_grab(seat->pointer, > &shseat->popup_grab.grab); > + > + if (type == POINTER) > + weston_pointer_start_grab(seat->pointer, > &shseat->popup_grab.grab); > + else if (type == TOUCH) > + weston_touch_start_grab(seat->touch, > &shseat->popup_grab.touch_grab); > } else { > wl_list_insert(&shseat->popup_grab.surfaces_list, > &shsurf->popup.grab_link); > } > @@ -3106,8 +3226,13 @@ remove_popup_grab(struct shell_surface *shsurf) > wl_list_remove(&shsurf->popup.grab_link); > wl_list_init(&shsurf->popup.grab_link); > if (wl_list_empty(&shseat->popup_grab.surfaces_list)) { > - weston_pointer_end_grab(shseat->popup_grab.grab.pointer); > - shseat->popup_grab.grab.interface = NULL; > + if (shseat->popup_grab.type == POINTER) { > + > weston_pointer_end_grab(shseat->popup_grab.grab.pointer); > + shseat->popup_grab.grab.interface = NULL; > + } else if (shseat->popup_grab.type == TOUCH) { > + > weston_touch_end_grab(shseat->popup_grab.touch_grab.touch); > + shseat->popup_grab.touch_grab.interface = NULL; > + } > } > } > > @@ -3126,7 +3251,10 @@ shell_map_popup(struct shell_surface *shsurf) > > if (shseat->seat->pointer && > shseat->seat->pointer->grab_serial == shsurf->popup.serial) { > - add_popup_grab(shsurf, shseat); > + add_popup_grab(shsurf, shseat, POINTER); > + } else if (shseat->seat->touch && > + shseat->seat->touch->grab_serial == shsurf->popup.serial) { > + add_popup_grab(shsurf, shseat, TOUCH); > } else { > shell_surface_send_popup_done(shsurf); > shseat->popup_grab.client = NULL; > @@ -4916,9 +5044,12 @@ idle_handler(struct wl_listener *listener, void *data) > container_of(listener, struct desktop_shell, idle_listener); > struct weston_seat *seat; > > - wl_list_for_each(seat, &shell->compositor->seat_list, link) > + wl_list_for_each(seat, &shell->compositor->seat_list, link) { > if (seat->pointer) > popup_grab_end(seat->pointer); > + if (seat->touch) > + touch_popup_grab_end(seat->touch); > + } > > shell_fade(shell, FADE_OUT); > /* lock() is called from shell_fade_done() */ About serials... input.c:notify_touch() bumps the touch->grab_serial... wait, no, it does not *bump* the serial, it simply fetches the current serial; in the first touch-down. So is it still possible to open a sub-menu using down-up touches instead of sliding? To see what functions actually bump the serial in wl_display when retrieving it, one can grep for wl_display_next_serial(). I don't really know the serial mechanism well enough, but I get the feeling that it is quite broken, not bumping and recording new serials when it should. The serial checking in shell_map_popup() is also suspicious, because it checks for equality, while any number of unrelated code paths may be bumping the serial in wl_display, and the grab functions read the serial value from wl_display to be sent to clients. So it might be possible, that a client is using a too *new* serial and gets its popup falsely rejected. Fixing all the serial stuff all over is not a matter for this patch, I'm just making the observation. For follow-up work, I could see things like: - rename popup_grab.grab to popup_grab.pointer_grab, so it goes nicely parallel to popup_grab.touch_grab - extract the common code from popup_grab_end() and touch_popup_grab_end(), as they are almost identical Do you want this patch to go in as is, and see about fixing stuff later? I'd be ok with that, since it is a new feature and looks like it should mostly work and not break anything (more). Thanks, pq _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel