On Thu, Feb 5, 2015 at 5:11 AM, Daniel Stone <[email protected]> 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. > Well, "set the role" means "call xdg_shell.get_xdg_surface". If you have a wl_surface, to map it, you call xdg_shell.get_xdg_surface, and to unmap it, you destroy the xdg_surface. If you'd rather that we fully destroy the wl_surface and create it anew, we can do that too, but both Pekka and I preferred this option. This also helps simplify things like cursors, where you get the cursor role by calling wl_pointer.set_cursor, and requiring a new wl_surface every time you want to set the I-beam or arrow cursor seems wasteful. > > 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 > -- Jasper
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
