On Thu, Feb 05, 2015 at 01:11:54PM +0000, Daniel Stone wrote: > Hi, > > On 5 February 2015 at 08:58, Jonas Ådahl <[email protected]> wrote: > > If a client calls xdg_shell.get_xdg_surface on a surface that is already > > an xdg_surface wold, prior to this patch, succeed, but cause weston to > > crash later when trying to configure. This patch instead sends a role > > error to the client complaining that it already is an xdg_surface. > > > > Note that .._set_role() only fails when changing roles, not when setting > > the same role twice. > > Yeah, that one was interesting to me. I wonder if there's any benefit > to making it fail unconditionally if we try to re-set the same role - > but that's a whole different patchset.
Yeah, I considered that, but I suspected the pointer cursor and dnd surfaces might not like that. As you said, can maybe look into that after 1.7. Jonas > > > The same is done for xdg_popup. > > > > Signed-off-by: Jonas Ådahl <[email protected]> > > --- > > desktop-shell/shell.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c > > index a7514f7..cc345b5 100644 > > --- a/desktop-shell/shell.c > > +++ b/desktop-shell/shell.c > > @@ -3959,6 +3959,14 @@ xdg_get_xdg_surface(struct wl_client *client, > > struct desktop_shell *shell = sc->shell; > > struct shell_surface *shsurf; > > > > + if ((shsurf = get_shell_surface(surface)) && > > Please assign shsurf in the declarations above - I hate assignments in > if statements. > > > + shell_surface_is_xdg_surface(shsurf)) { > > + wl_resource_post_error(resource, XDG_SHELL_ERROR_ROLE, > > + "This wl_surface is already an " > > + "xdg_surface"); > > + return; > > + } > > + > > if (weston_surface_set_role(surface, "xdg_surface", > > resource, XDG_SHELL_ERROR_ROLE) < 0) > > return; > > @@ -4052,6 +4060,14 @@ xdg_get_xdg_popup(struct wl_client *client, > > struct weston_surface *parent; > > struct shell_seat *seat; > > > > + if ((shsurf = get_shell_surface(surface)) && > > Ditto. > > Ultimately I'd like to see weston_surface_set_role() deal with this > and not have to have the split codepaths, but not for 1.7. With those > two nitpicks fixed: > Reviewed-by: Daniel Stone <[email protected]> > > Cheers, > Daniel _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
