Respond to TextInputClient.reattach messages. by dnfield · Pull Request #43959 · flutter/flutter

dnfield · 2019-11-01T05:59:53Z

Description

When the new Android embedding detaches from the engine (e.g. because it goes to the background), it loses state related to text input client(s).

It's being updated to send this message to the framework so that the framework can reattach the text input client, and the keyboard can continue to work.

Related Issues

This and the related engine PR will combine to resolve #35054

Tests

I need to figure out how to test this. Testing will require some refactoring, I'm thinking at the very lease making _TextInputClientHandler @visibleForTesting, or figure out a way to have the test binding simulate a message coming from the engine for this. For now I just want it up so reviewers of the engine side can see.

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 Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

dnfield · 2019-11-01T08:33:37Z

This test is .. very ugly. But it's a test.

goderbauer · 2019-11-01T18:25:48Z

packages/flutter/lib/src/services/text_input.dart

-final _TextInputClientHandler _clientHandler = _TextInputClientHandler();
+final TextInputClientHandler _clientHandler = TextInputClientHandler(
+  SystemChannels.textInput,
+  null,


Make this an optional parameter?

goderbauer · 2019-11-01T18:26:59Z

packages/flutter/lib/src/services/text_input.dart

  /// should call [TextInputConnection.close] on the returned
  /// [TextInputConnection].
-  static TextInputConnection attach(TextInputClient client, TextInputConfiguration configuration) {
+  static TextInputConnection attach(TextInputClient client) {


This is a breaking change, no?

goderbauer · 2019-11-01T18:28:21Z

packages/flutter/lib/src/services/text_input.dart

+    return connection;
+  }
+
+  static void _attach(TextInputClientHandler handler, TextInputConnection connection) {


There should probably be some documentation on what the difference between attach and _attach is.

goderbauer · 2019-11-01T18:30:42Z

packages/flutter/lib/src/services/text_input.dart

+  ///
+  /// This configuration is requested when establishing or re-establishing the
+  /// connection between the [TextInput] and the engine.
+  TextInputConfiguration createConfiguration();


This also looks like a breaking change? Implementors/Subclasses of this class will have to implement a new method?

packages/flutter/lib/src/services/text_input.dart

goderbauer · 2019-11-01T18:35:01Z

packages/flutter/lib/src/services/text_input.dart

+class TextInputClientHandler {
+  /// Creates a [TextInputClientHandler].
+  @visibleForTesting
+  TextInputClientHandler(this._channel, this._currentConnection) : assert(_channel != null) {


The arguments are only there for testing, right? I wonder if we should default them to what we need them in production.

I'm refactoring the test.

matthew-carroll · 2019-11-01T19:22:53Z

packages/flutter/lib/src/services/text_input.dart

    final String method = methodCall.method;
+
+    // Reattach request needs to be handled regardless of the client ID.
+    if (method == 'TextInputClient.reattach') {


Same question here as the engine side PR. Can we call this something like requestExistingInputState or requestInputConfiguration so that we can avoid implying a previous attachment and express the goal of the message?

goderbauer

Some other notes:

Do Android and Framework agree on the state of the text field after re-attach, i.e. with regards to the text that's present in the text field?
Can you also double check that this doesn't break textfields that switch the keyboard (as we have seen with the previous attempt)?
We need separate API for this and #43466? Or can we consolidate? (Maybe at least only do one breaking change announcement for both)

goderbauer · 2019-11-01T19:34:26Z

packages/flutter/lib/src/services/text_input.dart

    final String method = methodCall.method;
+
+    // Reattach request needs to be handled regardless of the client ID.
+    if (method == 'TextInputClient.reattach') {


Can you also add the new message to the documentation in system_channels.dart

matthew-carroll

I'll defer to @goderbauer for the LGTM on this because I'm less familiar with the Dart side of input connections. But nothing jumps out at me.

goderbauer · 2019-11-01T21:51:56Z

packages/flutter/lib/src/services/system_channels.dart

  ///    second argument is a [String] consisting of the stringification of one
  ///    of the values of the [TextInputAction] enum.
  ///
+  ///  * `TextInputClient.requestExistingInputState`: The embedding may have


Let's document what should happen if there's nothing to send.

goderbauer · 2019-11-01T21:55:08Z

packages/flutter/lib/src/services/text_input.dart

+    if (method == 'TextInputClient.requestExistingInputState') {
+      assert(_currentConnection._client != null);
+      _attach(_currentConnection);
+      if (_lastTextEditingValue != null) {


Does it seem odd that we cache the lastTextEditingValue, but don't cache the configuration?

Just caching them all now. I think this is probably cleaner and it's no longer breaking.

goderbauer · 2019-11-01T21:55:54Z

dev/integration_tests/codegen/android/.project

@@ -1,17 +0,0 @@
-<?xml version="1.0" encoding="UTF-8"?>


Maybe revert this and clean it up in a separate PR, potentially by adding something to gitignore?

Going to file a separate PR, this was unintentional. Cleaned it up for now here.

goderbauer

LGTM

I like that all the channel calls are now encapsulated in one object and not all over the place.

goderbauer · 2019-11-04T17:33:36Z

packages/flutter/lib/src/services/text_input.dart

+    // Reattach request needs to be handled regardless of the client ID.
+    if (method == 'TextInputClient.requestExistingInputState') {
+      assert(_currentConnection._client != null);
+      _attach(_currentConnection, _currentConfiguration);


I was wondering here if _currentConfiguration could ever be null at this point. Maybe add an assert(_currentConfiguration != null) above this one, possibly with a comment saying that we always have a configuration if we have a connection?

We have an early return at line 926 if _currentConfiguration == null.

goderbauer · 2019-11-04T17:35:03Z

packages/flutter/lib/src/services/text_input.dart

+      return;
+    final String method = methodCall.method;
+
+    // Reattach request needs to be handled regardless of the client ID.


Update this comment? Since the method is no longer called "reattach"?

goderbauer · 2019-11-04T17:37:22Z

packages/flutter/test/services/text_input_test.dart

+
+    setUp(() {
+      fakeTextChannel = FakeTextChannel((MethodCall call) async {
+        return;


nit: do you actually need the return? Just an empty body is not enough?

I thought that would make the analyzer unhappy, I was wrong. Done

goderbauer · 2019-11-04T17:39:18Z

packages/flutter/test/services/text_input_test.dart

+
+    test('text input client handler responds to reattach with setClient', () async {
+      TextInput.attach(client, client.configuration);
+


Maybe expect here that there has been one method call to ensure that requestExistingInputState only calls setClient once and not twice.

goderbauer · 2019-11-04T17:39:36Z

packages/flutter/test/services/text_input_test.dart

+      final TextInputConnection connection = TextInput.attach(client, client.configuration);
+      const TextEditingValue editingState = TextEditingValue(text: 'foo');
+      connection.setEditingState(editingState);
+


goderbauer · 2019-11-04T17:40:34Z

packages/flutter/test/services/text_input_test.dart

+
+  @override
+  void updateEditingValue(TextEditingValue value) => throw UnimplementedError();
+  @override


uber-nit: add an empty line above this one for constancy?

goderbauer · 2019-11-04T17:43:36Z

packages/flutter/test/services/text_input_test.dart

+      final String expectedString = utf8.decode(expectedData.buffer.asUint8List());
+
+      if (outgoingString != expectedString) {
+        print('''Index $i did not match:


This would format a little nicer as:

print( 'Index $i did not match:\n' ' actual: ${outgoingCalls[i]}\n' ' expected: ${calls[i]}' );

Agreed, done.

dnfield · 2019-11-04T22:26:57Z

/cc @nturgut for review of merge

nturgut

Thanks for the merge Dan!

dnfield · 2019-12-17T00:33:54Z

packages/flutter/lib/src/services/text_input.dart

+      if (_currentTextEditingValue != null) {
+        _setEditingState(_currentTextEditingValue);
+      }


It turns out these lines never get called because of #47137 - see #47177

Quick question: in Android or in general?

In Flutter for Web, this is the main part that we are syncing the state from Framework to Platform. It works for the web engine so far.

Let's debug and try to see why it is not the case for Android.

Android is the only platform that would go down this codepath today. It shouldn't be a problem for web.

See the linked PR for more details - I don't think this is a problem for web, but it is a problem for Android where the engine side loses state.

Companion to flutter/engine#13474

193d3e1

dnfield requested review from goderbauer, matthew-carroll, mdebbar and xster November 1, 2019 05:59

fluttergithubbot added framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Nov 1, 2019

googlebot added the cla: yes label Nov 1, 2019

Test, I guess.

aa4ad6a

dnfield marked this pull request as ready for review November 1, 2019 08:33

dnfield removed the work in progress; do not review label Nov 1, 2019

goderbauer reviewed Nov 1, 2019

View reviewed changes

matthew-carroll reviewed Nov 1, 2019

View reviewed changes

goderbauer reviewed Nov 1, 2019

View reviewed changes

goderbauer mentioned this pull request Nov 1, 2019

Adding handling of TextInputClient.onConnectionClosed messages handli… #43466

Merged

9 tasks

goderbauer reviewed Nov 1, 2019

View reviewed changes

matthew-carroll approved these changes Nov 1, 2019

View reviewed changes

fix tests, send editing state

e21092a

goderbauer reviewed Nov 1, 2019

View reviewed changes

dnfield force-pushed the reattach_text_input branch from 9686425 to ed02d70 Compare November 1, 2019 22:11

unbreak, revert unintended changes, move all invokemethod into TextInput

629c3d9

dnfield force-pushed the reattach_text_input branch from ed02d70 to 629c3d9 Compare November 1, 2019 22:12

Fix test by resetting IDs

77c9968

dnfield requested a review from goderbauer November 1, 2019 23:29

goderbauer approved these changes Nov 4, 2019

View reviewed changes

review

c7ec0a8

dnfield added the waiting for tree to go green label Nov 4, 2019

One more

d4ad315

dnfield removed the waiting for tree to go green label Nov 4, 2019

dnfield added the waiting for tree to go green label Nov 4, 2019

merge

8b2860a

nturgut approved these changes Nov 4, 2019

View reviewed changes

dnfield merged commit 7957c56 into flutter:master Nov 5, 2019

dnfield deleted the reattach_text_input branch November 5, 2019 00:28

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019

Respond to TextInputClient.reattach messages. (flutter#43959)

8488a8f

dnfield commented Dec 17, 2019

View reviewed changes

dnfield mentioned this pull request Dec 19, 2019

Fix requestExistingInputState response #47472

Merged

13 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021


		test('text input client handler responds to reattach with setClient', () async {
		TextInput.attach(client, client.configuration);

Conversation

dnfield commented Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

dnfield commented Nov 1, 2019

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!

goderbauer 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!

matthew-carroll 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!

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!

goderbauer 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!

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

dnfield commented Nov 1, 2019 •

edited

Loading