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

[Thomas Huth]

On 12/05/2022 14.16, Markus Armbruster wrote:
[...]>           if (strstart(p, "sdl", &opts)) {
>      +        /*
>      +         * sdl DisplayType needs hand-crafted parser instead of
>      +         * parse_display_qapi() due to some options not in
>      +         * DisplayOptions, specifically:
>      +         *   - frame
>      +         *     Already deprecated.
>      +         *   - ctrl_grab + alt_grab
>      +         *     Not clear yet what happens to them long-term.  Should
>      +         *     replaced by something better or deprecated and dropped.
>
> This sounds like it was mostly reluctance to drag undesirables into the
> QAPI schema.

Yeah, ctrl_grab and alt_grab were just too ugly and inflexible to drag them 
along into the QAPI world.

>      @@ -2179,6 +2189,10 @@ static void parse_display(const char *p)
>                   opts = nextopt;
>               }
>           } else if (strstart(p, "vnc", &opts)) {
>      +        /*
>      +         * vnc isn't a (local) DisplayType but a protocol for remote
>      +         * display access.
>      +         */
>               if (*opts == '=') {
>                   vnc_parse(opts + 1, &error_fatal);
>               } else {
>
> This remains, and that's fine.  One step at time.

Not sure how we want to proceed with that in the long run, though ... IIRC 
clearly, Paolo once said that it doesn't really belong into "-display" 
anyway and should be handled with the separate "-vnc" option instead?

> >           Now that the problematic parameters have been removed, we can
> > switch to use the QAPI parser instead.
>
> Here's my attempt at a more accurate commit message.
>
>    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.

Ok, I can update the description, thanks!

> > 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" which is modeled as a string, so that
> > it could be extended for other arbitrary modifiers later more easily.
>
> Are the values of @grab-mod parsed in any way, or do we recognize a set
> of fixed strings?
>
> The former would be problematic.  We try hard to represent complex data
> as JSON instead of inventing little ad hoc languages.
>
> If it's the latter, use an enum.  Makes introspection more useful, and
> adding enumeration values is no harder than adding string literals.

It's currently only two strings that are used to replace the old behavior. 
But in the long run, I think it would be nice to have more flexibility here, 
so that a user could specify an arbitrary combination of modifier keys. I 
don't think that an enum will really scale here, so I'd prefer to go with 
the current approach and use the string for more flexibility.

 Thomas