[PATCH] weston: implement inert objects for keyboard/pointer/touch

[Jonas Ådahl]

On Wed, Oct 21, 2015 at 12:23:10AM +0200, Hardening wrote:
> Le 20/10/2015 17:12, Bryce Harrington a écrit :
> > On Tue, Oct 20, 2015 at 10:39:51AM +0200, Hardening wrote:
> >> Le 20/10/2015 10:24, Bryce Harrington a écrit :
> >>> On Mon, Oct 19, 2015 at 03:47:19PM +0200, David FORT wrote:
> >>>> This patch implements inert objects for wl_keyboard, wl_pointer and wl_touch.
> >>>> The target case is when the server has just send a capability event about a
> >>>> disappearing object, and the client binds the corresponding object. We bind an
> >>>> inert object: an object does nothing when it is requested. If the client behave
> >>>> correctly, this object should be released when the capability event is received
> >>>> and treated (calling the corresponding release request).
> >>>> As a consequence, we can rely on seat->[keyboard|pointer|touch]_state to know
> >>>> if the seat has the corresponding object.
> >>>
> >>> This is a big patch but it looks like the vast bulk is merely adding in
> >>> pointer checks for keyboard, pointer, etc. everywhere and subsequent
> >>> retabbing.  If you broke that refactoring step out as a preliminary
> >>> patch, it may make reviewing the pertinent changes (i.e. tracking and
> >>> checking state for the input devices rather than just testing the
> >>> reference count) a bit easier.
> >>
> >> I'm not sure to understand, what should I do then ?
> > 
> > Create two patches, the first of which just adds all the "if
> > (keyboard)..." checks, and the second (much smaller) patch which adds
> > the inert_pointer_interface, inert_keyboard_interface, etc. parts which
> > are what you actually need the review on.  The pointer checking is
> > obviously a safe refactoring that can be landed with minimal review.
> > 
> 
> Well I can do this way but the 2 patches are linked. In the current
> code, once a keyboard has been created, you can't have a null
> seat->keyboard_state. So in the current situation, I'm not sure the
> checks are really needed .

Note that for grabs, when you loose the seat i.e. the actual device, I
think it makes sense to cancel the grab before the device is destroyed
which means there may be places where added NULL checks are unnecessary
(i.e. everywhere in or calls originating from grab handlers).


Jonas

> 
> >>> Thanks for tending to the test code too in the refactor; it would be
> >>> grand to see a test case added to keyboard-test or devices-test to
> >>> exercise the case of handling inert input objects.
> >>>
> >>
> >> Such a test is already there in the existing tests. In
> >> get_device_after_destroy of devices-test.c, we already exercise inert
> >> objects.
> > 
> > Apparently it is not exercised sufficiently, because it passes already
> > without your fix.
> > 
> 
> Well you're right: in the current code input objects are never released,
> so the calls are done on used objects without any visible side effect.
> In fact, there's a side effect when calling wl_pointer.set_cursor on a
> released pointer: the surface is given the mouse pointer role. Using
> inert objects, you're just sure that there will be no side effect at
> all. As I said It think it's also the first step to make it possible to
> safely release the full seat.
> 
> > But you're right that this test case looks like a good place for you to
> > add more coverage for this bug.  Does the current test pass without your
> > fix because it is not making deep enough wayland calls?  I.e. does it
> > need to do more than merely a wl_pointer_set_cursor call?
> > 
> > Also, it appears this test isn't doing the same checking for keyboard
> > and touch as it does for pointer; even though internally all three are
> > implemented the same way, there are three different protocol routines
> > involved here (wl_seat_get_pointer, wl_seat_get_keyboard, and
> > wl_seat_get_touch), any of which could present their own bugs, so proper
> > test coverage should methodically check all three.
> > 
> 
> +1 for testing all input objects.
> 
> Best regards.
> 
> -- 
> David FORT
> website: http://www.hardening-consulting.com/
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel