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

[Bryce Harrington]

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.

> > 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.

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.

Bryce

> Best regards
> 
> -- 
> David FORT
> website: http://www.hardening-consulting.com/