<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 5, 2015 at 5:11 AM, Daniel Stone <span dir="ltr"><<a href="mailto:daniel@fooishbar.org" target="_blank">daniel@fooishbar.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<span class=""><br>
On 5 February 2015 at 08:58, Jonas Ådahl <<a href="mailto:jadahl@gmail.com">jadahl@gmail.com</a>> wrote:<br>
> If a client calls xdg_shell.get_xdg_surface on a surface that is already<br>
> an xdg_surface wold, prior to this patch, succeed, but cause weston to<br>
> crash later when trying to configure. This patch instead sends a role<br>
> error to the client complaining that it already is an xdg_surface.<br>
><br>
> Note that .._set_role() only fails when changing roles, not when setting<br>
> the same role twice.<br>
<br>
</span>Yeah, that one was interesting to me. I wonder if there's any benefit<br>
to making it fail unconditionally if we try to re-set the same role -<br>
but that's a whole different patchset.<span class=""><br></span></blockquote><div><br></div><div>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.<br><br>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> The same is done for xdg_popup.<br>
><br>
> Signed-off-by: Jonas Ådahl <<a href="mailto:jadahl@gmail.com">jadahl@gmail.com</a>><br>
> ---<br>
>  desktop-shell/shell.c | 16 ++++++++++++++++<br>
>  1 file changed, 16 insertions(+)<br>
><br>
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c<br>
> index a7514f7..cc345b5 100644<br>
> --- a/desktop-shell/shell.c<br>
> +++ b/desktop-shell/shell.c<br>
> @@ -3959,6 +3959,14 @@ xdg_get_xdg_surface(struct wl_client *client,<br>
>         struct desktop_shell *shell = sc->shell;<br>
>         struct shell_surface *shsurf;<br>
><br>
> +       if ((shsurf = get_shell_surface(surface)) &&<br>
<br>
</span>Please assign shsurf in the declarations above - I hate assignments in<br>
if statements.<br>
<span class=""><br>
> +           shell_surface_is_xdg_surface(shsurf)) {<br>
> +               wl_resource_post_error(resource, XDG_SHELL_ERROR_ROLE,<br>
> +                                      "This wl_surface is already an "<br>
> +                                      "xdg_surface");<br>
> +               return;<br>
> +       }<br>
> +<br>
>         if (weston_surface_set_role(surface, "xdg_surface",<br>
>                                     resource, XDG_SHELL_ERROR_ROLE) < 0)<br>
>                 return;<br>
> @@ -4052,6 +4060,14 @@ xdg_get_xdg_popup(struct wl_client *client,<br>
>         struct weston_surface *parent;<br>
>         struct shell_seat *seat;<br>
><br>
> +       if ((shsurf = get_shell_surface(surface)) &&<br>
<br>
</span>Ditto.<br>
<br>
Ultimately I'd like to see weston_surface_set_role() deal with this<br>
and not have to have the split codepaths, but not for 1.7. With those<br>
two nitpicks fixed:<br>
Reviewed-by: Daniel Stone <<a href="mailto:daniels@collabora.com">daniels@collabora.com</a>><br>
<br>
Cheers,<br>
Daniel<br>
<div class="HOEnZb"><div class="h5">_______________________________________________<br>
wayland-devel mailing list<br>
<a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a><br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature">  Jasper<br></div>
</div></div>