On Wed, 31 Jan 2018 17:14:49 +0200 Alexandros Frantzis <alexandros.frant...@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.frant...@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.frant...@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
pgpmQE5rkFB0g.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
