Skip to content

Fix requestExistingInputState response#47472

Merged
fluttergithubbot merged 2 commits intoflutter:masterfrom
dnfield:fix_text_setstate
Dec 20, 2019
Merged

Fix requestExistingInputState response#47472
fluttergithubbot merged 2 commits intoflutter:masterfrom
dnfield:fix_text_setstate

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Dec 19, 2019 •
edited
Loading

Description

#47177 attempted to fix this issue by more frequently calling setEditingState. This resulted in races/contention between the framework and platform side about who owned the editing state value, and caused failures on some Espresso based tests. I'm adding some tests to make sure that behavior is more clearly exercised in #47464

Related Issues

#47177 Was a different attempt to fix this.
#43959 Was where a lot of this code was originally introduced.

Fixes #47137.

Tests

I added the following tests:

Removed a test that isn't valid - we shouldn't be using setEditingState that way.

Updated a test to use the new property on TextEditingClient.

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 read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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

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.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@dnfield dnfield requested review from goderbauer and xster December 19, 2019 18:39
@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Dec 19, 2019
@dnfield
Copy link
Contributor Author

dnfield commented Dec 19, 2019

@Hixie - this is an example of a breaking change that will be really hard to make soft.

We have an internal customer who implements the interface I want to change.

I could make it a soft break in a really ugly way by adding the new field to a subclass, checking if I have that subclass, and if not doing something to alert the developer that they're going to lose state when the app is backgrounded on Android.

That seems wrong to me. We'd be writing code that's really ugly and hard to understand, e.g.:

if (_currentConnection._client is TextEditingClientWithTextValue) {
  // send state up to platform
} else {
  //  report error with instructions on how to migrate?  Except we'll eventually want to remove the subclass?
}

While the migration is actually fairly simple, and the usage of this interface is very limited.

@dnfield
Copy link
Contributor Author

dnfield commented Dec 19, 2019

I'm going to make this a soft migration for the internal client by patching them to have the new value as an override with a temporary interface that we'll delete once this rolls.

@dnfield
Copy link
Contributor Author

dnfield commented Dec 19, 2019

xref cl/286430188 and b/146568373

goderbauer
goderbauer approved these changes Dec 19, 2019
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

packages/flutter/lib/src/services/text_input.dart
assert(_currentConnection._client != null);
_attach(_currentConnection, _currentConfiguration);
// This will be null if we've never had a call to [_setEditingState].
if (_currentTextEditingValue != null) {
Copy link
Member

@goderbauer goderbauer Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the code would be a little bit clearer leaving this if outside the _attach method. The setEditingState seems like kinda a strange parameter to that method.

Copy link
Contributor Author

@dnfield dnfield Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure - it seemed a little awkward to me either way but this way it'll be closer to the actual use case for it.

Copy link
Contributor Author

@dnfield dnfield Dec 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually redoing this uncovered a potential bug (TextEditingValue can be null).

dnfield added a commit to dnfield/website that referenced this pull request Dec 19, 2019
@dnfield
migration for flutter/flutter#47472
0c0aa0a
@dnfield
Copy link
Contributor Author

dnfield commented Dec 19, 2019

This should wait for the internal CL to land first in case it gets grabbed by a roll soon

dnfield added a commit to flutter/website that referenced this pull request Dec 20, 2019
@dnfield
migration for flutter/flutter#47472 (#3462)
b46714f
@fluttergithubbot fluttergithubbot merged commit 93f718b into flutter:master Dec 20, 2019
@dnfield dnfield deleted the fix_text_setstate branch January 7, 2020 03:57
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@goderbauer goderbauer goderbauer approved these changes

@xster xster Awaiting requested review from xster

Assignees

No one assigned

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Add2App] The typed characters in a TextField are lost when backgrounding the app with focus on the TextField on Android

4 participants

@dnfield @goderbauer @googlebot @fluttergithubbot