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

Add a case-insensitive check for Key::Character value #11691

Closed
wants to merge 6 commits into from

Conversation

Shute052
Copy link

@Shute052 Shute052 commented Feb 4, 2024

Objective

Fixes #11689 if logical letter keys should remain consistent regardless of external factors.
Reporting both lowercase and UPPERCASE logical letter keys is adding a burden on us to make shortcuts in the games.

Solution

Apply the upstream suggestion.
- Use key_without_modifiers in convert_keyboard_input for convert_logical_key.

- Use to_uppercase to all OSs because some of them don't support key_without_modifiers.

Add Key::to_uppercase_character().

Returns an uppercase version if self is a Key::Character.
Otherwise, returns a Clone of self.

For example:

use bevy_input::keyboard::Key;

assert_eq!(Key::Character("B".into()).to_uppercase_character(), Key::Character("B".into()));
assert_eq!(Key::Character("b".into()).to_uppercase_character(), Key::Character("B".into()));
assert_eq!(Key::ArrowUp.to_uppercase_character(), Key::ArrowUp);

Expose the SmolStr::eq_ignore_ascii_case(&str) to Key.

Returns true if self is a Key::Character(character_self) and character_self is an ASCII case-insensitive match with the given character.
Otherwise, it means they are not matched or self is not a Key::Character.

For example:

use bevy_input::keyboard::Key;

assert!(Key::Character("B".into()).eq_ignore_ascii_case("B"));
assert!(Key::Character("b".into()).eq_ignore_ascii_case("B"));
assert!(!Key::ArrowUp.eq_ignore_ascii_case("B"));

Changelog

Before

The main Bevy after the merge of #10702 occurs the situation that all letter logical keys, for example, the 'W' key is reporting a lowercase 'w' normally and an uppercase 'W' when CapsLock is activated or Shift is pressed.

If dealing with both case, we have to repeat the following boilerplate code:

/// Binds with the logical keys rather than physical keys without this PR
fn listen_keyboard_input_events(mut events: EventReader<KeyboardInput>) {
    for event in events.read() {
        if let Key::Character(character) = event.logical_key {
            if character.eq_ignore_ascii_case("A") {
              info!("A key has been pressed!")
            }
        }
        // Other cases...
        match event.logical_key {
            Key::Character("w".into()) | Key::Character("W".into()) => {
              info!("W key has been pressed!")
            }
            Key::Character(c) if c.eq_ignore_ascii_case("S") => {
              info!("S key has been pressed!")
            }
            Key::ArrowUp => {
              info!("ArrowUp key has been pressed!")
            }
            // Other cases...
            _ => continue,
        }
        // Or via if-let
    }
}

After

Now in this case, we have:

/// Binds with the logical keys rather than physical keys via current PR.
fn listen_keyboard_input_events(mut events: EventReader<KeyboardInput>) {
    for event in events.read() {
        if event.logical_key.eq_ignore_ascii_case("W") {
            info!("W key has been pressed!")
        }
        // Other cases...
        match event.logical_key.to_uppercase_character() {
            Key::Character("W".into()) => {
              info!("W key has been pressed!")
            }
            Key::ArrowUp => {
              info!("ArrowUp key has been pressed!")
            }
            // Other cases...
            _ => continue,
        }
    }
}

Copy link
Contributor

github-actions bot commented Feb 4, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@rparrett
Copy link
Contributor

rparrett commented Feb 4, 2024

This particular fix would be a bit of a bummer for me. ReceivedCharacter is not adequate for implementing a text input and logical_key has been helpful. (Further explanation here: #11297)

@Shute052
Copy link
Author

Shute052 commented Feb 4, 2024

This particular fix would be a bit of a bummer for me. ReceivedCharacter is not adequate for implementing a text input and logical_key has been helpful. (Further explanation here: #11297)

It seems that you need KeyEvent::logical_key::to_text()?

Wouldn't it be better to provide both always UPPERCASE logical_key field and a text field for reporting Key::to_text() in KeyboardInput?

@Shute052 Shute052 changed the title Use key_without_modifiers in convert_keyboard_input Use to_uppercase in convert_logical_key Feb 4, 2024
@Shute052 Shute052 changed the title Use to_uppercase in convert_logical_key Add Key::to_uppercase_character() Feb 4, 2024
Copy link
Member

@Vrixyz Vrixyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this has value 👍 ; and low surprise code for users.

A user can still match like to avoid clone

match event.logical_key {
    Key::Character(c) if c.to_uppercase().to_string() == "A") =>
...

@Shute052
Copy link
Author

Shute052 commented Feb 4, 2024

I think this has value 👍 ; and low surprise code for users.

A user can still match like to avoid clone

match event.logical_key {
    Key::Character(c) if c.to_uppercase().to_string() == "A") =>
...

Added an alternative, or perhaps having both functions could be a viable option?

@Shute052 Shute052 changed the title Add Key::to_uppercase_character() Add a case-insensitive check for Key::Character value Feb 4, 2024
@Shute052
Copy link
Author

Shute052 commented Feb 4, 2024

I think this has value 👍 ; and low surprise code for users.

A user can still match like to avoid clone

match event.logical_key {
    Key::Character(c) if c.to_uppercase().to_string() == "A") =>
...

Is there a way to use ButtonInput<Key>? I tried initially, but ButtonInput<T> demands that T has the Copy trait. However, the Keys include Key::Character, preventing them from deriving Copy.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Input Player input via keyboard, mouse, gamepad, and more X-Controversial There is active debate or serious implications around merging this PR labels Feb 4, 2024
@alice-i-cecile
Copy link
Member

We could remove the Copy bound on ButtonInput, but I'm still nervous about trying to exact match more complex data like that.

P.S. Closing this out, as the linked issue has been closed and I don't think this is the right approach to the ultimate problem.

@Vrixyz
Copy link
Member

Vrixyz commented Feb 4, 2024

Is there a way to use ButtonInput<Key>?

There is no such thing, it's not trivial to consistently support released after pressed for characters, so it didn't make it in. More context there: Vrixyz#3

@Shute052 Shute052 deleted the fix_key_value branch May 2, 2024 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Bug An unexpected or incorrect behavior X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected Logical Key Value
4 participants