[PATCH weston v1 01/17] tests: simplify move_client and make it more generic

[Pekka Paalanen]

Hi Artie,

you wrote the initial test plugin code in commit
65e7e7a65bf17f7b886653bfee850ff728bbd904. Would you happen to remember
why the move_surface request requires a configure call (i.e. attach &
commit of wl_surface) to apply the move?

Was it perhaps to test that the configure hook gets called, or was
there a deeper meaning?

More inline below.

On Fri,  5 Dec 2014 14:36:34 +0100
Marek Chalupa <mchqwerty at gmail.com> wrote:

> Move client right in the move_client funciton. This allows the surface
> use its own configure function, so from now the client can be any
> weston surface (xdg, wl_shell, ..)
> 
> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> ---
>  tests/weston-test.c | 62 +++++++++++++++++++++--------------------------------
>  1 file changed, 24 insertions(+), 38 deletions(-)
> 
> diff --git a/tests/weston-test.c b/tests/weston-test.c
> index 77eaa23..17c93f5 100644
> --- a/tests/weston-test.c
> +++ b/tests/weston-test.c
> @@ -41,13 +41,6 @@ struct weston_test {
>  	struct weston_process process;
>  };
>  
> -struct weston_test_surface {
> -	struct weston_surface *surface;
> -	struct weston_view *view;
> -	int32_t x, y;
> -	struct weston_test *test;
> -};
> -
>  static void
>  test_client_sigchld(struct weston_process *process, int status)
>  {
> @@ -88,19 +81,15 @@ notify_pointer_position(struct weston_test *test, struct wl_resource *resource)
>  }
>  
>  static void
> -test_surface_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
> +update_position(struct weston_test *test, struct weston_view *view,
> +		int32_t x, int32_t y)
>  {
> -	struct weston_test_surface *test_surface = surface->configure_private;
> -	struct weston_test *test = test_surface->test;
> -
> -	if (wl_list_empty(&test_surface->view->layer_link.link))
> +	if (wl_list_empty(&view->layer_link.link))
>  		weston_layer_entry_insert(&test->layer.view_list,
> -					  &test_surface->view->layer_link);
> +					  &view->layer_link);
>  
> -	weston_view_set_position(test_surface->view,
> -				 test_surface->x, test_surface->y);
> -
> -	weston_view_update_transform(test_surface->view);
> +	weston_view_set_position(view, x, y);
> +	weston_view_update_transform(view);

Marek, I think after this change, wl_test.move_surface might be enough
to cause the surface to be mapped. I don't think that's right. If we
want to test wl_shell and xdg_shell surfaces, we need to let the real
shell code do the mapping.

Test surfaces used to be an exception, because the real shell would not
map them, the test plugin would.

But all this means that wl_test.move_surface would be something that
will work behind the shell's back, and they may fight. This means that
we probably need to recheck all the protocol sequences in the tests.

If you move_surface before the surface becomes mapped, the shell will
overwrite the position. We need to always move after map, but maybe
that's not that hard after all.

>  }
>  
>  static void
> @@ -110,31 +99,28 @@ move_surface(struct wl_client *client, struct wl_resource *resource,
>  {
>  	struct weston_surface *surface =
>  		wl_resource_get_user_data(surface_resource);
> -	struct weston_test_surface *test_surface;
> -
> -	test_surface = surface->configure_private;
> -	if (!test_surface) {
> -		test_surface = malloc(sizeof *test_surface);
> -		if (!test_surface) {
> -			wl_resource_post_no_memory(resource);
> -			return;
> -		}
> +	struct weston_test *test = wl_resource_get_user_data(resource);
> +	struct weston_view *view;
>  
> -		test_surface->view = weston_view_create(surface);
> -		if (!test_surface->view) {
> -			wl_resource_post_no_memory(resource);
> -			free(test_surface);
> -			return;
> +	/* has shell surface? */
> +	if (surface->configure) {
> +		view = wl_container_of(surface->views.next, view, surface_link);
> +		assert (view && "No view in surface views list");
> +	} else {
> +		if (!surface->configure_private) {
> +			view = weston_view_create(surface);
> +			if (!view) {
> +				wl_resource_post_no_memory(resource);
> +				return;
> +			}
> +
> +			surface->configure_private = view;
> +		} else {
> +			view = surface->configure_private;
>  		}

This is pretty subtle. If the surface doesn't have the configure hook
assigned, you make it a test surface. I'd prefer if making a test
surface (role) would be more explicit, and move would be just move
always.

Would we even need to force the position of real-shell controlled
surfaces at all? We just need to know where it went, no?

Tests might want it to be deterministic, but that's a slightly
different thing.

> -
> -		surface->configure_private = test_surface;
> -		surface->configure = test_surface_configure;
>  	}
>  
> -	test_surface->surface = surface;
> -	test_surface->test = wl_resource_get_user_data(resource);
> -	test_surface->x = x;
> -	test_surface->y = y;
> +	update_position(test, view, x, y);
>  }
>  
>  static void

I think this patches causes 'make check BACKEND=x11-backend.so' to
stop waiting for something in the subsurface tests. This happens only
if the pointer happens to end up in the Weston window. Moving the
pointer lets the tests continue.

It is somewhat known, that moving the pointer while running check on
the x11 backend might cause failures.


Thanks,
pq