anchors 5/n: Control scroll position #1436
Conversation
|
Hooray for tests 🙂 — a pair of failed tests in CI:
show that the scroll-to-bottom button doesn't work properly after the main turn-the-switch-on commit: I noticed the second failure during normal use of the app today, having used Both of those will be straightforward to fix for this stage of things, where the bottom sliver is short and we start out basically scrolled to the end of it. The second one (the button's functionality) gets somewhat more complex in a future with #82 where we might be in the middle of a long history; but we'll figure that out when we come to it. There's one other failed test, also at that commit:
testWidgets('animation state persistence', (tester) async {
// Check that _UnreadMarker maintains its in-progress animation
// as the number of items changes in MessageList. See
// `findChildIndexCallback` passed into [SliverStickyHeaderList]
// at [_MessageListState._buildListView].I believe that test failure is saying that if the latest message was unread, and you mark it read; and then a moment later, while the animation for the unread marker to fade away is still going, a new message arrives; then because that pushes the existing message across the boundary from the lower sliver to the upper sliver, the animation is interrupted and the unread marker abruptly finishes vanishing. That's an issue that actually only applies at this stage, and won't apply when #82 is complete: when that's done, new messages will get added to the end of the lower sliver, without displacing messages from there into the upper sliver. I think this is a pretty tolerable glitch. For this PR, I might fix it by just having the target message in the test be the next-to-last message, so it's in the upper sliver from the beginning and doesn't trigger the issue. After those other changes for #82, I guess the whole situation this is testing will become less common, because new messages won't trigger it at all: it'll only apply in less-common situations like a message getting deleted, or moved into or out of the narrow. To keep the test effective, we'll want to switch it to exercise one of those. Anyway, so the last 2 commits aren't quite ready, even apart from the need to write tests. I think the 8 commits before them are, though (again apart from the need to write tests for some of them), in addition to the 6 commits that are in #1435. |
…bottom This `i == 1` condition was never true, because in that situation the caller would build a MarkAsReadWidget instead of calling this method. This 8px vs. 11px distinction dates back to the prototype: 731b199 made it 8px instead of 0px, and the distinction itself goes back to the commit 9916194 msglist: Start on rendering messages in the prototype's first hours. The logic that drove it, though, became fragile with e7fe06c which changed it from "i == 0" (the end of the list, OK that's fairly canonical as a special value) to "i == 1" (more arbitrary). So then it naturally got broken a little later, in 56ab395 in 2023-11, and it's been broken ever since: we just always show 11px of padding here. We might further change the layout in the future, but if we do we'll fix it forward starting from the behavior the app has already had for over a year.
…ream Some of the behavior we'd like to customize isn't currently cleanly exposed to subclasses any more than it is to parent widgets passing constructor arguments. In particular, we'll want to change a few bits of logic in [RenderViewport.performLayout], replacing the handling of the `anchor` field with something more flexible. In order to do that, we'll start from a copy of that method, so that we can edit the copy. Then the base class's `performLayout` refers to a private helper method `_attemptLayout`, so we need a copy of that too; and they each refer to a number of private fields, so we need copies of those too; and to make those work correctly, we need copies of all the other members that refer to those fields, so that they're all referring correctly to the same version of those fields (namely the one on the subclass) rather than to a mix of the versions on the base class and those on the subclass. Fortunately, flood-filling that graph of members which refer to private members, which are referred to by other members, etc., terminates with a connected component which is... not small, but a lot smaller and less unwieldy than if we had to copy the whole upstream file these are defined in.
We'll rely on this assumption to simplify some upcoming customizations.
gnprice
force-pushed
the
pr-scroll-position
branch
2 times, most recently
from
d54c65d to
5c98640
Compare
This is NFC as to the real message list, because so far the bottom sliver there always has height 0, so that maxScrollExtent is always 0. This is a step toward letting us move part of the message list into the bottom sliver, because it means that doing so would preserve the list's current behavior of starting out scrolled to the end.
This is NFC as to the real message list, because so far the bottom sliver there always has height 0. Before this change, the user could always scroll up (moving the content down) so that the bottom sliver was entirely off the bottom of the viewport, even if that exposed blank space at the top of the viewport because the top sliver was shorter than the viewport. After this change, it's never in bounds to have part of the viewport be blank for lack of content while there's content scrolled out of the viewport at the other end. This is a step toward letting us move part of the message list into the bottom sliver, because it fixes a bug that would otherwise create in the case where the top sliver fits entirely on the screen.
This is NFC as to the real message list, because so far the bottom sliver there always has height 0, so that both maxScrollExtent and this.maxScrollExtent are always 0. This is a step toward letting us move part of the message list into the bottom sliver, because it means that doing so would preserve the list's current behavior of remaining scrolled to the end once there as e.g. new messages arrive.
gnprice
force-pushed
the
pr-scroll-position
branch
from
5c98640 to
6e03b9e
Compare
|
With today's revision, I believe all the commits are ready except for the one that enables the new scroll behavior for the actual message list: In particular all the other commits, including the various behavior of MessageListScrollView, are now tested, and they've also had some other cleanups. That main commit needs fixes for the issues mentioned in the previous comment, and should probably also get a test or two (though the detailed tests will be those that are on MessageListScrollView itself). |
gnprice
force-pushed
the
pr-scroll-position
branch
from
6e03b9e to
0f7bc7a
Compare
gnprice
changed the title
This is the next round after #1435
(and is stacked atop it), toward #82.In this PR, we take more control of how the scroll position works in the presence of back-to-back slivers. In particular the ScrollView.anchor property doesn't really have enough flexibility to express the behavior we need, so we replace the logic that consumes it.
Once that's done, we're very close to being able to start splitting the real message list into back-to-back slivers. That will come in the next PR, #1468.
Selected commit messages
8702ad3 msglist [nfc]: Introduce MessageListScrollView, not yet doing anything different
92676fd scroll [nfc]: Copy RenderViewport.performLayout and friends from upstream
Some of the behavior we'd like to customize isn't currently cleanly
exposed to subclasses any more than it is to parent widgets passing
constructor arguments. In particular, we'll want to change a few
bits of logic in [RenderViewport.performLayout], replacing the
handling of the
anchorfield with something more flexible.In order to do that, we'll start from a copy of that method, so that
we can edit the copy.
Then the base class's
performLayoutrefers to a private helpermethod
_attemptLayout, so we need a copy of that too; and theyeach refer to a number of private fields, so we need copies of
those too; and to make those work correctly, we need copies of all
the other members that refer to those fields, so that they're all
referring correctly to the same version of those fields (namely
the one on the subclass) rather than to a mix of the versions on
the base class and those on the subclass.
Fortunately, flood-filling that graph of members which refer to
private members, which are referred to by other members, etc.,
terminates with a connected component which is... not small, but a
lot smaller and less unwieldy than if we had to copy the whole
upstream file these are defined in.
96b81dc msglist [nfc]: Introduce MessageListScrollPosition
1667c6e scroll: Start out scrolled to bottom of list
This is NFC as to the real message list, because so far the bottom
sliver there always has height 0, so that maxScrollExtent is always 0.
This is a step toward letting us move part of the message list into
the bottom sliver, because it means that doing so would preserve the
list's current behavior of starting out scrolled to the end.
07a3905 scroll: Keep short list pinned at bottom of viewport, not past bottom
This is NFC as to the real message list, because so far the bottom
sliver there always has height 0.
Before this change, the user could always scroll up (moving the
content down) so that the bottom sliver was entirely off the bottom
of the viewport, even if that exposed blank space at the top of the
viewport because the top sliver was shorter than the viewport.
After this change, it's never in bounds to have part of the viewport
be blank for lack of content while there's content scrolled out of
the viewport at the other end.
This is a step toward letting us move part of the message list into
the bottom sliver, because it fixes a bug that would otherwise create
in the case where the top sliver fits entirely on the screen.
0f7bc7a scroll: Stay at end once there
This is NFC as to the real message list, because so far the bottom
sliver there always has height 0, so that both maxScrollExtent and
this.maxScrollExtent are always 0.
This is a step toward letting us move part of the message list into
the bottom sliver, because it means that doing so would preserve the
list's current behavior of remaining scrolled to the end once there
as e.g. new messages arrive.