Re: [PATCH v2 2/3] ui: Switch "-display sdl" to use the QAPI parser

[Thomas Huth <thuth@redhat.com>]

On 19/05/2022 09.51, Daniel P. Berrangé wrote:
> On Thu, May 19, 2022 at 09:27:08AM +0200, Thomas Huth wrote:
>> On 19/05/2022 09.08, Thomas Huth wrote:
>>> On 19/05/2022 08.39, Thomas Huth wrote:
>>>> On 18/05/2022 17.08, Markus Armbruster wrote:
>>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>>
>>>>>> The "-display sdl" option still uses a hand-crafted parser for its
>>>>>> parameters since we didn't want to drag an interface we considered
>>>>>> somewhat flawed into the QAPI schema. Since the flaws are gone now,
>>>>>> it's time to QAPIfy.
>>>>>>
>>>>>> This introduces the new "DisplaySDL" QAPI struct that is used to hold
>>>>>> the parameters that are unique to the SDL display. The only specific
>>>>>> parameter is currently "grab-mod" that is used to specify the required
>>>>>> modifier keys to escape from the mouse grabbing mode.
>>>>>>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>>    qapi/ui.json            | 27 +++++++++++++++-
>>>>>>    include/sysemu/sysemu.h |  2 --
>>>>>>    softmmu/globals.c       |  2 --
>>>>>>    softmmu/vl.c            | 70 +----------------------------------------
>>>>>>    ui/sdl2.c               | 10 ++++++
>>>>>>    5 files changed, 37 insertions(+), 74 deletions(-)
>>>>>>
>>>>>> diff --git a/qapi/ui.json b/qapi/ui.json
>>>>>> index 11a827d10f..a244e26e0f 100644
>>>>>> --- a/qapi/ui.json
>>>>>> +++ b/qapi/ui.json
>>>>>> @@ -1295,6 +1295,30 @@
>>>>>>          '*swap-opt-cmd': 'bool'
>>>>>>      } }
>>>>>> +##
>>>>>> +# @GrabMod:
>>>>>> +#
>>>>>> +# Set of modifier keys that need to be hold for shortcut key actions.
>>>>>> +#
>>>>>> +# Since: 7.1
>>>>>> +##
>>>>>> +{ 'enum'  : 'GrabMod',
>>>>>> +  'data'  : [ 'lctrl-lalt', 'lshift-lctrl-lalt', 'rctrl' ] }
>>>>>
>>>>> This is fine now.  If we ever generalize to "arbitrary set of modifier
>>>>> keys", it'll become somewhat awkward.  No objection from me.
>>
>> Oh well, I just noticed that we already have a GrabToggleKeys enum in
>> qapi/common.json ... I wonder whether I should try to use that instead? It
>> seems to be used in a slightly different context, though, if I get that
>> right ...?
> 
> It also doesn't distinguish left & right control/alt/shift keys
> for some reason.  So you would end up having to add more enum
> entries for SDL, none of which overlap with existing enum entries.

We could also extend the SDL code to work with the other combos from 
GrabToggleKeys, I guess.

> Rather a pity, as the consistency would have been nice

I wonder which way would cause less "WTF?" situations in the future ... if 
we have one enum with slightly different naming between the entries, or if 
we have two enums that seems to be there for the same or at least very 
similar things, which still have still inconsistent namings between the 
entries...
I'm slightly inclined to go for the unified GrabToggleKeys enum, I think...

  Thomas