这是indexloc提供的服务,不要输入任何密码
Skip to content

Add ScrollController.onAttach & onDetach, samples/docs on listening/getting scrolling info #124823

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

Merged
merged 11 commits into from
May 15, 2023

Conversation

Piinks
Copy link
Contributor

@Piinks Piinks commented Apr 13, 2023

This PR does a couple of things!

Screen.Recording.2023-04-13.at.4.28.27.PM.mov

Screenshot 2023-04-13 at 3 24 28 PM

Fixes #20819
Fixes #41910
Fixes #121419

Adds ScrollController.onAttach and ScrollController.onDetach

This resolves a long held pain point for developers. When using a scroll controller, there is not scroll position until the scrollable widget is built, and almost all methods of notification are only triggered when scrolling happens. Adding these two methods will help developers gain access to the scroll position when it is created. A common workaround for this was using a post frame callback to access controller.position after the first frame, but this is ripe for issues such as having multiple positions attached to the controller, or the scrollable no longer existing after that post frame callback. I think this can also be helpful for folks to debug cases when the scroll controller has multiple positions attached.

In particular, this also resolves this commented case: #20819 (comment)
The isScrollingNotifier is hard for developers to access.

Docs & samples

I was surprised we did not have samples on scroll notification or scroll controller, so I overhauled it and added a lot of docs on all the different ways to access scrolling information, when it is available and how they differ.

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.

@Piinks Piinks added c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation labels Apr 13, 2023
@flutter-dashboard flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems c: contributor-productivity Team-specific productivity, code health, technical debt. labels Apr 13, 2023
@Piinks Piinks requested a review from thkim1011 April 13, 2023 22:34
@Piinks Piinks force-pushed the scrollControllerNotify branch from 74555f0 to 725879c Compare April 18, 2023 22:25
Copy link
Contributor

@thkim1011 thkim1011 left a comment

Choose a reason for hiding this comment

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

Left a couple of comments; this was my first time reading through the ScrollController code so I'm a bit confused about the implementation. What does it mean for a ScrollController to have multiple ScrollPosition's?

Comment on lines +43 to +45
// Returning false allows the notification to continue bubbling up to
// ancestor listeners. If we wanted the notification to stop bubbling,
// return true.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be done via passing the ScrollController to descendants and allowing any one of them to call ScrollController.addListener?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this method is not associated with the ScrollController. This is the part of the example that shows how to listen without a scroll controller. Using ScrollController.addListener does not allow the developer to affect the notification bubbling up the tree.

Comment on lines 46 to 49
_controller = ScrollController(
onAttach: _handlePositionAttach,
onDetach: _handlePositionDetach,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a comment here to clarify when _handlePositionAttach and _handlePositionDetach will be called might be helpful. I had to go back and look through the code to figure out what was happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I'm not too sure when ScrollController.attach gets called (which would then call _handlePositionAttach?) if at all in this demo. Perhaps this could be clarified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

attach is called by the Scrollable after the ScrollPosition was created. I'll add more docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code sample will be embedded with the docs in scroll_controller.dart. Those docs are meant to provide a lot of the context here rather than duplicating them. If some of this is unclear in the docs on the ScrollController class, we should improve them there rather than in the sample. Let me know if the docs in scroll_controller.dart are not helpful.

Comment on lines +117 to +129
final ScrollControllerCallback? onAttach;

/// Called when a [ScrollPosition] is detached from the scroll controller.
///
/// {@tool dartpad}
/// This sample shows how to apply a listener to the
/// [ScrollPosition.isScrollingNotifier] using [ScrollController.onAttach]
/// & [ScrollController.onDetach].
/// This is used to change the [AppBar]'s color when scrolling is occurring.
///
/// ** See code in examples/api/lib/widgets/scroll_position/scroll_controller_on_attach.0.dart **
/// {@end-tool}
final ScrollControllerCallback? onDetach;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it's worth it to call this onAttachPosition and onDetachPosition instead. I can see that we already call the method for attaching/detaching a position attach and detach, but when I was reading through the demo code without having read through the actual implementation, it wasn't very clear if onAttach happens on attaching a position or if it happens in a more general scenario of attaching anything to the ScrollController (which may happen to someone else who isn't completely familiar with ScrollController?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to match the convention of the named method in the class, attach/detach. I think reading the sample code before reading the docs is definitely difficult to follow, but when the docs are rendered, the sample comes after all of the info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, there is a @macro in scroll_controller.dart, this replicates the docs from where it is defined (@template) so all of the docs in scroll_position.dart in that template are also filled in at the scroll controller docs.

@Piinks
Copy link
Contributor Author

Piinks commented Apr 24, 2023

What does it mean for a ScrollController to have multiple ScrollPosition's?

Good question! The ScrollController is used to create the ScrollPosition in Scrollable. If Scrollable has not been provided a controller (via ScrollView), then it creates one in order to create the position.
The most common case of multiple positions attached to the ScrollController is during the transition of a TabView that scrolls. While the page animation is executing, the page on the way out has not been disposed yet (because we can still see it), and the incoming page is also visible, so during the transition (if both are using the same scroll controller), two positions are attached.

@Piinks Piinks requested a review from thkim1011 April 25, 2023 16:33
Copy link
Contributor

@thkim1011 thkim1011 left a comment

Choose a reason for hiding this comment

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

LGTM

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label May 15, 2023
@auto-submit auto-submit bot merged commit 27caa7f into flutter:master May 15, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
…etting scrolling info (flutter#124823)

This PR does a couple of things!

https://user-images.githubusercontent.com/16964204/231897483-416287f9-50ce-468d-a714-2a4bc0f2e011.mov

![Screenshot 2023-04-13 at 3 24 28 PM](https://user-images.githubusercontent.com/16964204/231897497-f5bee17d-43ed-46e5-acd7-e1bd64768274.png)

Fixes flutter#20819 
Fixes flutter#41910 
Fixes flutter#121419

### Adds ScrollController.onAttach and ScrollController.onDetach

This resolves a long held pain point for developers. When using a scroll controller, there is not scroll position until the scrollable widget is built, and almost all methods of notification are only triggered when scrolling happens. Adding these two methods will help developers gain access to the scroll position when it is created. A common workaround for this was using a post frame callback to access controller.position after the first frame, but this is ripe for issues such as having multiple positions attached to the controller, or the scrollable no longer existing after that post frame callback. I think this can also be helpful for folks to debug cases when the scroll controller has multiple positions attached.

In particular, this also resolves this commented case: flutter#20819 (comment)
The isScrollingNotifier is hard for developers to access.

### Docs & samples

I was surprised we did not have samples on scroll notification or scroll controller, so I overhauled it and added a lot of docs on all the different ways to access scrolling information, when it is available and how they differ.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App c: contributor-productivity Team-specific productivity, code health, technical debt. c: new feature Nothing broken; request for a new capability d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
2 participants