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

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, May 23, 2022 at 09:23:48PM +0200, Thomas Huth wrote:
>> On 23/05/2022 15.45, 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            | 26 ++++++++++++++-
>> > >   include/sysemu/sysemu.h |  2 --
>> > >   softmmu/globals.c       |  2 --
>> > >   softmmu/vl.c            | 70 +----------------------------------------
>> > >   ui/sdl2.c               | 10 ++++++
>> > >   5 files changed, 36 insertions(+), 74 deletions(-)
>> > > 
>> > > diff --git a/qapi/ui.json b/qapi/ui.json
>> > > index 11a827d10f..413371d5e8 100644
>> > > --- a/qapi/ui.json
>> > > +++ b/qapi/ui.json
>> > > @@ -1295,6 +1295,29 @@
>> > >         '*swap-opt-cmd': 'bool'
>> > >     } }
>> > > +##
>> > > +# @HotKeyMod:
>> > > +#
>> > > +# Set of modifier keys that need to be held for shortcut key actions.
>> > > +#
>> > > +# Since: 7.1
>> > > +##
>> > > +{ 'enum'  : 'HotKeyMod',
>> > > +  'data'  : [ 'lctrl-lalt', 'lshift-lctrl-lalt', 'rctrl' ] }
>> > 
>> > I have a somewhat uneasy feeling about encoding what is essentially a
>> > subset of the sets of modifier keys as an enumeration, but it's what we
>> > have to do to QAPIfy existing grab-mod.
>> 
>> Well, that's exactly what you suggested here:
>> 
>>  https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg03401.html
>> 
>> So I really don't understand your uneasy feeling now?

I did mention the set of modifiers (encoded as list) design alternative.
You pointed out that you're merely QAPIfying what we have.  Valid point,
well taken, I don't want to block that.

> I think we should consider what happens if we want to allow arbitrary
> key combinations in future, consisting of one or more modifiers together
> witha non-modifier key. For example CtrlL+ShiftL+F12.  We won't want
> to be expressing the combinatorial expansion of all possible key
> combinations as an enum.  Instead I think we would want to have an
> enum for keycode names and accept an array of them. We already
> have QKeyCode, so for SDL grab sequence we need to accept an
> arrray of QKeyCode.
>
>   { 'struct'  : 'DisplaySDL',
>     'data'    : { '*grab-mod'   : [ 'QKeyCode' ] }
>
> Good for QMP but not entirely pretty on the CLI. But we need back
> compat for existing CLI syntax too, so we would have to use an
> alternate allowing plain string for the legacy key codes combinations.
>
> IOW we end up needing
>
>    { 'alternate': 'SDLGrabSeq',
>      'data': { 'grab-mod': 'HotKeyMod',
>                'grab-keys: [ 'QKeyCode' ] } }
>
>   { 'struct'  : 'DisplaySDL',
>     'data'    : { '*grab-mod' : [ 'SDLGrabSeq' ] }
>
> deprecating 'grab-mod' for a while, eventually leaving just the
> first example above.
>
> Since  IIUC, we can retrofit the alternate after the fact if we
> decide to allow arbitrary key combos, it means we could safely
> start with what Thomas proposes
>
>   { 'struct'  : 'DisplaySDL',
>     'data'    : { '*grab-mod' : 'HotKeyMod' }
>
> and worry about the more general solution another day/month/year

Right.

[...]