[weston,4/6] xdg-shell: Send an error when the client uses the not-topmost popup

Submitted by Jasper St. Pierre on Nov. 22, 2014, 8:28 p.m.

Details

Message ID 1416688110-21188-5-git-send-email-jstpierre@mecheye.net
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Jasper St. Pierre Nov. 22, 2014, 8:28 p.m.
Either in destroy or get_xdg_popup.
---
 desktop-shell/shell.c  | 21 ++++++++++++++++++++-
 protocol/xdg-shell.xml |  7 +++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index 7650884..9a69657 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -224,6 +224,7 @@  struct shell_seat {
 	struct {
 		struct weston_pointer_grab grab;
 		struct weston_touch_grab touch_grab;
+		struct shell_surface *top_surface;
 		struct wl_list surfaces_list;
 		struct wl_client *client;
 		int32_t initial_up;
@@ -3231,6 +3232,7 @@  popup_grab_end(struct weston_pointer *pointer)
 		}
 		wl_list_init(&prev->popup.grab_link);
 		wl_list_init(&shseat->popup_grab.surfaces_list);
+		shseat->popup_grab.top_surface = NULL;
 	}
 }
 
@@ -3291,6 +3293,8 @@  add_popup_grab(struct shell_surface *shsurf, struct shell_seat *shseat, int32_t
 	} else {
 		wl_list_insert(&shseat->popup_grab.surfaces_list, &shsurf->popup.grab_link);
 	}
+
+	shseat->popup_grab.top_surface = shsurf;
 }
 
 static void
@@ -3298,6 +3302,13 @@  remove_popup_grab(struct shell_surface *shsurf)
 {
 	struct shell_seat *shseat = shsurf->popup.shseat;
 
+	if (shell_surface_is_xdg_popup(shsurf) && shseat->popup_grab.top_surface != 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.");
+		return;
+	}
+
 	wl_list_remove(&shsurf->popup.grab_link);
 	wl_list_init(&shsurf->popup.grab_link);
 	if (wl_list_empty(&shseat->popup_grab.surfaces_list)) {
@@ -3945,7 +3956,15 @@  create_xdg_popup(struct shell_client *owner, void *shell,
 		 uint32_t serial,
 		 int32_t x, int32_t y)
 {
-	struct shell_surface *shsurf;
+	struct shell_surface *shsurf, *parent_shsurf;
+
+	parent_shsurf = get_shell_surface(parent);
+	if (seat->popup_grab.top_surface != NULL && seat->popup_grab.top_surface != parent_shsurf) {
+		wl_resource_post_error(owner->resource,
+				       XDG_POPUP_ERROR_NOT_THE_TOPMOST_POPUP,
+				       "xdg_popup was not created on the topmost popup");
+		return NULL;
+	}
 
 	shsurf = create_common_surface(owner, shell, surface, client);
 	if (!shsurf)
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"/>
+    </enum>
+
     <request name="destroy" type="destructor">
       <description summary="remove xdg_surface interface">
 	The xdg_surface interface is removed from the wl_surface object

Comments

On Sat, 22 Nov 2014 12:28:28 -0800
"Jasper St. Pierre" <jstpierre@mecheye.net> wrote:

> Either in destroy or get_xdg_popup.
> ---
>  desktop-shell/shell.c  | 21 ++++++++++++++++++++-
>  protocol/xdg-shell.xml |  7 +++++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 7650884..9a69657 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -224,6 +224,7 @@ struct shell_seat {
>  	struct {
>  		struct weston_pointer_grab grab;
>  		struct weston_touch_grab touch_grab;
> +		struct shell_surface *top_surface;

So 'top_surface' is per shell_seat...

You intend to support nested menus (popups), will these be an xdg_popup
each? I assume they are and the (new) docs seem to say so.

>  		struct wl_list surfaces_list;
>  		struct wl_client *client;
>  		int32_t initial_up;
> @@ -3231,6 +3232,7 @@ popup_grab_end(struct weston_pointer *pointer)
>  		}
>  		wl_list_init(&prev->popup.grab_link);
>  		wl_list_init(&shseat->popup_grab.surfaces_list);
> +		shseat->popup_grab.top_surface = NULL;

...shouldn't this restore the previous top_surface rather than NULL?
That is, shouldn't top_surface act like a stack?


Thanks,
pq


>  	}
>  }
>  
> @@ -3291,6 +3293,8 @@ add_popup_grab(struct shell_surface *shsurf, struct shell_seat *shseat, int32_t
>  	} else {
>  		wl_list_insert(&shseat->popup_grab.surfaces_list, &shsurf->popup.grab_link);
>  	}
> +
> +	shseat->popup_grab.top_surface = shsurf;
>  }
>  
>  static void
> @@ -3298,6 +3302,13 @@ remove_popup_grab(struct shell_surface *shsurf)
>  {
>  	struct shell_seat *shseat = shsurf->popup.shseat;
>  
> +	if (shell_surface_is_xdg_popup(shsurf) && shseat->popup_grab.top_surface != 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.");
> +		return;
> +	}
> +
>  	wl_list_remove(&shsurf->popup.grab_link);
>  	wl_list_init(&shsurf->popup.grab_link);
>  	if (wl_list_empty(&shseat->popup_grab.surfaces_list)) {
> @@ -3945,7 +3956,15 @@ create_xdg_popup(struct shell_client *owner, void *shell,
>  		 uint32_t serial,
>  		 int32_t x, int32_t y)
>  {
> -	struct shell_surface *shsurf;
> +	struct shell_surface *shsurf, *parent_shsurf;
> +
> +	parent_shsurf = get_shell_surface(parent);
> +	if (seat->popup_grab.top_surface != NULL && seat->popup_grab.top_surface != parent_shsurf) {
> +		wl_resource_post_error(owner->resource,
> +				       XDG_POPUP_ERROR_NOT_THE_TOPMOST_POPUP,
> +				       "xdg_popup was not created on the topmost popup");
> +		return NULL;
> +	}
>  
>  	shsurf = create_common_surface(owner, shell, surface, client);
>  	if (!shsurf)
> 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"/>
> +    </enum>
> +
>      <request name="destroy" type="destructor">
>        <description summary="remove xdg_surface interface">
>  	The xdg_surface interface is removed from the wl_surface object
Are you sure this supports a client replacing the top popup with a 
different one? It seems to set the top to null rather than the next one 
down when one is removed. I would also be concerned that it may be 
difficult for a client to insure that the previous popup is destroyed 
before the new one is created, some possible toolkit designs may like to 
do all the creation first, then the destruction, to allow reuse of 
shared data.