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

[Pekka Paalanen]

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.

> 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.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180201/4bd27ba8/attachment.sig>