[PATCH v2] shell: grab the parent popup when a sub popup window is deleted or hidden

[Pekka Paalanen]

On Thu, 28 Feb 2013 21:50:48 +0100
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:

> If the client opens a popup menu and submenu, when it closes or hides the
> submenu the pointer grab should return to the parent menu.
> Furthermore, when clicking outside the client area the popup_done event
> is sent to the active popup surface and recursively to its parent.
> ---
>  src/shell.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/src/shell.c b/src/shell.c
> index 3da5321..2353bab 100644
> --- a/src/shell.c
> +++ b/src/shell.c
> @@ -194,6 +194,7 @@ struct shell_surface {
>  		int32_t initial_up;
>  		struct wl_seat *seat;
>  		uint32_t serial;
> +		struct shell_surface *prev;

Hi,

I didn't really review this patch, but as you are storing a pointer to
another shell_surface, is there anything that will reset this pointer
or destroy this shell_surface, if the other shell_surface gets
destroyed? Like if a client goes haywire, and destroys the wl_surface
of the parent popup? A client doing so may not make any sense, but we
still need to protect weston from it.

We likely already have other oversights in object lifetime and
reference handling in shell.c.


Thanks,
pq

>  	} popup;
>  
>  	struct {
> @@ -1894,11 +1895,20 @@ popup_grab_end(struct wl_pointer *pointer)
>  	struct wl_pointer_grab *grab = pointer->grab;
>  	struct shell_surface *shsurf =
>  		container_of(grab, struct shell_surface, popup.grab);
> +	struct shell_surface *tmp;
>  
>  	if (pointer->grab->interface == &popup_grab_interface) {
> -		wl_shell_surface_send_popup_done(&shsurf->resource);
>  		wl_pointer_end_grab(grab->pointer);
> -		shsurf->popup.grab.pointer = NULL;
> +		/* Send the popup_done event to all the popups open, and not just
> +		 * the one that currently has the grab. */
> +		while (shsurf && shsurf->popup.grab.pointer) {
> +			wl_shell_surface_send_popup_done(&shsurf->resource);
> +			shsurf->popup.grab.pointer = NULL;
> +
> +			tmp = shsurf;
> +			shsurf = shsurf->popup.prev;
> +			tmp->popup.prev = NULL;
> +		}
>  	}
>  }
>  
> @@ -1908,6 +1918,7 @@ shell_map_popup(struct shell_surface *shsurf)
>  	struct wl_seat *seat = shsurf->popup.seat;
>  	struct weston_surface *es = shsurf->surface;
>  	struct weston_surface *parent = shsurf->parent;
> +	struct shell_surface *popup_parent;
>  
>  	/* Remove the old transform. We don't want to add it twice
>  	 * otherwise Weston will go into an infinite loop when going
> @@ -1941,9 +1952,25 @@ shell_map_popup(struct shell_surface *shsurf)
>  	weston_surface_set_position(es, shsurf->popup.x, shsurf->popup.y);
>  	weston_surface_update_transform(es);
>  
> +	if (seat->pointer->grab->interface == &popup_grab_interface)
> +		popup_parent = container_of(seat->pointer->grab,
> +				      struct shell_surface, popup.grab);
> +	else
> +		popup_parent = NULL;
> +
>  	/* We don't require the grab to still be active, but if another
>  	 * grab has started in the meantime, we end the popup now. */
> -	if (seat->pointer->grab_serial == shsurf->popup.serial) {
> +
> +	/* Need to make sure here that a sub-popup doesn't trigger the
> +	 * send_popup_done case.  And we need to set the popup.prev
> +	 * pointer.  We can look at seat->pointer->grab.interface to
> +	 * see if it's &popup_grab_interface and if it is use
> +	 * container_of on seat->pointer->grab to get back to the
> +	 * shsurf. */
> +
> +	if (seat->pointer->grab_serial == shsurf->popup.serial ||
> +		(popup_parent && popup_parent->resource.client == shsurf->resource.client)) {
> +		shsurf->popup.prev = popup_parent;
>  		wl_pointer_start_grab(seat->pointer, &shsurf->popup.grab);
>  	} else {
>  		wl_shell_surface_send_popup_done(&shsurf->resource);
> @@ -1982,10 +2009,24 @@ static const struct wl_shell_surface_interface shell_surface_implementation = {
>  };
>  
>  static void
> +grab_parent_popup(struct shell_surface *shsurf)
> +{
> +	wl_pointer_end_grab(shsurf->popup.grab.pointer);
> +	shsurf->popup.grab.pointer = NULL;
> +	struct shell_surface *parent = shsurf->popup.prev;
> +	if (parent) {
> +		wl_pointer_start_grab(parent->popup.seat->pointer, &parent->popup.grab);
> +		shsurf->popup.prev = NULL;
> +	}
> +}
> +
> +static void
>  destroy_shell_surface(struct shell_surface *shsurf)
>  {
> -	if (shsurf->popup.grab.pointer)
> -		wl_pointer_end_grab(shsurf->popup.grab.pointer);
> +	/* If this surface has a parent popup, grab it. */
> +	if (shsurf->popup.grab.pointer) {
> +		grab_parent_popup(shsurf);
> +	}
>  
>  	if (shsurf->fullscreen.type == WL_SHELL_SURFACE_FULLSCREEN_METHOD_DRIVER &&
>  	    shell_surface_is_top_fullscreen(shsurf)) {
> @@ -3189,6 +3230,12 @@ shell_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32
>  
>  	int type_changed = 0;
>  
> +	/* If this surface has just been unmapped and we have a parent popup,
> +	 * grab it. */
> +	if (!weston_surface_is_mapped(es) && shsurf->popup.grab.pointer) {
> +		grab_parent_popup(shsurf);
> +	}
> +
>  	if (width == 0)
>  		return;
>