-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Update Scrollable on ScrollBehaviour change.
#131164
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
Update Scrollable on ScrollBehaviour change.
#131164
Conversation
ae68cd6 to
76899bc
Compare
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.
Maybe add condition
if((widget.scrollBehavior != null) != (oldWidget.scrollBehavior != null)) {
return true;
}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 in such a case a call to _updatePosition is made either way, so it's handled.
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 am a bit confused, if oldWidget.scrollBehavior == null but widget.scrollBehavior != null, which code path will call _updatePosition?
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.
Hey @chunhtai in this case _updatePosition is called from didChangeDependencies.
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.
Which dependency triggers didChangeDependencies? The scrollBehavior is a pass-in parameter and not a dependency. If you merely changing the pass-in parameter, it won't trigger didChangeDependencies.
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.
@chinmoy12c can you take a look at this? Thanks!
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.
Hey @chunhtai @Piinks sorry for the late reply, I've been away for a while.
@chunhtai you are right, the code does not trigger didChangeDependencies, my bad. The actual code that triggers the update is this:
ScrollPhysics? newPhysics = widget.physics ?? widget.scrollBehavior?.getScrollPhysics(context);
ScrollPhysics? oldPhysics = oldWidget.physics ?? oldWidget.scrollBehavior?.getScrollPhysics(context);
In this case either the newPhysics or the oldPhysics is null, hence
if (newPhysics?.runtimeType != oldPhysics?.runtimeType) {
return true;
}
returns true.
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 won't hit this case if both widget.physics and oldWidget.physics are not null even if
oldWidget.scrollBehavior == null and widget.scrollBehavior != null
If it fails to call updatePosition in this case, the _configuration will not be update with the newest widget.scrollBehavior.
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've removed the approvals while we sort this out. @chinmoy12c maybe we can add a test case for the case @chunhtai mentioned above here to better figure out this fix? :)
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.
Piinks
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! Thank you for fixing this!
|
auto label is removed for flutter/flutter, pr: 131164, due to This PR has not met approval requirements for merging. You are not a member of flutter-hackers and need 1 more review(s) in order to merge this PR.
|
|
Oh whoops! Let me find another reviewer here. :) |
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 👍 . Thanks!
76899bc to
fcaa7b0
Compare
|
Is this ready to be merged? |
fcaa7b0 to
29a5b06
Compare
chunhtai
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
Piinks
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.

Fixes: #130793
Pre-launch Checklist
///).