[Android Text Input] Make the editing state listenable and allow batch edits #21534

LongCatIsLooong · 2020-10-01T03:20:16Z

Description

Make the Editable held by both the input connection and the text input plugin listenable and allows for batch edits. I might have reinvented the wheel since there're already mechanisms like TextWatcher and SpanWatcher, but they don't seem to account for batch editing.

Most of the time when the editing state changes we need to notify the input method manager or the autofill manager. They do not care where each change comes from (framework or input method or autofill), nor do they directly modify the current editing state.

Now TextInputPlugin and InputConnectionAdaptor both listen to the shared ListenableEditingState:

TextInputPlugin sends text editing state update to the framework when a change is detected, also it notifies the autofill manager of text changes.
InputConnectionAdaptor sends updates to the input method manager when the editing state is changed.

Related PR

Fixes: flutter/flutter#69111
flutter/flutter#66864
#20160

Tests

I added the following tests:

setTextInputEditingState_doesNotInvokeUpdateEditingState
inputConnectionAdaptor_skipCallsAfterEditingValueUpdateFromFramework

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

…o-echoing

LongCatIsLooong · 2020-10-01T03:35:07Z

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

-    mRepeatCheckNeeded = false;
+  // Called when the current text editing state held by the text input plugin is overwritten by a
+  // newly received value from the framework.
+  public void didUpdateEditingValue() {


The android text input plugin doesn't seem to completely overwrite the input state with the newly received value, the composing range is ignored. If the text is composing and the user replaces the entire string with something else, would that put the input method in a broken state?

Have you tried it out? Ignoring the composing region definitely seems incorrect. If there's no visible bug, maybe it's fixing itself when subsequent edits happen.

I tried a 3rd party japanese IME that uses composing region, it's not working in flutter apps (not composing at all). I'll see if there's an easy fix.

update it seems the Japanese input bug fixed itself.

justinmc

LGTM as long as the tests pass. There is one failing now but I think it's just a problem with a test.

Thanks for jumping on all of these refactors, it's going to be a lot cleaner.

justinmc · 2020-10-01T15:39:43Z

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

-    mRepeatCheckNeeded = false;
+  // Called when the current text editing state held by the text input plugin is overwritten by a
+  // newly received value from the framework.
+  public void didUpdateEditingValue() {


Have you tried it out? Ignoring the composing region definitely seems incorrect. If there's no visible bug, maybe it's fixing itself when subsequent edits happen.

justinmc · 2020-10-01T15:44:52Z

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


  @Override
  public boolean performEditorAction(int actionCode) {
-    markDirty();


It's great seeing all these markDirty's going away. This seems so much simpler.

chinmaygarde · 2020-10-08T21:38:35Z

I don't think the failure is a flake. Can we fix the presubmit and land this?

chinmaygarde · 2020-10-15T21:37:51Z

@LongCatIsLooong: Can you tell if the failure is a flake? And, if not, land this patch?

LongCatIsLooong · 2020-10-15T23:00:36Z

The failure doesn't look related. There's still something in the PR I'm trying to fix. Turning this into a draft.

…o-echoing

LongCatIsLooong · 2020-11-09T18:10:02Z

@justinmc @GaryQian FYI the test device came in this past Saturday (SM-T510). I managed to repro the Samsung issue on Flutter 1.12.13 hotfix 8, but not on upstream/master or this branch.

GaryQian · 2020-11-10T23:32:55Z

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

+    }
+  }
+
+  public final int getSelecionStart() {


Typo, did you mean getSelection?

GaryQian · 2020-11-10T23:33:17Z

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

+    return Selection.getSelectionStart(this);
+  }
+
+  public final int getSelecionEnd() {


Same as above, getSelection

GaryQian · 2020-11-10T23:49:49Z

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

  @Override
  public boolean deleteSurroundingText(int beforeLength, int afterLength) {
-    if (Selection.getSelectionStart(mEditable) == -1) return true;
+    if (mEditable.getSelecionStart() == -1) {


*getSelectionStart

GaryQian · 2020-11-10T23:54:24Z

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

+
+  private BaseInputConnection mDummyConnection;
+
+  // The View is only use for creating a dummy BaseInputConnection for setComposingRegion. The View


is only used

GaryQian · 2020-11-10T23:55:29Z

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

+    }
+    if (mBatchEditNestDepth == 1 && !mListeners.isEmpty()) {
+      mTextWhenBeginBatchEdit = toString();
+      mSelectionStartWhenBeginBatchEdit = getSelecionStart();


getSelection

GaryQian · 2020-11-11T00:02:41Z

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

+        boolean textChanged, boolean selectionChanged, boolean composingRegionChanged);
+  }
+
+  private static final String TAG = "flutter";


We seem to use a tag specific to the class name in some cases, but (don't quote me on this) the tool may filter for flutter explicitly, so you should probably verify the message is visible when you want it to be.

Updated all 3 classes to use their class names.

If I change the level to Log.v it doesn't show up even if I change the TAG to "flutter" and use flutter run -v.

GaryQian · 2020-11-11T00:04:20Z

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

            view, inputTarget.id, textInputChannel, keyProcessor, mEditable, outAttrs);
-    outAttrs.initialSelStart = Selection.getSelectionStart(mEditable);
-    outAttrs.initialSelEnd = Selection.getSelectionEnd(mEditable);
+    outAttrs.initialSelStart = mEditable.getSelecionStart();


getSelectionStart and getSelectionEnd

GaryQian · 2020-11-11T00:05:28Z

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

+  // ### Keep the AFM updated
+  //
+  // The autofill session connected to The AFM keeps a copy of the current state for each reported
+  // field in "AutofillVirtualStructure"(instead of holding a reference to those fields), so the AFM


nit, space between " and (

GaryQian

LGTM, with a few nits

…low batch edits (#21534)" This reverts commit 81f219c.

…low batch edits (flutter/engine#21534)

…low batch edits (flutter/engine#21534) (#70266)

…low batch edits (#21534)" (#22434)

batch edits (flutter#21534)", reverted in (flutter#22434) This reverts commit 23b6310.

…h edits (flutter#21534)

…low batch edits (flutter#21534)" (flutter#22434)

LongCatIsLooong added 4 commits September 29, 2020 22:42

wip

592f161

add test

fd88233

add test

69f2ece

Merge remote-tracking branch 'upstream/master' into textinputplugin-n…

ce03d81

…o-echoing

LongCatIsLooong requested review from GaryQian and justinmc October 1, 2020 03:20

flutter-dashboard bot added the platform-android label Oct 1, 2020

googlebot added the cla: yes label Oct 1, 2020

LongCatIsLooong added the affects: text input label Oct 1, 2020

LongCatIsLooong commented Oct 1, 2020

View reviewed changes

LongCatIsLooong changed the title ~~[Android Text Input] G~~ [Android Text Input] Further reduce updates sent to the framework Oct 1, 2020

justinmc approved these changes Oct 1, 2020

View reviewed changes

justinmc mentioned this pull request Oct 1, 2020

iOS Text Editing Infinite Loop #20160

Merged

LongCatIsLooong force-pushed the textinputplugin-no-echoing branch from ca21217 to 0f1e22f Compare October 12, 2020 22:25

fix tests

163685d

LongCatIsLooong force-pushed the textinputplugin-no-echoing branch from 0f1e22f to 163685d Compare October 12, 2020 22:26

LongCatIsLooong marked this pull request as draft October 15, 2020 23:00

LongCatIsLooong added 4 commits October 16, 2020 23:25

Merge remote-tracking branch 'upstream/master' into textinputplugin-n…

73bbb6a

…o-echoing

update composing region from framework

810d53a

implement updateCursorAnchorInfo and updateExtractedText

7d9865c

wip

a33db84

LongCatIsLooong force-pushed the textinputplugin-no-echoing branch from 55d4578 to 0339d06 Compare October 29, 2020 06:24

Merge remote-tracking branch 'upstream/master' into textinputplugin-n…

f6905d7

…o-echoing

LongCatIsLooong force-pushed the textinputplugin-no-echoing branch from 0339d06 to f6905d7 Compare October 29, 2020 06:33

LongCatIsLooong added 3 commits October 30, 2020 00:54

update

420568f

format

e7085d5

code comments

3a0ba3b

GaryQian reviewed Nov 10, 2020

View reviewed changes

GaryQian reviewed Nov 11, 2020

View reviewed changes

review

5aa1561

GaryQian approved these changes Nov 11, 2020

View reviewed changes

format

b7d48b9

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 Nov 11, 2020

fluttergithubbot merged commit 81f219c into flutter:master Nov 11, 2020

LongCatIsLooong deleted the textinputplugin-no-echoing branch November 11, 2020 03:21

LongCatIsLooong added a commit that referenced this pull request Nov 11, 2020

Revert "[Android Text Input] Make the editing state listenable and al…

469242b

…low batch edits (#21534)" This reverts commit 81f219c.

LongCatIsLooong mentioned this pull request Nov 11, 2020

Revert "[Android Text Input] Make the editing state listenable and allow batch edits " #22434

Merged

engine-flutter-autoroll mentioned this pull request Nov 11, 2020

Roll Engine from 3de24322acf5 to 81f219c59c62 (1 revision) flutter/flutter#70266

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 11, 2020

81f219c [Android Text Input] Make the editing state listenable and al…

80d5db4

…low batch edits (flutter/engine#21534)

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Nov 11, 2020

81f219c [Android Text Input] Make the editing state listenable and al…

018467c

…low batch edits (flutter/engine#21534) (#70266)

hawkinsjb1 mentioned this pull request Nov 11, 2020

AutoFill unexpected behavior; TextInput.finishAutofillContext() doing nothing; Cannot save user input flutter/flutter#69111

Closed

fluttergithubbot pushed a commit that referenced this pull request Nov 12, 2020

Revert "[Android Text Input] Make the editing state listenable and al…

23b6310

…low batch edits (#21534)" (#22434)

LongCatIsLooong added a commit to LongCatIsLooong/engine that referenced this pull request Nov 12, 2020

Reland "[Android Text Input] Make the editing state listenable and allow

438126b

batch edits (flutter#21534)", reverted in (flutter#22434) This reverts commit 23b6310.

GaryQian mentioned this pull request Nov 12, 2020

[Android text input] Reland #21534 #22435

Merged

9 tasks

fluttergithubbot pushed a commit that referenced this pull request Nov 12, 2020

[Android text input] Reland #21534 (#22435)

778eb84

LongCatIsLooong mentioned this pull request Nov 13, 2020

TextField focus triggering java.lang.NullPointerException on Android flutter/flutter#70376

Closed

gaaclarke mentioned this pull request Nov 13, 2020

Dart version check tripping up on copyright notices with local engine flutter/flutter#70498

Closed

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

[Android Text Input] Make the editing state listenable and allow batc…

ef29928

…h edits (flutter#21534)

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Revert "[Android Text Input] Make the editing state listenable and al…

bf25eb7

…low batch edits (flutter#21534)" (flutter#22434)

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

[Android text input] Reland flutter#21534 (flutter#22435)

d3eb383


		private BaseInputConnection mDummyConnection;

		// The View is only use for creating a dummy BaseInputConnection for setComposingRegion. The View

[Android Text Input] Make the editing state listenable and allow batch edits #21534

[Android Text Input] Make the editing state listenable and allow batch edits #21534

Uh oh!

Conversation

LongCatIsLooong commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related PR

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Oct 13, 2020 • 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!

justinmc left a comment

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 Oct 8, 2020

Uh oh!

chinmaygarde commented Oct 15, 2020

Uh oh!

LongCatIsLooong commented Oct 15, 2020

Uh oh!

LongCatIsLooong commented Nov 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaryQian Nov 10, 2020 • 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!

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!

GaryQian left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LongCatIsLooong commented Oct 1, 2020 •

edited

Loading

LongCatIsLooong Oct 13, 2020 •

edited

Loading

GaryQian Nov 10, 2020 •

edited

Loading