[PATCH v5] compositor: call configure on surfaces with a null buffer too
Pekka Paalanen
ppaalanen at gmail.com
Thu Feb 21 01:30:23 PST 2013
On Wed, 20 Feb 2013 16:28:48 +0100
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> This way the shell can know when a surface has been unmapped by
> checking the value returned by weston_surface_is_mapped(surface).
> The configure handlers have now width and height parameters, so
> they do not need anymore to check manually the buffer size.
> If a surface's buffer is NULL the width and height passed to the
> configure are both 0.
> Configure is now only called after an attach. The variable
> weston_surface.pending.newly_attached is set to 1 on attach, and
> after the configure call is reset to 0.
> ---
> src/compositor.c | 37 ++++++++++++++++++++++++-------------
> src/compositor.h | 4 ++--
> src/shell.c | 48 +++++++++++++++++++++++++++++-------------------
> src/tablet-shell.c | 8 ++------
> tests/weston-test.c | 6 +++---
> 5 files changed, 60 insertions(+), 43 deletions(-)
>
> diff --git a/src/compositor.c b/src/compositor.c
> index 64d0830..4f3c443 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -290,6 +290,7 @@ weston_surface_create(struct weston_compositor *compositor)
> surface->pending.buffer_transform = surface->buffer_transform;
> surface->output = NULL;
> surface->plane = &compositor->primary_plane;
> + surface->pending.newly_attached = 0;
>
> pixman_region32_init(&surface->damage);
> pixman_region32_init(&surface->opaque);
> @@ -1273,12 +1274,10 @@ surface_attach(struct wl_client *client,
> surface->pending.sx = sx;
> surface->pending.sy = sy;
> surface->pending.buffer = buffer;
> + surface->pending.newly_attached = 1;
> if (buffer) {
> wl_signal_add(&buffer->resource.destroy_signal,
> &surface->pending.buffer_destroy_listener);
> - surface->pending.remove_contents = 0;
> - } else {
> - surface->pending.remove_contents = 1;
> }
> }
>
> @@ -1391,6 +1390,8 @@ surface_commit(struct wl_client *client, struct wl_resource *resource)
> {
> struct weston_surface *surface = resource->data;
> pixman_region32_t opaque;
> + int buffer_width = 0;
> + int buffer_height = 0;
>
> if (surface->pending.sx || surface->pending.sy ||
> (surface->pending.buffer &&
> @@ -1401,14 +1402,22 @@ surface_commit(struct wl_client *client, struct wl_resource *resource)
> surface->buffer_transform = surface->pending.buffer_transform;
>
> /* wl_surface.attach */
> - if (surface->pending.buffer || surface->pending.remove_contents)
> + if (surface->pending.buffer ||
> + (surface->pending.newly_attached && surface->pending.buffer == NULL))
I think you can drop the "&& surface->pending.buffer == NULL".
> weston_surface_attach(surface, surface->pending.buffer);
>
> - if (surface->buffer_ref.buffer && surface->configure)
> + if (surface->buffer_ref.buffer) {
> + buffer_width = weston_surface_buffer_width(surface);
> + buffer_height = weston_surface_buffer_height(surface);
> + }
> +
> + if (surface->configure && surface->pending.newly_attached)
> surface->configure(surface, surface->pending.sx,
> - surface->pending.sy);
> + surface->pending.sy, buffer_width, buffer_height);
> +
> surface->pending.sx = 0;
> surface->pending.sy = 0;
> + surface->pending.newly_attached = 0;
Yes, the logic looks just like I would expect, nice.
>
> /* wl_surface.damage */
> pixman_region32_union(&surface->damage, &surface->damage,
> @@ -2144,11 +2153,14 @@ pointer_handle_sprite_destroy(struct wl_listener *listener, void *data)
>
> static void
> pointer_cursor_surface_configure(struct weston_surface *es,
> - int32_t dx, int32_t dy)
> + int32_t dx, int32_t dy, int32_t width, int32_t height)
> {
> struct weston_seat *seat = es->private;
> int x, y;
>
> + if (width == 0)
> + return;
Hmm, I wonder, would it actually fix a bug if we did not test for
width==0, but instead just executed this?
I recall that before this patch, configure was not called, if a client
explicitly attached a NULL buffer, right? That means we would just
ignore dx,dy, and that is fine since they usually are zero anyway.
However, would it not be more correct to actually handle dx,dy
regardless?
Anyway, don't change that in this patch, it should be a separate patch
that just removes the width==0 check, if we want it. This patch should
just keep all existing logic as close as possible to what it was.
> +
> assert(es == seat->sprite);
>
> seat->hotspot_x -= dx;
> @@ -2158,8 +2170,7 @@ pointer_cursor_surface_configure(struct weston_surface *es,
> y = wl_fixed_to_int(seat->seat.pointer->y) - seat->hotspot_y;
>
> weston_surface_configure(seat->sprite, x, y,
> - es->buffer_ref.buffer->width,
> - es->buffer_ref.buffer->height);
> + width, height);
>
> empty_region(&es->pending.input);
>
> @@ -2226,7 +2237,8 @@ pointer_set_cursor(struct wl_client *client, struct wl_resource *resource,
> seat->hotspot_y = y;
>
> if (surface->buffer_ref.buffer)
> - pointer_cursor_surface_configure(surface, 0, 0);
> + pointer_cursor_surface_configure(surface, 0, 0, weston_surface_buffer_width(surface),
> + weston_surface_buffer_height(surface));
> }
>
> static const struct wl_pointer_interface pointer_interface = {
> @@ -2604,14 +2616,13 @@ weston_seat_release(struct weston_seat *seat)
> }
>
> static void
> -drag_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy)
> +drag_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height)
> {
> empty_region(&es->pending.input);
>
> weston_surface_configure(es,
> es->geometry.x + sx, es->geometry.y + sy,
> - es->buffer_ref.buffer->width,
> - es->buffer_ref.buffer->height);
> + width, height);
> }
>
> static int
> diff --git a/src/compositor.h b/src/compositor.h
> index fdde762..03d1031 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -460,7 +460,7 @@ struct weston_surface {
> /* All the pending state, that wl_surface.commit will apply. */
> struct {
> /* wl_surface.attach */
> - int remove_contents;
> + int newly_attached;
Yes! This is the change I have wanted to do for some time now. :-)
> struct wl_buffer *buffer;
> struct wl_listener buffer_destroy_listener;
> int32_t sx;
> @@ -487,7 +487,7 @@ struct weston_surface {
> * a new buffer has been set up for this surface. The integer params
> * are the sx and sy paramerters supplied to surface::attach .
> */
> - void (*configure)(struct weston_surface *es, int32_t sx, int32_t sy);
> + void (*configure)(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height);
> void *private;
> };
>
> diff --git a/src/shell.c b/src/shell.c
> index af802a5..e355aa0 100644
> --- a/src/shell.c
> +++ b/src/shell.c
> @@ -1627,7 +1627,7 @@ shell_surface_set_maximized(struct wl_client *client,
> }
>
> static void
> -black_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy);
> +black_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height);
>
> static struct weston_surface *
> create_black_surface(struct weston_compositor *ec,
> @@ -2023,7 +2023,7 @@ shell_handle_surface_destroy(struct wl_listener *listener, void *data)
> }
>
> static void
> -shell_surface_configure(struct weston_surface *, int32_t, int32_t);
> +shell_surface_configure(struct weston_surface *, int32_t, int32_t, int32_t, int32_t);
>
> static struct shell_surface *
> get_shell_surface(struct weston_surface *surface)
> @@ -2172,10 +2172,13 @@ terminate_screensaver(struct desktop_shell *shell)
> }
>
> static void
> -configure_static_surface(struct weston_surface *es, struct weston_layer *layer)
> +configure_static_surface(struct weston_surface *es, struct weston_layer *layer, int32_t width, int32_t height)
> {
> struct weston_surface *s, *next;
>
> + if (width == 0)
> + return;
> +
> wl_list_for_each_safe(s, next, &layer->surface_list, layer_link) {
> if (s->output == es->output && s != es) {
> weston_surface_unmap(s);
> @@ -2183,9 +2186,7 @@ configure_static_surface(struct weston_surface *es, struct weston_layer *layer)
> }
> }
>
> - weston_surface_configure(es, es->output->x, es->output->y,
> - weston_surface_buffer_width(es),
> - weston_surface_buffer_height(es));
> + weston_surface_configure(es, es->output->x, es->output->y, width, height);
>
> if (wl_list_empty(&es->layer_link)) {
> wl_list_insert(&layer->surface_list, &es->layer_link);
> @@ -2194,11 +2195,11 @@ configure_static_surface(struct weston_surface *es, struct weston_layer *layer)
> }
>
> static void
> -background_configure(struct weston_surface *es, int32_t sx, int32_t sy)
> +background_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height)
> {
> struct desktop_shell *shell = es->private;
>
> - configure_static_surface(es, &shell->background_layer);
> + configure_static_surface(es, &shell->background_layer, width, height);
> }
>
> static void
> @@ -2227,11 +2228,11 @@ desktop_shell_set_background(struct wl_client *client,
> }
>
> static void
> -panel_configure(struct weston_surface *es, int32_t sx, int32_t sy)
> +panel_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height)
> {
> struct desktop_shell *shell = es->private;
>
> - configure_static_surface(es, &shell->panel_layer);
> + configure_static_surface(es, &shell->panel_layer, width, height);
> }
>
> static void
> @@ -2260,10 +2261,13 @@ desktop_shell_set_panel(struct wl_client *client,
> }
>
> static void
> -lock_surface_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
> +lock_surface_configure(struct weston_surface *surface, int32_t sx, int32_t sy, int32_t width, int32_t height)
> {
> struct desktop_shell *shell = surface->private;
>
> + if (width == 0)
> + return;
> +
> center_on_output(surface, get_default_output(shell->compositor));
>
> if (!weston_surface_is_mapped(surface)) {
> @@ -2718,7 +2722,7 @@ activate(struct desktop_shell *shell, struct weston_surface *es,
>
> /* no-op func for checking black surface */
> static void
> -black_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy)
> +black_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height)
> {
> }
>
> @@ -3084,14 +3088,16 @@ configure(struct desktop_shell *shell, struct weston_surface *surface,
> }
>
> static void
> -shell_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy)
> +shell_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height)
> {
> struct shell_surface *shsurf = get_shell_surface(es);
> struct desktop_shell *shell = shsurf->shell;
> - int32_t width = weston_surface_buffer_width(es);
> - int32_t height = weston_surface_buffer_height(es);
> +
> int type_changed = 0;
>
> + if (width == 0)
> + return;
> +
> if (shsurf->next_type != SHELL_SURFACE_NONE &&
> shsurf->type != shsurf->next_type) {
> set_surface_type(shsurf);
> @@ -3204,10 +3210,13 @@ bind_desktop_shell(struct wl_client *client,
> }
>
> static void
> -screensaver_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
> +screensaver_configure(struct weston_surface *surface, int32_t sx, int32_t sy, int32_t width, int32_t height)
> {
> struct desktop_shell *shell = surface->private;
>
> + if (width == 0)
> + return;
> +
> /* XXX: starting weston-screensaver beforehand does not work */
> if (!shell->locked)
> return;
> @@ -3275,13 +3284,14 @@ bind_screensaver(struct wl_client *client,
> }
>
> static void
> -input_panel_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
> +input_panel_configure(struct weston_surface *surface, int32_t sx, int32_t sy, int32_t width, int32_t height)
> {
> struct weston_mode *mode;
> - int32_t width = weston_surface_buffer_width(surface);
> - int32_t height = weston_surface_buffer_height(surface);
> float x, y;
>
> + if (width == 0)
> + return;
> +
> if (!weston_surface_is_mapped(surface))
> return;
>
> diff --git a/src/tablet-shell.c b/src/tablet-shell.c
> index 9e88e27..66b1a44 100644
> --- a/src/tablet-shell.c
> +++ b/src/tablet-shell.c
> @@ -124,17 +124,13 @@ tablet_shell_set_state(struct tablet_shell *shell, int state)
>
> static void
> tablet_shell_surface_configure(struct weston_surface *surface,
> - int32_t sx, int32_t sy)
> + int32_t sx, int32_t sy, int32_t width, int32_t height)
> {
> struct tablet_shell *shell = get_shell(surface->compositor);
> - int32_t width, height;
>
> - if (weston_surface_is_mapped(surface))
> + if (weston_surface_is_mapped(surface) || width == 0)
> return;
>
> - width = surface->buffer_ref.buffer->width;
> - height = surface->buffer_ref.buffer->height;
Heh, I think that actually fixes a bug in there.
> -
> weston_surface_configure(surface, 0, 0, width, height);
>
> if (surface == shell->lockscreen_surface) {
> diff --git a/tests/weston-test.c b/tests/weston-test.c
> index a9def4b..7655d55 100644
> --- a/tests/weston-test.c
> +++ b/tests/weston-test.c
> @@ -74,7 +74,7 @@ notify_pointer_position(struct weston_test *test, struct wl_resource *resource)
> }
>
> static void
> -test_surface_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
> +test_surface_configure(struct weston_surface *surface, int32_t sx, int32_t sy, int32_t width, int32_t height)
> {
> struct weston_test_surface *test_surface = surface->private;
> struct weston_test *test = test_surface->test;
> @@ -85,8 +85,8 @@ test_surface_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
>
> surface->geometry.x = test_surface->x;
> surface->geometry.y = test_surface->y;
> - surface->geometry.width = surface->buffer_ref.buffer->width;
> - surface->geometry.height = surface->buffer_ref.buffer->height;
> + surface->geometry.width = width;
> + surface->geometry.height = height;
Ooh, and here, this one was missing the transformed buffer handling,
too, but now it looks like it's fixed.
> surface->geometry.dirty = 1;
>
> if (!weston_surface_is_mapped(surface))
Good job! Only one tiny nitpick in the whole patch. :-)
I didn't try to run it, but it looks fine.
Thanks,
pq
More information about the wayland-devel
mailing list