[PATCH weston 06/10] xdg-shell: Send an error when the client uses the not-topmost popup

[Jonas Ådahl]

On Thu, Feb 26, 2015 at 11:33:48AM +0200, Pekka Paalanen wrote:
> On Thu, 26 Feb 2015 00:52:36 -0800
> "Jasper St. Pierre" <jstpierre at mecheye.net> wrote:
> 
> > 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.
> 
> Yes, this is exactly the case I was worrying about.
> 
> I don't think we need to make sure mixing wl_shell and xdg_shell works.
> IMHO, if a client makes a top-level via wl_shell or xdg_shell,
> then it better use the same shell interfaces for everything that
> touches that top-level. But I wouldn't go as far as enforcing that in
> the code.
> 
> I do care about wl_shell working alone.

The v2 patch I sent should fix these issues. We can't mix wl_shell and
xdg_shell in one popup chain, as xdg_shell doesn't allow that, and that
we should (and do) enforce (more properly in v2).


Jonas

> 
> 
> > > > >  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.
> 
> I'm fine with the word, as long as there is a doc that explains it
> somewhere. It does describe the situation correctly from z-order
> perspective.
> 
> > > > You can ignore this question if the documentation rewrite explains
> > > > "top-most".
> 
> > > > > --- 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
> > :)
> 
> Cool.
> 
> 
> Thanks,
> pq