On Thu, Feb 26, 2015 at 12:48 AM, Jonas Ådahl <[email protected]> wrote:
> On Thu, Feb 26, 2015 at 09:59:21AM +0200, Pekka Paalanen wrote: > > 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? > > No you are right. If a client mixes both wl_shell and xdg_shell, we'd be > in trouble. > 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. > > > > 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 > > > > > 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. > "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. > > 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? > Whoops. The "not_the_topmost" should probably clue you in that, yes, I did :) > > > + </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 > -- Jasper
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
