Stop autocomplete options builder from calling on text selection changes#148995
Stop autocomplete options builder from calling on text selection changes#148995victorsanni wants to merge 2 commits intoflutter:masterfrom
Conversation
github-actions
bot
added
the
framework
| } | ||
| _lastFieldText = value.text; | ||
|
|
||
| final Iterable<T> options = await widget.optionsBuilder(value); |
There was a problem hiding this comment.
This is a bit weird: optionsBuilder takes a TextEditingValue, so any TextEditingValue change should send a new signal.
There was a problem hiding this comment.
Technically you can achieve the filtering by doing the filtering in the optionsBuilder callback I think? Although I have to admit it is not convenient.
There was a problem hiding this comment.
That's a good point, I think you're right.
What if instead we put a check around _updateOptionsViewVisibility on line 371 so that it's only called if the _options have changed since it was last called? Then remove all of the added lines above (356-363).
@victorsanni would that still solve the debounce bug that we were looking at yesterday?
Or maybe it doesn't even matter very much if we call it multiple times, just remove all of this optimization?
Either way, it means that it's up to the user to avoid making duplicate calls if the selection changes. We should update our examples to do this, and maybe mention it in the docs.
There was a problem hiding this comment.
It wouldn't, because optionsBuilder will be called again before checking that _options have changed (so the behavior remains as is before this PR). But I agree, the onus of filtering can continue being on the user in the optionsBuilder callback. If we don't want the selection to cause rebuilds, then we should solve the problem once and for all by making optionsBuilder take only the string textEditingController value. So, I will go ahead and close this PR.
On a side note, #150776 does provide quite a clever solution for the debounce bug.
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
I think I'm agreeing that we should call optionsBuilder whenever the TextEditingValue changes. Really I probably should have passed only the String to optionsBuilder in the first place, because I really can't think of a reason that someone would want to base their optionsBuilder on the selection or composing region... But they could.
So if we buy into that, we have 2 options I can think of:
- Only call _updateOptionsViewVisibility if the _options have changed.
- Forget all of this optimization, and call optionsBuilder and _updateOptionsViewVisibility every time.
I think I'm leaning option 1 to avoid rebuilding like crazy when the user is dragging the selection around.
| } | ||
| _lastFieldText = value.text; | ||
|
|
||
| final Iterable<T> options = await widget.optionsBuilder(value); |
There was a problem hiding this comment.
That's a good point, I think you're right.
What if instead we put a check around _updateOptionsViewVisibility on line 371 so that it's only called if the _options have changed since it was last called? Then remove all of the added lines above (356-363).
@victorsanni would that still solve the debounce bug that we were looking at yesterday?
Or maybe it doesn't even matter very much if we call it multiple times, just remove all of this optimization?
Either way, it means that it's up to the user to avoid making duplicate calls if the selection changes. We should update our examples to do this, and maybe mention it in the docs.
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
3 participants
Currently, in
RawAutocomplete, changing the text selection also updates the options view, even when it shouldn't:flutter/packages/flutter/lib/src/widgets/autocomplete.dart
Lines 364 to 365 in de0fbde
This is because
optionsBuilderis called before checking if only the text changes and not the selection. Thus, ifoptionsBuilderis async, the options view is updated only after waiting for theoptionsBuilderfuture to resolve, even when only the selection changes. This causes a loading display when highlighting in the text field.This PR moves the check to before
optionsBuilderis called.Before:
After:
Related Issues: #108258, #147900
Related PRs: #147377
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.