[PATCH weston 5/8] tests: Support setting the test client input dynamically

[Alexandros Frantzis]

On Thu, Feb 01, 2018 at 12:20:44PM +0200, Pekka Paalanen wrote:
> On Fri, 26 Jan 2018 18:47:59 +0200
> Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> 
> > The current test client code waits for all wl_seat globals to arrive
> > before checking them and deciding which one is the test seat global to
> > use for the input object. This method doesn't support dynamic addition
> > of the test seat global (i.e., after client start-up), which will be
> > needed in upcoming commits.
> > 
> > This commit changes the code to check for the test seat and set up the
> > input object while handling the wl_seat information events.
> > 
> > Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> > ---
> >  tests/weston-test-client-helper.c | 78 ++++++++++++++++++++++-----------------
> >  tests/weston-test-client-helper.h |  1 +
> >  2 files changed, 46 insertions(+), 33 deletions(-)
> 
> Hi,
> 
> I feel this patch might be doing more than what it says on the tin:
> - move input_update_devices() call from client_set_input() into the
>   event handlers
> - remove the check for all seats must have a name
> 
> I suppose justifying these in the commit message would be enough in
> this case.
> 
<snip>

Hi Pekka,

I have been thinking a bit more about the direction that we might want
this code to move toward.

One thought was to provide struct input objects for all seats (perhaps
also rename struct input -> struct seat), and provide a
client_get_seat_with_name(name) helper function, or even
client_get_test_seat() for extra convenience. I think this would keep
the code relatively simple and also general enough for future use (e.g.
test multiple seats).

What do you think?

Thanks,
Alexandros

> > diff --git a/tests/weston-test-client-helper.c b/tests/weston-test-client-helper.c
> > index 6e0a5246..854978d0 100644
> > --- a/tests/weston-test-client-helper.c
> > +++ b/tests/weston-test-client-helper.c
> > @@ -538,6 +538,14 @@ static const struct weston_test_listener test_listener = {
> >  	test_handle_capture_screenshot_done,
> >  };
> >  
> > +static void
> > +input_destroy(struct input *inp)
> > +{
> > +	wl_list_remove(&inp->link);
> > +	wl_seat_destroy(inp->wl_seat);
> > +	free(inp);
> > +}
> > +
> >  static void
> >  input_update_devices(struct input *input)
> >  {
> > @@ -598,22 +606,56 @@ seat_handle_capabilities(void *data, struct wl_seat *seat,
> >  
> >  	/* we will create/update the devices only with the right (test) seat.
> >  	 * If we haven't discovered which seat is the test seat, just
> > -	 * store capabilities and bail out */
> > -	if (input->seat_name && strcmp(input->seat_name, "test-seat") == 0)
> > +	 * store capabilities and bail out. Note that we don't need to
> > +	 * check the name contents here, since only the test seat input will
> > +	 * have its name set */
> > +	if (input->seat_name) {
> >  		input_update_devices(input);
> 
> In fact, this bit of old code was not actually necessary, because
> client_set_input() would have done the right thing anyway. Your patch
> fixes this "imbalance" by making both event handlers call
> input_update_devices().
> 
> > +		input->client->input = input;
> > +	}
> > +
> >  
> >  	fprintf(stderr, "test-client: got seat %p capabilities: %x\n",
> >  		input, caps);
> >  }
> >  
> > +static struct input *
> > +client_find_input_with_seat_name(struct client *client, const char *name)
> > +{
> 
> This function seems unnecessary, because...
> 
> > +	struct input *input;
> > +
> > +	wl_list_for_each(input, &client->inputs, link) {
> > +		if (input->seat_name && strcmp(input->seat_name, name) == 0)
> > +			return input;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> >  static void
> >  seat_handle_name(void *data, struct wl_seat *seat, const char *name)
> >  {
> >  	struct input *input = data;
> >  
> > +	/* We don't care about seats other than the test seat */
> > +	if (strcmp(name, "test-seat") != 0) {
> > +		input_destroy(input);
> > +		return;
> > +	}
> > +
> > +	assert(!client_find_input_with_seat_name(input->client, name) &&
> > +	       "Multiple test seats detected!");
> 
> ..this test could as well be done...
> 
> > +
> >  	input->seat_name = strdup(name);
> >  	assert(input->seat_name && "No memory");
> >  
> > +	/* We assume that the test seat always has some capabilities,
> > +	 * so caps == 0 just means that we haven't received them yet */
> 
> We don't actually need this assumption, because whether the caps really
> are zero or are not yet handled, the result should be exactly the same.
> We can uncoditionally set client->input here, and remove the setting
> from the capabilities handler. input_update_devices() also already does
> the right thing regardless whether capabilities have been received yet.
> 
> > +	if (input->caps != 0) {
> > +		input_update_devices(input);
> > +		input->client->input = input;
> 
> ..as an assert on this assignment that client->input must be NULL
> before we set it.
> 
> 
> Thanks,
> pq
> 
> > +	}
> > +
> >  	fprintf(stderr, "test-client: got seat %p name: \'%s\'\n",
> >  		input, name);
> >  }
> > @@ -705,6 +747,7 @@ handle_global(void *data, struct wl_registry *registry,
> >  					 &wl_compositor_interface, version);
> >  	} else if (strcmp(interface, "wl_seat") == 0) {
> >  		input = xzalloc(sizeof *input);
> > +		input->client = client;
> >  		input->wl_seat =
> >  			wl_registry_bind(registry, id,
> >  					 &wl_seat_interface, version);
> > @@ -809,34 +852,6 @@ log_handler(const char *fmt, va_list args)
> >  	vfprintf(stderr, fmt, args);
> >  }
> >  
> > -static void
> > -input_destroy(struct input *inp)
> > -{
> > -	wl_list_remove(&inp->link);
> > -	wl_seat_destroy(inp->wl_seat);
> > -	free(inp);
> > -}
> > -
> > -/* find the test-seat and set it in client.
> > - * Destroy other inputs */
> > -static void
> > -client_set_input(struct client *cl)
> > -{
> > -	struct input *inp, *inptmp;
> > -	wl_list_for_each_safe(inp, inptmp, &cl->inputs, link) {
> > -		assert(inp->seat_name && "BUG: input with no name");
> > -		if (strcmp(inp->seat_name, "test-seat") == 0) {
> > -			cl->input = inp;
> > -			input_update_devices(inp);
> > -		} else {
> > -			input_destroy(inp);
> > -		}
> > -	}
> > -
> > -	/* we keep only one input */
> > -	assert(wl_list_length(&cl->inputs) == 1);
> > -}
> > -
> >  struct client *
> >  create_client(void)
> >  {
> > @@ -862,9 +877,6 @@ create_client(void)
> >  	 * events */
> >  	client_roundtrip(client);
> >  
> > -	/* find the right input for us */
> > -	client_set_input(client);
> > -
> >  	/* must have WL_SHM_FORMAT_ARGB32 */
> >  	assert(client->has_argb);
> >  
> > diff --git a/tests/weston-test-client-helper.h b/tests/weston-test-client-helper.h
> > index 09a5df4a..f16356e5 100644
> > --- a/tests/weston-test-client-helper.h
> > +++ b/tests/weston-test-client-helper.h
> > @@ -74,6 +74,7 @@ struct test {
> >  };
> >  
> >  struct input {
> > +	struct client *client;
> >  	struct wl_seat *wl_seat;
> >  	struct pointer *pointer;
> >  	struct keyboard *keyboard;
>