Modifier keys remain pressed after reload

g3r commented 3 months ago

Regression?

Yes

System Info and Version

System/Version info

```sh Hyprland, built from branch main at commit 682865632fe986d179ca07e45ae89e65b8058b33 (xwayland: fix high cpu idle usage). Date: Sat Jul 27 08:58:30 2024 Tag: v0.41.2-101-g68286563, commits: 4987 flags: (if any) System Information: System name: Linux Node name: KRONOS Release: 6.10.1-1-MANJARO Version: #1 SMP PREEMPT_DYNAMIC Wed Jul 24 17:43:42 UTC 2024 GPU information: 01:00.0 VGA compatible controller [0300]: NVIDIA Corporation GM206 [GeForce GTX 960] [10de:1401] (rev a1) (prog-if 00 [VGA controller]) NVRM version: NVIDIA UNIX x86_64 Kernel Module 555.58.02 Tue Jun 25 01:39:15 UTC 2024 os-release: NAME="Manjaro Linux" PRETTY_NAME="Manjaro Linux" ID=manjaro ID_LIKE=arch BUILD_ID=rolling ANSI_COLOR="32;1;24;144;200" HOME_URL="https://manjaro.org/" DOCUMENTATION_URL="https://wiki.manjaro.org/" SUPPORT_URL="https://forum.manjaro.org/" BUG_REPORT_URL="https://docs.manjaro.org/reporting-bugs/" PRIVACY_POLICY_URL="https://manjaro.org/privacy-policy/" LOGO=manjarolinux plugins: ```

Description

Modifier keys remain pressed after hyprctl reload. I tested SUPER ALT CONTROL and SHIFT

How to reproduce

sleep 2 && hyprctl reload While on sleep keep pressed a modifier and release it after reload is complete After reload, the modifier key remains active until it is pressed again

Crash reports, logs, images, videos

No response

MahouShoujoMivutilde commented 2 months ago

Running on a399f98c68d017152883fbf81d67624ac3254073

Now they still get stuck, but you have to press some other modifier after reload for it to kick in, and you still can get them unstuck by pressing the stuck modifier another time.

MahouShoujoMivutilde commented 2 months ago

Wrote a patch

diff --git a/src/devices/IKeyboard.cpp b/src/devices/IKeyboard.cpp
index e05cbd04..29c05a6d 100644
--- a/src/devices/IKeyboard.cpp
+++ b/src/devices/IKeyboard.cpp
@@ -112,6 +112,15 @@ void IKeyboard::setKeymap(const SStringRuleNames& rules) {

     const auto NUMLOCKON = g_pConfigManager->getDeviceInt(hlName, "numlock_by_default", "input:numlock_by_default");

+    // THE """FIX:""" reset modifiers
+    // probably a wrong place to put it, but IDK
+    modifiersState.depressed = 0;
+    // modifiersState.latched = 0; // no, will fuck up diacritics input: e.g. `br` layout, press ` (to the right of P) then A
+    // modifiersState.locked = 0; // are those even a problem?
+    updateModifiers(modifiersState.depressed, modifiersState.latched, modifiersState.locked, modifiersState.group);
+    Debug::log(LOG, "Reset modifier keys for keyboard {}:  depressed = {}, latched = {}, locked = {}, group as before",
+               deviceName, modifiersState.depressed, modifiersState.latched, modifiersState.locked);
+
     if (NUMLOCKON == 1) {
         // lock numlock
         const auto IDX = xkb_map_mod_get_index(xkbKeymap, XKB_MOD_NAME_NUM);

So on reload with it whatever mods you were holding down (if any) will be considered not pressed until you lift the key up and press again, works on 2d552fbaa25f1457c3819521a2750dd30820271b.

Maybe it should be not 0, but instead some XKB_MOD_VERY_UNDEPRESSED, but I haven't found anything like that.

Unsolved problems:

Caps is actually reset somewhere else, but LED stays on after reload. Can be pressed again to correct it. Can be fixed by setting it to 0, like in the comment, but not sure if it's the right place - are those even supposed to be reset?
Diacritics input: if `, the key for all those áńḿṕóý etc to the right of P in br (Brazil) layout, is latched right before the reload - until another mod is pressed you can't latch it again (also a problem on main).

I think it solves the worst of it.

But maybe should be done in some other place. I've tried to add something similar at the end of void CInputManager::setupKeyboard() here:

https://github.com/hyprwm/Hyprland/blob/9a09eac79b85c846e3a865a9078a3f8ff65a9259/src/managers/input/InputManager.cpp#L922-L924

But it didn't do anything.

So - try it out.

shouya commented 2 months ago

Now they still get stuck, but you have to press some other modifier after reload for it to kick in, and you still can get them unstuck by pressing the stuck modifier another time.

There are cases where pressing the stuck modifier won't destuck it. When that happens, even unplugging the usb keyboard won't help so I have to resort to restarting the compositor. I'm guessing it may have to do with extra kb_options I have. Here's my config for reference.

input {
    kb_layout = us
    kb_options = ctrl:nocaps,altwin:swap_lalt_lwin
    repeat_rate = 45
    repeat_delay = 200

    follow_mouse = 0
    mouse_refocus = false
    float_switch_override_focus = 0

    touchpad {
        natural_scroll = false
    }
    accel_profile = flat
}

MahouShoujoMivutilde commented 2 months ago

Idk, I just tested it and couldn't reproduce on 0.42.0 release and 83d88fa56467a2b749fb2320e1595281107bd326. Are you on something older?

In fact, thanks to your input config, I've learned that the root cause that allows mods to be stuck is input:numlock_by_default = yes.

When it's no/unset I can't get mods to get stuck on reload.

How I test it:

sleep 2; hyprctl reload
hold Shift until reload triggers
type something - letters aren't capitalized
tap Ctrl
type something - letters are capitalized (if numlock_by_default = yes)
tap Shift
type something - letters aren't capitalized

Here is a new, smaller patch, accounting for it.

diff --git a/src/devices/IKeyboard.cpp b/src/devices/IKeyboard.cpp
index e05cbd04..c9b467c4 100644
--- a/src/devices/IKeyboard.cpp
+++ b/src/devices/IKeyboard.cpp
@@ -119,6 +119,9 @@ void IKeyboard::setKeymap(const SStringRuleNames& rules) {
         if (IDX != XKB_MOD_INVALID)
             modifiersState.locked |= (uint32_t)1 << IDX;

+        // make sure mods aren't stuck on reload
+        modifiersState.depressed = 0;
+
         updateModifiers(modifiersState.depressed, modifiersState.latched, modifiersState.locked, modifiersState.group);
     }

When numlock_by_default is yes, pressed modifiers are forcefully reset on reload.

With it:

caps led no longer becomes desynced with its state, the state is kept too
latched keys (diacritics input) no longer ignored until another mod is pressed

Doesn't seem to break anything else...?

shogeki commented 2 months ago

Thanks for the patch.

This is problematic if I want to hold down a mod key to do multiple commands in a row with one or more scripts that modify the hyprland config. (e.g. open hyprland config in micro editor, edit config, hold CTRL, press S then Q to save then quit)

at some point it was working normally with keyboard state after a change, might be possible to bisect?

MahouShoujoMivutilde commented 2 months ago

After greping around v0.41.2, it seems it used to have a slightly different logic.

https://github.com/hyprwm/Hyprland/blame/918d8340afd652b011b937d29d5eea0be08467f5/src/managers/input/InputManager.cpp#L993-L1002

Instead of changing modifiersState variable and calling updateModifiers() using its new values, it just called wlroots' equivalent of updateModifiers() with everything aside from .locked set to zero (meaning no mod keys are considered pressed/active except the num/caps/etc-locks), while hyprland's modifiersState equivalent stayed unchanged aside from .locked.

wlr_keyboard_notify_modifiers() looks very similar to hyprland's updateModifiers(), so that's not a problem here.

I don't remember why I decided to change the modifiersState instead of calling updateModifiers() with zeros, I did try it in the first versions of the patch (that were useless), that added the reset at the end of CInputManager::setupKeyboard(), I know that it didn't work there.

Well, doesn't hurt to try again here.

New patch:


diff --git a/src/devices/IKeyboard.cpp b/src/devices/IKeyboard.cpp
index e05cbd04..a5fd1c1e 100644
--- a/src/devices/IKeyboard.cpp
+++ b/src/devices/IKeyboard.cpp
@@ -119,7 +119,7 @@ void IKeyboard::setKeymap(const SStringRuleNames& rules) {
         if (IDX != XKB_MOD_INVALID)
             modifiersState.locked |= (uint32_t)1 << IDX;

-        updateModifiers(modifiersState.depressed, modifiersState.latched, modifiersState.locked, modifiersState.group);
+        updateModifiers(0, 0, modifiersState.locked, modifiersState.group);
     }

     for (size_t i = 0; i < LEDNAMES.size(); ++i) {

This is pretty much the logic that hyprland used to have, and for a long time (see blame above), with a small exception that it doesn't reset your keyboard layout (language).

NOTE: modifiersState is still updated to the correct values, but it's done inside updateModifiersState() after new xkbState takes effect.

I tried editing config in micro like you said - and yes, it now works, saves and quits without lifting a finger from Ctrl just fine.

MahouShoujoMivutilde commented 2 months ago

I was about to submit PR to get things moving, but after testing one last time, I found that this part

I tried editing config in micro like you said - and yes, it now works, saves and quits without lifting a finger from Ctrl just fine.

doesn't work anymore.

Naturally, I became very confused, because as you can see in blame - it was updating with 0 for everything except latched since about 2 years ago (disregarding new hyprland wrapper to store keyboard device since it's still using wlroots function to update the mods), and as we established here update.../notify... functions inside are almost identical.

But if I press it fast enough - it works again. Not because it somehow preserves depressed state without making it stuck, but simply because its processed faster than reload is detected/performed.

So you can still use it like you did, but you can't (as I falsely assumed) do: hold Ctrl, press S, (reload), wait 5 seconds, press Q.

hyprwm / Hyprland