Skip to content

Conversation

@chinmoy12c
Copy link
Member

Fixes: #130793

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Jul 23, 2023
@chinmoy12c chinmoy12c force-pushed the fix/update_drag_devices_on_state_change branch from ae68cd6 to 76899bc Compare July 24, 2023 05:30

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;
}

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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!

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @chunhtai thanks for pointing that out, I've added another test case to check for that behavior.
@Piinks its ready for another review :).

@goderbauer goderbauer requested a review from Piinks July 25, 2023 22:15
Piinks
Piinks previously approved these changes Jul 26, 2023
Copy link
Contributor

@Piinks Piinks left a 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!

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 26, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 26, 2023

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.

  • Merge guidelines: You need at least one approved review if you are already part of flutter-hackers or two member reviews if you are not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@Piinks
Copy link
Contributor

Piinks commented Jul 26, 2023

Oh whoops! Let me find another reviewer here. :)

justinmc
justinmc previously approved these changes Jul 26, 2023
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 . Thanks!

@chinmoy12c chinmoy12c force-pushed the fix/update_drag_devices_on_state_change branch from 76899bc to fcaa7b0 Compare July 27, 2023 05:09
@Renzo-Olivares
Copy link
Contributor

Is this ready to be merged?

@Piinks Piinks dismissed stale reviews from justinmc and themself August 14, 2023 17:42

Pending changes

@chinmoy12c chinmoy12c force-pushed the fix/update_drag_devices_on_state_change branch from fcaa7b0 to 29a5b06 Compare August 16, 2023 20:03
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flutter_LGTM
Thank you!

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 17, 2023
@auto-submit auto-submit bot merged commit f1d04a4 into flutter:master Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scrollable not changing ScrollBehaviour

6 participants