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

[Alexandros Frantzis]

On Thu, Feb 01, 2018 at 02:09:32PM +0200, Pekka Paalanen wrote:
> On Thu, 1 Feb 2018 13:30:25 +0200
> Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> 
> > 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.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,
> > > 
> > > 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?
> 
> Hi,
> 
> I think a good general guideline is to not implement any infrastructure
> that is not going to get used very soon. In that sense going for the
> smallest modification is a good plan. Keeping that in mind, it is of
> course nice to clean up and streamline the existing infrastructure.
> 
> Anticipating future needs is good, but it should not go too far to
> possibly end up unsuitable and wasted work.

I agree about not doing unecessary work ("YAGNI"), but this is
functionality that could be immediately useful. For example the explicit
test to check that all seats have a name could be written very easily if
we exposed all seats.

This change would also simplify the logic of handling seat
addition/removal, since we wouldn't need the dynamic
creation/destruction of input objects inside the seat information event
handlers.

> We could create input objects for all seats, but I'd be more wary of
> creating the wl_pointer/wl_keyboard/wl_touch objects for non-test
> seats. So far non-test seats are never useful in tests and unexpected
> input events could even cause confusion.

Each input event should only affect the relevant input object (and
sub-objects), so I don't expect any interference with the tests if they
are properly written, but perhaps I am missing some interaction. Plus,
as you say, we could actually skip creating the pointer/keyboard/touch
sub-objects for non-test seats.

Having said all the above, mostly to present a more complete argument
for this idea, I am fine continuing with a minimal change approach at
this time. We can consider alternative approaches in the future as we
get more need for them.

Thanks,
Alexandros