SimulaVR / Simula

Linux VR Desktop
MIT License
2.97k stars 91 forks source link

Alt-tabbing causes the alt key to stay registered as stuck down #85

Closed georgewsinger closed 4 years ago

georgewsinger commented 5 years ago

@NerveCoordinator found this error by alt-tabbing in Simula.

Video. Here is a video demonstration of this error in godotston using xterm:

https://www.youtube.com/watch?v=5p8VZRvEQ9o&feature=youtu.be

In the video: first typing works fine in xterm, and then after I Alt + Tab I get weird characters showing up in xterm. Those weird characters show up normally when you hold Alt and type.

Quick fix. Pressing the Alt key again after experiencing this error makes it go away.

Bug replication. I am able to replicate this in godotston/Simula, but not rootston, suggesting it's a godot/gdwlroots issue.

Source code trace. Here are some relevant code snippets through gdwlroots/godotston:

# Main.gd
func _on_wlr_key(keyboard, event):
    var seat = get_node("WaylandDisplay/WlrSeat")
    seat.keyboard_notify_key(event)

func _on_wlr_modifiers(keyboard):
    var seat = get_node("WaylandDisplay/WlrSeat")
    seat.keyboard_notify_modifiers()

func _ready():
  #..
  keyboard.connect("key", self, "_on_wlr_key")
  keyboard.connect("modifiers", self, "_on_wlr_modifiers")
  #..
//wlr_keyboard.cpp
void WlrKeyboard::handle_key(struct wl_listener *listener, void *data) {
    WlrKeyboard *keyboard = wl_container_of(listener, keyboard, key);
    struct wlr_event_keyboard_key *event =
        (struct wlr_event_keyboard_key *)data;
    auto gdevent = new WlrEventKeyboardKey(event);
    keyboard->emit_signal("key", keyboard, gdevent);
}

void WlrKeyboard::handle_modifiers(struct wl_listener *listener, void *data) {
    WlrKeyboard *keyboard = wl_container_of(listener, keyboard, modifiers);
    keyboard->emit_signal("modifiers", keyboard);
}

void WlrKeyboard::_input(const Ref<InputEvent> &p_event) {
  struct timespec now;
  clock_gettime(CLOCK_MONOTONIC, &now);
  Ref<InputEventKey> k = p_event;
  if (k.is_valid()) {
    struct wlr_event_keyboard_key event = { 0 };
    event.time_msec = timespec_to_msec(&now);
    event.keycode = eudev_from_godot(k->get_scancode()); //old
    //event.keycode = k->get_raw_keycode(); //Attempt to use David hack; causes weird keyboard behavior
    event.state = k->is_pressed() ? WLR_KEY_PRESSED : WLR_KEY_RELEASED;
    event.update_state = true;
    wlr_keyboard_notify_key(&wlr_keyboard, &event);
  }
}

//wlr_seat.cpp
void WlrSeat::keyboard_notify_key(Variant _key_event) {
  struct timespec now;
  clock_gettime(CLOCK_MONOTONIC, &now);
  auto key_event = dynamic_cast<WlrEventKeyboardKey *>((Object *)_key_event);
  auto event = key_event->get_wlr_event();
  wlr_seat_keyboard_notify_key(wlr_seat, timespec_to_msec(&now),
      event->keycode, event->state);
}

void WlrSeat::keyboard_notify_modifiers() {
  struct timespec now;
  clock_gettime(CLOCK_MONOTONIC, &now);
  struct wlr_keyboard *keyboard = wlr_seat_get_keyboard(wlr_seat);
  wlr_seat_keyboard_notify_modifiers(wlr_seat, &keyboard->modifiers);
}

void WlrSeat::keyboard_notify_enter(Variant _surface) {
  auto surface = dynamic_cast<WlrSurface *>((Object *)_surface);
  if (surface) {
  struct wlr_keyboard *keyboard = wlr_seat_get_keyboard(wlr_seat);
  wlr_seat_keyboard_notify_enter(wlr_seat, surface->get_wlr_surface(),
    keyboard->keycodes, keyboard->num_keycodes, &keyboard->modifiers);
  }
}

Tracing a godot keypress thus looks something like:

Godot keypress -> WlrKeyboard::_input(InputEventKey) 
               -> wlr_keyboard_notify_key(wlr_keyboard, 
                                          {wlr_event_keyboard_key.time_msec, 
                                           wlr_event_keyboard_key.eudev_from_godot(InputEventKey->get_scancode()), //keycode
                                           wlr_event_keyboard_key.WLR_KEY_(PRESSED|RELEASED) //state
                                           wlr_event_keyboard_key.update_state = true}}

               -> Emits wlroots "key" and "modifiers" signal

               -> WlrKeyboard::handle_key emits Godot signal key(WlrKeyboard, WlrEventKeyboardKey)
               -> "key" 
               -> _on_wlr_key 
               -> WlrSeat::keyboard_notify_key  
               -> wlr_seat_keyboard_notify_key(.., event->keycode, event->state) //update active client with keypress

xev test. A godotston xev test shows that Alt-Tabbing registers neither the Alt key release nor the Tab press/release:

  1. Normal alt press in godotston.

    KeyPress event, serial 35, synthetic NO, window 0x400001,
        root 0x29c, subw 0x0, time 269261332, (1021,169), root:(1021,169),
        state 0x0, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES,
        XLookupString gives 0 bytes: 
        XmbLookupString gives 0 bytes: 
        XFilterEvent returns: False
    
    KeyRelease event, serial 35, synthetic NO, window 0x400001,
        root 0x29c, subw 0x0, time 269261565, (1021,169), root:(1021,169),
        state 0x8, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES,
        XLookupString gives 0 bytes: 
        XFilterEvent returns: False
  2. Alt-tab.

KeyPress event, serial 35, synthetic NO, window 0x400001,
    root 0x29c, subw 0x0, time 269313049, (1023,365), root:(1023,365),
    state 0x0, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES,
    XLookupString gives 0 bytes: 
    XmbLookupString gives 0 bytes: 
    XFilterEvent returns: False

Hypothesis. So I think what is happening is:

  1. A user presses Alt, so godotston/Simula registers an Alt.
  2. The user then presses Tab, which their system intercepts (before Simula) and translates into an app change.
  3. …but Simula/godotston still just sees Alt and no Tab.
  4. So when you keep typing Simula/godotston still thinks Alt is down, causing the weird behavior.
georgewsinger commented 5 years ago

Disabling the global Alt+Tab shortcut in Ubuntu and pressing the key combo in xev causes it to successfully register.

KeyPress event, serial 35, synthetic NO, window 0x400001,
    root 0x29c, subw 0x0, time 334851848, (1022,462), root:(1022,462),
    state 0x0, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES,
    XLookupString gives 0 bytes: 
    XmbLookupString gives 0 bytes: 
    XFilterEvent returns: False

KeyPress event, serial 35, synthetic NO, window 0x400001,
    root 0x29c, subw 0x0, time 334851898, (1022,462), root:(1022,462),
    state 0x8, keycode 23 (keysym 0xff09, Tab), same_screen YES,
    XLookupString gives 1 bytes: (09) " "
    XmbLookupString gives 1 bytes: (09) "   "
    XFilterEvent returns: False

KeyRelease event, serial 35, synthetic NO, window 0x400001,
    root 0x29c, subw 0x0, time 334852014, (1022,462), root:(1022,462),
    state 0x8, keycode 64 (keysym 0xffe9, Alt_L), same_screen YES,
    XLookupString gives 0 bytes: 
    XFilterEvent returns: False

KeyRelease event, serial 35, synthetic NO, window 0x400001,
    root 0x29c, subw 0x0, time 334852014, (1022,462), root:(1022,462),
    state 0x0, keycode 23 (keysym 0xff09, Tab), same_screen YES,
    XLookupString gives 1 bytes: (09) " "
    XFilterEvent returns: False
georgewsinger commented 5 years ago

Printing keycodes from Main.gd shows that Godot never receives the Tab press when pressing Alt + Tab.

# Main.gd print test
func _input(ev):
  if ev is InputEventKey:
    print(ev.scancode)
16777240

With Alt+Tab enabled in Ubuntu (default).

16777240

With Alt+Tab disabled in Ubuntu.

16777240
16777218
16777240
16777218
georgewsinger commented 5 years ago

WM_FOCUS_OUT hack. One solution to this problem is sending an Alt KeyRelease event whenever Godot loses focus. Attempting to do this in wlr_seat.cpp

//wlr_seat.cpp
void WlrSeat::_notification(int p_what) {
  switch (p_what) {
  case MainLoop::NOTIFICATION_WM_FOCUS_IN:
    std::cout << "SEAT FOCUS IN" << std::endl;
    break;
  case MainLoop::NOTIFICATION_WM_FOCUS_OUT:
    std::cout << "SEAT FOCUS OUT" << std::endl;
    //Send an Alt KeyRelease event in order to prevent Alt-Tabbing causing Alt to stay stuck down
    struct timespec now;
    clock_gettime(CLOCK_MONOTONIC, &now);
    wlr_seat_keyboard_notify_key(wlr_seat, timespec_to_msec(&now), 56, 0);
    break;
  default:
    return;
  }
}

causes crash (immediately on startup):

OpenGL ES 2.0 Renderer: GeForce GTX 1070/PCIe/SSE2
Running Wayland server on display godot-0
handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x43f60) [0x7f108717bf60] (??:0)
[2] /usr/local/lib/x86_64-linux-gnu/libwlroots.so.0(wlr_seat_set_keyboard+0x2e) [0x7f10876d144f] (??:0)
[3] WlrSeat::set_keyboard(Variant) (/home/george/Simula/build/godot/modules/gdwlroots/wlr_seat.cpp:197 (discriminator 4))
[4] MethodBind1<Variant>::call(Object*, Variant const**, int, Variant::CallError&) (/home/george/Simula/build/godot/./core/method_bind.gen.inc:729 (discriminator 12))
[5] Object::call(StringName const&, Variant const**, int, Variant::CallError&) (/home/george/Simula/build/godot/core/object.cpp:945 (discriminator 1))
[6] Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) (/home/george/Simula/build/godot/core/variant_call.cpp:1048 (discriminator 1))
[7] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) (/home/george/Simula/build/godot/modules/gdscript/gdscript_function.cpp:1073)
[8] GDScriptInstance::_ml_call_reversed(GDScript*, StringName const&, Variant const**, int) (/home/george/Simula/build/godot/modules/gdscript/gdscript.cpp:1204)
[9] GDScriptInstance::call_multilevel_reversed(StringName const&, Variant const**, int) (/home/george/Simula/build/godot/modules/gdscript/gdscript.cpp:1213)
[10] Node::_notification(int) (/home/george/Simula/build/godot/scene/main/node.cpp:139)
[11] Node::_notificationv(int, bool) (/home/george/Simula/build/godot/./scene/main/node.h:46 (discriminator 14))
[12] CanvasItem::_notificationv(int, bool) (/home/george/Simula/build/godot/./scene/2d/canvas_item.h:166 (discriminator 3))
[13] Node2D::_notificationv(int, bool) (/home/george/Simula/build/godot/./scene/2d/node_2d.h:38 (discriminator 3))
[14] Object::notification(int, bool) (/home/george/Simula/build/godot/core/object.cpp:957)
[15] Node::_propagate_ready() (/home/george/Simula/build/godot/scene/main/node.cpp:184)
[16] Node::_propagate_ready() (/home/george/Simula/build/godot/scene/main/node.cpp:173 (discriminator 2))
[17] Node::_set_tree(SceneTree*) (/home/george/Simula/build/godot/scene/main/node.cpp:2520)
[18] SceneTree::init() (/home/george/Simula/build/godot/scene/main/scene_tree.cpp:457)
[19] OS_X11::run() (/home/george/Simula/build/godot/platform/x11/os_x11.cpp:2953)
[20] ../../../bin/godot.x11.tools.64(main+0xdc) [0x115f91e] (/home/george/Simula/build/godot/platform/x11/godot_x11.cpp:56)
[21] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xeb) [0x7f108715eb6b] (??:0)
[22] ../../../bin/godot.x11.tools.64(_start+0x2a) [0x115f78a] (??:?)
-- END OF BACKTRACE --
bash: line 1: 16088 Aborted                 (core dumped) ../../../bin/godot.x11.tools.64 -m
georgewsinger commented 5 years ago

I started a thread on /r/godot to help figure out what's causing this crash.

georgewsinger commented 5 years ago

Setting WlrSeat::_notification causes wlr_seat to become NULL. The issue is that when we set

//wlr_seat.cpp
void WlrSeat::_notification(int p_what) {
  //..
}

then the cal to seat.set_keyboard(keyboard) in Main.gd crashes godotston:

func _ready():
  # ..
  var seat = get_node("WaylandDisplay/WlrSeat")
  var keyboard = get_node("WaylandDisplay/WlrKeyboard")
  seat.set_keyboard(keyboard)
  # ..

because the seat is NULL:

0x00007ffff746e44f in wlr_seat_set_keyboard (seat=0x0, device=0x60b6d98)
    at ../types/seat/wlr_seat_keyboard.c:126
126     if (seat->keyboard_state.keyboard == keyboard) {

This crash occurs even when _notification is literally blank:

void WlrSeat::_notification(int p_what) {
}

Question: How could setting _notification cause a break like this?

georgewsinger commented 5 years ago

The issue was that WlrSeat inherits from WaylandGlobal, which implements its own _notification:

void WaylandGlobal::_notification(int p_what) {
  switch (p_what) {
  case NOTIFICATION_ENTER_TREE:
    ensure_wl_global(get_wayland_display());
    break;
  case NOTIFICATION_EXIT_TREE:
    destroy_wl_global(get_wayland_display());
    break;
  }
}

These functions are crucial for the functioning of WlrSeat (including for setting wlr_seat state). Thus extending WlrSeat's _notification guts to include the above prevents godotston from crashing:

void WlrSeat::_notification(int p_what) {
  switch (p_what) {
  case NOTIFICATION_ENTER_TREE:
    ensure_wl_global(get_wayland_display());
    break;
  case NOTIFICATION_EXIT_TREE:
    destroy_wl_global(get_wayland_display());
    break;
  case MainLoop::NOTIFICATION_WM_FOCUS_IN:
    break;
  case MainLoop::NOTIFICATION_WM_FOCUS_OUT:
    //Send an Alt KeyRelease event in order to prevent Alt-Tabbing causing Alt to stay stuck down
    struct timespec now;
    clock_gettime(CLOCK_MONOTONIC, &now);
    wlr_seat_keyboard_notify_key(wlr_seat, timespec_to_msec(&now), 56, 0);
    break;
  default:
    return;
  }
}

I'm commuting to my VR station now to test whether this solution actualy fixes the Alt-Tab issue. If it does I'll push the code and commit.

georgewsinger commented 5 years ago

:heavy_check_mark: The code above seems to work.

georgewsinger commented 5 years ago

New problems. Unfortunately, we're still having problems with the following usage patterns (@NerveCoordinator):

1) open Simula
2) DISPLAY=:2 xterm
3) can type fine
4) alt-tab, click back to Simula
5) can still type fine (this is new)
6) DISPLAY=:2 gedit
7) alt-typing problems on both windows

1) open Simula
2) DISPLAY=:2 xterm
3) DISPLAY=:2 gedit
4) can type normally
5) alt-tab
6) click back to Simula
7) can type normally
8) point controller at the other window
9) alt-typing problems on both windows

1) open Simula
2) alt-tab to terminal
3) DISPLAY=:2 xterm
4) alt-typing problems

Querying XKB State to see if Alt is pressed. A more sophisticated approach fails to fix the above issues:

    void WlrSeat::_notification(int p_what) {
      switch (p_what) {
      case NOTIFICATION_ENTER_TREE:
        ensure_wl_global(get_wayland_display());
        break;
      case NOTIFICATION_EXIT_TREE:
        destroy_wl_global(get_wayland_display());
        break;
      case MainLoop::NOTIFICATION_WM_FOCUS_IN:
        break;
      case MainLoop::NOTIFICATION_WM_FOCUS_OUT:
        {
        //Check if Alt is pressed and, if so, send an Alt KeyRelease event
    Send an Alt KeyPress + KeyRelease event in order to prevent Alt-Tabbing causing Alt to stay stuck down
        struct timespec now;
        clock_gettime(CLOCK_MONOTONIC, &now);
        auto w_keyboard = wlr_seat_get_keyboard(wlr_seat);
        auto is_alt_pressed = xkb_state_mod_name_is_active(w_keyboard->xkb_state, XKB_MOD_NAME_ALT, XKB_STATE_MODS_EFFECTIVE);
        if (is_alt_pressed) {
          //wlr_seat_keyboard_notify_key(wlr_seat, timespec_to_msec(&now), 56, 1);  //adding this also doesn't help
          wlr_seat_keyboard_notify_key(wlr_seat, timespec_to_msec(&now), 56, 0);
          //wlr_seat_keyboard_notify_modifiers(wlr_seat, &w_keyboard->modifiers); //nor does this
        }
        }
        break;
      default:
        return;
      }
    }

I think what is going on is that wlr_seat_keyboard_notify_key only sends the keyboard-focused surface Alt KeyRelease event. For all other new surfaces, Alt is still somehow stuck pressed down. Possibly relevant functions from wlroots:

    /**
     * Send the keyboard key to focused keyboard resources. Compositors should use
     * `wlr_seat_notify_key()` to respect keyboard grabs.
     */
    void wlr_seat_keyboard_send_key(struct wlr_seat *seat, uint32_t time,
        uint32_t key, uint32_t state);

    /**
     * Notify the seat that a key has been pressed on the keyboard. Defers to any
     * keyboard grabs.
     */
    void wlr_seat_keyboard_notify_key(struct wlr_seat *seat, uint32_t time,
        uint32_t key, uint32_t state);

    /**
     * Send the modifier state to focused keyboard resources. Compositors should use
     * `wlr_seat_keyboard_notify_modifiers()` to respect any keyboard grabs.
     */
    void wlr_seat_keyboard_send_modifiers(struct wlr_seat *seat,
        struct wlr_keyboard_modifiers *modifiers);

    /**
     * Notify the seat that the modifiers for the keyboard have changed. Defers to
     * any keyboard grabs.
     */
    void wlr_seat_keyboard_notify_modifiers(struct wlr_seat *seat,
        struct wlr_keyboard_modifiers *modifiers);

Alt-Tab Ubuntu script. One hack is to use the following Ubuntu script adapted for Simula which monitors the currently active program and, if it's Simula, disables the Alt-Tab shortcut at the OS level:

    #!/bin/bash

    keySwitchApplication="switch-applications"
    keySwitchApplicationBackward="switch-applications-backward"

    backupSwitchApplications="$(gsettings get org.gnome.desktop.wm.keybindings "$keySwitchApplication")"
    disableSwitchApplications="$(gsettings get org.gnome.desktop.wm.keybindings "$keySwitchApplication" | sed "s/\,*\s*'<Alt>Tab'//")"

    backupSwitchApplicationsBackward="$(gsettings get org.gnome.desktop.wm.keybindings "$keySwitchApplicationBackward")"
    disableSwitchApplicationsBackward="$(gsettings get org.gnome.desktop.wm.keybindings "$keySwitchApplicationBackward" | sed "s/\,*\s*'<Shift><Alt>Tab'//")"

    disabled="0"

    while true; do
      isActive=$(wmctrl -lx | awk -v search=$(printf 0x0%x $(xdotool getactivewindow)) -v wm_class="$wm_class" '{ if($1 ~ search && $3 ~ /Simula/) print $3 }')

      if [[ "$isActive" != "" ]]; then
        # echo "active"
        if [[ "$disabled" == "0" ]]; then
          # echo "disable shortcut"
          gsettings set org.gnome.desktop.wm.keybindings "$keySwitchApplication" "$disableSwitchApplications"
          gsettings set org.gnome.desktop.wm.keybindings "$keySwitchApplicationBackward" "$disableSwitchApplicationsBackward"
          disabled="1";
        fi
      else
        # echo "not active"
        if [[ "$disabled" == "1" ]]; then
          # echo "enable shortcut"
          gsettings set org.gnome.desktop.wm.keybindings "$keySwitchApplication" "$backupSwitchApplications"
          gsettings set org.gnome.desktop.wm.keybindings "$keySwitchApplicationBackward" "$backupSwitchApplicationsBackward"
          disabled="0"
        fi;
      fi;
      sleep 1
    done

This can be reset via

$ gsettings reset org.gnome.desktop.wm.keybindings switch-applications  
$ gsettings get org.gnome.desktop.wm.keybindings switch-applications   
['<Super>Tab', '<Alt>Tab']

$ gsettings reset org.gnome.desktop.wm.keybindings switch-applications-backward
$ gsettings get org.gnome.desktop.wm.keybindings switch-applications-backward  
['<Shift><Super>Tab', '<Shift><Alt>Tab']

The problem is that this script doesn't work on my machine :[.

georgewsinger commented 5 years ago

I opened up a post on askubuntu to see if we can get some community help here.

georgewsinger commented 4 years ago

Fixed in https://github.com/SimulaVR/Simula/commit/0b8ce56d16ea799dcb6adf55fb575f4497cb436f