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

Deprecate ReceivedCharacter #12868

Merged
merged 8 commits into from Apr 30, 2024

Conversation

chompaa
Copy link
Contributor

@chompaa chompaa commented Apr 3, 2024

Objective

Solution

  • Deprecate ReceivedCharacter.
  • Replace ReceivedCharacter with KeyboardInput in the relevant examples.

Migration Guide

  • ReceivedCharacter is now deprecated, use KeyboardInput instead.

  • Before:

    fn listen_characters(events: EventReader<ReceivedCharacter>) {
      for event in events.read() {
        info!("{}", event.char);
      }
    }

    After:

    fn listen_characters(events: EventReader<KeyboardInput>) {
      for event in events.read() {
        // Only check for characters when the key is pressed.
        if event.state == ButtonState::Released {
          continue;
        }
        // Note that some keys such as `Space` and `Tab` won't be detected as before.
        // Instead, check for them with `Key::Space` and `Key::Tab`.
        if let Key::Character(character) = &event.logical_key {
          info!("{}", character);
        }
      }
    }

@james7132 james7132 added A-Windowing Platform-agnostic interface layer to run your app in A-Input Player input via keyboard, mouse, gamepad, and more C-Code-Quality A section of code that is hard to understand or change labels Apr 3, 2024
@chompaa chompaa force-pushed the deprecate_received-character branch from da31c8f to fd8b331 Compare April 3, 2024 23:30
@alice-i-cecile alice-i-cecile added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 3, 2024
@alice-i-cecile
Copy link
Member

What needs to be done before you can take this out of draft?

@hymm
Copy link
Contributor

hymm commented Apr 4, 2024

If we deprecate received character, what's the recommended way to say get a capital character for a text box

@chompaa
Copy link
Contributor Author

chompaa commented Apr 4, 2024

What needs to be done before you can take this out of draft?

Just need to fix the CI errors, should have time shortly :)

@chompaa chompaa marked this pull request as ready for review April 4, 2024 01:37
@chompaa
Copy link
Contributor Author

chompaa commented Apr 4, 2024

If we deprecate received character, what's the recommended way to say get a capital character for a text box

See the updated char_input_events example in this PR. It should record capital characters.

@@ -1,3 +1,4 @@
#![allow(deprecated)]
Copy link
Member

Choose a reason for hiding this comment

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

These should be localized to where they're supposed to be used.

Copy link
Contributor Author

@chompaa chompaa Apr 4, 2024

Choose a reason for hiding this comment

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

How is this achieved for the WinitEvent enum in winit_event.rs? I've tried the following placements:

#[allow(deprecated)]
pub enum WinitEvent {
  // ..
  #[allow(deprecated)]
  ReceivedCharacter(#[allow(deprecated)] ReceivedCharacter),
  // ..
}

but clippy still seems to complain.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it needs to go on the functions that use the WinitEvent

@hymm
Copy link
Contributor

hymm commented Apr 4, 2024

See the updated char_input_events example in this PR. It should record capital characters.

I tried it out and it does seem to provide what is needed, but it's not an exact 1-1 replacement, since it includes both pressed and released events. The equivalent to ReceivedCharacter would be if you filtered out the Released events. Could you something like the following code snippet in the migration guide.

// Before
fn my_system(events: EventReader<ReceivedCharater>) {
  for event in events.read() {
    info!("{}", event.char);
  }
}
// After
fn my_system(events: EventReader<ReceivedCharater>) {
  for event in events.read().filter(|event| event.state == ButtonState::Released) {
    if let Key::Character(character) = &event.logical_key {
      info!("{}", character);
    }
  }
}

@chompaa
Copy link
Contributor Author

chompaa commented Apr 4, 2024

I tried it out and it does seem to provide what is needed, but it's not an exact 1-1 replacement, since it includes both pressed and released events. The equivalent to ReceivedCharacter would be if you filtered out the Released events. Could you something like the following code snippet in the migration guide.

Yeah I accounted for this in the text_input example but not in char_input_events one it seems. I'll add it to the migration guide too.

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Apr 4, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Generally looks good: the migration guide is solid. I won't block on figuring out how to localize those deprecated warnings: they'll be cleaned up shortly after the release anyways.

@hymm
Copy link
Contributor

hymm commented Apr 16, 2024

I won't block on figuring out how to localize those deprecated warnings: they'll be cleaned up shortly after the release anyways.

Does clippy warn on unused allows? If it doesn't we might want to leave some TODO's to remove the allow(deprecated)'s once ReceivedCharacter is removed.

@Vrixyz Vrixyz added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 17, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 30, 2024
Merged via the queue into bevyengine:main with commit 7b4b596 Apr 30, 2024
28 checks passed
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 A-Windowing Platform-agnostic interface layer to run your app in C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate then remove ReceivedCharacter
5 participants