[PATCH weston] input: Don't recreate the cursor sprite when only the hotspot changes

[Jonas Ådahl]

On Fri, Mar 20, 2015 at 04:44:41PM -0500, Derek Foreman wrote:
> Currently we unmap and re-map the cursor when the hotspot changes which
> causes spurious enter/leave events.
> 
> This changes the pointer_set_cursor() logic to avoid this.
> 
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
>  src/input.c | 45 +++++++++++++++++++++++++++++----------------
>  1 file changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/src/input.c b/src/input.c
> index 469d5ce..a24ae1a 100644
> --- a/src/input.c
> +++ b/src/input.c
> @@ -1632,6 +1632,7 @@ pointer_set_cursor(struct wl_client *client, struct wl_resource *resource,
>  {
>  	struct weston_pointer *pointer = wl_resource_get_user_data(resource);
>  	struct weston_surface *surface = NULL;
> +	bool full_set = true;
>  
>  	if (surface_resource)
>  		surface = wl_resource_get_user_data(surface_resource);
> @@ -1648,31 +1649,43 @@ pointer_set_cursor(struct wl_client *client, struct wl_resource *resource,
>  	if (pointer->focus_serial - serial > UINT32_MAX / 2)
>  		return;
>  
> -	if (surface) {
> -		if (weston_surface_set_role(surface, "wl_pointer-cursor",
> -					    resource,
> -					    WL_POINTER_ERROR_ROLE) < 0)
> -			return;
> +	if (!surface) {
> +		if (pointer->sprite)
> +			pointer_unmap_sprite(pointer);
> +		return;
>  	}
>  
> +	/* Nothing changed.  Unmapping and recreating the cursor sprite
> +	 * generates extra enter and leave events, so we avoid it if
> +	 * at all possible.
> +	 */

I think this comments mostly adds confusion. When I read it (without
reading the code), it sounds like we have came to a place where there is
no state change. It'd be better with something like "Avoid unmapping and
recreating if no state was changed." if you think the condition itself
is not enough documentation.

>  	if (pointer->sprite && pointer->sprite->surface == surface &&
>  	    pointer->hotspot_x == x && pointer->hotspot_y == y)
>  		return;
>  
> -	if (pointer->sprite)
> -		pointer_unmap_sprite(pointer);
> +	/* Only the hotspot changed. */
> +	if (pointer->sprite && pointer->sprite->surface == surface)
> +		full_set = false;
>  
> -	if (!surface)
> -		return;
> +	if (full_set) {

You only ever set 'full_set' to false in the statement above this, so
why not just do:

	if (!pointer->sprite || pointer->sprite->surface != surface) {
		if (weston_surface_set_role ...)

		...
	}

and drop the 'full_set' variable?


Jonas

> +		if (weston_surface_set_role(surface, "wl_pointer-cursor",
> +					    resource,
> +					    WL_POINTER_ERROR_ROLE) < 0)
> +			return;
>  
> -	wl_signal_add(&surface->destroy_signal,
> -		      &pointer->sprite_destroy_listener);
> +		if (pointer->sprite)
> +			pointer_unmap_sprite(pointer);
> +
> +		wl_signal_add(&surface->destroy_signal,
> +			      &pointer->sprite_destroy_listener);
> +
> +		surface->configure = pointer_cursor_surface_configure;
> +		surface->configure_private = pointer;
> +		weston_surface_set_label_func(surface,
> +					    pointer_cursor_surface_get_label);
> +		pointer->sprite = weston_view_create(surface);
> +	}
>  
> -	surface->configure = pointer_cursor_surface_configure;
> -	surface->configure_private = pointer;
> -	weston_surface_set_label_func(surface,
> -				      pointer_cursor_surface_get_label);
> -	pointer->sprite = weston_view_create(surface);
>  	pointer->hotspot_x = x;
>  	pointer->hotspot_y = y;
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel