Re: [PATCH v3 2/6] ui: add the infrastructure to support MT events

[Sergio Lopez]

On Mon, Apr 17, 2023 at 12:57:08PM +0200, Markus Armbruster wrote:
> Sergio Lopez <slp@redhat.com> writes:
> 
> > Add the required infrastructure to support generating multitouch events.
> >
> > Signed-off-by: Sergio Lopez <slp@redhat.com>
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/ui/input.h    |  3 +++
> >  qapi/ui.json          | 46 ++++++++++++++++++++++++++++++++++++++++---
> >  replay/replay-input.c | 18 +++++++++++++++++
> >  ui/input.c            |  6 ++++++
> >  ui/trace-events       |  1 +
> >  5 files changed, 71 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/ui/input.h b/include/ui/input.h
> > index c86219a1c1..2a3dffd417 100644
> > --- a/include/ui/input.h
> > +++ b/include/ui/input.h
> > @@ -8,9 +8,12 @@
> >  #define INPUT_EVENT_MASK_BTN   (1<<INPUT_EVENT_KIND_BTN)
> >  #define INPUT_EVENT_MASK_REL   (1<<INPUT_EVENT_KIND_REL)
> >  #define INPUT_EVENT_MASK_ABS   (1<<INPUT_EVENT_KIND_ABS)
> > +#define INPUT_EVENT_MASK_MTT   (1<<INPUT_EVENT_KIND_MTT)
> >  
> >  #define INPUT_EVENT_ABS_MIN    0x0000
> >  #define INPUT_EVENT_ABS_MAX    0x7FFF
> > +#define INPUT_EVENT_SLOTS_MIN  0x0
> > +#define INPUT_EVENT_SLOTS_MAX  0xa
> >  
> >  typedef struct QemuInputHandler QemuInputHandler;
> >  typedef struct QemuInputHandlerState QemuInputHandlerState;
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 98322342f7..83369bdae8 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1014,7 +1014,7 @@
> >  ##
> >  { 'enum'  : 'InputButton',
> >    'data'  : [ 'left', 'middle', 'right', 'wheel-up', 'wheel-down', 'side',
> > -  'extra', 'wheel-left', 'wheel-right' ] }
> > +  'extra', 'wheel-left', 'wheel-right', 'touch' ] }
> >  
> >  ##
> >  # @InputAxis:
> > @@ -1026,6 +1026,17 @@
> >  { 'enum'  : 'InputAxis',
> >    'data'  : [ 'x', 'y' ] }
> >  
> > +##
> > +# @InputMultitouchType:
> 
> Suggest InputMultiTouchType, because...
> 
> > +#
> > +# Type of a multitouch event.
> 
> ... the common spelling is multi-touch.
> 
> More of the same below.

Ack, let's use this in v4.

> > +#
> > +# Since: 8.1
> > +##
> > +{ 'enum'  : 'InputMultitouchType',
> > +  'data'  : [ 'begin', 'update', 'end', 'cancel', 'data' ] }
> > +
> > +
> >  ##
> >  # @InputKeyEvent:
> >  #
> > @@ -1069,13 +1080,32 @@
> >    'data'  : { 'axis'    : 'InputAxis',
> >                'value'   : 'int' } }
> >  
> > +##
> > +# @InputMultitouchEvent:
> > +#
> > +# Multitouch input event.
> > +#
> > +# @slot: Which slot has generated the event.
> 
> Ignorant question: what's a "slot"?

The Multi-touch protocol [1] talks about them without describing them in much
detail. In my understanding, the HW has as many slots as simultaneous contact
points is able to track. When a new contact is detected is assigned a
particular slot, and keeps using that one until the contact is released.

> > +# @tracking-id: ID to correlate this event with previously generated 
> > events.
> > +# @axis: Which axis is referenced by @value.
> > +# @value: Contact position.
> > +#
> > +# Since: 8.1
> > +##
> > +{ 'struct'  : 'InputMultitouchEvent',
> > +  'data'  : { 'type'       : 'InputMultitouchType',
> > +              'slot'       : 'int',
> > +              'tracking-id': 'int',
> > +              'axis'       : 'InputAxis',
> > +              'value'      : 'int' } }
> > +
> >  ##
> >  # @InputEventKind:
> >  #
> >  # Since: 2.0
> >  ##
> >  { 'enum': 'InputEventKind',
> > -  'data': [ 'key', 'btn', 'rel', 'abs' ] }
> > +  'data': [ 'key', 'btn', 'rel', 'abs', 'mtt' ] }
> 
> While we generally avoid abbreviations in QAPI, local consistency is a
> strong argument for this one.  Okay.
> 
> >  
> >  ##
> >  # @InputKeyEventWrapper:
> > @@ -1101,6 +1131,14 @@
> >  { 'struct': 'InputMoveEventWrapper',
> >    'data': { 'data': 'InputMoveEvent' } }
> >  
> > +##
> > +# @InputMultitouchEventWrapper:
> > +#
> > +# Since: 8.1
> > +##
> > +{ 'struct': 'InputMultitouchEventWrapper',
> > +  'data': { 'data': 'InputMultitouchEvent' } }
> 
> The only reason for wrapping is consistency with the other branches.
> Okay.
> 
> > +
> >  ##
> >  # @InputEvent:
> >  #
> > @@ -1112,6 +1150,7 @@
>    # @type: the input type, one of:
>    #
>    #        - 'key': Input event of Keyboard
> >  #        - 'btn': Input event of pointer buttons
> >  #        - 'rel': Input event of relative pointer motion
> >  #        - 'abs': Input event of absolute pointer motion
> > +#        - 'mtt': Input event of Multitouch
> 
> You're imitating the existing "Input event of" pattern, which is fair.
> But the pattern is bad.  The phrasing awkward, and so is the place.  By
> documenting the values of InputEventKind only here, and not in
> InputEventKind's doc comment, the generated documentation for
> InputEventKind looks like this:
> 
>     "InputEventKind" (Enum)
>     -----------------------
> 
>     Values
>     ~~~~~~
> 
>     "key"
>        Not documented
> 
>     "btn"
>        Not documented
> 
>     "rel"
>        Not documented
> 
>     "abs"
>        Not documented
> 
>     "mtt"
>        Not documented
> 
> 
>     Since
>     ~~~~~
> 
>     2.0
> 
> We should document them right in InputEventKind's doc comment, roughly
> like this:
> 
>    ##
>    # @InputEventKind:
>    #
>    # @key: a keyboard input event
>    # @btn: a pointer button input event
>    # @rel: a relative pointer motion input event
>    # @abs: an absolute pointer motion input event
>    # @mtt: a multi-touch input event
>    #
>    # Since: 2.0
>    ##
> 
> We can then dumb down the documentation of InputEvent member @type to
> just
> 
>    # @type: the type of input event
> 
> What do you think?

Yeah, this definitely looks better. Fixed in v4.

Thanks,
Sergio.

> Many more doc comments neglect to document members in this file, and in
> others.  I'm not asking you to fix them all.
> 
> >  #
> >  # Since: 2.0
> >  ##
> > @@ -1121,7 +1160,8 @@
> >    'data'  : { 'key'     : 'InputKeyEventWrapper',
> >                'btn'     : 'InputBtnEventWrapper',
> >                'rel'     : 'InputMoveEventWrapper',
> > -              'abs'     : 'InputMoveEventWrapper' } }
> > +              'abs'     : 'InputMoveEventWrapper',
> > +              'mtt'     : 'InputMultitouchEventWrapper' } }
> >  
> >  ##
> >  # @input-send-event:
> 
> [...]
>

signature.asc
Description: PGP signature