[PATCH weston 8/14] ivi-shell: added libweston-desktop-api implementation

[Quentin Glidic]

Hi,

I will limit my review to the libweston-desktop usage, as it’s the only 
part I know.


On 10/17/17 11:59 AM, Michael Teyfel wrote:
> Signed-off-by: Michael Teyfel <mteyfel at de.adit-jv.com>
> ---
>   ivi-shell/ivi-shell.c | 172 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 172 insertions(+)
> 
> diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
> index 84db2c9..049aa43 100644
> --- a/ivi-shell/ivi-shell.c
> +++ b/ivi-shell/ivi-shell.c
> @@ -490,6 +490,178 @@ shell_add_bindings(struct weston_compositor *compositor,
>   }
>   
>   /*
> + * libweston-desktop
> + */
> +
> +static void
> +desktop_surface_ping_timeout(struct weston_desktop_client *client,
> +			     void *user_data)
> +{
> +	weston_log("ivi-shell: desktop_surface_ping_timeout is not supported\n");

You can just let these functions to NULL, if you want, as only 
surface_added and surface_removed are mandatory.


> +}
> +
> +static void
> +desktop_surface_pong(struct weston_desktop_client *client,
> +		     void *user_data)
> +{
> +	weston_log("ivi-shell: desktop_surface_pong is not supported\n");
> +}
> +
> +static void
> +desktop_surface_added(struct weston_desktop_surface *surface,
> +		      void *user_data)
> +{
> +	struct ivi_shell *shell = (struct ivi_shell *) user_data;
> +	struct ivi_layout_surface *layout_surface;
> +	struct ivi_shell_surface *ivisurf;
> +	struct weston_surface *weston_surf =
> +			weston_desktop_surface_get_surface(surface);
> +
> +	layout_surface = ivi_layout_desktop_surface_create(weston_surf,
> +						       IVI_INVALID_ID);
> +	if (!layout_surface) {
> +		return;
> +	}
> +
> +	layout_surface->weston_desktop_surface = surface;
> +
> +	ivisurf = zalloc(sizeof *ivisurf);
> +	if (!ivisurf) {
> +		return;
> +	}
> +
> +	wl_list_init(&ivisurf->link);
> +	wl_list_insert(&shell->ivi_surface_list, &ivisurf->link);
> +
> +	ivisurf->shell = shell;
> +	ivisurf->id_surface = IVI_INVALID_ID;
> +
> +	ivisurf->width = 0;
> +	ivisurf->height = 0;
> +	ivisurf->layout_surface = layout_surface;
> +	ivisurf->surface = weston_surf;

Here, you have a big issue. libweston-desktop handles popups internally, 
but to do that, it needs to know when you create a view for a toplevel 
surface. This is done via weston_desktop_surface_create_view(), which 
you never call (I traced the full call stack to ivi_view_create()).
One solution is to change ivi_view_create() to check for 
weston_surface_is_desktop_surface(), and calling 
weston_desktop_surface_create_view() instead of weston_view_create() in 
this case.


> +}
> +
> +static void
> +desktop_surface_removed(struct weston_desktop_surface *surface,
> +			void *user_data)
> +{
> +	struct ivi_shell *shell = (struct ivi_shell *) user_data;
> +	struct ivi_shell_surface *ivisurf = NULL;
> +
> +	wl_list_for_each(ivisurf, &shell->ivi_surface_list, link) {
> +		if(ivisurf->layout_surface->weston_desktop_surface == surface)

This loop (and the same in committed) are not needed. Just use 
weston_desktop_surface_set_user_data(surface, ivisurf); in surface_added 
and use get_user_data() in other callbacks. you can check for NULL since 
you have 1 code path that may let it NULL, but otherwise, surface_added 
is guaranteed to be call before any other callback.


Thanks,


> +		{
> +			assert(ivisurf != NULL);
> +
> +			if (ivisurf->layout_surface)
> +				layout_surface_cleanup(ivisurf);
> +
> +			break;
> +		}
> +	}
> +}
> +
> +static void
> +desktop_surface_committed(struct weston_desktop_surface *surface,
> +			  int32_t sx, int32_t sy, void *user_data)
> +{
> +	struct ivi_shell_surface *ivisurf = NULL;
> +	struct ivi_shell *shell = user_data;
> +	struct weston_surface *weston_surf =
> +			weston_desktop_surface_get_surface(surface);
> +	int found = 0;
> +
> +	wl_list_for_each(ivisurf, &shell->ivi_surface_list, link) {
> +		if (ivisurf->surface == weston_surf) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +
> +	if(!found)
> +		return;
> +
> +	if (weston_surf->width == 0 || weston_surf->height == 0)
> +		return;
> +
> +	if (ivisurf->width != weston_surf->width ||
> +	    ivisurf->height != weston_surf->height) {
> +		ivisurf->width  = weston_surf->width;
> +		ivisurf->height = weston_surf->height;
> +
> +		ivi_layout_desktop_surface_configure(ivisurf->layout_surface,
> +						 weston_surf->width,
> +						 weston_surf->height);
> +	}
> +}
> +
> +static void
> +desktop_surface_move(struct weston_desktop_surface *surface,
> +		     struct weston_seat *seat, uint32_t serial, void *user_data)
> +{
> +	weston_log("ivi-shell: desktop_surface_move is not supported\n");
> +}
> +
> +static void
> +desktop_surface_resize(struct weston_desktop_surface *surface,
> +		       struct weston_seat *seat, uint32_t serial,
> +		       enum weston_desktop_surface_edge edges, void *user_data)
> +{
> +	weston_log("ivi-shell: desktop_surface_resize is not supported\n");
> +}
> +
> +static void
> +desktop_surface_fullscreen_requested(struct weston_desktop_surface *surface,
> +				     bool fullscreen,
> +				     struct weston_output *output,
> +				     void *user_data)
> +{
> +	weston_log("ivi-shell: desktop_surface_fullscreen_requested is not supported\n");
> +}
> +
> +static void
> +desktop_surface_maximized_requested(struct weston_desktop_surface *surface,
> +				    bool maximized, void *user_data)
> +{
> +	weston_log("ivi-shell: desktop_surface_maximized_requested is not supported\n");
> +}
> +
> +static void
> +desktop_surface_minimized_requested(struct weston_desktop_surface *surface,
> +				    void *user_data)
> +{
> +	weston_log("ivi-shell: desktop_surface_minimized_requested is not supported\n");
> +}
> +
> +static void
> +desktop_surface_set_xwayland_position(struct weston_desktop_surface *surface,
> +				      int32_t x, int32_t y, void *user_data)
> +{
> +	weston_log("ivi-shell: desktop_surface_set_xwayland_position is not supported\n");
> +}
> +
> +static const struct weston_desktop_api shell_desktop_api = {
> +	.struct_size = sizeof(struct weston_desktop_api),
> +	.ping_timeout = desktop_surface_ping_timeout,
> +	.pong = desktop_surface_pong,
> +	.surface_added = desktop_surface_added,
> +	.surface_removed = desktop_surface_removed,
> +	.committed = desktop_surface_committed,
> +
> +	.move = desktop_surface_move,
> +	.resize = desktop_surface_resize,
> +	.fullscreen_requested = desktop_surface_fullscreen_requested,
> +	.maximized_requested = desktop_surface_maximized_requested,
> +	.minimized_requested = desktop_surface_minimized_requested,
> +	.set_xwayland_position = desktop_surface_set_xwayland_position,
> +};
> +
> +/*
> + * end of libweston-desktop
> + */
> +
> +/*
>    * Initialization of ivi-shell.
>    */
>   WL_EXPORT int
> 


-- 

Quentin “Sardem FF7” Glidic