Call setEditingState when text changes. by dnfield · Pull Request #47177 · flutter/flutter

dnfield · 2019-12-17T00:12:13Z

Description

The removed line prevents us from mutating the tracking variable too early (it gets mutated by a later function anyway, but mutating it this early means we don't end up notifying the engine side of changes). This wasn't a problem before because the engine stayed running when the app was backgrounded, which meant the state was never really lost. Now that the engine detaches, we lose the state and need to have it to send it up again.

This results in #47137.

I also found that some tests were mutating global state, so I updated those, and I updated the tester harness to reset some global state between tests to make it more useful (this is now asserted by those tests). I also noticed that isRegistered was not trustworthy because of this global state it's connected to, so I tried to make it work better.

Related Issues

fixes #47137

Tests

I added the following tests:

Assert the expected textInput method call is made (setEditingState) after mutating the text.

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.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/docs/release/breaking-changes/test-text-input

dnfield · 2019-12-17T00:12:41Z

/cc @dannyvalentesonos

dnfield · 2019-12-17T00:13:27Z

packages/flutter/lib/src/widgets/editable_text.dart

        _obscureLatestCharIndex = _value.selection.baseOffset;
      }
    }
-    _lastKnownRemoteTextEditingValue = value;


This is the key piece here - we update this value in the call path of _formatAndSetValue anyway, and if we update it here, a later check that compares the value being passed with _lastKnownRemoteTextEditingValue always returns true.

This is just too early to set the value -we haven't actually notified the remote side yet.

dnfield · 2019-12-17T00:13:57Z

packages/flutter/test/widgets/editable_text_test.dart

+      'TextInput.setEditingState',
+      'TextInput.setEditingState',
+      'TextInput.show',
+      'TextInput.setEditingState',


This line is the key part of this test - without the change, we don't get the last setEditingState, which contains the value ....

dnfield · 2019-12-17T00:15:16Z

packages/flutter_test/lib/src/test_text_input.dart

 import 'package:flutter/services.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:flutter/foundation.dart';
+import 'package:pedantic/pedantic.dart';


@jonahwilliams is this ok? It's for unawaited.

This would introduce a new (direct) dependency on package:pedantic. I don't think we want that.

Removing this, but FWIW it looks like we directly depend on it elsewhere (although our pubspec doesn't indicate that).

dnfield · 2019-12-17T00:23:22Z

packages/flutter_test/lib/src/test_text_input.dart

+  // Someone could have unregistered us by calling
+  // `SystemChannels.textInput.setMockMethodCallHandler` in the test. Actually
+  // check if we're registered by calling the method, which should work
+  // immediately if we are registered since that method does no real async work.
+  bool get isRegistered {
+    final int oldLogLength = log.length;
+    unawaited(SystemChannels.textInput.invokeMethod<void>('__isRegistered'));
+    if (log.length == oldLogLength) {
+      return false;
+    }
+    log.removeLast();
+    return true;
+  }


I can move this to a separate PR if that works better - it's not needed for this issue.

goderbauer · 2019-12-17T00:23:46Z

packages/flutter_test/lib/src/test_text_input.dart

 import 'package:flutter/services.dart';
 import 'package:flutter_test/flutter_test.dart';
 import 'package:flutter/foundation.dart';
+import 'package:pedantic/pedantic.dart';


This would introduce a new (direct) dependency on package:pedantic. I don't think we want that.

goderbauer · 2019-12-17T00:27:21Z

packages/flutter_test/lib/src/test_text_input.dart

+  // immediately if we are registered since that method does no real async work.
+  bool get isRegistered {
+    final int oldLogLength = log.length;
+    unawaited(SystemChannels.textInput.invokeMethod<void>('__isRegistered'));


This may have unintended side effects if somebody else is currently registered and receives this, no?

Hmm. Yes. I'm going to back this change out, don't need it for this PR, - will file an issue about it.

goderbauer · 2019-12-17T00:28:02Z

packages/flutter/lib/src/widgets/editable_text.dart

        _obscureLatestCharIndex = _value.selection.baseOffset;
      }
    }
-    _lastKnownRemoteTextEditingValue = value;


goderbauer · 2019-12-17T00:31:44Z

packages/flutter_test/lib/src/widget_tester.dart

      return binding.runTest(
        () async {
          debugResetSemanticsIdCounter();
+          tester.testTextInput.register();


Should there maybe be a convenience method and tester (or testTextInput) to reset everything that does both of this?

I wonder if this is the right place to reset these things, it feels odd here.

I tried doing it before the binding.runTest and that didn't work. FWIW, this is where we reset the semantics ID counter as well so...

I'll move these to a convenience method.

goderbauer

LGTM

dnfield · 2019-12-17T01:20:19Z

Fixing the test API is breaking one other test, which, IMO is buggy.

I'm going to spit out the package:flutter_test changes into a separate PR so we can do a proper breaking change/migration guide for it (and not have to roll this back if it breaks something internally).

dnfield · 2019-12-17T01:30:41Z

No, it's my actual code change in the framework causing the test breakage. Investigating why, but it looks like this may be a breaking change.

dnfield · 2019-12-17T02:00:36Z

This is breaking whether or not I fix the issue in the tester.

If I fix the tester issue, it's breaking because the existing test relied on dirty state from the previous test.

If I avoid fixing the tester issue, it's still breaking because the existing test now gets different dirty state from the previous test.

dnfield · 2019-12-17T02:00:44Z

Created flutter/website#3430 for migration.

dnfield · 2019-12-17T21:37:29Z

Migration guide has landed. Going ot land this on green.

This reverts commit d4b49ce.

…" (flutter#47397)" This reverts commit d51956a.

Call setEditingState when text changes.

850602d

fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Dec 17, 2019

googlebot added the cla: yes label Dec 17, 2019

dnfield requested review from goderbauer, matthew-carroll and xster December 17, 2019 00:12

dnfield commented Dec 17, 2019

View reviewed changes

unawaited

4c15b9b

dnfield commented Dec 17, 2019

View reviewed changes

dnfield added the a: text input Entering text in a text field or keyboard related problems label Dec 17, 2019

dnfield requested a review from jonahwilliams December 17, 2019 00:16

..

dceb699

dnfield commented Dec 17, 2019

View reviewed changes

goderbauer reviewed Dec 17, 2019

View reviewed changes

dnfield mentioned this pull request Dec 17, 2019

Respond to TextInputClient.reattach messages. #43959

Merged

9 tasks

dnfield added 4 commits December 16, 2019 16:36

undo isRegistered changes

23656fb

remove pedantic

5aae472

convenience method

b2ba2bc

todo

d26f3ca

goderbauer approved these changes Dec 17, 2019

View reviewed changes

..

3920e65

dnfield mentioned this pull request Dec 17, 2019

TestTextInput maintains state across tests and can lose control of the system channel across tests #47182

Closed

dnfield added the c: API break Backwards-incompatible API changes label Dec 17, 2019

dnfield mentioned this pull request Dec 17, 2019

Migration guide for https://github.com/flutter/flutter/pull/47177 flutter/website#3430

Merged

fix breakages

1aed656

dnfield requested a review from goderbauer December 17, 2019 01:58

fluttergithubbot added the f: material design flutter/packages/flutter/material repository. label Dec 17, 2019

expectation

a8b021e

dnfield mentioned this pull request Dec 17, 2019

Start running tests with --test-randomize-ordering-seed=random #47233

Closed

dnfield added a commit to flutter/website that referenced this pull request Dec 17, 2019

Migration guide for flutter/flutter#47177 (#3430)

39da3cd

dnfield merged commit d4b49ce into flutter:master Dec 17, 2019

dnfield deleted the setEditingState branch December 17, 2019 22:03

chingjun added a commit that referenced this pull request Dec 19, 2019

Revert "Call setEditingState when text changes. (#47177)"

09b9172

This reverts commit d4b49ce.

chingjun mentioned this pull request Dec 19, 2019

Revert "Call setEditingState when text changes." #47397

Merged

chingjun added a commit that referenced this pull request Dec 19, 2019

Revert "Call setEditingState when text changes. (#47177)" (#47397)

d51956a

This reverts commit d4b49ce.

dnfield added a commit to dnfield/flutter that referenced this pull request Dec 19, 2019

Revert "Revert "Call setEditingState when text changes. (flutter#47177)…

67583db

…" (flutter#47397)" This reverts commit d51956a.

This was referenced Dec 19, 2019

Reland text state #47464

Merged

Fix requestExistingInputState response #47472

Merged

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

Conversation

dnfield commented Dec 17, 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 Dec 17, 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!

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!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Dec 17, 2019

Uh oh!

dnfield commented Dec 17, 2019

Uh oh!

dnfield commented Dec 17, 2019

Uh oh!

dnfield commented Dec 17, 2019

Uh oh!

dnfield commented Dec 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dnfield commented Dec 17, 2019 •

edited

Loading