<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Thu, Feb 26, 2015 at 12:48 AM, Jonas Ådahl <<a href="mailto:jadahl@gmail.com" target="_blank">jadahl@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Feb 26, 2015 at 09:59:21AM +0200, Pekka Paalanen wrote: > On Fri, 13 Feb 2015 14:01:58 +0800 > Jonas Ådahl <<a href="mailto:jadahl@gmail.com">jadahl@gmail.com</a>> wrote: > > > From: "Jasper St. Pierre" <<a href="mailto:jstpierre@mecheye.net">jstpierre@mecheye.net</a>> > > > > 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? </div></div>No you are right. If a client mixes both wl_shell and xdg_shell, we'd be in trouble. </blockquote><div> </div><div>If a client simply maps two wl_shell popups, then we're also in trouble, since get_top_xdg_popup is called unconditionally by add_popup_grab, which is also called from set_popup. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > Maybe the fix is as simple as just checking for both wl_shell and > xdg_shell popup types and accepting either? No, we also need to move the top most verification inside a if (is_xdg_popup) block, so we don't put unintended restrictions on wl_shell popups. I'll send a v2 that renames back the getter to get_top_popup, removes the assert and puts the verification in add_popup_grab() inside an if. Jonas <div class="HOEnZb"><div class="h5"> > > 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. </div></div></blockquote><div> </div><div>"Topmost" means the one that's on top of all the others, so, popup B. If you have a better word, feel free to say so! It was the best I could think of. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> > 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? </div></div></blockquote><div> </div><div>Whoops. The "not_the_topmost" should probably clue you in that, yes, I did :) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> > > + </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 </div></div></blockquote></div> -- <div class="gmail_signature"> Jasper </div> </div></div>