| Message ID | 1447279028-2114-2-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Our documentation states that we prefer 'lower-case', rather than > 'CamelCase', for qapi enum values. The InputButton and InputAxis > enums violated this convention. However, they are currently used > primarily for generating code that is used internally; their only > exposure through QMP is via the experimental 'x-input-send-event' > command. Since this is experimental, changing the QMP wire format > for that command is acceptable. > > The existing c_enum_const() code in the generator for turning the > enum names into C constants happens to munge both pre- and > post-patch spellings to the same C code, which means making the > change now touches very few files. But we are considering a > future patch which would change c_enum_const() to use > c_name(V).upper() rather than camel_to_upper(), which would render > 'WheelUp' as INPUT_BUTTON_WHEELUP instead of its current > INPUT_BUTTON_WHEEL_UP. Making the change to the enum values now > will isolate these enums from any impact if the generator munging > algorithm is changed. > > Note that SDL code uses the spelling WHEELUP rather than WHEEL_UP > in its constants, but that shouldn't drive our decision. > > Fix a typo in the qapi docs for InputAxis while at it. > > CC: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> I can take this through my tree if Gerd doesn't object.
On 11/12/2015 01:23 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Our documentation states that we prefer 'lower-case', rather than >> 'CamelCase', for qapi enum values. The InputButton and InputAxis >> enums violated this convention. However, they are currently used >> primarily for generating code that is used internally; their only >> exposure through QMP is via the experimental 'x-input-send-event' >> command. Since this is experimental, changing the QMP wire format >> for that command is acceptable. > > I can take this through my tree if Gerd doesn't object. If we aren't changing the spelling of x-, then we shouldn't change the spelling of the arguments to x-. How about this instead: add a FIXME comment to qapi-schema.json that documents that the names of the enum values will likely be changed at the (later) point where x- is removed. In the meantime, if we add any namespace enforcing, we'll just have to whitelist these two.
diff --git a/qapi-schema.json b/qapi-schema.json index c3f95ab..ecefb17 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3531,17 +3531,17 @@ # Since: 2.0 ## { 'enum' : 'InputButton', - 'data' : [ 'Left', 'Middle', 'Right', 'WheelUp', 'WheelDown' ] } + 'data' : [ 'left', 'middle', 'right', 'wheel-up', 'wheel-down' ] } ## -# @InputButton +# @InputAxis # # Position axis of a pointer input device (mouse, tablet). # # Since: 2.0 ## { 'enum' : 'InputAxis', - 'data' : [ 'X', 'Y' ] } + 'data' : [ 'x', 'y' ] } ## # @InputKeyEvent diff --git a/qmp-commands.hx b/qmp-commands.hx index 02c0c5b..8f25fe0 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -4504,13 +4504,13 @@ Press left mouse button. -> { "execute": "x-input-send-event", "arguments": { "console": 0, "events": [ { "type": "btn", - "data" : { "down": true, "button": "Left" } } ] } } + "data" : { "down": true, "button": "left" } } ] } } <- { "return": {} } -> { "execute": "x-input-send-event", "arguments": { "console": 0, "events": [ { "type": "btn", - "data" : { "down": false, "button": "Left" } } ] } } + "data" : { "down": false, "button": "left" } } ] } } <- { "return": {} } Example (2): @@ -4533,8 +4533,8 @@ Move mouse pointer to absolute coordinates (20000, 400). -> { "execute": "x-input-send-event" , "arguments": { "console": 0, "events": [ - { "type": "abs", "data" : { "axis": "X", "value" : 20000 } }, - { "type": "abs", "data" : { "axis": "Y", "value" : 400 } } ] } } + { "type": "abs", "data" : { "axis": "x", "value" : 20000 } }, + { "type": "abs", "data" : { "axis": "y", "value" : 400 } } ] } } <- { "return": {} } EQMP
Our documentation states that we prefer 'lower-case', rather than 'CamelCase', for qapi enum values. The InputButton and InputAxis enums violated this convention. However, they are currently used primarily for generating code that is used internally; their only exposure through QMP is via the experimental 'x-input-send-event' command. Since this is experimental, changing the QMP wire format for that command is acceptable. The existing c_enum_const() code in the generator for turning the enum names into C constants happens to munge both pre- and post-patch spellings to the same C code, which means making the change now touches very few files. But we are considering a future patch which would change c_enum_const() to use c_name(V).upper() rather than camel_to_upper(), which would render 'WheelUp' as INPUT_BUTTON_WHEELUP instead of its current INPUT_BUTTON_WHEEL_UP. Making the change to the enum values now will isolate these enums from any impact if the generator munging algorithm is changed. Note that SDL code uses the spelling WHEELUP rather than WHEEL_UP in its constants, but that shouldn't drive our decision. Fix a typo in the qapi docs for InputAxis while at it. CC: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- qapi-schema.json | 6 +++--- qmp-commands.hx | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-)