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.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,
>
> 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;
>
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
