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

[Derek Foreman]

On 22/03/15 09:32 PM, Jonas Ådahl wrote:
> 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.

Hmm, a confusing comment is worse than no comment at all.  removed.

>>  	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?

I thought it would look clearer my way, but must confess that either way
looks fine. :)

New version posted with changes made.

Thanks for taking a look!

> 
> 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