Recommended implementation of combining characters implementation. #7758
Conversation
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
gspencergoog
reviewed
Feb 8, 2019
shell/platform/android/io/flutter/embedding/engine/android/AndroidKeyProcessor.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/engine/android/AndroidKeyProcessor.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/engine/android/AndroidKeyProcessor.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/engine/systemchannels/KeyEventChannel.java
Show resolved
Hide resolved
|
@gspencergoog I can go ahead and clean this up and pending your final review we can merge this in. Other than your comments above, is there anything else that needs to be addressed before you'd be happy with a merge? |
|
Ok, sounds good. The only other thing I can think of to address is having a test. |
|
Unfortunately, we still have no testing infrastructure in the engine embedding. Literally none of the Java code is tested.... |
|
OK, then that's it, I guess. Thanks! |
|
@gspencergoog I gave it another pass. Please let me know if the comments, names, and implementation look acceptable. |
gspencergoog
previously requested changes
Feb 9, 2019
shell/platform/android/io/flutter/embedding/engine/android/AndroidKeyProcessor.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/engine/android/AndroidKeyProcessor.java
Outdated
Show resolved
Hide resolved
matthew-carroll
dismissed
gspencergoog’s stale review
Made requested changes
gspencergoog
approved these changes
Feb 11, 2019
shell/platform/android/io/flutter/embedding/engine/android/AndroidKeyProcessor.java
Outdated
Show resolved
Hide resolved
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Feb 12, 2019
engine-flutter-autoroll
added a commit
to flutter/flutter
that referenced
this pull request
Feb 12, 2019
flutter/engine@3a1a604...4663d35 git log 3a1a604..4663d35 --no-merges --oneline 4663d35 Recommended implementation of combining characters implementation. (flutter/engine#7758) d521212 Revert "Roll src/third_party/dart fdfe40ea95..de2e7e7721 (7 commits)" (flutter/engine#7789) a003c46 Roll src/third_party/dart fdfe40ea95..de2e7e7721 (7 commits) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (chinmaygarde@google.com), and stop the roller if necessary.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a recommended approach to achieve the behavior in #7722 without introducing state into the KeyEventChannel.
The approach in this recommendation roughly follows the breakdown that can be seen between
TextInputPluginandTextInputChannelas well asAccessibilityBridgeandAccessibilityChannel. At the moment those examples are only visible, themselves, in another PR:#7738
Specific recommendations in this PR:
FlutterKeyEventwhich explicitly declares all of the information that Flutter might want for a key event, regardless of what Android reports in its standardKeyEventcombiningCharacterstobaseCharacterbased on my reading of the javadocs. That might be wrong. The crux of the issue is that I'm not (and probably most people are not) familiar with this level of key encoding behavior. Therefore, I would recommend excessively commenting this new code with links to relevant docs and/or explanations of these concepts that an average reader may have never heard of.