anchors 8/n: Let new messages accumulate in bottom sliver #1507
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
gnprice
force-pushed
the
pr-preserve-sliver-boundary
branch
from
e21f601 to
0811136
Compare
Otherwise, if several different test cases in this file fail due to checks failing inside checkInvariants, then only the first one gets reported in detail with the comparison and the stack trace.
This will allow the model to maintain it over time as newer messages arrive or get fetched.
This brings the behavior closer to what we'll want in the future, and so allows the next commit affecting `middleItem` to be NFC. This itself is practically NFC. It's entirely NFC except in the case where there are no messages, so that `items` consists only of a history-start marker; and in that case, it causes that marker to go in the top sliver rather than the bottom. I believe the only observable effect of that change is that if the viewport is so very short that the history-start marker is too tall to fit (so that it's possible to scroll when that's the only item), then the initial scroll position will be different.
This gives a bit more structured of an idea of what `middleItem` is supposed to mean. We'll use this for maintaining `middleItem` as a more dynamic value in upcoming commits.
This new logic maintains `middleItem` according to its documented relationship with `middleMessage`. Because of the current definition of `middleMessage`, that produces the same result as the previous definition of `middleItem`. The key reasoning for why this logic works is: this touches all the code that modifies `items`, to ensure that code keeps `middleItem` up to date. And all the code which modifies `messages` (which is the only way to modify `middleMessage`) already calls `_reprocessAll` to compute `items` from scratch, except one site in `_addMessage`. Studying `_addMessage`, it also maintains `middleItem` correctly, though for that conclusion one needs the specifics of the definition of `middleMessage`. This change involves no new test code: all this logic is in scenarios well exercised by existing tests, and the invariant-checks introduced in the previous commit then effectively test this logic. To be sure of that, I also confirmed that commenting out any one of these updates to `middleItem` causes some tests to fail.
This is NFC for the behavior at initial fetch. But thereafter, with this change, messages never move between slivers, and new messages go into the bottom sliver. I believe the main user-visible consequence of this change is that if the user is scrolled up in history and then a new message comes in, the new message will no longer cause all the messages to shift upward. This is the "90% solution" to zulip#83. On the other hand, if the user is scrolled all the way to the end, then they still remain that way when a new message comes in -- there's specific logic to ensure that in MessageListScrollPosition, and an existing test in test/widgets/message_list_test.dart verifies it end to end. The main motivation for this change is that it brings us closer to having a `fetchNewer` method, and therefore to being able to have the message list start out in the middle of history. This change also allows us to revert a portion of fca651b, where a test had had to be weakened slightly because messages began to get moved between slivers.
gnprice
force-pushed
the
pr-preserve-sliver-boundary
branch
from
0811136 to
5aafc03
Compare
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.
This is the next round after #1468.
After this PR, not only does the last message from the initial fetch go into the bottom sliver, but it remains there even while new messages arrive as events, and those new messages go into the bottom sliver below it.
The main motivation for this change is that it brings us closer to having a
fetchNewermethod, and therefore to being able to have the message list start out in the middle of history, #82. Along the way it also gives us the "90% solution" described in #83, so that new messages don't cause everything on the screen to jump up when you're already scrolled up in history.Selected commit messages
f4b6e41 msglist test: Ensure later errors get reported in full too
Otherwise, if several different test cases in this file fail due to
checks failing inside checkInvariants, then only the first one gets
reported in detail with the comparison and the stack trace.
48983a3 msglist [nfc]: Move sliver boundary into the view-model
This will allow the model to maintain it over time as newer messages
arrive or get fetched.
e21f601 msglist: Let new messages accumulate in bottom sliver
This is NFC for the behavior at initial fetch. But thereafter, with
this change, messages never move between slivers, and new messages
go into the bottom sliver.
I believe the main user-visible consequence of this change is that if
the user is scrolled up in history and then a new message comes in,
the new message will no longer cause all the messages to shift upward.
This is the "90% solution" to #83.
On the other hand, if the user is scrolled all the way to the end,
then they still remain that way when a new message comes in --
there's specific logic to ensure that in MessageListScrollPosition,
and an existing test in test/widgets/message_list_test.dart verifies
it end to end.
The main motivation for this change is that it brings us closer to
having a
fetchNewermethod, and therefore to being able to have themessage list start out in the middle of history.
This change also allows us to revert a portion of fca651b, where
a test had had to be weakened slightly because messages began to
get moved between slivers.