[PATCH weston 1/3] compositor: Update input region when updating surface transfomations

[Pekka Paalanen]

On Wed, 14 Nov 2012 23:39:51 +0100
Jonas Ådahl <jadahl at gmail.com> wrote:

> If the compositor updated the geometry of a surface, the input region
> would still represent the original geometry. By intersecting the input
> region specified by the client, if any, by the new geometry when
> updating surface transformations the active input region can be kept up
> to date.

Did you hit this problem when a surface radically changes size in a
single step, leaving the input region obviously wrong? Or something
more subtle? That would be nice to mention.

> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
>  src/compositor.c |   23 +++++++++++++++++------
>  src/compositor.h |    1 +
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/src/compositor.c b/src/compositor.c
> index 83bd244..75c66ab 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -249,6 +249,7 @@ weston_surface_create(struct weston_compositor *compositor)
>  	pixman_region32_init(&surface->opaque);
>  	pixman_region32_init(&surface->clip);
>  	region_init_infinite(&surface->input);
> +	region_init_infinite(&surface->full_input);
>  	pixman_region32_init(&surface->transform.opaque);
>  	wl_list_init(&surface->frame_callback_list);
>  
> @@ -508,6 +509,17 @@ weston_surface_update_transform_enable(struct weston_surface *surface)
>  	return 0;
>  }
>  
> +static void
> +weston_surface_update_input_region(struct weston_surface *surface)
> +{
> +	pixman_region32_fini(&surface->input);
> +	pixman_region32_init_rect(&surface->input, 0, 0,
> +				  surface->geometry.width,
> +				  surface->geometry.height);
> +	pixman_region32_intersect(&surface->input,
> +				  &surface->input, &surface->full_input);
> +}
> +
>  WL_EXPORT void
>  weston_surface_update_transform(struct weston_surface *surface)
>  {
> @@ -522,6 +534,8 @@ weston_surface_update_transform(struct weston_surface *surface)
>  	pixman_region32_fini(&surface->transform.opaque);
>  	pixman_region32_init(&surface->transform.opaque);
>  
> +	weston_surface_update_input_region(surface);
> +
>  	/* transform.position is always in transformation_list */
>  	if (surface->geometry.transformation_list.next ==
>  	    &surface->transform.position.link &&
> @@ -791,6 +805,7 @@ destroy_surface(struct wl_resource *resource)
>  	pixman_region32_fini(&surface->opaque);
>  	pixman_region32_fini(&surface->clip);
>  	pixman_region32_fini(&surface->input);
> +	pixman_region32_fini(&surface->full_input);
>  
>  	wl_list_for_each_safe(cb, next, &surface->frame_callback_list, link)
>  		wl_resource_destroy(&cb->resource);
> @@ -1286,12 +1301,8 @@ surface_commit(struct wl_client *client, struct wl_resource *resource)
>  	pixman_region32_fini(&opaque);
>  
>  	/* wl_surface.set_input_region */
> -	pixman_region32_fini(&surface->input);
> -	pixman_region32_init_rect(&surface->input, 0, 0,
> -				  surface->geometry.width,
> -				  surface->geometry.height);
> -	pixman_region32_intersect(&surface->input,
> -				  &surface->input, &surface->pending.input);
> +	pixman_region32_copy(&surface->full_input, &surface->pending.input);
> +	weston_surface_update_input_region(surface);

Hi,

just wondering, do we actually want to call
weston_surface_update_input_region() here?

If we think about what's on screen, that won't change until repaint, so
wouldn't it be logical to actually update the input region only at
repaint? Yeah, this is a change from how it was before, but I think
this could be more correct.

If the server sends input to the client surface after
wl_surface.commit, but before repaint, then from user perspective the
input corresponds to the old surface contents. Hence, also clients
should be expecting input relative to the old contents until they
actually get the frame callback.

Or should we just not care, since even pointer motion triggers a
repaint, and it's practically impossible for a user to tell the
difference between (to react to the change, actually) whether the input
was for this or previous content?

Well... like I said, just wondering.


Thanks,
pq


>  	/* wl_surface.frame */
>  	wl_list_insert_list(&surface->frame_callback_list,
> diff --git a/src/compositor.h b/src/compositor.h
> index d2e121b..933831c 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -415,6 +415,7 @@ struct weston_surface {
>  	pixman_region32_t damage;
>  	pixman_region32_t opaque;
>  	pixman_region32_t input;
> +	pixman_region32_t full_input;
>  	int32_t pitch;
>  	struct wl_list link;
>  	struct wl_list layer_link;