anchors 6/n: Make scroll-to-end work for split slivers #1486
+638
−156
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
And update Flutter's supporting libraries to match. In particular this pulls in a recent PR of mine so we can use it: flutter/flutter#166730
This isn't a "value"; it's a "value notifier", which has a value. We could call it "… notifier" instead; but the type seems adequate already for disambiguating that.
There's no "navigation" happening here -- the user remains on the same page of the app.
Fixes zulip#1485. This logic for deciding how long the scroll-to-bottom animation should take, introduced in 5abeb88 (zulip#223), didn't have any tests. As a result, when its unstated assumption about the message list -- that scrolling up from the end was represented by positive scroll positions -- was broken a few months later (in bdac26f), nothing alerted us to that. I did notice a few times over the past year or so that the effect of the scroll-to-bottom button seemed jerky, as if it were trying to move much farther in each frame than it should. Now I know why. (Discovered this in the course of revisiting this code in order to adapt it to the more radical change to the message list's scroll positions which is coming up: "zero" won't be the end, but somewhere in the middle.)
Fundamentally an easing curve like this relies on knowing in advance how far the animation is going to end up going. When we start letting the message list scroll from the middle of history, it'll no longer be possible to know that. Switch instead to a behavior that can be based on only what's happened so far, not on a prediction of the future. In the future, if we want, we could get fancy and make the speed change over time; but to start, keep it simple, and just move at the same speed from start to finish.
Nothing awaits this future anyway; this method is only called as a gesture callback. The callback is expected to return void, emphasizing that nothing will inspect its return value.
…class This makes a more comfortable home for extending this logic, because it naturally uses a lot of ScrollPosition members. To make the logic fit in better on this class, also loosen its assumptions slightly, allowing maxScrollExtent to be nonzero. It's still expected not to change, though -- fixing that is a more complex job, and will come over the remainder of this commit series.
…ew tests This will be convenient for testing scrollToEnd.
This version keeps the numbers in the form of doubles, with seconds as the unit of time, until the end. That's a bit more typical Flutter style, and also brings it closer to how the logic will look when we flip this around to produce a velocity instead of a duration.
This gets us hands-on control of the ScrollActivity being used, which we'll want to customize in order to get the behavior we want.
We'll use this to customize the behavior. It also makes a handy marker in tests, when using the testing-only [ScrollPosition.activity] getter for inspecting what's going on in the scroll behavior.
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 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.
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. By describing the animation in terms of velocity, rather than a duration and exact target position, this lets us smoothly handle the case where we may not know exactly what the position coordinate of the end will be. A previous commit handled the case where the end comes sooner than estimated, by promptly stopping when that happens. This commit ensures the scroll continues past the original estimate, in the case where the end comes later. 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 taller; 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. And naturally that will become even more salient of an issue when we enable the message list to jump into the middle of a long history, so that the bottom sliver may have content that hasn't yet been scrolled to, has never been built as widgets, and may not even have yet been fetched from the server.
This makes it possible to see in a self-contained way, in this class's own code, that it always starts moving at a velocity that isn't zero, or less than zero, or at risk of being conflated with zero. This doesn't have a big effect in practice, because the only call site already does something else whenever the distance to travel is negative or very close to zero. But there is a small range -- namely where the distance to travel is between 1 and 12 physical pixels, given the default behavior of ScrollPhysics.toleranceFor -- in which this minimum speed does apply.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1485.
This is the next round after #1436, toward #82. It fixes the "scroll to bottom" button in the message list so that it will work correctly when we start splitting the message list into back-to-back slivers.
Along the way it also fixes that button so that it works smoothly even in the current form of the message list — it fixes #1485, which I'd encountered occasionally as a user in the year or so since we apparently introduced it, and then discovered in the code in the course of writing these changes.
This work also produced two upstream PRs to the framework:
flutter/flutter#166730
flutter/flutter#166731
One of those is merged, and this PR uses it; the other is still pending, so there's a bit of code effectively copy-pasted from the framework in order to let us make the customization we need.
Selected commit messages
c32edc8 deps: Upgrade Flutter to 3.32.0-1.0.pre.257
And update Flutter's supporting libraries to match.
In particular this pulls in a recent PR of mine so we can use it:
flutter/flutter#166730
7309915 msglist: Fix speed calculations in scroll to bottom
Fixes #1485.
This logic for deciding how long the scroll-to-bottom animation
should take, introduced in 5abeb88 (#223), didn't have any tests.
As a result, when its unstated assumption about the message list --
that scrolling up from the end was represented by positive scroll
positions -- was broken a few months later (in bdac26f), nothing
alerted us to that.
I did notice a few times over the past year or so that the effect of
the scroll-to-bottom button seemed jerky, as if it were trying to
move much farther in each frame than it should. Now I know why.
(Discovered this in the course of revisiting this code in order to
adapt it to the more radical change to the message list's scroll
positions which is coming up: "zero" won't be the end, but somewhere
in the middle.)
3db7323 msglist: Drop easing curve in scroll-to-bottom
Fundamentally an easing curve like this relies on knowing in advance
how far the animation is going to end up going. When we start
letting the message list scroll from the middle of history, it'll no
longer be possible to know that.
Switch instead to a behavior that can be based on only what's happened
so far, not on a prediction of the future. In the future, if we want,
we could get fancy and make the speed change over time; but to start,
keep it simple, and just move at the same speed from start to finish.
6a4e418 scroll test: Test basic behavior of scroll-to-end feature
aa42c38 scroll [nfc]: Explain magic numbers in scroll-to-end calculations
a0de83f scroll [nfc]: Inline animateTo implementation into scrollToEnd
This gets us hands-on control of the ScrollActivity being used,
which we'll want to customize in order to get the behavior we want.
5df7411 test [nfc]: Organize Flutter checks-extensions by library
40abc88 test [nfc]: Organize Flutter checks-extensions thematically within each library
e738a08 scroll [nfc]: Introduce ScrollToEndActivity
We'll use this to customize the behavior.
It also makes a handy marker in tests, when using the testing-only
[ScrollPosition.activity] getter for inspecting what's going on in
the scroll behavior.
5233d6d scroll: Avoid overscroll in "scroll to end"
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.
f09976a scroll [nfc]: Introduce SimulationDrivenScrollActivity
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
animateTomethod).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
.simulationconstructor, ispending. A "TODO(upstream)" comment points to that PR, because
once it also merges we can dispense with this class.
ac21040 scroll: Drive "scroll to end" through uncertainty about endpoint
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.
By describing the animation in terms of velocity, rather than a
duration and exact target position, this lets us smoothly handle the
case where we may not know exactly what the position coordinate of the
end will be. A previous commit handled the case where the end comes
sooner than estimated, by promptly stopping when that happens. This
commit ensures the scroll continues past the original estimate, in the
case where the end comes later.
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 taller; 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.
And naturally that will become even more salient of an issue when we
enable the message list to jump into the middle of a long history, so
that the bottom sliver may have content that hasn't yet been scrolled
to, has never been built as widgets, and may not even have yet been
fetched from the server.