[PATCH weston] smoke: Don't commit an xdg_surface with a NULL buffer

[Pekka Paalanen]

On Sun, 12 Oct 2014 18:57:31 -0700
"Jasper St. Pierre" <jstpierre at mecheye.net> wrote:

> Committing to an xdg_surface with a NULL buffer is currently illegal in
> the mutter implementation, so this simply causes the client to error and
> exit.

The patch is good, but I'd like to reiterate on what this actually
fixes.

AFAICS, neither toytoolkit nor smoke ever actually does a
wl_surface(attach, NULL, x, y). If you do a WAYLAND_DEBUG=client dump
from 'smoke' without this patch, there is not a single attach with a
null buffer.

However, there is a sequence in the beginning:

[1995969.194]  -> wl_compositor at 3.create_surface(new id wl_surface at 14)
[1995969.214]  -> xdg_shell at 11.get_xdg_surface(new id xdg_surface at 15, wl_surface at 14)
[1995969.227]  -> xdg_surface at 15.set_title("smoke")
[1995969.254]  -> wl_compositor at 3.create_region(new id wl_region at 16)
[1995969.523]  -> wl_shm at 6.create_pool(new id wl_shm_pool at 17, fd 10, 160000)
[1995969.546]  -> wl_shm_pool at 17.create_buffer(new id wl_buffer at 18, 0, 200, 200, 800, 0)
[1995969.571]  -> wl_surface at 14.frame(new id wl_callback at 19)
[1995985.307]  -> wl_surface at 14.damage(0, 0, 200, 200)
[1995985.339]  -> wl_surface at 14.frame(new id wl_callback at 20)
[1995985.348]  -> wl_surface at 14.commit()

There is a commit of damage without ever attaching a buffer. That is
quite useless indeed, especially if asking for frame events, since the
surface won't be mapped without content, and so the frame callback
won't be replied until a buffer actually is attached and committed.

xdg_surface specification should probably say something about
committing surfaces without content, and what removing content from a
wl_surface does in the context of xdg_shell.

> It seems the reason the client did this was so it could add its own
> frame callback, but toytoolkit actually provides accurate everything we
> need. Just use its functions instead to get the time and schedule a
> redraw.

Yup, pushed. This patch fixes the strange init sequence.


Thanks,
pq


> ---
>  clients/smoke.c | 24 ++----------------------
>  1 file changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/clients/smoke.c b/clients/smoke.c
> index 65b6e03..7f0d5e5 100644
> --- a/clients/smoke.c
> +++ b/clients/smoke.c
> @@ -39,7 +39,6 @@ struct smoke {
>  	struct widget *widget;
>  	int width, height;
>  	int current;
> -	uint32_t time;
>  	struct { float *d, *u, *v; } b[2];
>  };
>  
> @@ -171,27 +170,10 @@ static void render(struct smoke *smoke, cairo_surface_t *surface)
>  }
>  
>  static void
> -frame_callback(void *data, struct wl_callback *callback, uint32_t time)
> -{
> -	struct smoke *smoke = data;
> -
> -	window_schedule_redraw(smoke->window);
> -	smoke->time = time;
> -
> -	if (callback)
> -		wl_callback_destroy(callback);
> -}
> -
> -static const struct wl_callback_listener listener = {
> -	frame_callback,
> -};
> -
> -static void
>  redraw_handler(struct widget *widget, void *data)
>  {
>  	struct smoke *smoke = data;
> -	uint32_t time = smoke->time;
> -	struct wl_callback *callback;
> +	uint32_t time = widget_get_last_time(smoke->widget);
>  	cairo_surface_t *surface;
>  
>  	diffuse(smoke, time / 30, smoke->b[0].u, smoke->b[1].u);
> @@ -222,9 +204,7 @@ redraw_handler(struct widget *widget, void *data)
>  
>  	cairo_surface_destroy(surface);
>  
> -	callback = wl_surface_frame(window_get_wl_surface(smoke->window));
> -	wl_callback_add_listener(callback, &listener, smoke);
> -	wl_surface_commit(window_get_wl_surface(smoke->window));
> +	widget_schedule_redraw(smoke->widget);
>  }
>  
>  static void