Skip to content
Snippets Groups Projects

winewayland.drv: Update key state from Wayland information.

Open Alexandros Frantzis requested to merge afrantzis/wine:wayland-sync-keyboard-state into master
5 unresolved threads

Recent changes allowed the Wayland driver to properly deal with numpad keys depending on numlock status. This MR ensures that numlock and other lock state is properly synced, and concludes the fix for https://bugs.winehq.org/show_bug.cgi?id=56397.

This MR also syncs the key press state when a window gains keyboard focus, including any modifier press state. Note that we currently ignore the modifier press information from the modifier event, since: 1. it doesn't differentiate between left-right keys, 2. mod press state changes will be normally preceded by a matching key event, so we are able to sync properly. However, if we find that we need to handle mod press state changes without corresponding key events, we will need to implement some sensible way to sync these, too.

Although I would like for the driver work exclusively in terms of scancodes, I still needed to use vkeys in this case, since this is how wineserver expresses keyboard state at the moment. This means that I had to introduce/duplicate some extra scan->vkey logic in the driver (e.g., numpad keys based on numlock state) to get things right.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
684 /* If the vkey already has the requested lock state there is nothing to do. */
685 if (prev_lock == lock) return;
686
687 TRACE_(key)("vkey=0x%03x lock=%d state=0x%02x\n", vkey, lock, keystate[vkey]);
688
689 send_vkey(hwnd, vkey, 0);
690 send_vkey(hwnd, vkey, KEYEVENTF_KEYUP);
691
692 /* Ensure we have the proper state in case key events were blocked by hooks. */
693 if (get_async_key_state(keystate) && !!(keystate[vkey] & 0x01) == prev_lock)
694 {
695 WARN("keystate %x not changed (%#.2x), probably blocked by hooks\n",
696 vkey, keystate[vkey]);
697 keystate[vkey] ^= 0x01;
698 set_async_key_state(keystate);
699 }
  • Comment on lines +689 to +699

    I understand the need to update the keyboard state, and I know winex11 does this, but sending key input when window gains focus feels wrong to me.

  • Please register or sign in to reply
  • Rémi Bernon
    Rémi Bernon @rbernon started a thread on commit 1207c9ac
  • 1019 if ((have_pressed_keys = keyboard->have_pressed_keys_on_enter))
    1020 {
    1021 pressed_keys = keyboard->pressed_keys_on_enter;
    1022 wl_array_init(&keyboard->pressed_keys_on_enter);
    1023 keyboard->have_pressed_keys_on_enter = FALSE;
    1024 }
    922 1025 pthread_mutex_unlock(&keyboard->mutex);
    923 1026
    924 1027 set_current_xkb_group(xkb_group);
    925 1028
    1029 /* Update the modifier key press state first, so that subsequent pressed
    1030 * keys will be modified accordingly and the lock state state logic, which
    1031 * depends on having mod key events before modifiers, will work properly. */
    1032 if (have_pressed_keys) update_keyboard_pressed_keys(hwnd, &pressed_keys, TRUE);
    926 1033 update_keyboard_lock_state(hwnd, keyboard->xkb_state);
    1034 if (have_pressed_keys) update_keyboard_pressed_keys(hwnd, &pressed_keys, FALSE);
  • Rémi Bernon
    Rémi Bernon @rbernon started a thread on an outdated change in commit 3fc62849
  • 648 658 /* Skip left/right-agnostic modifier vkeys. */
    649 659 if (vkey == VK_SHIFT || vkey == VK_CONTROL || vkey == VK_MENU) continue;
    650 660
    651 if (state[vkey] & 0x80)
    652 {
    653 UINT scan = NtUserMapVirtualKeyEx(vkey, MAPVK_VK_TO_VSC_EX,
    654 keyboard_hkl);
    655 input.ki.wVk = vkey;
    656 input.ki.wScan = scan & 0xff;
    657 input.ki.dwFlags = KEYEVENTF_KEYUP;
    658 if (scan & ~0xff) input.ki.dwFlags |= KEYEVENTF_EXTENDEDKEY;
    659 NtUserSendHardwareInput(hwnd, 0, &input, 0);
    660 }
    • Comment on lines -651 to -660

      Fwiw, I probably missed that previously and it may be what Wayland spec suggest, but I also don't think we should send key release input when focus is lost. That's not what Windows does.

    • So the concern (in this and other cases) is strictly about the key events, and you would prefer if we just updated the async keyboard state?

      Although that should work fine in many cases, one issue is autorepeat functionality, since it is handled (at the moment) in win32u based on key messages. We would like to:

      1. Stop autorepeat when a window loses focus.
      2. Start autorepeat of key if it is pressed when gaining focus.

      (1) is critical to get right, but seems easy enough to achieve without key event involvement. If this matches Windows behavior, it would make sense to integrate a "has focus" check into the win32u repeat timer callback. At the moment it needs to happen from the driver too because we don't update window focus when handling the keyboard leave event (but also a special NtUserSendHardwareInput invocation with all scancodes up (see below) could deal with that in win32u).

      (2) seems less straightforward, since there isn't currently any feedback from the wineserver to hwnd when the async key state changes. But if we use a special NtUserSendHardwareInput invocation as you suggest below, perhaps wineserver can detect the scenario and do something sensible here.

    • Hmm... after looking a bit more into it seems like I'm maybe wrong, but the trick may actually be that Windows apparently handles key repeat on the driver or hardware level. (Still, it doesn't send KEYUP on focus loss)

      So, the way I've implemented it, with a process timer that repeats WM_KEYDOWN message isn't correct, and in particular it won't trigger repetitions of rawinput messages (which could cause other input issues with winewayland).

      Maybe it would be better to implement it the same way as we now have pointer input repeat (see queue_pointer_message in server/queue.c), repeating input for any key that is pressed.

      Then, assuming input is dispatched correctly, and assuming wineserver is informed of focus loss so that the old window doesn't receive the repeated input, you could simply keep the repeating active until another window gains focus and can tell wineserver of the updated key state, clearing repeat timer or starting repeat again if any keys are still pressed.

      Edited by Rémi Bernon
    • Then, assuming input is dispatched correctly, and assuming wineserver is informed of focus loss so that the old window doesn't receive the repeated input, you could simply keep the repeating active until another window gains focus and can tell wineserver of the updated key state, clearing repeat timer or starting repeat again if any keys are still pressed.

      Alright, let me experiment a bit with these ideas. A couple of early thoughts:

      1. Concerning giving up focus on the keyboard leave event, I remember that we don't do it because of unfortunate side-effects, but I will need to revisit to remember the details and see if/how we can make some progress here.

      2. Wouldn't keeping the repeat active in wineserver cause unnecessary cpu usage? Should we effectively pause the repeat until we have a new focused window?

    • Wouldn't keeping the repeat active in wineserver cause unnecessary cpu usage? Should we effectively pause the repeat until we have a new focused window?

      I don't think we need to worry about a couple of wake-ups for key input for the rare cases where keys are kept pressed on focus loss, but sure, that's a possible optimization.

      Note that Windows applications can listen to input while in the background, and although that doesn't work very well for the moment, it's still supposedly the correct approach.

      OTOH we also don't receive any input when no Wine window is focused, so we could consider that it's not worth trying to be correct here. We also currently kind of miss the ability to tell whether no Wine window is active or whether the desktop window really is: winex11 focuses the desktop window when all windows are losing focus, but the virtual desktop window can very well be focused on its own.

      Edited by Rémi Bernon
    • Please register or sign in to reply
    • In order to avoid having to duplicate the scan2vk mapping in the drivers, I would suggest maybe extending the NtUserSendHardwareInput call, for instance with a custom flag and add support for syncing the full keystate, passing the state array in lparam (and some dummy INPUT_KEYBOARD structure).

      You could then probably re-use the send_hardware_message request HID report data to pass the new vkey state to wineserver, and handle the key updates there, that would save some get_async_keystate roundtrips.

    • If the driver needs to pass vkeys (that's what I understood by 'state array'), it already needs to do some translation along the lines of dlls/win32u/input.c:map_scan_to_kbd_vkey. Perhaps we can directly send scan codes instead and win32u can perform the translation to vkey[256] itself?:

      NtUserSendHardwareInput(SEND_HWMSG_SCANCODES, lparam=<array of pressed scancodes>)

    • Yes that's what I meant.

    • Please register or sign in to reply
  • llyyr
    llyyr @llyyr started a thread on commit 1207c9ac
  • 827 915
    828 916 pthread_mutex_lock(&keyboard->mutex);
    829 917 keyboard->focused_hwnd = hwnd;
    918 /* Store pressed keys to be processed after we receive the modifier state. */
    • Contributor

      How is this information used by winewayland? The protocol does not guarantee that the keys in the array are in the order they were pressed, this information only exists for things like on screen keyboards that may want to e.g. highlight currently held keys.

    • Please register or sign in to reply
    Please register or sign in to reply
    Loading