-
-
Notifications
You must be signed in to change notification settings - Fork 846
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
core: Key input overhaul #19616
Merged
+4,092
−343
Merged
core: Key input overhaul #19616
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
b2c6a47
core: Key input overhaul
kjarosh 5bd2fcf
tests: Add avm2/key_input_80percent test
kjarosh 474883f
tests: Add avm2/key_input_numpad test
kjarosh 207915d
avm2: Add support for keyLocation in KeyboardEvent
kjarosh b593080
tests: Add avm2/key_input_location test
kjarosh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggestions on how to jam the contents of this neat struct (especially
LogicalKey
) all the way across this pipeline? 😵💫https://github.com/ruffle-rs/ruffle-android/blob/main/app/src/main/res/layout/keyboard.xml#L121-L127
https://github.com/ruffle-rs/ruffle-android/blob/main/app/src/main/java/rs/ruffle/PlayerActivity.kt#L179-L188
https://github.com/ruffle-rs/ruffle-android/blob/main/app/src/main/java/rs/ruffle/PlayerActivity.kt#L97-L98
https://github.com/ruffle-rs/ruffle-android/blob/main/src/lib.rs#L487-L523
https://github.com/ruffle-rs/ruffle-android/blob/main/src/lib.rs#L400-L421
Or maybe I should leave all the XML/Kotlin parts operating on "softkeyboard scancodes" of a made-up layout, and translate it all in the Rust part? But that creates an uncomfy tie (or gap, depending on how you look at it) between how and where the layout is defined (including the button text), and what it "means" for the Ruffle player...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO when it comes to a simple, custom keyboard, you should use an approach similar to one used in tests, where each key has an identifier and is mapped into
PhysicalKey
/LogicalKey
/KeyLocation
. For more advanced keyboards I would expect all 3 fields to come from the virtual keyboard.Maybe we can explore the idea of using the system keyboard on Android?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can get separate down/up events from that, nor can it handle pressing multiple keys at the same time...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For pure text entry, sure, it would be great, but not for playing games, and such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The same mechanisms could/should trigger hiding our handmade keyboard and bringing up the system one as currently used for the same purpose on web IMO.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bur yeah, making each key have a proper identifier in its
tag
attribute, and carrying that string all the way into Rust, then dispatching on it there (turning it into aKeyIdentifier
) sounds doable. But passing strings through the JNI FFI boundary is just so much more inconvenient than simple scalar values IIRC... 😩 Sure it's not impossible, just less nice...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can always map it in Java and pass numerical values down. Not sure if that would require code duplication or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but that would just require more messing around with encoding different (sometimes optional) info about all the keys into numbers and then those into Rust enumerants... This way it would be in one place at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it would still require separating the methods to pass in the key events coming from our handmade virtual keyboard (with the string identifiers), and those coming from an actual physical keyboard connected say via Bluetooth (or USB or adb/scrcpy or whatever), in the form of an Android
KeyEvent
. Oh well, this isn't a big deal either I guess...