-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Fix the inconsistency between the local state of the input and the engine state #65754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
||
| // Always attempt to send the value. If the value has changed, then it will send, | ||
| // otherwise, it will short-circuit. | ||
| _updateRemoteEditingValueIfNeeded(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be automatically called by the controller's notifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be removed, see #67892
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for investigating this issue! This was not a simple problem.
Just a small naming change and a request for an issue to be created.
| log.clear(); | ||
|
|
||
| // Currently `_receivedRemoteTextEditingValue` equals 'I will be modified by the formatter.', | ||
| // setEditingState will be called when set the [controller.value] to `_receivedRemoteTextEditingValue` by local. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tricky, good call testing this case.
|
|
||
| bool _isSelectionOnlyChange(TextEditingValue value) { | ||
| return value.text == _value.text && value.composing == _value.composing; | ||
| bool _isSelectionOnlyChanged(TextEditingValue value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should stay as "Change", as I understand it. I think it meant like: "is selection the only change"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| bool _isSelectionOnlyChange(TextEditingValue value) { | ||
| return value.text == _value.text && value.composing == _value.composing; | ||
| bool _isSelectionOnlyChanged(TextEditingValue value) { | ||
| return value.text == _value.text && value.composing == _value.composing && value.selection != _value.selection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why the selection wasn't checked here in the first place. This seems to make more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this, with the changes made in line 1650 through 1656 (i.e., the additional case handling when _value == value )?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LongCatIsLooong
Yes, you are correct. But the name of this function is ambiguous, or change the name instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears the method is only used here. You can get rid of it and use the expression instead (and maybe leave a comment that says if the change is selection only)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
Done.
| if (_isSelectionOnlyChange(value)) { | ||
| if (value == _value) { | ||
| // This is possible, for example, when the numeric keyboard is input, | ||
| // the engine will notify twice for the same value. Perhaps this is an issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you create an actual issue for this and link to it here? I just want to make sure this has the chance to get looked at, in case it is a problem. It doesn't make sense to me that the engine should notify twice for the numeric keyboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The track at #65811
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
| @override | ||
| TextEditingValue get currentTextEditingValue => _value; | ||
|
|
||
| bool _updateEditingValueInProgress = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR really reminds me of this issue, which also deals with the asynchronous communication between the engine and framework: #61282
My solution there wasn't anything like this PR, but I think using this flag _updateEditingValueInProgress could maybe help us handle problems like that.
|
Thanks for the PR and the detailed explanation! This looks like an Android-specific issue, as I failed to reproduce this on iOS. The android input plugin seems to be not syncing back the editing state when there's a selection only change. Maybe this bug needs to be fixed from the engine side (i.e., the android input plugin)? |
If we can ensure that all platforms(android, iOS, Windows, Web, etc.) will sync back to any local changes(including I also reproduce this issue at the Windows platform. In summary, fixing this problem in the framework may be the best choice. Steps to Reproduce1, Clean the textField and input 123 Sample Codeimport 'package:flutter/material.dart';
import 'package:flutter/services.dart';
void main() => runApp(MyApp());
class MyApp extends StatelessWidget {
@override
Widget build(BuildContext context) {
return MaterialApp(
title: 'Retrieve Text Input',
home: MyCustomForm(),
);
}
}
// Define a custom Form widget.
class MyCustomForm extends StatefulWidget {
@override
_MyCustomFormState createState() => _MyCustomFormState();
}
// Define a corresponding State class.
// This class holds data related to the Form.
class _MyCustomFormState extends State<MyCustomForm> {
// Create a text controller and use it to retrieve the current value
// of the TextField.
final myController = TextEditingController(text: 'I love flutter!');
final newController = TextEditingController(text: 'I am a new controller.');
TextEditingController _controller;
@override
void initState() {
super.initState();
_controller = myController;
_controller.addListener(_printLatestValue);
}
@override
void dispose() {
// Clean up the controller when the widget is removed from the widget tree.
// This also removes the _printLatestValue listener.
myController.dispose();
newController.dispose();
super.dispose();
}
_printLatestValue() {
print("Second text field: ${myController.text}");
}
@override
Widget build(BuildContext context) {
return Scaffold(
appBar: AppBar(
title: Text('Retrieve Text Input'),
),
body: Padding(
padding: const EdgeInsets.all(16.0),
child: Column(
children: <Widget>[
// TextField(
// onChanged: (text) {
// print("First text field: $text");
// },
// ),
TextField(
controller: _controller,
keyboardType: TextInputType.multiline,
maxLines: 7,
// inputFormatters: [LengthLimitingTextInputFormatter(3)],
),
MaterialButton(
child: Text("set to '123'"),
onPressed: () {
setState(() {
_controller.value = TextEditingValue(
text: '123',
composing:TextRange(start: -1, end: -1),
selection: TextSelection(
baseOffset: 3,
extentOffset: 3,
affinity: TextAffinity.downstream,
isDirectional: false));
});
}),
MaterialButton(
child: Text("set to '12'"),
onPressed: () {
setState(() {
_controller.text = '12';
});
}),
MaterialButton(
child: Text('change controller'),
onPressed: () {
setState(() {
_controller = newController;
});
}),
],
),
),
);
}
}
|
|
@justinmc @LongCatIsLooong I also create an issue to track #65059 (comment) |
LongCatIsLooong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I thought the text input platform plugin should send back the value until I unearthed flutter/engine@600567e. Yeah you're right we shouldn't enforce the platform text input plugin to send back the value. Thank you!
| bool _isSelectionOnlyChange(TextEditingValue value) { | ||
| return value.text == _value.text && value.composing == _value.composing; | ||
| bool _isSelectionOnlyChanged(TextEditingValue value) { | ||
| return value.text == _value.text && value.composing == _value.composing && value.selection != _value.selection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this, with the changes made in line 1650 through 1656 (i.e., the additional case handling when _value == value )?
justinmc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think this is ready to merge when the tree is green. Thanks so much for working on this!
* [Icons][iOS] Pointing to version of material icon font that includes more metadata in the xml. (#66684) * apply engine cherrypicks * Page-subclasses to take children instead of builder (#66694) * Update pub dependencies to support dart 2.10.0 * cherry-pick 76ad864 * Fix the inconsistency between the local state of the input and the engine state (#65754) Co-authored-by: Will Larche <larche@google.com> Co-authored-by: Michael Goderbauer <goderbauer@google.com> Co-authored-by: xubaolin <xubaolin@oppo.com>
* [Icons][iOS] Pointing to version of material icon font that includes more metadata in the xml. (flutter#66684) * apply engine cherrypicks * Page-subclasses to take children instead of builder (flutter#66694) * Update pub dependencies to support dart 2.10.0 * cherry-pick 76ad864 * Fix the inconsistency between the local state of the input and the engine state (flutter#65754) Co-authored-by: Will Larche <larche@google.com> Co-authored-by: Michael Goderbauer <goderbauer@google.com> Co-authored-by: xubaolin <xubaolin@oppo.com>

Description
See #65059 (comment) and #65059 (comment)
@justinmc @GaryQian Please review, thank you guys very much:)
Related Issues
Fixes #65059
Tests
See the change files.