Re: [PATCH] ui: Remove deprecated parameters of -display sdl and switch to QAPI parser

[Thomas Huth <thuth@redhat.com>]

On 11/05/2022 14.40, Daniel P. Berrangé wrote:
> On Wed, May 11, 2022 at 02:17:25PM +0200, Thomas Huth wrote:
>> The "-display sdl" option still used a hand-crafted parser for its
>> parameters since some of them used underscores which is forbidden
>> in QAPI. Now that they've been deprecated and the deprecation period
>> is over, we can remove the problematic parameters and switch to use
>> the QAPI parser instead.
> 
> I'd say the removal of deprecated bits ought to be separate
> from the addition of new QAPI bits.

Ok, agreed, that likely makes sense.

> 
>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index 059302a5ef..7d5097808a 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -1309,6 +1309,19 @@
>>         '*swap-opt-cmd': 'bool'
>>     } }
>>   
>> +##
>> +# @DisplaySDL:
>> +#
>> +# SDL2 display options.
>> +#
>> +# @grab-mod:  Modifier keys that should be pressed together
>> +#             with "G" to release the mouse grab.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'struct'  : 'DisplaySDL',
>> +  'data'    : { '*grab-mod'   : 'str' } }
> 
> So any arbitrary string here, but...
> 
>> diff --git a/ui/sdl2.c b/ui/sdl2.c
>> index d3741f9b75..18c63e1fc9 100644
>> --- a/ui/sdl2.c
>> +++ b/ui/sdl2.c
>> @@ -40,6 +40,8 @@ static struct sdl2_console *sdl2_console;
>>   
>>   static SDL_Surface *guest_sprite_surface;
>>   static int gui_grab; /* if true, all keyboard/mouse events are grabbed */
>> +static bool alt_grab;
>> +static bool ctrl_grab;
>>   
>>   static int gui_saved_grab;
>>   static int gui_fullscreen;
>> @@ -853,6 +855,17 @@ static void sdl2_display_init(DisplayState *ds, DisplayOptions *o)
>>   
>>       gui_fullscreen = o->has_full_screen && o->full_screen;
>>   
>> +    if (o->u.sdl.has_grab_mod) {
>> +        if (g_str_equal(o->u.sdl.grab_mod, "lshift-lctrl-lalt")) {
>> +            alt_grab = true;
>> +        } else if (g_str_equal(o->u.sdl.grab_mod, "rctrl")) {
>> +            ctrl_grab = true;
>> +        } else {
>> +            error_report("Unsupported grab-mod: %s", o->u.sdl.grab_mod);
>> +            exit(1);
>> +        }
>> +    }
> 
> We're treating this more like an enum here. It does leave the door
> open for adding a generic parsing of arbitrary grab mods later
> I guess

Yes, that's the basic idea - so one day, the user might specify an arbitrary 
string here consisting of their favorite modifiers.

  Thomas