diff mbox series

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

Message ID 20240909061552.6122-1-vr_qemu@t-online.de (mailing list archive)
State New, archived
Headers show
Series [1/3] ui/sdl2: reenable the SDL2 Windows keyboard hook procedure | expand

Commit Message

Volker Rümelin Sept. 9, 2024, 6:15 a.m. UTC
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(-)

Comments

Marc-André Lureau Sept. 9, 2024, 7:26 a.m. UTC | #1
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)

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.

>      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
>
Stefan Weil Sept. 9, 2024, 8:02 a.m. UTC | #2
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
[...]
>> @@ -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)
>
> 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.

[...]

I agree. My builds for Windows typically use the very latest versions, 
for example mingw-w64-i686-SDL2-2.30.7-1-any for the next build. So 
depending on a recent SDL version would be fine for me.

Stefan W.
Volker Rümelin Sept. 9, 2024, 7:38 p.m. UTC | #3
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.

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
>>
Philippe Mathieu-Daudé Sept. 11, 2024, 11:57 a.m. UTC | #4
Hi Volker,

On 9/9/24 21:38, Volker Rümelin wrote:
> 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(-)


>> 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.

Queuing patches 2 & 3 meanwhile. Please do not bury series within
threads, if Bernhard hadn't sent his T-b tag, I'd have missed it.

Regards,

Phil.
Philippe Mathieu-Daudé Sept. 11, 2024, 11:59 a.m. UTC | #5
On 11/9/24 13:57, Philippe Mathieu-Daudé wrote:
> Hi Volker,
> 
> On 9/9/24 21:38, Volker Rümelin wrote:
>> 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(-)
> 
> 
>>> 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.
> 
> Queuing patches 2 & 3 meanwhile. Please do not bury series within
> threads, if Bernhard hadn't sent his T-b tag, I'd have missed it.

Oh actually this is a series with a cover, but the cover subject
doesn't contains 'PATCH' which is why my git tool missed it.
Bernhard Beschow Sept. 11, 2024, 12:16 p.m. UTC | #6
Am 11. September 2024 11:59:25 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 11/9/24 13:57, Philippe Mathieu-Daudé wrote:
>> Hi Volker,
>> 
>> On 9/9/24 21:38, Volker Rümelin wrote:
>>> 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(-)
>> 
>> 
>>>> 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.
>> 
>> Queuing patches 2 & 3 meanwhile. Please do not bury series within
>> threads, if Bernhard hadn't sent his T-b tag, I'd have missed it.
>
>Oh actually this is a series with a cover, but the cover subject
>doesn't contains 'PATCH' which is why my git tool missed it.
>

For me, `git am` didn't work after downloading the series' mbox. I had to download each patch mail individually and apply one by one. Then I could test the series.
Bernhard Beschow Dec. 7, 2024, 12:46 p.m. UTC | #7
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
>>>
>
diff mbox series

Patch

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;
+    }
 
     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;