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

[Alexandros Frantzis]

On Thu, Feb 01, 2018 at 12:00:44PM +0200, Pekka Paalanen wrote:
> On Wed, 31 Jan 2018 17:14:49 +0200
> Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> 
> > 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.
> 
> You seemed to have removed the check for "each seat has a name". I also
> had trouble following the code flow.

The "each seat has a name" part was indeed removed, since it doesn't
work with this method and it seemed to be a secondary concern. I focused
on the primary function which is "find the test seat and use it for the
input object".

My proposal would be to reintroduce this check by means of a new
explicit test, instead of implicitly testing it in the test
infrastructure. I think this would be good to do regardless of this
proposal. How does this sound?

> > 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);
> 
> Right.
> 
> > 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).
> 
> Ok, let's see if detailed comments can push the code into a more clear
> direction.

I will update/add comments and will try to simplify where possible.

Thanks,
Alexandros