On Fri, 13 Feb 2015 14:01:58 +0800 Jonas Ådahl <[email protected]> wrote:
> From: "Jasper St. Pierre" <[email protected]> > > 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 _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
