Skip to content

Document on ScrollPhysics the requirement to override applyTo #121850

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 3 commits into from
Mar 10, 2023
Merged

Document on ScrollPhysics the requirement to override applyTo #121850

auto-submit merged 3 commits into from
Mar 10, 2023

Conversation

@gnprice
Copy link
Member

@gnprice gnprice commented Mar 3, 2023

I missed this when first experimenting with a custom ScrollPhysics subclass a few weeks ago, which led me into about a half-hour of very confused debugging into the framework as I tried to figure out why my code wasn't running.

This requirement is already stated on applyTo itself, but one won't necessarily see that unless there's already a reason to pay attention to applyTo. So it seems best to mention it on the documentation of the class itself.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Mar 3, 2023
@HansMuller HansMuller requested a review from Piinks March 3, 2023 22:47
Piinks
Piinks reviewed Mar 8, 2023
View reviewed changes
Piinks
Piinks approved these changes Mar 8, 2023
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment •
edited
Loading

Choose a reason for hiding this comment

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

TIL! LGTM, thank you!

This feels like a foot gun though.
I wonder if we could instead make this more like ScrollBehavior, which has a unique copyWith that creates a _WrappedScrollBehavior to preserve custom subclasses.

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 8, 2023
gnprice
gnprice commented Mar 8, 2023
View reviewed changes
@gnprice
Copy link
Member Author

gnprice commented Mar 8, 2023

Thanks!

This feels like a foot gun though.
I wonder if we could instead make this more like ScrollBehavior, which has a unique copyWith that creates a _WrappedScrollBehavior to preserve custom subclasses.

Agreed. I haven't thought about what that could look like (or how to avoid or mitigate breakage from an API change), but something like that would be welcome.

gnprice and others added 3 commits March 9, 2023 15:58
@gnprice
Document on ScrollPhysics the requirement to override applyTo
ae75273 
I missed this when first experimenting with a custom ScrollPhysics
subclass a few weeks ago, which led me into about a half-hour of
very confused debugging into the framework as I tried to figure out
why my code wasn't running.

This requirement is already stated on `applyTo` itself, but one
won't necessarily see that unless there's already a reason to
pay attention to `applyTo`.  So it seems best to mention it on
the documentation of the class itself.
@Piinks @gnprice
Update packages/flutter/lib/src/widgets/scroll_physics.dart
30ed2b0
@gnprice
wrap line
97d53d7
@gnprice gnprice force-pushed the pr-scrollphysics-applyto-doc branch from 7b00393 to 97d53d7 Compare March 9, 2023 23:59
@auto-submit auto-submit bot merged commit 4792297 into flutter:master Mar 10, 2023
@gnprice gnprice deleted the pr-scrollphysics-applyto-doc branch March 10, 2023 01:54
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 10, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 10, 2023
@engine-flutter-autoroll
4792297 Document on ScrollPhysics the requirement to override applyTo (…
995e924 
…flutter/flutter#121850)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 10, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 10, 2023
@engine-flutter-autoroll
4792297 Document on ScrollPhysics the requirement to override applyTo (…
d075c86 
…flutter/flutter#121850)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 10, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 10, 2023
@engine-flutter-autoroll
4792297 Document on ScrollPhysics the requirement to override applyTo (…
56d70af 
…flutter/flutter#121850)
hannah-hyj pushed a commit to hannah-hyj/flutter that referenced this pull request Mar 11, 2023
@gnprice @hannah-hyj
Document on ScrollPhysics the requirement to override applyTo (flutte…
884ac77 
…r#121850)

Document on ScrollPhysics the requirement to override applyTo
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 11, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 11, 2023
@engine-flutter-autoroll
4792297 Document on ScrollPhysics the requirement to override applyTo (…
e18dea2 
…flutter/flutter#121850)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 12, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 12, 2023
@engine-flutter-autoroll
4792297 Document on ScrollPhysics the requirement to override applyTo (…
eb4f735 
…flutter/flutter#121850)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
4792297 Document on ScrollPhysics the requirement to override applyTo (…
f0b262b 
…flutter/flutter#121850)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
@engine-flutter-autoroll
4792297 Document on ScrollPhysics the requirement to override applyTo (…
938de17 
…flutter/flutter#121850)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Piinks Piinks Piinks approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gnprice @Piinks