[PATCH] compositor: Ensure views on hiden workspace are listening correct output

[Zhang, Xiong Y]

> @@ -406,7 +406,8 @@ weston_view_output_destroy_handler(struct wl_listener *listener,
>   					    struct weston_output, link);
>
>   		x = first_output->x + first_output->width / 4;
> -		y = first_output->y + first_output->height / 4;
> +		if (y > first_output->y)
> +			y = first_output->y + first_output->height / 4;
>   	}
> We should move this repositioning logic into the shell. I think the proper fix for the workspace case then would be to cause all the hidden surfaces to have their saved positions invalidated and then run the initial positioning algorithm when the user makes them visible by switching workspaces.

There is no saved positions for the views on hiden workspace. Shell change views->geometry.transformation_list to show or hide views in workspace_translate_in() and workspace_translate_out() function. We can't change hiden view's y coordinate in weston_view_output_destroy_handler(), otherwise this hiden view will never see again.

> weston_view_assign_output(struct weston_view *ev)
>   	}
>   	pixman_region32_fini(&region);
>
> -	if (ev->output_mask != 0) {
> +	if (!wl_list_empty(&ev->output_destroy_listener.link) && mask != 0) 
> +{
>   		wl_list_remove(&ev->output_move_listener.link);
>   		wl_list_remove(&ev->output_destroy_listener.link);
>   	}
>I sent a patch yesterday to fix a case where we couldn't just
>wl_list_remove() the listener. Do you still need the wl_list_empty() check after that?

You mean this patch:
24dff2b7 compositor:Clean up view output move and destroy listeners
If it is the above patch, I still need the wl_list_empty() check.
Even without output unplug, the list of output->destroy_signal is mess when we change workspace.
Consider the following situation:
1. the view is visible,  
  view is in the list of output->destroy_signal
2. Changing workspace, the view is invisible. 
   View is still in the output->destroy_signal with ev->output_mask = 0
3. Changing workspace, the view is visible again. 
   Because ev->output_mask is 0 after step 2,  we won't remove view from the list of output->destroy_signal and add view to the list of output->destroy_signal again in this step.

So from now on, the list of output->signal is mess. Once this output is unplugged, we will iterate this list,  if there are many views in the list, system will enter into deadloop to iterate this list again and again.

thanks
-----Original Message-----
From: Ander Conselvan de Oliveira [mailto:conselvan2 at gmail.com] 
Sent: Saturday, December 21, 2013 3:18 AM
To: Zhang, Xiong Y; Conselvan De Oliveira, Ander
Cc: wayland-devel at lists.freedesktop.org
Subject: Re: [PATCH] compositor: Ensure views on hiden workspace are listening correct output

On 12/19/2013 11:26 AM, Xiong Zhang wrote:
> The views on hiden workspace can't restored correctly when their 
> output was unplugged. There are two reasons:
>
> First:  the views on hiden workspace is restored through animation 
> which will change y coordinate continually, if the y coordinate of 
> invisible views are setted to first_output->height/4, these views will 
> be changed to some invisible region by animation.
>
> Second: the wrong way to manage view->output_destroy_listener.link.
> Considering the following serials:
> a. the view is visible, so the view listen one output signal b. the 
> view is invisible, view->output_mask = 0 and view is still listenering 
> the previous output signal c. the view is visible again. We will add 
> view to previous output signal again without removing the view from 
> the previous output signal. So the listener list of previous output's 
> signal is wrong at this point.When the output is unplgged, the system 
> will dead loop on the listener list of this output signal.
>
> Signed-off-by: Xiong Zhang <xiong.y.zhang at intel.com>
> ---
>   src/compositor.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/compositor.c b/src/compositor.c index 
> 6ca297a..9929be1 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -383,7 +383,7 @@ weston_view_output_destroy_handler(struct 
> wl_listener *listener,
>
>   	/* the child window's view->geometry is a relative coordinate to
>   	 * parent view, no need to move child_view. */
> -	if (ev->geometry.parent)
> +	if (ev->geometry.parent || wl_list_empty(&ec->output_list))
>   		return;
>
>   	x = ev->geometry.x;
> @@ -406,7 +406,8 @@ weston_view_output_destroy_handler(struct wl_listener *listener,
>   					    struct weston_output, link);
>
>   		x = first_output->x + first_output->width / 4;
> -		y = first_output->y + first_output->height / 4;
> +		if (y > first_output->y)
> +			y = first_output->y + first_output->height / 4;
>   	}
> We should move this repositioning logic into the shell. I think the proper fix for the workspace case then would be to cause all the hidden surfaces to have their saved positions invalidated and then run the initial positioning algorithm when the user makes them visible by switching workspaces.


>   	weston_view_set_position(ev, x, y); @@ -905,7 +906,7 @@ 
> weston_view_assign_output(struct weston_view *ev)
>   	}
>   	pixman_region32_fini(&region);
>
> -	if (ev->output_mask != 0) {
> +	if (!wl_list_empty(&ev->output_destroy_listener.link) && mask != 0) 
> +{
>   		wl_list_remove(&ev->output_move_listener.link);
>   		wl_list_remove(&ev->output_destroy_listener.link);
>   	}

I sent a patch yesterday to fix a case where we couldn't just
wl_list_remove() the listener. Do you still need the wl_list_empty() check after that?

Cheers,
Ander