Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken keyboard input API #517

Open
narodnik opened this issue Jan 24, 2025 · 1 comment
Open

Fix broken keyboard input API #517

narodnik opened this issue Jan 24, 2025 · 1 comment

Comments

@narodnik
Copy link
Contributor

narodnik commented Jan 24, 2025

Proposed API:

fn key_event(KeyEvent, KeyPhase);

KeyEvent then has this API: https://doc.qt.io/qt-6/qkeyevent.html

KeyPhase has Press, Release, Canceled

Check this log:

char_event(") shift=true, repeat=false
char_event(") shift=true, repeat=true
char_event(") shift=true, repeat=true
char_event(') shift=false, repeat=true
char_event(') shift=false, repeat=true
char_event(') shift=false, repeat=true

This is the same key being held, but the API does not give any indication of that.

The proposed API would fix this, since KeyEvent gives access to the native virtual keycodes for that platform. They should not be relied upon to be consistent across platforms but to simply map to unique keys on that device. This would allow us to clearly see key down and release events despite the char/key changing.

Also you can call .chr() to get the actual char, or try to get the KeyCode. This would make existing code simpler too, since right now I have code like:

/// Filter these char events from being handled since we handle them
/// using the key_up/key_down events.
/// Avoids duplicate processing of keyboard input events.
pub static DISALLOWED_CHARS: &'static [char] = &['\r', '\u{8}', '\u{7f}', '\t', '\n'];

/// These keycodes are handled via normal key_up/key_down events.
/// Anything in this list must be disallowed char events.
pub static ALLOWED_KEYCODES: &'static [KeyCode] = &[
    KeyCode::Left,
    KeyCode::Right,
    KeyCode::Up,
    KeyCode::Down,
    KeyCode::Enter,
    KeyCode::Kp0,
    KeyCode::Kp1,
    KeyCode::Kp2,
    KeyCode::Kp3,
    KeyCode::Kp4,
    KeyCode::Kp5,
    KeyCode::Kp6,
    KeyCode::Kp7,
    KeyCode::Kp8,
    KeyCode::Kp9,
    KeyCode::KpDecimal,
    KeyCode::KpEnter,
    KeyCode::Delete,
    KeyCode::Backspace,
    KeyCode::Home,
    KeyCode::End,
];

With this change, I can simply handle all key events in one place directly rather than having to deal with crossover from both key_down(KeyCode::A) and char_event('A').

@bolphen
Copy link
Contributor

bolphen commented Mar 1, 2025

I think there are two separated issues here.

About the key repeat issue: you shouldn't be handling key repeat on your own outside Miniquad. This is just a Wayland backend issue: right now for Wayland, the key down events are sent every frame as soon as you press the key, and you shouldn't be relying on this behavior that is completely different from other platforms (but now corrected in #535, so you won't need to handle it anymore).

On the other hand, I do agree that we should replace the current KeyCode with some sort of keysym. Right now KeyCode is always mapped to the QWERTY layout. So on a French AZERTY when I press A I get a KeyCode::Q (this affects Macroquad users as well not-fl3/macroquad/issues/686). A workaround is to use the char_event, but it doesn't emit a "char_release", and you need to handle them in two different places.

Ideally we should have key_down_event(KeySym, KeyMods, repeat) where KeySym is tied to the underlying symbol (e.g. KeySym::Exclam when you type Shift + Key1 on a QWERTY keyboard, or if you hit a physical ! key on a layout that has it). This fixes the layout issue and there's no need for a char_event anymore. This could actually be (partly) non-breaking: since there isn't currently a KeyCode::Exclam we could just reuse KeyCode, add the missing symbols, and make an alias type KeySym = KeyCode; This would only be breaking for people who are explicitly matching Shift + Key1 for exclamation marks, and actually a bug fix for people using non-QWERTY layouts. But of course, the safer way is to introduce another event and deprecate the current ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants