emoon / rust_minifb

Cross platfrom window and framebuffer crate for Rust
MIT License
1k stars 95 forks source link

Strange keyboard behavior on wayland #288

Closed vmedea closed 2 years ago

vmedea commented 2 years ago

I'm seeing strange keyboard behavior on wayland. It looks like keycodes are misinterpreted. This is most apparent running the char_callback example:

It seems like the key to character mapping is wrong, but I don't know anything about xkb internals as to what this could mean.

Context:

I tried forcing to use Xwayland by unsetting WAYLAND_DISPLAY, but this runs into a different issue.

vmedea commented 2 years ago

I think the problem is that a keycode gets interpreted as a XCB key without going through the keymap. The following seems to work, except for modifiers:

diff --git a/src/os/posix/wayland.rs b/src/os/posix/wayland.rs
index 1783d519a983ab4328ec2f83e800d0d040030667..2713ba1f0a38a6c131475f2de552481cf8d0465b 100644
--- a/src/os/posix/wayland.rs
+++ b/src/os/posix/wayland.rs
@@ -777,16 +777,17 @@ impl Window {
                             state,
                             &mut self.key_handler,
                         );
-                    }

-                    if state == wl_keyboard::KeyState::Pressed {
-                        let keysym = xkb::Keysym(key);
-                        let code_point = keysym.utf32();
-                        if code_point != 0 {
-                            // Taken from GLFW
-                            if !(code_point < 32 || (code_point > 126 && code_point < 160)) {
-                                if let Some(ref mut callback) = self.key_handler.key_callback {
-                                    callback.add_char(code_point);
+                        if state == wl_keyboard::KeyState::Pressed {
+                            let keymap_state = keymap.state();
+                            let key_xkb = keymap_state.key(key + KEY_XKB_OFFSET);
+                            if let Some(keysym) = key_xkb.sym() {
+                                let code_point = keysym.utf32();
+                                // Taken from GLFW
+                                if !(code_point < 32 || (code_point > 126 && code_point < 160)) {
+                                    if let Some(ref mut callback) = self.key_handler.key_callback {
+                                        callback.add_char(code_point);
+                                    }
                                 }
                             }
                         }
emoon commented 2 years ago

Thanks for the report. I'm not sure what makes sense for Wayland as I have only accepeted PRs for this. If you think this is the correct fix can you make a PR for it?

vmedea commented 2 years ago

No, I think my fix there is awful, it duplicates the code from handle_key, I was trying to illustrate the problem :smile: It's also not complete. For example I don't know why modifiers (shift for caps, mainly!) don't work. As far as I see the ModifierEvent updates get correctly fed into xkb, but something wonky must be going on behind the scenes.

I might get around to debugging it and coming up with a better fix, but can't guarantee anything.

BTW: The xkb crate is unmaintained and personally think its API is a bit messy. Would you mind depending on sctk for wayland support? (it's the same crate that winit uses for that) It's still very low-level but has the keymap and xkb functionality and gives utf-8 in its keyboard events so no need to reinvent it.

Edit: oh, I see you use xkb for X11 too. Probably better to stick with it, then.

emoon commented 2 years ago

I actually would like a xkb replacement as it drags in quite a bunch of dependencies as outlined here https://github.com/emoon/rust_minifb/issues/237

vmedea commented 2 years ago

I actually would like a xkb replacement as it drags in quite a bunch of dependencies as outlined here https://github.com/emoon/rust_minifb/issues/237

Right—I looked at this, but to do this properly would mean implementing keymap parsing (a text-based format as documented not very thoroughly here) in pure rust *. A subset would likely be enough for minifb. Alternatively it might work to hardcode a static map (also mentioned in https://github.com/emoon/rust_minifb/issues/237#issuecomment-855416160), but that creates i18n problems.

* crate xkb-parser looks like it maybe? It depends on pest for parsing.

emoon commented 2 years ago

That is kinda annoying :( xbk-parser doesn't look that bad, but no updates in 3 years seems a bit worring. Maybe it would be possible to duplicate some of the xkb code, but get rid of the clang dependencies and such? Open for suggestions here.

vmedea commented 2 years ago

That is kinda annoying :( xbk-parser doesn't look that bad, but no updates in 3 years seems a bit worring.

Agree. And I found no (public) uses of that crate, at all. That's worrying too.

Maybe it would be possible to duplicate some of the xkb code, but get rid of the clang dependencies and such? Open for suggestions here.

Bindgen is kinda infamous in using clang etc. As xkb has a stable interface I'd say we don't care about automatically generated bindings. Quite some dependencies can be avoided by borrowing, say, the manual xkb-common ffi wrapper from sctk instead of xkb and xkbcommon-sys crates.

emoon commented 2 years ago

Yeah. That sounds good to me.