Re: [PATCH 1/3] ui/sdl2: reenable the SDL2 Windows keyboard hook procedure

[Bernhard Beschow]

Am 9. September 2024 19:38:26 UTC schrieb "Volker Rümelin" 
<vr_qemu@t-online.de>:
>Am 09.09.24 um 09:26 schrieb Marc-André Lureau:
>> Hi
>>
>> On Mon, Sep 9, 2024 at 10:22 AM Volker Rümelin <vr_qemu@t-online.de> wrote:
>>> Windows only:
>>>
>>> The libSDL2 Windows message loop needs the libSDL2 Windows low
>>> level keyboard hook procedure to grab the left and right Windows
>>> keys correctly. Reenable the SDL2 Windows keyboard hook procedure.
>>>
>>> Because the QEMU Windows keyboard hook procedure is still needed
>>> to filter out the special left Control key event for every Alt Gr
>>> key event, it's important to install the two keyboard hook
>>> procedures in the following order. First the SDL2 procedure, then
>>> the QEMU procedure.
>>>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2139
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2323
>>> Tested-by: Howard Spoelstra <hsp.cat7@gmail.com>
>>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>>> ---
>>>  ui/sdl2.c           | 53 ++++++++++++++++++++++++++++++---------------
>>>  ui/win32-kbd-hook.c |  3 +++
>>>  2 files changed, 38 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/ui/sdl2.c b/ui/sdl2.c
>>> index 98ed974371..ac37c173a1 100644
>>> --- a/ui/sdl2.c
>>> +++ b/ui/sdl2.c
>>> @@ -42,6 +42,7 @@ 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 bool win32_kbd_grab;
>>>
>>>  static int gui_saved_grab;
>>>  static int gui_fullscreen;
>>> @@ -202,6 +203,19 @@ static void sdl_update_caption(struct sdl2_console 
>>> *scon)
>>>      }
>>>  }
>>>
>>> +static void *sdl2_win32_get_hwnd(struct sdl2_console *scon)
>>> +{
>>> +#ifdef CONFIG_WIN32
>>> +    SDL_SysWMinfo info;
>>> +
>>> +    SDL_VERSION(&info.version);
>>> +    if (SDL_GetWindowWMInfo(scon->real_window, &info)) {
>>> +        return info.info.win.window;
>>> +    }
>>> +#endif
>>> +    return NULL;
>>> +}
>>> +
>>>  static void sdl_hide_cursor(struct sdl2_console *scon)
>>>  {
>>>      if (scon->opts->has_show_cursor && scon->opts->show_cursor) {
>>> @@ -259,9 +273,16 @@ static void sdl_grab_start(struct sdl2_console *scon)
>>>      } else {
>>>          sdl_hide_cursor(scon);
>>>      }
>>> +    /*
>>> +     * Windows: To ensure that QEMU's low level keyboard hook procedure is
>>> +     * called before SDL2's, the QEMU procedure must first be removed and
>>> +     * then the SDL2 and QEMU procedures must be installed in this order.
>>> +     */
>>> +    win32_kbd_set_window(NULL);
>>>      SDL_SetWindowGrab(scon->real_window, SDL_TRUE);
>>> +    win32_kbd_set_window(sdl2_win32_get_hwnd(scon));
>>>      gui_grab = 1;
>>> -    win32_kbd_set_grab(true);
>>> +    win32_kbd_set_grab(win32_kbd_grab);
>>>      sdl_update_caption(scon);
>>>  }
>>>
>>> @@ -370,19 +391,6 @@ static int get_mod_state(void)
>>>      }
>>>  }
>>>
>>> -static void *sdl2_win32_get_hwnd(struct sdl2_console *scon)
>>> -{
>>> -#ifdef CONFIG_WIN32
>>> -    SDL_SysWMinfo info;
>>> -
>>> -    SDL_VERSION(&info.version);
>>> -    if (SDL_GetWindowWMInfo(scon->real_window, &info)) {
>>> -        return info.info.win.window;
>>> -    }
>>> -#endif
>>> -    return NULL;
>>> -}
>>> -
>>>  static void handle_keydown(SDL_Event *ev)
>>>  {
>>>      int win;
>>> @@ -605,7 +613,7 @@ static void handle_windowevent(SDL_Event *ev)
>>>          sdl2_redraw(scon);
>>>          break;
>>>      case SDL_WINDOWEVENT_FOCUS_GAINED:
>>> -        win32_kbd_set_grab(gui_grab);
>>> +        win32_kbd_set_grab(win32_kbd_grab && gui_grab);
>>>          if (qemu_console_is_graphic(scon->dcl.con)) {
>>>              win32_kbd_set_window(sdl2_win32_get_hwnd(scon));
>>>          }
>>> @@ -849,6 +857,7 @@ static void sdl2_display_init(DisplayState *ds, 
>>> DisplayOptions *o)
>>>      uint8_t data = 0;
>>>      int i;
>>>      SDL_SysWMinfo info;
>>> +    SDL_version ver;
>>>      SDL_Surface *icon = NULL;
>>>      char *dir;
>>>
>>> @@ -866,10 +875,7 @@ static void sdl2_display_init(DisplayState *ds, 
>>> DisplayOptions *o)
>>>  #ifdef SDL_HINT_VIDEO_X11_NET_WM_BYPASS_COMPOSITOR /* only available since 
>>> SDL 2.0.8 */
>>>      SDL_SetHint(SDL_HINT_VIDEO_X11_NET_WM_BYPASS_COMPOSITOR, "0");
>>>  #endif
>>> -#ifndef CONFIG_WIN32
>>> -    /* QEMU uses its own low level keyboard hook procedure on Windows */
>>>      SDL_SetHint(SDL_HINT_GRAB_KEYBOARD, "1");
>>> -#endif
>>>  #ifdef SDL_HINT_ALLOW_ALT_TAB_WHILE_GRABBED
>>>      SDL_SetHint(SDL_HINT_ALLOW_ALT_TAB_WHILE_GRABBED, "0");
>>>  #endif
>>> @@ -877,6 +883,17 @@ static void sdl2_display_init(DisplayState *ds, 
>>> DisplayOptions *o)
>>>      SDL_EnableScreenSaver();
>>>      memset(&info, 0, sizeof(info));
>>>      SDL_VERSION(&info.version);
>>> +    /*
>>> +     * Since version 2.16.0 under Windows, SDL2 has its own low level
>>> +     * keyboard hook procedure to grab the keyboard. The remaining task of
>>> +     * QEMU's low level keyboard hook procedure is to filter out the 
>>> special
>>> +     * left Control up/down key event for every Alt Gr key event on 
>>> keyboards
>>> +     * with an international layout.
>>> +     */
>>> +    SDL_GetVersion(&ver);
>>> +    if (ver.major == 2 && ver.minor < 16) {
>>> +        win32_kbd_grab = true;
>>> +    }
>>>
>> Note: there is no 2.16 release. They jumped from 2.0.22 to 2.24 (see
>> https://github.com/libsdl-org/SDL/releases/tag/release-2.24.0)
>
>Hi Marc-André
>
>Oh. This means that the comparison I wrote is true for SDL2 versions <
>2.24.0.
>
>>
>> The windows hook was indeed added in 2.0.16, released on Aug 10, 2021.
>>
>> Given the distribution nature of the Windows binaries, I think we
>> could simply depend on a much recent version without worrying about
>> compatibility with < 2.0.16. This would help reduce the potential
>> combinations of versions and bugs reports.
>
>Okay, I'll send a version 2 patch series.

Ping (for this patch, the others were merged)

>
>With best regards
>Volker
>
>>
>>>      gui_fullscreen = o->has_full_screen && o->full_screen;
>>>
>>> diff --git a/ui/win32-kbd-hook.c b/ui/win32-kbd-hook.c
>>> index 1ac237db9e..39d42134a2 100644
>>> --- a/ui/win32-kbd-hook.c
>>> +++ b/ui/win32-kbd-hook.c
>>> @@ -91,6 +91,9 @@ void win32_kbd_set_window(void *hwnd)
>>>              win32_unhook_notifier.notify = keyboard_hook_unhook;
>>>              qemu_add_exit_notifier(&win32_unhook_notifier);
>>>          }
>>> +    } else if (!hwnd && win32_keyboard_hook) {
>>> +        keyboard_hook_unhook(&win32_unhook_notifier, NULL);
>>> +        qemu_remove_exit_notifier(&win32_unhook_notifier);
>>>      }
>>>
>>>      win32_window = hwnd;
>>> --
>>> 2.35.3
>>>
>