[PATCH weston 06/10] xdg-shell: Send an error when the client uses the not-topmost popup
Jonas Ådahl
jadahl at gmail.com
Thu Feb 26 00:48:31 PST 2015
On Thu, Feb 26, 2015 at 09:59:21AM +0200, Pekka Paalanen wrote:
> 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?
No you are right. If a client mixes both wl_shell and xdg_shell, we'd be
in trouble.
>
> 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.
>
> 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
More information about the wayland-devel
mailing list