-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Let DrivenScrollActivity subclasses customize handling of overscroll #166731
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
Conversation
This functionality is useful in a custom DrivenScrollActivity subclass, just like it's useful in a BallisticScrollActivity subclass. For example, if one calls `animateTo(0, …)` to scroll to the beginning of a list, there will be no overscroll (provided the chosen curve doesn't go beyond its endpoint). The same is true if one calls `animateTo(position.maxScrollExtent, …)` to scroll to the end... as long as the value of maxScrollExtent at the beginning remains accurate when the scroll view reaches the end. If one wants to scroll to the end and avoid overscroll even when that turns out to be closer than the initial estimate found in maxScrollExtent, then that calls for customizing the overscroll behavior of a DrivenScrollActivity. Fortunately we don't need to invent any new API in order to do this: BallisticScrollActivity already has an API for the exact same need. So copy that API -- name, docs, and all -- to keep these two classes aligned. (It might also be useful to take much of what these have in common and factor that out to a common subclass; but that'd be for another PR.)
Concretely, I'm using this functionality in combination with what's added in #166730. (Currently doing so downstream using basically a copy-paste of DrivenScrollActivity, with these changes applied.) |
|
Side comment: I ran into a similar situation recently and had to animate over jumpTo to disable the overscroll. (see the example in #159809) So this PR would be useful for that. |
victorsanni
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 pending review from someone with more context.
|
@yakagami Yeah, agreed — this would be just as effective for the use case you described there as it is for the use case I have. Thanks for the additional example! @Piinks Requested your review because @victorsanni wanted someone with more context to take a look :-) |
In order to implement the "scroll to bottom" button in a way that behaves well when scrolling to the growth end of a sliver -- in particular, when scrolling to the end of the message list after we split it into back-to-back slivers -- we'll want some differences from the behavior provided by DrivenScrollActivity, which we've used up until now (originally via the `animateTo` method). That calls for our own ScrollActivity subclass, ScrollToEndActivity. We'll want most of the same behavior as DrivenScrollActivity, with just a couple of changes; but one of the places we want to change isn't among the places that DrivenScrollActivity exposes for subclassing. So add this class, based on DrivenScrollActivity but with a customization point added in the additional spot we'll need. Originally there were two additional customization points needed. After first drafting this change, I sent those upstream as two PRs: flutter/flutter#166730 flutter/flutter#166731 The first one has already landed in DrivenScrollActivity: the applyMoveTo method now overridden by ScrollToEndActivity in a previous commit. The other one, a `.simulation` constructor, is pending. A "TODO(upstream)" comment points to that PR, because once it also merges we can dispense with this class.
In order to implement the "scroll to bottom" button in a way that behaves well when scrolling to the growth end of a sliver -- in particular, when scrolling to the end of the message list after we split it into back-to-back slivers -- we'll want some differences from the behavior provided by DrivenScrollActivity, which we've used up until now (originally via the `animateTo` method). That calls for our own ScrollActivity subclass, ScrollToEndActivity. We'll want most of the same behavior as DrivenScrollActivity, with just a couple of changes; but one of the places we want to change isn't among the places that DrivenScrollActivity exposes for subclassing. So add this class, based on DrivenScrollActivity but with a customization point added in the additional spot we'll need. Originally there were two additional customization points needed. After first drafting this change, I sent those upstream as two PRs: flutter/flutter#166730 flutter/flutter#166731 The first one has already landed in DrivenScrollActivity: the applyMoveTo method now overridden by ScrollToEndActivity in a previous commit. The other one, a `.simulation` constructor, is pending. A "TODO(upstream)" comment points to that PR, because once it also merges we can dispense with this class.
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.
Thanks for your patience while I caught up on my backlog. 🙏
This LGTM, nice consistency with BallisticScrollActivity.
And update Flutter's supporting libraries to match. In particular this pulls in this recent PR of mine so we can use it: flutter/flutter#166731 This also causes all the generated l10n files to get reformatted. That's due to this upstream PR: flutter/flutter#167029
As long as the bottom sliver is size zero (or more generally, as long as maxScrollExtent does not change during the animation), this is nearly NFC: I believe the only changes in behavior would come from differences in rounding. This change handles the case where the end turns out to be closer than it looked at the beginning of the animation. Before this change, the animation would try to scroll past the end in that case. Now it stops at exactly the end -- just like it already did in the case where the end was known exactly in advance, as it currently always is in the actual message list. That case is a possibility as soon as there's a bottom sliver with a message in it: scroll up so the message is offscreen and no longer built; then have the message edited so it becomes shorter; then scroll back down. It's impossible for the viewport to know that the bottom sliver's content has gotten taller until we actually scroll back down and cause the message's widget to get built. In order to customize this behavior, this change uses a feature I added recently (for this purpose) to DrivenScrollActivity upstream: flutter/flutter#166731
And update Flutter's supporting libraries to match. In particular this pulls in this recent PR of mine so we can use it: flutter/flutter#166731 This also causes all the generated l10n files to get reformatted. That's due to this upstream PR: flutter/flutter#167029
As long as the bottom sliver is size zero (or more generally, as long as maxScrollExtent does not change during the animation), this is nearly NFC: I believe the only changes in behavior would come from differences in rounding. This change handles the case where the end turns out to be closer than it looked at the beginning of the animation. Before this change, the animation would try to scroll past the end in that case. Now it stops at exactly the end -- just like it already did in the case where the end was known exactly in advance, as it currently always is in the actual message list. That case is a possibility as soon as there's a bottom sliver with a message in it: scroll up so the message is offscreen and no longer built; then have the message edited so it becomes shorter; then scroll back down. It's impossible for the viewport to know that the bottom sliver's content has gotten taller until we actually scroll back down and cause the message's widget to get built. In order to customize this behavior, this change uses a feature I added recently (for this purpose) to DrivenScrollActivity upstream: flutter/flutter#166731
…lutter#166731) This functionality is useful in a custom DrivenScrollActivity subclass, just like it's useful in a BallisticScrollActivity subclass. For example, if one calls `animateTo(0, …)` to scroll to the beginning of a list, there will be no overscroll (provided the chosen curve doesn't go beyond its endpoint). The same is true if one calls `animateTo(position.maxScrollExtent, …)` to scroll to the end... as long as the value of maxScrollExtent at the beginning remains accurate when the scroll view reaches the end. If one wants to scroll to the end and avoid overscroll even when that turns out to be closer than the initial estimate found in maxScrollExtent, then that calls for customizing the overscroll behavior of a DrivenScrollActivity. Fortunately we don't need to invent any new API in order to do this: BallisticScrollActivity already has an API for the exact same need. So copy that API -- name, docs, and all -- to keep these two classes aligned. (It might also be useful to take much of what these have in common and factor that out to a common subclass; but that'd be for another PR.) ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
This functionality is useful in a custom DrivenScrollActivity subclass, just like it's useful in a BallisticScrollActivity subclass.
For example, if one calls
animateTo(0, …)to scroll to the beginning of a list, there will be no overscroll (provided the chosen curve doesn't go beyond its endpoint). The same is true if one callsanimateTo(position.maxScrollExtent, …)to scroll to the end... as long as the value of maxScrollExtent at the beginning remains accurate when the scroll view reaches the end. If one wants to scroll to the end and avoid overscroll even when that turns out to be closer than the initial estimate found in maxScrollExtent, then that calls for customizing the overscroll behavior of a DrivenScrollActivity.Fortunately we don't need to invent any new API in order to do this: BallisticScrollActivity already has an API for the exact same need. So copy that API -- name, docs, and all -- to keep these two classes aligned.
(It might also be useful to take much of what these have in common and factor that out to a common subclass; but that'd be for another PR.)
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.