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

android: Add mapping from NDK Keycode to VirtualKeyCode #2226

Merged
merged 2 commits into from
Apr 1, 2022

Conversation

torokati44
Copy link
Contributor

@torokati44 torokati44 commented Mar 26, 2022

When working on https://github.com/torokati44/ruffle-android, I discovered that all keypress events arrive with their scancode set to 0, and their keycode set to None. Which I found less than useful.

The NDK docs for AKeyEvent_getScanCode mention that "These values are not reliable and vary from device to device.".
So I guess always setting them to 0 is acceptable from their end.

Tested this on Android 12, with a keyboard connected to my computer, through wired adb and https://github.com/Genymobile/scrcpy/, and it seemed to work fine.

AFAICT This fixes #1867.

  • Tested on all platforms changed
  • Only Android is affected, and I am testing on it.
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • I have not found any existing documentation that would have to be updated due to this change.
  • Created or updated an example program if it would help users understand this functionality
  • No new functionality, only implementing/fixing an existing one.
  • Updated feature matrix, if new features were added or implemented
  • The change is not significant enough to warrant updating the matrix.

@torokati44 torokati44 marked this pull request as ready for review March 26, 2022 19:57
@torokati44 torokati44 changed the title WIP android: Add mapping from NDK Keycode to VirtualKeyCode android: Add mapping from NDK Keycode to VirtualKeyCode Mar 26, 2022
@torokati44
Copy link
Contributor Author

torokati44 commented Mar 26, 2022

I think this is now reviewable.

I have left quite a few ??? comments in there where it wasn't immediately obvious what the mapping should be.
What should I do about those? Delete those comments and accept the current mapping as a whole as "good enough", or remove any mapped keys which may not be a perfect match? Or leave the uncertainty in there with those comments?

@msiglreith msiglreith self-requested a review March 26, 2022 21:52
@msiglreith msiglreith added the C - waiting on maintainer A maintainer must review this code label Mar 26, 2022
Copy link
Member

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

thanks! the less clear cases are fine in my eyes

@msiglreith msiglreith merged commit 52c4670 into rust-windowing:master Apr 1, 2022
Keycode::MoveEnd => Some(VirtualKeyCode::End),
Keycode::Insert => Some(VirtualKeyCode::Insert),

Keycode::Del => Some(VirtualKeyCode::Back), // Backspace (above Enter)
Copy link
Member

@MarijnS95 MarijnS95 May 26, 2022

Choose a reason for hiding this comment

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

Started playing about with these codes, and it seems you haven't implemented a mapping for KeyCode::Back which would be needed to handle a graceful app exit (or going to the previous page) when the user presses it. The implementation for VirtualKeyCode::Back reads:

winit/src/event.rs

Lines 962 to 964 in f04fa5d

/// The Backspace key, right over Enter.
// TODO: rename
Back,

So I don't think we should add a new Back VirtualKeyCode and rename existing uses to Backspace, as that's really hard to notice for end-users upgrading their winit. Perhaps we can piggyback NavigateBackward but android already supports that as KeyCode::NavigatePrevious:

Keycode::Back => Some(VirtualKeyCode::NavigateBackward),

How do you think we should go forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on maintainer A maintainer must review this code DS - android
Development

Successfully merging this pull request may close these issues.

Android: Provide keycode for KeyEvents
3 participants