Repository navigation
RawAutocomplete: Add controller param for caller-managed options - #183705
chrisbobbe wants to merge 3 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
39f25d0 to
7467c52
Compare
|
Looks like my commit messages' Claude Code "co-authored-by" lines triggered that CLA-not-signed failure:
I believe I've followed Flutter's AI contribution guidelines and I'm prepared to discuss this code. 🙂 I did remove those co-authored-by lines to get CI passing. |
There was a problem hiding this comment.
Code Review
This pull request introduces a RawAutocompleteController to allow for programmatic control over the options in a RawAutocomplete widget. This is a valuable addition, providing more flexibility to developers. The changes also include a fix to ensure that the optionsBuilder is called on any change to the TextEditingValue, not just text changes, which improves the widget's responsiveness. The implementation is clean, well-documented, and includes appropriate tests for the new functionality.
| set options(Iterable<T> value) { | ||
| _options = value; | ||
| notifyListeners(); | ||
| } |
There was a problem hiding this comment.
To prevent unnecessary notifications and rebuilds when the options haven't changed, consider adding a check for equality before calling notifyListeners(). A deep equality check might be needed if a new Iterable with the same elements is provided.
For example, you could use listEquals from package:flutter/foundation.dart, but be mindful of the performance cost of converting iterables to lists if they can be large:
set options(Iterable<T> value) {
if (listEquals(value.toList(growable: false), _options.toList(growable: false))) {
return;
}
_options = value;
notifyListeners();
}There was a problem hiding this comment.
This seems like the kind of detail that would be fine to leave up to callers? We should not do a deep equality check.
There was a problem hiding this comment.
Agreed, there shouldn't be a deep equality check here. (So that's a bad suggestion from Gemini.)
I think it'd be typical for a setter like this to have a quick identity check before doing something potentially expensive like notifying listeners. That could be explicit as identical; or some places in the framework use == and rely on the convention that == should only be overridden when the implementation will still be cheap.
There was a problem hiding this comment.
Makes sense; updated to add the == check.
victorsanni
left a comment
There was a problem hiding this comment.
Thanks for finally getting back to this.
Can you fix the failing tests?
7467c52 to
b6d9140
Compare
b6d9140 to
1a2198e
Compare
|
Thanks for the review! Revision pushed. |
1a2198e to
3b76fe2
Compare
|
Fixed the |
3b76fe2 to
90c3cb5
Compare
|
Fixed the dartdoc issue flagged in CI. |
90c3cb5 to
4d9a598
Compare
|
Fixed a leak flagged in CI: Windows framework_tests_widgets_leak_tracking This was a bit complicated and involves a latent bug:
Details testWidgets('RawAutocomplete does not crash when hiding non-displayed overlay', (
WidgetTester tester,
) async {
final controller = TextEditingController();
final focusNode = FocusNode();
addTearDown(controller.dispose);
addTearDown(focusNode.dispose);
await tester.pumpWidget(
TestWidgetsApp(
home: RawAutocomplete<String>(
key: const ValueKey('initial'),
textEditingController: controller,
focusNode: focusNode,
optionsBuilder: (TextEditingValue value) => [],
fieldViewBuilder: (context, ctrl, node, onFieldSubmitted) {
return TestTextField(controller: ctrl, focusNode: node);
},
optionsViewBuilder: (context, onSelected, options) => const SizedBox(),
),
),
);
// Change the key to force the RawAutocomplete to dispose and re-initialize.
await tester.pumpWidget(
TestWidgetsApp(
home: RawAutocomplete<String>(
key: const ValueKey('new-key'),
textEditingController: controller,
focusNode: focusNode,
optionsBuilder: (TextEditingValue value) => [],
fieldViewBuilder: (context, ctrl, node, onFieldSubmitted) {
return TestTextField(controller: ctrl, focusNode: node);
},
optionsViewBuilder: (context, onSelected, options) => const SizedBox(),
),
),
);
await tester.enterText(find.byType(TestTextField), 'trigger');
await tester.pump();
expect(tester.takeException(), isNull);
});
Fixed by fixing the latent bug: I added a commit changing |
|
Fixed a docs issue flagged in CI, which was that the Material Fixed by surfacing the controller-based interface in |
Ah 🙂 I see from the current CI failure that this won't be possible during the code freeze: #184093 Looks like the freeze is expected to end sometime in May, so I'd prefer to leave the Material support out of this PR and do it as a followup after the freeze instead. So I'll add a commit removing the |
856069a to
a5fc3ba
Compare
Fixes flutter#159443. RawAutocomplete.optionsBuilder only receives a TextEditingValue, so it cannot react to changes in external state (e.g. a data source that loads asynchronously after focus). This adds AutocompleteController, a ChangeNotifier with an options getter/setter, and a new controller param on RawAutocomplete that is mutually exclusive with optionsBuilder. When a controller is provided, the caller sets controller.options directly and the widget reacts. Internally, the state now uses AutocompleteController for both paths: optionsBuilder results flow through an internal controller, while an external controller is used directly. The post-update work (semantics announcements, highlight clamping, selection validity, visibility) is extracted into _handleOptionsChanged, which listens to whichever controller is active.
a5fc3ba to
01aa1b6
Compare
|
Revision pushed, PTAL! |
justinmc
left a comment
There was a problem hiding this comment.
Thanks for working on this! I've wanted to get it fixed for awhile.
Do you have any thoughts on passing the list of options in to RawAutocomplete directly versus using a controller as you've done here? I think you could achieve the same thing by calling setState for the most part, but I'm curious if/why the controller pattern is better for your use case. I'm open to either way, just making sure we think it through.
|
|
||
| // Makes sure that options change only when content of the field changes. | ||
| var shouldUpdateOptions = false; | ||
| if (value.text != _lastFieldText) { |
There was a problem hiding this comment.
Was there any specific reason that you wanted the options to rebuild when any part of the TextEditingValue changes instead of just the text? I believe this choice was deliberate (though I'm not 100% sure it was the right decision). If I play around with HTML's datalist, it doesn't update for composing text (https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/datalist).
There was a problem hiding this comment.
Also the new controller should be a workaround for people that do want this behavior, right? I think you could listen to the TextEditingValue and update the options whenever you want.
There was a problem hiding this comment.
the options to rebuild when any part of the TextEditingValue changes instead of just the text?
I remember deciding to keep this behavior a while back:
https://github.com/flutter/flutter/pull/148995/changes#r1612467521 because the optionsBuilder API exposes the entire TextEditingValue, and so there might exist a user who monitors selection changes that will be broken if we only rebuild on text changes.
But I agree it makes it unnecessarily expensive.
There was a problem hiding this comment.
So the current behavior when just the selection or composing changes (not the text) is to call and await optionsBuilder and drop the result on the floor 🙂 without updating the options:
`_onChangedField` in `main`
See that shouldUpdateOptions is false if the text has not changed, and see the conditional return when that is false (marked "HERE"):
// Assigning an ID to every call of _onChangedField is necessary to avoid a
// situation where _options is updated by an older call when multiple
// _onChangedField calls are running simultaneously.
int _onChangedCallId = 0;
// Called when _textEditingController changes.
Future<void> _onChangedField() async {
// During a selection, changes to the field text should not trigger
// options update.
if (_selecting) {
return;
}
final TextEditingValue value = _textEditingController.value;
// Makes sure that options change only when content of the field changes.
var shouldUpdateOptions = false;
if (value.text != _lastFieldText) {
shouldUpdateOptions = true;
_onChangedCallId += 1;
}
_lastFieldText = value.text;
final int callId = _onChangedCallId;
final Iterable<T> options = await widget.optionsBuilder(value);
if (!mounted) {
return;
}
// Makes sure that previous call results do not replace new ones.
if (callId != _onChangedCallId || !shouldUpdateOptions) {
return; // <----------------- HERE
}
if (_options.isEmpty != options.isEmpty) {
_announceSemantics(options.isNotEmpty);
}
_options = options;
_updateHighlight(_highlightedOptionIndex.value);
final T? selection = _selection;
if (selection != null && value.text != widget.displayStringForOption(selection)) {
_selection = null;
}
_updateOptionsViewVisibility();
}So it doesn't seem hard to fully commit to updating only on text changes, if we want to. We'd just want to remove the cases where optionsBuilder is called and its results are ignored, to save the unnecessary work. To do this, I guess there's a breaking-change question along the lines @victorsanni mentioned—
the
optionsBuilderAPI exposes the entireTextEditingValue, and so there might exist a user who monitors selection changes that will be broken if we only rebuild on text changes.
—but the breakage is limited to callers whose optionsBuilder has some desired side effect on composing/IME changes, and I think chances of that are probably low.
In my read of the Git history, it was an accident in 7695582 to stop updating the options on non-text changes. See my second commit in this revision:
RawAutocomplete: Update options on selection/composing changes, not just text
Previously, _onChangedField would call optionsBuilder on every
TextEditingValue change but silently discard the result unless
.text had changed. This meant an optionsBuilder that depended on
.selection or .composing would never see its results applied.
That guard was introduced in 919d20511 ("Dismiss Autocomplete with
ESC", #97790) for a narrow purpose: after the user pressed ESC to
hide the options overlay, a selection-only change (e.g. moving the
cursor) should not re-show it. Commit 76955826a (#150776) then
repurposed the same _lastFieldText check to also gate all option
updates, as part of adding race-condition protection for async
optionsBuilder.
This change separates the two concerns:
- Race-condition protection (_onChangedCallId) now applies to every
call, not just text changes.
- ESC-dismiss behavior is handled by a dedicated _userHidOptions
flag, which is set when the user presses ESC and cleared when the
text changes, the field is re-focused, or the user navigates
options with arrow keys.
Happy to go either way, i.e. commit optionsBuilder to text-changes-only (by removing unnecessary calls) or keep the approach in this revision where we update on selection/IME changes too.
As one data point, in Zulip we do want to change the autocomplete options on selection changes, especially cursor-position changes (i.e. when the selection is collapsed), as part of supporting @-mentions in the middle of a Zulip message you're composing. But as you say @justinmc that's easy to do in the new API. 🙂
victorsanni
left a comment
There was a problem hiding this comment.
LGTM so far, left a few comments
| /// The current list of selectable options. | ||
| Iterable<T> get options => _options; | ||
| Iterable<T> _options = Iterable<T>.empty(); | ||
| set options(Iterable<T> value) { |
There was a problem hiding this comment.
Do we need to document the setter as well? Add add that if options is set, it will trigger a rebuild of the options view etc?
|
|
||
| // Makes sure that options change only when content of the field changes. | ||
| var shouldUpdateOptions = false; | ||
| if (value.text != _lastFieldText) { |
There was a problem hiding this comment.
the options to rebuild when any part of the TextEditingValue changes instead of just the text?
I remember deciding to keep this behavior a while back:
https://github.com/flutter/flutter/pull/148995/changes#r1612467521 because the optionsBuilder API exposes the entire TextEditingValue, and so there might exist a user who monitors selection changes that will be broken if we only rebuild on text changes.
But I agree it makes it unnecessarily expensive.
| } | ||
|
|
||
| // Makes sure that previous call results do not replace new ones. | ||
| if (callId != _onChangedCallId) { |
There was a problem hiding this comment.
Can we consolidate this if-block into the one just above?
| void didUpdateWidget(RawAutocomplete<T> oldWidget) { | ||
| super.didUpdateWidget(oldWidget); | ||
| if (!identical(oldWidget.controller, widget.controller)) { | ||
| oldWidget.controller?.removeListener(_handleOptionsChanged); |
There was a problem hiding this comment.
Do we need a test for this as well? That swapping out controllers at runtime works correctly?
| expect(lastOptions, <String>['dingo', 'flamingo', 'goose']); | ||
| }); | ||
|
|
||
| group('AutocompleteController', () { |
There was a problem hiding this comment.
Should we test for the async case? Since optionsBuilder can be asynchronous, is it worth it to test asynchronous updates to controller.options as well?
|
Coming back to this today; I left #183705 (comment) and hope to share more thoughts and a new revision tomorrow, but I've run out of time for today :) |
victorsanni
left a comment
There was a problem hiding this comment.
Hi @chrisbobbe, can you fix the merge conflicts in this PR?
|
Hi @chrisbobbe is this something you would still like to work on? |
|
Absolutely, yes; sorry I've let this sit. Will return to it as soon as I can. |
|
Looks like there are some conflicts here, still planning on returning to this one @chrisbobbe ? |
|
Coming back to this today. Apologies again for letting it sit. I'd like to send a few PRs as prep for this (I'll edit this comment to list them), to shrink the diff for this PR. I've also re-sequenced the work so that we can defer the decision about non-text changes (IME, selection; see #183705 (comment)) for a separate PR after this one. Those prep PRs are:
|
I think you can open more than 1, as long as they are drafts. Also there are new merge conflicts here. |
…ispose (flutter#190686) This is a minor bugfix I'm proposing as prep for my work on flutter#159443. For details on how that work blocks on this, see flutter#183705 (comment). initState registers _onFocusChange on the passed FocusNode, but didUpdateWidget and dispose were removing/adding _updateOptionsViewVisibility instead. This meant the _onFocusChange listener was never properly removed, so it could fire on an already-disposed State. Also, when the focus node changes, refresh _hasFocus from the new node, since its focus state may not match the old node's. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Victor Sanni <[email protected]>
…ispose (flutter#190686) This is a minor bugfix I'm proposing as prep for my work on flutter#159443. For details on how that work blocks on this, see flutter#183705 (comment). initState registers _onFocusChange on the passed FocusNode, but didUpdateWidget and dispose were removing/adding _updateOptionsViewVisibility instead. This meant the _onFocusChange listener was never properly removed, so it could fire on an already-disposed State. Also, when the focus node changes, refresh _hasFocus from the new node, since its focus state may not match the old node's. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Victor Sanni <[email protected]>
Fixes #159443.
This lets
RawAutocomplete's callers just specify the autocomplete options directly, on their own schedule, rather than viaoptionsBuilderwhich is only called on text-field-value changes. This is non-breaking: callers opt in withRawAutocomplete's newcontrollerparam.In a separate commit, this also fixes a behavior that seems undesirable: for callers who use
optionsBuilder, the passed callback was only being called when the text field'sTextEditingValue.textchanged. Now the callback is also called when the selection or composing range changes.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.