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

[Alexandros Frantzis]

On Wed, Jan 31, 2018 at 04:25:15PM +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 Alexandros,

Hi Pekka,

thanks for the review.

> essentially patches 5-7 want to support dynamically creating and
> removing wl_seats. The current test protocol is poorly suited for it as
> it assumes the single test-seat in all requests. I would like to have a
> protocol that better matches the structure of input devices and seats
> and how weston core consumes input. However, that's a big task and it
> would be outrageous to ask for that right here, so I think your
> intention here is fine.
> 
> I presume the idea is that device_add("seat") and
> device_release("seat") will create and destroy the test-seat,
> respectively, regardless of the current capabilities.

Correct. Patches 5-7 prepare the test infrastructure so we can
add a test that removes and re-adds the test seat (patch 8).

> > 
> > 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
> 
> > @@ -862,9 +877,6 @@ create_client(void)
> >  	 * events */
> >  	client_roundtrip(client);
> >  
> > -	/* find the right input for us */
> > -	client_set_input(client);
> > -
> 
> The original idea here was that the two roundtrips above guarantee that
> we have processed all global advertisements and all wl_seat name and
> capapbility events. Then we can ensure that Weston indeed provided
> exactly one test-seat, and that all seats had a name provided.
> 
> I don't think that's in any way contradictory to allowing the test-seat
> then to be removed and re-added, so I'm not quite sure why this patch
> is needed or what it even does?

The client_set_input() call (and function) is removed because the same
functionality is now implemented when we handle information events about
seats. The two roundtrips are still required for the reasons you
mention above.

As an example exhibiting the reason for this change, assume that the
server has removed the seat, in which case the client->input object has
been freed (see patch 7). A test that wants to re-add the test seat will
call (copied from patch 8):

  weston_test_device_add(cl->test->weston_test, "seat");
  /* First roundtrip to send request and receive new seat global */
  client_roundtrip(cl);
  /* Second roundtrip to handle seat events and set up input devices */
  client_roundtrip(cl);

Without this patch, the test would also need to call client_set_input()
in order for the seat addition to take effect (populate client->input
etc). However, this is an extra internal implementation detail of the
test infrastructure which I would prefer not to reveal to tests. This
patch automatically updates the client->input as seats come and go, so
the code above just works. Similarly for when we remove the test seat.

So, to sum up, it's not that client_set_input couldn't be used in this
case, we could make it public and call it in the tests, but I think the
proposed approach provides a more intuitive interface for writing tests
(for our current needs, at least).

Thanks,
Alexandros