[PATCH] compositor: Fix pick an incorrect surface due to dirty surface_list

[wuzhiwen]

Hi Kristian,


>-----Original Message-----
>From: Kristian Hoegsberg [mailto:hoegsberg at gmail.com]
>Sent: Monday, March 12, 2012 4:23 AM
>To: zhiwen.wu at linux.intel.com
>Cc: wayland-devel at lists.freedesktop.org
>Subject: Re: [PATCH] compositor: Fix pick an incorrect surface due to dirty
>surface_list
>
>On Fri, Mar 09, 2012 at 11:57:36AM +0800, zhiwen.wu at linux.intel.com wrote:
>> From: Alex Wu <zhiwen.wu at linux.intel.com>
>>
>> When a new toplevel window is activated and positioned under the
>> cursor, the pointer focus still on the previous window. The botton
>> event can not deliver to the new window. The root cause is that
>> weston_surface_restack() change the surface stack but not update the
>> weston_compositor::surface_list, and then calling
>> weston_compositor_repick() picks an incorrect surface.
>
>Ah yes, nice catch.  The patch does go a bit in the other direction than
where
>I'd like to go with the layer code and input handling though.  I've been
trying to
>move most real work to the repaint loop and rather than rebuilding the
surface
>list in more places, I prefer to just move repicking (in case of restacking
or new
>or deleted
>surfaces) to the repaint loop also.

[Wu, Zhiwen] Glad to see that.
I think, there are other shell-specific things should be done in the repaint
loop after restack/new/delete surface.
e.g. display mode may change due to the top fullscreen surface switching to
non-fullscreen or being deleted.
Another case is that if a focused surface is deleted, shell/compositor may
move the focus to another
proper surface automatically. All these things are proper to be done in the
repaint loop. So how about adding a
vmethod in the struct weston_shell and calling it in repaint loop to handle
these things? 
This is really helpful to deal with the fullscreen stuff.

>
>What I'd like to do is something like the attached patch, but we're not
quite
>ready to do that yet, since the X backend still handles events whenever
they
>comes in.  That means we may hit notify_motion with an out-of-date surface
>list.  I think we can just move the X11 fd into the input_loop and that
should be
>ok, but I haven't had time to try it.
>
>Kristian
>
>
>> ---
>>  src/compositor.c |   25 ++++++++++++++++++-------
>>  1 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/compositor.c b/src/compositor.c index
>> 96e7fe7..2b84e4b 100644
>> --- a/src/compositor.c
>> +++ b/src/compositor.c
>> @@ -165,6 +165,20 @@ weston_client_launch(struct weston_compositor
>> *compositor,  }
>>
>>  static void
>> +weston_compositor_rebuild_surface_list(struct weston_compositor *ec)
>> +{
>> +	struct weston_layer *layer;
>> +	struct weston_surface *es;
>> +
>> +	wl_list_init(&ec->surface_list);
>> +	wl_list_for_each(layer, &ec->layer_list, link) {
>> +		wl_list_for_each(es, &layer->surface_list, layer_link) {
>> +			wl_list_insert(ec->surface_list.prev, &es->link);
>> +		}
>> +	}
>> +}
>> +
>> +static void
>>  surface_handle_buffer_destroy(struct wl_listener *listener,
>>  			      struct wl_resource *resource, uint32_t time)
{ @@
>-546,6
>> +560,7 @@ weston_compositor_pick_surface(struct weston_compositor
>> *compositor,  {
>>  	struct weston_surface *surface;
>>
>> +	weston_compositor_rebuild_surface_list(compositor);
>>  	wl_list_for_each(surface, &compositor->surface_list, link) {
>>  		weston_surface_from_global(surface, x, y, sx, sy);
>>  		if (pixman_region32_contains_point(&surface->input,
>> @@ -914,13 +929,9 @@ weston_output_repaint(struct weston_output
>*output, int msecs)
>>  	glViewport(0, 0, width, height);
>>
>>  	/* Rebuild the surface list and update surface transforms up front.
*/
>> -	wl_list_init(&ec->surface_list);
>> -	wl_list_for_each(layer, &ec->layer_list, link) {
>> -		wl_list_for_each(es, &layer->surface_list, layer_link) {
>> -			weston_surface_update_transform(es);
>> -			wl_list_insert(ec->surface_list.prev, &es->link);
>> -		}
>> -	}
>> +	weston_compositor_rebuild_surface_list(ec);
>> +	wl_list_for_each(es, &ec->surface_list, link)
>> +		weston_surface_update_transform(es);
>>
>>  	if (output->assign_planes)
>>  		/*
>> --
>> 1.7.5.4
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel