[Android KeyEvents] Split AndroidKeyProcessor into separate classes #25628

LongCatIsLooong · 2021-04-16T21:04:13Z

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

LongCatIsLooong · 2021-04-16T21:05:06Z

shell/platform/android/io/flutter/embedding/android/AndroidKeyProcessor.java

-  @NonNull private final KeyEventChannel keyEventChannel;
-  @NonNull private final TextInputPlugin textInputPlugin;
+/** A class for handling combining characters in a {@link KeyEvent} stream. */
+class AndroidKeyProcessor {


Might need a name change. This class now only handles dead keys, and only has an applyCombiningCharacterToBaseCharacter method.

Could we eliminate this class/file, and just incorporate the functionality in one of the other classes?

According to @dkwingsmt we're going to add a different type of primary responder so I thought we might want to keep this around in case it's useful in the other responder as well?

Greg is right: merge the logic of this class into the channel responder. The embedder responder will use a completely separate logic. This make it easier to design embedder responder in the future. If shared logic is useful we will write them again.

LongCatIsLooong · 2021-04-16T21:06:55Z

shell/platform/android/io/flutter/embedding/android/AndroidKeyProcessor.java

   */
-  @Nullable
-  private Character applyCombiningCharacterToBaseCharacter(int newCharacterCodePoint) {
-    if (newCharacterCodePoint == 0) {


Is there a reason that we treated 0 differently and returned null specifically in this case (and didn't clear the combining key).

If so, I don't remember it. Maybe zero is a special code point that indicates some change in state, or informational event that should be ignored? It could also be that zero indicates a non-printable character like shift or meta, in which case we might ignore it since it might allow for combining with an uppercase character or something. Anyhow, seems like we should look into why we might get a zero here.

Looking at the original call site:
Character complexCharacter = applyCombiningCharacterToBaseCharacter(keyEvent.getUnicodeChar());

It looks like the method takes an unicode code point as input, and 0 is occupied in unicode: https://codepoints.net/U+0000, wikipedia says it's just the string terminator \0 in C: https://en.wikipedia.org/wiki/Null_character so it probably doesn't have another special meaning assigned to it?

gspencergoog · 2021-04-21T22:17:28Z

shell/platform/android/io/flutter/embedding/android/AndroidKeyProcessor.java

   */
-  @Nullable
-  private Character applyCombiningCharacterToBaseCharacter(int newCharacterCodePoint) {
-    if (newCharacterCodePoint == 0) {


If so, I don't remember it. Maybe zero is a special code point that indicates some change in state, or informational event that should be ignored? It could also be that zero indicates a non-printable character like shift or meta, in which case we might ignore it since it might allow for combining with an uppercase character or something. Anyhow, seems like we should look into why we might get a zero here.

gspencergoog · 2021-04-21T22:17:56Z

shell/platform/android/io/flutter/embedding/android/AndroidKeyProcessor.java

-  @NonNull private final KeyEventChannel keyEventChannel;
-  @NonNull private final TextInputPlugin textInputPlugin;
+/** A class for handling combining characters in a {@link KeyEvent} stream. */
+class AndroidKeyProcessor {


Could we eliminate this class/file, and just incorporate the functionality in one of the other classes?

gspencergoog · 2021-04-21T22:22:12Z

shell/platform/android/io/flutter/embedding/android/KeyboardManager.java

+  public boolean handleEvent(@NonNull KeyEvent keyEvent) {
+    final boolean isRedispatchedEvent = redispatchedEvents.remove(keyEvent);
+    if (isRedispatchedEvent) {
+      return !isRedispatchedEvent;


Could just return false here.

dkwingsmt · 2021-04-22T23:14:48Z

shell/platform/android/io/flutter/embedding/android/AndroidKeyProcessor.java

-  @NonNull private final KeyEventChannel keyEventChannel;
-  @NonNull private final TextInputPlugin textInputPlugin;
+/** A class for handling combining characters in a {@link KeyEvent} stream. */
+class AndroidKeyProcessor {


Greg is right: merge the logic of this class into the channel responder. The embedder responder will use a completely separate logic. This make it easier to design embedder responder in the future. If shared logic is useful we will write them again.

shell/platform/android/io/flutter/embedding/android/KeyboardManager.java

dkwingsmt · 2021-04-22T23:44:26Z

shell/platform/android/io/flutter/embedding/android/KeyChannelResponder.java

+import androidx.annotation.NonNull;
+import io.flutter.embedding.engine.systemchannels.KeyEventChannel;
+
+/** A light wrapper around a {@link KeyEventChannel}, turning it into a {@link PrimaryResponder}. */


This documentation is very underwhelming. Consider using the one from macOS (as below) or other platforms.

/** * A primary responder of |FlutterKeyboardManager| that handles events by * sending the raw information through the method channel. * * This class corresponds to the RawKeyboard API in the framework. */

dkwingsmt · 2021-04-22T23:47:30Z

shell/platform/android/io/flutter/embedding/android/KeyboardManager.java

+ * re-dispatches those not handled by the primary responders.
+ *
+ * <p>Flutter uses asynchronous event handling to avoid blocking the UI thread, but Android requires
+ * that events are handled synchronously. So, when a key event is received by Flutter, it tells


Suggested change

* that events are handled synchronously. So, when a key event is received by Flutter, it tells

* that events are handled synchronously. So, when a key event (that is not recognized as redispatched) is received by Flutter, it tells

dkwingsmt · 2021-04-22T23:51:47Z

shell/platform/android/io/flutter/embedding/android/KeyboardManager.java

+ * synthesizes a new event to send to Android, without handling it this time.
+ *
+ * <p>A new {@link KeyEvent} sent to a {@link KeyboardManager} can be propagated to 3 different
+ * types of "responder"s (in the listed order):


Avoid using this term as anything but the class. Maybe:

Suggested change

* types of "responder"s (in the listed order):

* types of destinations (in the listed order):

There are other similar references in other documentations.

Hmm I still prefer "responders" here. I think it's a more established term. "destinations" or "handlers" doesn't sound as good to me since "responders" also conveys that it responds (whether the event is handled), in addition to handling the key event.

It's pretty confusing since it's the name for a class. It might not be that expressive either, since technically redispatching to the system does not really respond anything. What about "A new even sent to a KeyboardManager have 3 candidate processors:"?

dkwingsmt · 2021-04-22T23:56:41Z

shell/platform/android/io/flutter/embedding/android/KeyboardManager.java

+   *
+   * <p>If a {@link PrimaryResponder} fails to call the {@link OnKeyEventHandledCallback} callback,
+   * the {@link KeyEvent} will never be sent to the {@link TextInputPlugin}, and the {@link
+   * KeyboardManager} class can't detect such errors as there is no timeout.


Suggested change

*

* <p>If a {@link PrimaryResponder} fails to call the {@link OnKeyEventHandledCallback} callback,

* the {@link KeyEvent} will never be sent to the {@link TextInputPlugin}, and the {@link

* KeyboardManager} class can't detect such errors as there is no timeout.

We've said "must".

This is mostly for debugging. Knowing the symptoms would probably help people with debugging, since there's no good way to catch the programmer error in our implementation.

shell/platform/android/io/flutter/embedding/android/KeyboardManager.java

dkwingsmt · 2021-04-23T10:43:58Z

shell/platform/android/io/flutter/embedding/android/KeyboardManager.java

+      onUnhandled(keyEvent);
+    }
+
+    return !isRedispatchedEvent;


This would always be true.

This isn't resolved yet.

dkwingsmt · 2021-04-23T10:51:09Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/KeyEventChannel.java

    /**
     * Called whenever the framework responds that a given key event was handled by the framework.
     *
     * @param event the event to be marked as being handled by the framework. Must not be null.


Did you delete the wrong doc?

dkwingsmt · 2021-04-23T12:04:10Z

shell/platform/android/io/flutter/plugin/editing/TextInputPlugin.java

-
-  public void setKeyEventProcessor(AndroidKeyProcessor processor) {
-    keyProcessor = processor;
+  public void setKeyboardManager(KeyboardManager processor) {


This feels so wrong. If I understand coorectly, depending on the Android engine we're using, we either have

KeyboardManager -> TextInputPlugin -> InputConnectionAdapter; or

TextInputPlugin -> InputConnectionAdapter -> KeyboardManager.
This makes the entire logic entangled, and I'll guess is the reason why there is an instanceof in handleKeyEvent.

Can we somehow resolve this, either by merging some logic or splitting classes? I'm not familiar with the system but I'm glad to discuss if it helps.

This happens because KeyboardManger does not own TextInputPlugin, or vice versa, but they depend on each other. KeyboardManager clearly shouldn't own TextInputPlugin. TextInputPlugin can probably own KeyboardManager but I'm not sure we want to do that.

The instanceof exists because the InputConnection protocol by itself can not distinguish a redispatched event from regular key events. An alternative to this is to pass the keyboard manager to InputConnectionAdater and override its sendKeyEvent and let the keyboard manager shortcircuit redispatched events (this is what the previous implementation does).

On other platforms, KeyboardManger owns TextInputPlugin. Is there any problem specific for Android?

Basically on other platforms, KeyboardManger owns 3 things:

KeyboardManger -> responders -> textInputPlugin -> redispatchEvent // A callback

which correspond to the 3 "responders" in the comment here.

Looked deeper into this, it looks like the text input plugin only uses the keyboard manager to create new input connections which need the keyboard manager to send IME key events to. It makes more sense to change the createInputConnection method to take a keyboard manager.

…keys

chinmaygarde · 2021-05-06T21:54:09Z

@LongCatIsLooong Is this good for re-review now? @dkwingsmt Another review pass please?

…keys

chinmaygarde · 2021-05-13T21:58:20Z

@dkwingsmt Another review please?

dkwingsmt

LGTM

dkwingsmt · 2021-05-19T22:08:19Z

shell/platform/android/io/flutter/embedding/android/KeyboardManager.java

+      onUnhandled(keyEvent);
+    }
+
+    return !isRedispatchedEvent;


This isn't resolved yet.

chinmaygarde · 2021-05-21T18:02:30Z

The presubmit failures are related to the patch as well and need to be patched.

chinmaygarde · 2021-05-27T21:36:38Z

@LongCatIsLooong Any updates on patching the presubmits?

chinmaygarde · 2021-06-03T21:39:21Z

Closing as stale. Please re-open as necessary.

LongCatIsLooong · 2021-06-04T07:54:01Z

Sorry for the inactivity. Updated the PR so TextInputPlugin no longer directly keeps a KeyboardManager instance, instead its createInputConnection method takes an additional KeyboardManager argument.

…keys

…lasses (flutter/engine#25628)

…lutter#25628)

WIP

d4b9518

LongCatIsLooong requested review from dkwingsmt and gspencergoog April 16, 2021 21:04

flutter-dashboard bot added the platform-android label Apr 16, 2021

google-cla bot added the cla: yes label Apr 16, 2021

split AndroidKeyProcessor into different classes

d52523d

LongCatIsLooong force-pushed the android-platform-keys branch from 64fae02 to d52523d Compare April 16, 2021 21:11

LongCatIsLooong commented Apr 16, 2021

View reviewed changes

LongCatIsLooong added 3 commits April 16, 2021 15:10

fix GN format

c3deeee

fix license

e9501be

minor documentation changes

a9a01ba

gspencergoog reviewed Apr 21, 2021

View reviewed changes

review

6732e0f

dkwingsmt suggested changes Apr 23, 2021

View reviewed changes

LongCatIsLooong added 3 commits April 26, 2021 16:07

review

b0c0e0b

Merge remote-tracking branch 'upstream/master' into android-platform-…

ad715e3

…keys

format

6c1dde0

LongCatIsLooong force-pushed the android-platform-keys branch from 41d403b to d96fcb0 Compare April 29, 2021 00:48

review

001d9b7

LongCatIsLooong force-pushed the android-platform-keys branch from d96fcb0 to 001d9b7 Compare April 29, 2021 00:51

LongCatIsLooong added 2 commits May 11, 2021 01:50

review

14055bf

Merge remote-tracking branch 'upstream/master' into android-platform-…

f5197ab

…keys

dkwingsmt approved these changes May 19, 2021

View reviewed changes

chinmaygarde closed this Jun 3, 2021

LongCatIsLooong reopened this Jun 4, 2021

LongCatIsLooong force-pushed the android-platform-keys branch from 350bbce to 62ed56b Compare June 4, 2021 08:07

remove setKeyboardManager

fbf7eeb

LongCatIsLooong force-pushed the android-platform-keys branch 4 times, most recently from dd29487 to f8360ed Compare June 4, 2021 21:42

resolve symbols

e98ade9

LongCatIsLooong force-pushed the android-platform-keys branch from f8360ed to e98ade9 Compare June 4, 2021 21:56

Merge remote-tracking branch 'upstream/master' into android-platform-…

f3795d2

…keys

LongCatIsLooong added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 5, 2021

fluttergithubbot merged commit a882102 into flutter:master Jun 5, 2021

LongCatIsLooong deleted the android-platform-keys branch June 5, 2021 02:01

engine-flutter-autoroll mentioned this pull request Jun 6, 2021

Roll Engine from 03d645e782db to 66068e2e7dd9 (20 revisions) flutter/flutter#84083

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2021

a882102 [Android KeyEvents] Split AndroidKeyProcessor into separate c…

6b38304

…lasses (flutter/engine#25628)

dkwingsmt mentioned this pull request Jun 7, 2021

☂️ Device-specific keyboard events should happen in the embedder flutter/flutter#44918

Closed

dkwingsmt mentioned this pull request Jul 28, 2021

Hardware Keyboard: Android #23468

Closed

13 tasks

naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021

[Android KeyEvents] Split AndroidKeyProcessor into separate classes (f…

5b07b5b

…lutter#25628)

	* that events are handled synchronously. So, when a key event is received by Flutter, it tells
	* that events are handled synchronously. So, when a key event (that is not recognized as redispatched) is received by Flutter, it tells

	* types of "responder"s (in the listed order):
	* types of destinations (in the listed order):

[Android KeyEvents] Split AndroidKeyProcessor into separate classes #25628

[Android KeyEvents] Split AndroidKeyProcessor into separate classes #25628

Uh oh!

Conversation

LongCatIsLooong commented Apr 16, 2021

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented May 6, 2021

Uh oh!

chinmaygarde commented May 13, 2021

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented May 21, 2021

Uh oh!

chinmaygarde commented May 27, 2021

Uh oh!

chinmaygarde commented Jun 3, 2021

Uh oh!

dkwingsmt Apr 29, 2021 •

edited

Loading

LongCatIsLooong commented Jun 4, 2021 •

edited

Loading