Skip to content

Conversation

@gnprice
Copy link
Member

@gnprice gnprice commented Apr 8, 2025

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

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

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.)
@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Apr 8, 2025
@gnprice
Copy link
Member Author

gnprice commented Apr 8, 2025

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

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.)

@yakagami
Copy link
Contributor

yakagami commented Apr 8, 2025

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.

@justinmc justinmc requested a review from victorsanni April 8, 2025 22:12
Copy link
Contributor

@victorsanni victorsanni left a 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.

@gnprice gnprice requested a review from Piinks April 18, 2025 20:59
@gnprice
Copy link
Member Author

gnprice commented Apr 21, 2025

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

gnprice added a commit to gnprice/zulip-flutter that referenced this pull request Apr 25, 2025
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.
gnprice added a commit to gnprice/zulip-flutter that referenced this pull request Apr 28, 2025
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.
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.

Thanks for your patience while I caught up on my backlog. 🙏
This LGTM, nice consistency with BallisticScrollActivity.

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 30, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Apr 30, 2025
Merged via the queue into flutter:master with commit 47dd763 Apr 30, 2025
75 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 30, 2025
@gnprice gnprice deleted the pr-driven-overscroll branch April 30, 2025 05:51
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 30, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 30, 2025
gnprice added a commit to gnprice/zulip-flutter that referenced this pull request Apr 30, 2025
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
gnprice added a commit to gnprice/zulip-flutter that referenced this pull request Apr 30, 2025
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
chrisbobbe pushed a commit to zulip/zulip-flutter that referenced this pull request May 1, 2025
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
chrisbobbe pushed a commit to zulip/zulip-flutter that referenced this pull request May 1, 2025
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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 1, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
romanejaquez pushed a commit to romanejaquez/flutter that referenced this pull request Aug 14, 2025
…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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

4 participants