-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix SearchAnchor triggers unnecessary suggestions builder calls
#143443
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
Fix SearchAnchor triggers unnecessary suggestions builder calls
#143443
Conversation
|
cc: @sun-jiao |
d1cb932 to
3a8cbde
Compare
|
cc: @QuncCccccc |
3a8cbde to
451f182
Compare
QuncCccccc
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 the fix! LGTM! Sorry for the late response.
451f182 to
8fc89e3
Compare
|
It seems the |
Thanks! I'll take a closer look in the morning |
|
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.
IMO this should use a Timer. And the pending timer should be disposed if State.dispose is called while the timer is open
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.
unawaited was there so i can try work around it.
I applied your suggestion to use Timer and it does pass flutter_hooks test locally.
Can you please explain why Timer is better? unawaited is fairly new.
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.
Future internally creates a Timer. But that Timer is not disposed until the callback executes.
So this creates a race condition between the test assertion about "non-disposed Timers" and the callback execution.
If the Timer assert runs before the callback is executed, Future's Timer has yet to be cleaned-up, making the Timer assert fail.
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 appreciate your suggestion and explanation, it helped a lot.
cbb0a0f to
5dac0ff
Compare
|
With Remi's suggestion, all tests are green now. |
QuncCccccc
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:) Thanks for the fix!
5dac0ff to
24964b2
Compare
fixes
SearchAnchortriggers extra search operationsCode sample
expand to view the code sample
Before
before.mp4
After
after.mp4
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.