Re: [PATCH 2/3] ui: Switch "-display sdl" to use the QAPI parser
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