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