Sure, I'll remove it then. (I was going to remove it originally - but figured, it was useful for testing since it exposed this bug.)
On Wed, Apr 8, 2015 at 7:00 AM, Pekka Paalanen <[email protected]> wrote: > On Mon, 6 Apr 2015 13:27:23 -0700 > Dima Ryazanov <[email protected]> wrote: > > > Yeah, the logic is pretty sketchy now - "if it's a shell surface, do the > > error checking; otherwise, do nothing" - but I don't understand the code > > well enough to know if this is the expected behavior. > > > > Should the panel just be a shell surface? Then we could require that the > > popup's parent is a shell surface and simplify the error check. > > No, that won't work. "panel" is a special surface role, it cannot also > be a shell_surface. > > I suppose the answer to this whole problem is to remove the whole panel > popup thing. It never had anything useful, right? > > > Thanks, > pq > > > On Mon, Apr 6, 2015 at 12:33 PM, Derek Foreman <[email protected]> > > wrote: > > > > > This bug was introduced in commits da6ecd0cc52 and 24185e2561 > > > > > > I guess nobody right clicked on the panel for over a month. :) > > > > > > I've CC'd Jasper and Jonas in case they haven't noticed this yet... > > > > > > On 06/04/15 01:52 AM, Dima Ryazanov wrote: > > > > It looks like the error-checking code assumes the popup's parent is > > > > a shell surface - but that's not always the case. > > > > > > > > Signed-off-by: Dima Ryazanov <[email protected]> > > > > --- > > > > desktop-shell/shell.c | 9 +++++---- > > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c > > > > index f7c928e..0159547 100644 > > > > --- a/desktop-shell/shell.c > > > > +++ b/desktop-shell/shell.c > > > > @@ -3392,7 +3392,7 @@ add_popup_grab(struct shell_surface *shsurf, > > > > > > > > parent = get_shell_surface(shsurf->parent); > > > > top_surface = get_top_popup(shseat); > > > > - if (shell_surface_is_xdg_popup(shsurf) && > > > > + if (parent && shell_surface_is_xdg_popup(shsurf) && > > > > > > This part broke in da6ecd0cc52 > > > > > > Your patch definitely stops the crash for me, which is good. :) > > > > > > I think this part's ok, but I'm not 100% sure we're still going to send > > > the new not-top-level-popup error when we're supposed to now... > > > > > > > ((top_surface == NULL && > > > !shell_surface_is_xdg_surface(parent)) || > > > > (top_surface != NULL && parent != top_surface))) { > > > > wl_resource_post_error(shsurf->owner->resource, > > > > @@ -4098,12 +4098,13 @@ create_xdg_popup(struct shell_client *owner, > > > void *shell, > > > > { > > > > struct shell_surface *shsurf, *parent_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 > > > > + /* Verify that we are creating the topmost popup when mapping, > > > > + * as it's not until then we know whether it was mapped as most > > > > * top level or not. */ > > > > > > The grammar fix-ups look good to me. > > > > > > > parent_shsurf = get_shell_surface(parent); > > > > - if (!shell_surface_is_xdg_popup(parent_shsurf) && > > > > + if (parent_shsurf && > > > > > > This part broke in 24185e2561... > > > > > > I wonder if we should be doing a more comprehensive test here - it > looks > > > like some invalid parent resources could get past this test? > > > > > > > + !shell_surface_is_xdg_popup(parent_shsurf) && > > > > !shell_surface_is_xdg_surface(parent_shsurf)) { > > > > wl_resource_post_error(owner->resource, > > > > XDG_POPUP_ERROR_INVALID_PARENT, > > > > > > > > > > > > > > >
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
