On Tue,  7 Apr 2015 17:01:19 +0800
Jonas Ådahl <[email protected]> wrote:

> They are errors that may be as a result of calling get_xdg_popup on an
> xdg_shell, not a result of calling a request on xdg_popup.
> 
> Signed-off-by: Jonas Ådahl <[email protected]>

Yes, this is alright, and also not a reason to bump the experimental
version.

But...

> ---
>  desktop-shell/shell.c  | 37 +++++++++++++++++++------------------
>  protocol/xdg-shell.xml | 10 ++--------
>  2 files changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 778c120..847e2d8 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -3400,8 +3400,8 @@ add_popup_grab(struct shell_surface *shsurf,
>       if (shell_surface_is_xdg_popup(shsurf) &&
>           ((top_surface == NULL && !shell_surface_is_xdg_surface(parent)) ||
>            (top_surface != NULL && parent != top_surface))) {
> -             wl_resource_post_error(shsurf->owner->resource,
> -                                    XDG_POPUP_ERROR_NOT_THE_TOPMOST_POPUP,
> +             wl_resource_post_error(shsurf->owner_resource,
> +                                    XDG_SHELL_ERROR_NOT_THE_TOPMOST_POPUP,
>                                      "xdg_popup was not created on the "
>                                      "topmost popup");
>               return -1;
> @@ -3451,8 +3451,8 @@ remove_popup_grab(struct shell_surface *shsurf)
>  
>       if (shell_surface_is_xdg_popup(shsurf) &&
>           get_top_popup(shseat) != shsurf) {
> -             wl_resource_post_error(shsurf->resource,
> -                                    XDG_POPUP_ERROR_NOT_THE_TOPMOST_POPUP,
> +             wl_resource_post_error(shsurf->owner_resource,
> +                                    XDG_SHELL_ERROR_NOT_THE_TOPMOST_POPUP,
>                                      "xdg_popup was destroyed while it was "
>                                      "not the topmost popup.");
>               return;
> @@ -4121,20 +4121,7 @@ create_xdg_popup(struct shell_client *owner, void 
> *shell,
>                uint32_t serial,
>                int32_t x, int32_t y)
>  {
> -     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
> -      * top level or not. */
> -
> -     parent_shsurf = get_shell_surface(parent);
> -     if (!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,
> -                                    "xdg_popup parent was invalid");
> -             return NULL;
> -     }
> +     struct shell_surface *shsurf;
>  
>       shsurf = create_common_surface(owner, shell, surface, client);
>       if (!shsurf)
> @@ -4165,6 +4152,7 @@ xdg_get_xdg_popup(struct wl_client *client,
>       struct shell_client *sc = wl_resource_get_user_data(resource);
>       struct desktop_shell *shell = sc->shell;
>       struct shell_surface *shsurf;
> +     struct shell_surface *parent_shsurf;
>       struct weston_surface *parent;
>       struct shell_seat *seat;
>  
> @@ -4190,6 +4178,19 @@ xdg_get_xdg_popup(struct wl_client *client,
>       parent = wl_resource_get_user_data(parent_resource);
>       seat = get_shell_seat(wl_resource_get_user_data(seat_resource));;
>  
> +     /* Verify that we are creating the top most popup when mapping,
> +      * as its not until then we know whether it was mapped as most
> +      * top level or not. */
> +
> +     parent_shsurf = get_shell_surface(parent);
> +     if (!shell_surface_is_xdg_popup(parent_shsurf) &&
> +         !shell_surface_is_xdg_surface(parent_shsurf)) {
> +             wl_resource_post_error(resource,
> +                                    XDG_SHELL_ERROR_INVALID_POPUP_PARENT,
> +                                    "xdg_popup parent was invalid");
> +             return;
> +     }
> +

Ahh, first I was wondering that this move was about, but looking
closer, the only caller of create_xdg_popup() is xdg_get_xdg_popup().
To this is basically just a no-op refactoring.

>       shsurf = create_xdg_popup(sc, shell, surface, &xdg_popup_client,
>                                 parent, seat, serial, x, y);
>       if (!shsurf) {
> diff --git a/protocol/xdg-shell.xml b/protocol/xdg-shell.xml
> index 9dbf193..866bd85 100644
> --- a/protocol/xdg-shell.xml
> +++ b/protocol/xdg-shell.xml
> @@ -50,6 +50,8 @@
>      <enum name="error">
>        <entry name="role" value="0" summary="given wl_surface has another 
> role"/>
>        <entry name="defunct_surfaces" value="1" summary="xdg_shell was 
> destroyed before children"/>
> +      <entry name="not_the_topmost_popup" value="2" summary="the client 
> tried to map or destroy a non-topmost popup"/>
> +      <entry name="invalid_popup_parent" value="3" summary="the client 
> specified an invalid popup parent surface"/>
>      </enum>
>  
>      <request name="destroy" type="destructor">
> @@ -469,14 +471,6 @@
>        parent 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 map or destroy a non-topmost popup"/>
> -      <entry name="invalid_parent" value="1" summary="The client specified 
> an invalid parent surface"/>
> -    </enum>
> -
>      <request name="destroy" type="destructor">
>        <description summary="remove xdg_popup interface">
>       This destroys the popup. Explicitly destroying the xdg_popup

Reviewed-by: Pekka Paalanen <[email protected]>

Thanks,
pq
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to