Rust-SDL2 / rust-sdl2

SDL2 bindings for Rust
MIT License
2.65k stars 465 forks source link

Keycode::from_scancode(...) does not work correctly #1377

Closed hvox closed 2 weeks ago

hvox commented 3 months ago

Keycode::from_scancode(some_scancode) doesn't work: it almost always returns None instead of keycode. To be more precise, it returns None only if the user uses an English layout.

Expected result

If an user uses non-qwerty layout this code should print the key located where the W is on the qwerty layout.

extern crate sdl2;
use sdl2::keyboard::{Keycode, Scancode};

pub fn main() {
    sdl2::init().unwrap().video().unwrap();
    let key = Keycode::from_scancode(Scancode::W).unwrap();
    println!("[ {} ]", key.name());
}

Actual result

Code panics at Keycode::from_scancode(Scancode::W).unwrap();

hvox commented 3 months ago

So I spent some time and figured out where the bug is. So basically from_scancode calls SDL_GetKeyFromScancode and SDL_GetKeyFromScancode returns correct keycode_id as i32: https://github.com/Rust-SDL2/rust-sdl2/blob/master/src/sdl2/keyboard/keycode.rs#L511 Then from_scancode calls Keycode::from_i32 and from_i32 contains giant match statement that correctly handles ASCII and special codes like F10 but for anything else it just returns None: https://github.com/Rust-SDL2/rust-sdl2/blob/master/src/sdl2/keyboard/keycode.rs#L490 It can be easily fixed by changing line _ => return None to code => unsafe { transmute(code) }. However, I don't like that kind of fix, because Keycode is enum in Rust-SDL2 and by transmuting something like code for і(ukrain letter i) we get value 1110 into variable of incompatible type. In my opinion, the most appropriate solution would be changing the type definition for Keycode. It should not be enum, it contains unicode codepoints and a few extra integers.

P.S. I think I understand why you made it enum in the first place: SDL_Keycode is declared as enum in C code. But libsdl2 basically uses it as an integer and by declaring it in Rust as enum it got broken.

Cobrand commented 2 weeks ago

Fixed by #1378