[PATCH weston 06/10] xdg-shell: Send an error when the client uses the not-topmost popup

[Pekka Paalanen]

On Fri, 13 Feb 2015 14:01:58 +0800
Jonas Ådahl <jadahl at gmail.com> wrote:

> From: "Jasper St. Pierre" <jstpierre at mecheye.net>
> 
> Either in destroy or get_xdg_popup.
> 
> [jadahl: Verify that the new popup is the top most when mapping instead
> of creating. Some renaming.]
> ---
>  desktop-shell/shell.c  | 58 +++++++++++++++++++++++++++++++++++++++++++++-----
>  protocol/xdg-shell.xml |  7 ++++++
>  2 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 4990c4d..6697222 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -3370,12 +3370,40 @@ touch_popup_grab_end(struct weston_touch *touch)
>  	}
>  }
>  
> -static void
> +static struct shell_surface *
> +get_top_xdg_popup(struct shell_seat *shseat)
> +{
> +	struct shell_surface *shsurf;
> +
> +	if (wl_list_empty(&shseat->popup_grab.surfaces_list)) {
> +		return NULL;
> +	} else {
> +		shsurf = container_of(shseat->popup_grab.surfaces_list.next,
> +				      struct shell_surface,
> +				      popup.grab_link);
> +		assert(shell_surface_is_xdg_popup(shsurf));

Hi,

are you sure this assert() does not trigger when a client is using
wl_shell popups?

It looks like if a client tried to open a wl_shell popup while one is
already open, this would explode.

map() -> shell_map_popup() -> add_popup_grab() -> get_top_xdg_popup()

Am I missing something?

Maybe the fix is as simple as just checking for both wl_shell and
xdg_shell popup types and accepting either?

Wonder if we have any wl_shell apps that would do sub-menus...

> +		return shsurf;
> +	}
> +}
> +
> +static int
>  add_popup_grab(struct shell_surface *shsurf,
>  	       struct shell_seat *shseat,
>  	       int32_t type)
>  {
>  	struct weston_seat *seat = shseat->seat;
> +	struct shell_surface *parent, *top_surface;
> +
> +	parent = get_shell_surface(shsurf->parent);
> +	top_surface = get_top_xdg_popup(shseat);
> +	if ((top_surface == NULL && !shell_surface_is_xdg_surface(parent)) ||
> +	    (top_surface != NULL && parent != top_surface)) {
> +		wl_resource_post_error(shsurf->owner->resource,
> +				       XDG_POPUP_ERROR_NOT_THE_TOPMOST_POPUP,
> +				       "xdg_popup was not created on the "
> +				       "topmost popup");
> +		return -1;
> +	}
>  
>  	if (wl_list_empty(&shseat->popup_grab.surfaces_list)) {
>  		shseat->popup_grab.type = type;
> @@ -3410,6 +3438,8 @@ add_popup_grab(struct shell_surface *shsurf,
>  		wl_list_insert(&shseat->popup_grab.surfaces_list,
>  			       &shsurf->popup.grab_link);
>  	}
> +
> +	return 0;
>  }
>  
>  static void
> @@ -3417,6 +3447,15 @@ remove_popup_grab(struct shell_surface *shsurf)
>  {
>  	struct shell_seat *shseat = shsurf->popup.shseat;
>  
> +	if (shell_surface_is_xdg_popup(shsurf) &&
> +	    get_top_xdg_popup(shseat) != shsurf) {
> +		wl_resource_post_error(shsurf->resource,
> +				       XDG_POPUP_ERROR_NOT_THE_TOPMOST_POPUP,
> +				       "xdg_popup was destroyed while it was "
> +				       "not the topmost popup.");

Hmm, I'm confused. What does "top-most popup" mean?

If there is a chain of popups because of sub-menus:

surface -> popup A -> popup B

Then which popup is the top-most? The root of the popup hierarchy or
the most recently created popup?

It seems add_popup_grab() pushes the new popup to the head of the
popup_grab.surfaces_list, so top-most means the most recently mapped
popup, right? So that's popup B in the example.

You can ignore this question if the documentation rewrite explains
"top-most".

> +		return;
> +	}
> +
>  	wl_list_remove(&shsurf->popup.grab_link);
>  	wl_list_init(&shsurf->popup.grab_link);
>  	if (wl_list_empty(&shseat->popup_grab.surfaces_list)) {
> @@ -3430,7 +3469,7 @@ remove_popup_grab(struct shell_surface *shsurf)
>  	}
>  }
>  
> -static void
> +static int
>  shell_map_popup(struct shell_surface *shsurf)
>  {
>  	struct shell_seat *shseat = shsurf->popup.shseat;
> @@ -3445,14 +3484,18 @@ shell_map_popup(struct shell_surface *shsurf)
>  
>  	if (shseat->seat->pointer &&
>  	    shseat->seat->pointer->grab_serial == shsurf->popup.serial) {
> -		add_popup_grab(shsurf, shseat, POINTER);
> +		if (add_popup_grab(shsurf, shseat, POINTER) != 0)
> +			return -1;
>  	} else if (shseat->seat->touch &&
>  	           shseat->seat->touch->grab_serial == shsurf->popup.serial) {
> -		add_popup_grab(shsurf, shseat, TOUCH);
> +		if (add_popup_grab(shsurf, shseat, TOUCH) != 0)
> +			return -1;
>  	} else {
>  		shell_surface_send_popup_done(shsurf);
>  		shseat->popup_grab.client = NULL;
>  	}
> +
> +	return 0;
>  }
>  
>  static const struct wl_shell_surface_interface shell_surface_implementation = {
> @@ -4051,6 +4094,10 @@ create_xdg_popup(struct shell_client *owner, void *shell,
>  {
>  	struct shell_surface *shsurf;
>  
> +	/* Verify that we are creating the top most popup when mapping,
> +	 * as its not until then we know whether it was mapped as most
> +	 * top level or not. */

Ok!

> +
>  	shsurf = create_common_surface(owner, shell, surface, client);
>  	if (!shsurf)
>  		return NULL;
> @@ -5451,7 +5498,8 @@ map(struct desktop_shell *shell, struct shell_surface *shsurf,
>  		}
>  		break;
>  	case SHELL_SURFACE_POPUP:
> -		shell_map_popup(shsurf);
> +		if (shell_map_popup(shsurf) != 0)
> +			return;
>  		break;
>  	case SHELL_SURFACE_NONE:
>  		weston_view_set_position(shsurf->view,
> diff --git a/protocol/xdg-shell.xml b/protocol/xdg-shell.xml
> index 4414d46..360179d 100644
> --- a/protocol/xdg-shell.xml
> +++ b/protocol/xdg-shell.xml
> @@ -398,6 +398,13 @@
>        xdg_popup surfaces are always transient for another surface.
>      </description>
>  
> +    <enum name="error">
> +      <description summary="xdg_popup error values">
> +	These errors can be emitted in response to xdg_popup requests.
> +      </description>
> +      <entry name="not_the_topmost_popup" value="0" summary="The client tried to destroy a non-toplevel popup"/>

What is a toplevel popup? Did you mean top-most?

> +    </enum>
> +
>      <request name="destroy" type="destructor">
>        <description summary="remove xdg_surface interface">
>  	The xdg_surface interface is removed from the wl_surface object

Otherwise looks good to me.


Thanks,
pq