set onChangedField function to only update options with new results#150776
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
github-actions
bot
added
the
framework
|
@mohsinraza-fdev Could you please add a test as indicated by the bot message above to make sure we never regress this? Thank you. |
on it |
Test added. |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
There may be a more complete solution in progress by @victorsanni (#147900 and/or #148995). This PR should probably be on hold until we figure out those bigger changes.
Sorry for the confusion @mohsinraza-fdev. I'll assign @victorsanni to #150774 now. I appreciate you submitting your issue and this PR. We'll see if we can move forward with this or if we have to close it.
Sure |
|
I talked to @victorsanni offline and his PR should cover this, but we'll revisit it after his PR is merged and double check. If not then we can move forward with this PR. Sorry again for the duplicate work here. |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
Talking with @victorsanni, I think we want to merge this PR first and then make the bigger changes in his PR. So let's move forward with this if you're still available! I've left a bunch of small nit comments, except for one where I had an idea to simplify the two int variables. Otherwise I think this PR is ready.
|
Hi @mohsinraza-fdev are you still working on this pr? |
Yep, waiting for a reply from @justinmc |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
Sorry for missing this notification. Feel free to ping me on Discord if that happens again! I do want to get this PR merged.
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍
Sorry I went a little overboard trying to get rid of the shouldUpdateOptions boolean. Let's keep the behavior the same and merge this as-is.
victorsanni
added
the
autosubmit
…ust 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 919d205 ("Dismiss Autocomplete with ESC", flutter#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 7695582 (flutter#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.
6 participants
Added function and result identifiers into the RawAutocomplete onChangedField method, now the options value will not be updated if the data is from an optionsBuilder call older than the last call which updated the options
List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
Issue #150774 is fixed by this PR
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.