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

[Markus Armbruster]

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.

> +#
> +# 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"?

> +# @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?

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:

[...]