[PATCH weston 06/10] xdg-shell: Send an error when the client uses the not-topmost popup
Jasper St. Pierre
jstpierre at mecheye.net
Thu Feb 26 00:52:36 PST 2015
On Thu, Feb 26, 2015 at 12:48 AM, Jonas Ådahl <jadahl at gmail.com> 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 <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.
>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150226/704515ec/attachment-0001.html>
More information about the wayland-devel
mailing list