Skip to content

anchors 7/n: Start splitting slivers! #1468

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
May 1, 2025
Merged

anchors 7/n: Start splitting slivers! #1468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c6e4194
test [nfc]: Use `store.addMessage` shortcut everywhere
gnprice Apr 28, 2025
9070749
msglist test [nfc]: Shorten "controller" names in some tests
gnprice Apr 28, 2025
5653816
test [nfc]: Add checks-extensions for ScrollMetrics, ScrollPosition
gnprice May 1, 2025
231b054
msglist test: Generalize scroll-to-bottom tests to not assume 0 is end
gnprice Apr 28, 2025
e5983aa
msglist: Simplify computing semanticChildCount
gnprice Apr 29, 2025
271c55e
msglist [nfc]: Use names to clarify list-child index vs model-item index
gnprice May 1, 2025
fca651b
msglist: Split into back-to-back slivers
gnprice Jan 18, 2024
52d6909
scroll: Show start of latest message if long, instead of end
gnprice Mar 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 66 additions & 20 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,11 +580,20 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
}

Widget _buildListView(BuildContext context) {
final length = model!.items.length;
const centerSliverKey = ValueKey('center sliver');
final zulipLocalizations = ZulipLocalizations.of(context);

Widget sliver = SliverStickyHeaderList(
// The list has two slivers: a top sliver growing upward,
// and a bottom sliver growing downward.
// Each sliver has some of the items from `model!.items`.
const maxBottomItems = 1;
final totalItems = model!.items.length;
final bottomItems = totalItems <= maxBottomItems ? totalItems : maxBottomItems;
final topItems = totalItems - bottomItems;

// The top sliver has its child 0 as the item just before the
// sliver boundary, child 1 as the item before that, and so on.
final topSliver = SliverStickyHeaderList(
headerPlacement: HeaderPlacement.scrollingStart,
delegate: SliverChildBuilderDelegate(
// To preserve state across rebuilds for individual [MessageItem]
Expand All @@ -603,29 +612,70 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
// have state that needs to be preserved have not been given keys
// and will not trigger this callback.
findChildIndexCallback: (Key key) {
final valueKey = key as ValueKey<int>;
final index = model!.findItemWithMessageId(valueKey.value);
if (index == -1) return null;
return length - 1 - (index - 3);
final messageId = (key as ValueKey<int>).value;
final itemIndex = model!.findItemWithMessageId(messageId);
if (itemIndex == -1) return null;
final childIndex = totalItems - 1 - (itemIndex + bottomItems);
if (childIndex < 0) return null;
return childIndex;
},
childCount: length + 3,
(context, i) {
childCount: topItems,
(context, childIndex) {
final itemIndex = totalItems - 1 - (childIndex + bottomItems);
final data = model!.items[itemIndex];
final item = _buildItem(zulipLocalizations, data);
return item;
}));

// The bottom sliver has its child 0 as the item just after the
// sliver boundary (just after child 0 of the top sliver),
// its child 1 as the next item after that, and so on.
Widget bottomSliver = SliverStickyHeaderList(
key: centerSliverKey,
headerPlacement: HeaderPlacement.scrollingStart,
delegate: SliverChildBuilderDelegate(
// To preserve state across rebuilds for individual [MessageItem]
// widgets as the size of [MessageListView.items] changes we need
// to match old widgets by their key to their new position in
// the list.
//
// The keys are of type [ValueKey] with a value of [Message.id]
// and here we use a O(log n) binary search method. This could
// be improved but for now it only triggers for materialized
// widgets. As a simple test, flinging through All Messages in
// CZO on a Pixel 5, this only runs about 10 times per rebuild
// and the timing for each call is <100 microseconds.
//
// Non-message items (e.g., start and end markers) that do not
// have state that needs to be preserved have not been given keys
// and will not trigger this callback.
findChildIndexCallback: (Key key) {
final messageId = (key as ValueKey<int>).value;
final itemIndex = model!.findItemWithMessageId(messageId);
if (itemIndex == -1) return null;
final childIndex = itemIndex - topItems;
if (childIndex < 0) return null;
return childIndex;
},
childCount: bottomItems + 3,
(context, childIndex) {
// To reinforce that the end of the feed has been reached:
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603
if (i == 0) return const SizedBox(height: 36);
if (childIndex == bottomItems + 2) return const SizedBox(height: 36);

if (i == 1) return MarkAsReadWidget(narrow: widget.narrow);
if (childIndex == bottomItems + 1) return MarkAsReadWidget(narrow: widget.narrow);

if (i == 2) return TypingStatusWidget(narrow: widget.narrow);
if (childIndex == bottomItems) return TypingStatusWidget(narrow: widget.narrow);

final data = model!.items[length - 1 - (i - 3)];
final itemIndex = topItems + childIndex;
final data = model!.items[itemIndex];
return _buildItem(zulipLocalizations, data);
}));

if (!ComposeBox.hasComposeBox(widget.narrow)) {
// TODO(#311) If we have a bottom nav, it will pad the bottom inset,
// and this can be removed; also remove mention in MessageList dartdoc
sliver = SliverSafeArea(sliver: sliver);
bottomSliver = SliverSafeArea(key: bottomSliver.key, sliver: bottomSliver);
}

return MessageListScrollView(
Expand All @@ -641,17 +691,13 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
},

controller: scrollController,
semanticChildCount: length + 2,
semanticChildCount: totalItems, // TODO(#537): what's the right value for this?
center: centerSliverKey,
paintOrder: SliverPaintOrder.firstIsTop,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

      semanticChildCount: length + 2,

This seems like something that risks getting out-of-date. While we do have tests ("fetch older messages on scroll") for this, it might be helpful to note where the + 2 comes from.

Since nothing breaks in this PR, I assume that the sliver changes do not affect it, but that's otherwise hard to confirm without running the tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, good question.

I think in fact this already has gotten out of date. Using git log -L to see history of this method, it looks like 5c70c76 probably should have bumped this to match the change to childCount, making it length + 3.

These got decoupled in 6a8cf5c — previously, with just one sliver, we were using StickyHeaderListView.builder (which is a lot like ListView.builder) and it took just one argument itemCount instead of having semanticChildCount here and separately childCount elsewhere. Before that commit, I think the connection was fairly clear because the itemCount: length + 2 was just a few lines above the two if (i == 0) and if (i == 1) lines.

The original two bumps were in e7fe06c and then 56ab395, corresponding to the mark-as-read button and then the spacer.

OTOH the spacer doesn't seem very semantic, so it probably shouldn't have counted in the first place. And the other two (the mark-as-read button and the typing-status line) are often hidden, so probably shouldn't count either when that's the case.

I'll spend a few minutes trying to work out what the right value should actually be here. Then I'll try to do that, and also leave behind some restructuring and/or comments to help them stay in sync.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we do have tests ("fetch older messages on scroll") for this

Yeah, those tests exercise this (because they query semanticChildCount) but they don't really effectively test it.

That's no fault of those tests, because they weren't meant to test it: as the name says, they're about checking that we fetch older messages on scrolling up, and checking we don't do so when we shouldn't.

One piece of evidence that these tests don't test this line is that 5c70c76 didn't update it.

Another is a thought experiment: if you make a change that causes those checks to fail because of something it does that's specific to semanticChildCount, how would you tell whether that's an intended change and the tests just need updating? It'll probably just look like noise that needs to be updated, because the tests really don't tell a story about this handful of extra children. When they check that e.g. first there are 303 children, then 403 children, the point is that there are 300-plus-a-few and later 400-plus-a-few, indicating that the original 300 messages and later those plus the additional 100 messages are shown.

So for example when 56ab395 did update this line (and probably shouldn't have), it dutifully updated those tests.

I think this makes a good example of how in order to effectively test some logic it's not enough to exercise it: the test needs to tell a clear story about the checks it's making and how they relate to the intended spec. The test's main job is to prevent regressions. When someone drafts a change that would be a regression, the test's first step to prevent it is to get the author's attention by failing; but then the second step, equally essential, is to communicate what the problem is so the author can identify the right fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. These count checks reminds me of the self.assert_database_query_count's on the server, but over there the number is used to catch potential performance regressions. Here, the counts are used differently.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll spend a few minutes trying to work out what the right value should actually be here.

OK, just activated TalkBack and played with it for a bit — first refreshing myself on how to use it in general, and then trying it in Zulip.

My main conclusion is that for #535 we'll have some work to do in the message list: particularly when you try scrolling around, there are some glitches that seem to be related to the sticky header. (That'll be an M5b issue, like #535 itself.)

I don't see any direct effect of this semanticChildCount value. In particular it's not getting announced to the user. It probably has an influence on the scale of tones that get played when scrolling around, to indicate how close you are to the beginning vs. the end of the list — but that's pretty subtle.

So I think I'll just simplify this to length, leave a TODO(#537), and call it done for now.


slivers: [
sliver,

// This is a trivial placeholder that occupies no space. Its purpose is
// to have the key that's passed to [ScrollView.center], and so to cause
// the above [SliverStickyHeaderList] to run from bottom to top.
const SliverToBoxAdapter(key: centerSliverKey),
topSliver,
bottomSliver,
]);
}

Expand Down
7 changes: 5 additions & 2 deletions lib/widgets/scrolling.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,13 @@ class MessageListScrollPosition extends ScrollPositionWithSingleContext {

if (!_hasEverCompletedLayout) {
// The list is being laid out for the first time (its first performLayout).
// Start out scrolled to the end.
// Start out scrolled down so the bottom sliver (the new messages)
// occupies 75% of the viewport,
// or at the in-range scroll position closest to that.
// This also brings [pixels] within bounds, which
// the initial value of 0.0 might not have been.
final target = maxScrollExtent;
final target = clampDouble(0.75 * viewportDimension,
minScrollExtent, maxScrollExtent);
if (!hasPixels || pixels != target) {
correctPixels(target);
changed = true;
Expand Down
12 changes: 12 additions & 0 deletions test/flutter_checks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,18 @@ extension TextEditingControllerChecks on Subject<TextEditingController> {
Subject<String?> get text => has((t) => t.text, 'text');
}

extension ScrollMetricsChecks on Subject<ScrollMetrics> {
Subject<double> get minScrollExtent => has((x) => x.minScrollExtent, 'minScrollExtent');
Subject<double> get maxScrollExtent => has((x) => x.maxScrollExtent, 'maxScrollExtent');
Subject<double> get pixels => has((x) => x.pixels, 'pixels');
Subject<double> get extentBefore => has((x) => x.extentBefore, 'extentBefore');
Subject<double> get extentAfter => has((x) => x.extentAfter, 'extentAfter');
}

extension ScrollPositionChecks on Subject<ScrollPosition> {
Subject<ScrollActivity?> get activity => has((x) => x.activity, 'activity');
}

extension ScrollActivityChecks on Subject<ScrollActivity> {
Subject<double> get velocity => has((x) => x.velocity, 'velocity');
}
Expand Down
44 changes: 22 additions & 22 deletions test/model/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1450,28 +1450,28 @@ void main() {
.deepEquals(expected..insertAll(0, [101, 103, 105]));

// … and on MessageEvent.
await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 301, stream: stream1, topic: 'A')));
await store.addMessage(
eg.streamMessage(id: 301, stream: stream1, topic: 'A'));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(301));

await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 302, stream: stream1, topic: 'B')));
await store.addMessage(
eg.streamMessage(id: 302, stream: stream1, topic: 'B'));
checkNotNotified();
check(model.messages.map((m) => m.id)).deepEquals(expected);

await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 303, stream: stream2, topic: 'C')));
await store.addMessage(
eg.streamMessage(id: 303, stream: stream2, topic: 'C'));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(303));

await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 304, stream: stream2, topic: 'D')));
await store.addMessage(
eg.streamMessage(id: 304, stream: stream2, topic: 'D'));
checkNotNotified();
check(model.messages.map((m) => m.id)).deepEquals(expected);

await store.handleEvent(eg.messageEvent(
eg.dmMessage(id: 305, from: eg.otherUser, to: [eg.selfUser])));
await store.addMessage(
eg.dmMessage(id: 305, from: eg.otherUser, to: [eg.selfUser]));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(305));
});
Expand Down Expand Up @@ -1507,18 +1507,18 @@ void main() {
.deepEquals(expected..insertAll(0, [101, 102]));

// … and on MessageEvent.
await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 301, stream: stream, topic: 'A')));
await store.addMessage(
eg.streamMessage(id: 301, stream: stream, topic: 'A'));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(301));

await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 302, stream: stream, topic: 'B')));
await store.addMessage(
eg.streamMessage(id: 302, stream: stream, topic: 'B'));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(302));

await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 303, stream: stream, topic: 'C')));
await store.addMessage(
eg.streamMessage(id: 303, stream: stream, topic: 'C'));
checkNotNotified();
check(model.messages.map((m) => m.id)).deepEquals(expected);
});
Expand Down Expand Up @@ -1549,8 +1549,8 @@ void main() {
.deepEquals(expected..insertAll(0, [101]));

// … and on MessageEvent.
await store.handleEvent(eg.messageEvent(
eg.streamMessage(id: 301, stream: stream, topic: 'A')));
await store.addMessage(
eg.streamMessage(id: 301, stream: stream, topic: 'A'));
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(301));
});
Expand Down Expand Up @@ -1589,7 +1589,7 @@ void main() {
// … and on MessageEvent.
final messages = getMessages(301);
for (var i = 0; i < 3; i += 1) {
await store.handleEvent(eg.messageEvent(messages[i]));
await store.addMessage(messages[i]);
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(301 + i));
}
Expand Down Expand Up @@ -1627,7 +1627,7 @@ void main() {
// … and on MessageEvent.
final messages = getMessages(301);
for (var i = 0; i < 2; i += 1) {
await store.handleEvent(eg.messageEvent(messages[i]));
await store.addMessage(messages[i]);
checkNotifiedOnce();
check(model.messages.map((m) => m.id)).deepEquals(expected..add(301 + i));
}
Expand Down Expand Up @@ -1718,11 +1718,11 @@ void main() {
checkNotified(count: 2);

// Then test MessageEvent, where a new header is needed…
await store.handleEvent(eg.messageEvent(streamMessage(13)));
await store.addMessage(streamMessage(13));
checkNotifiedOnce();

// … and where it's not.
await store.handleEvent(eg.messageEvent(streamMessage(14)));
await store.addMessage(streamMessage(14));
checkNotifiedOnce();

// Then test UpdateMessageEvent edits, where a header is and remains needed…
Expand Down
10 changes: 5 additions & 5 deletions test/model/message_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ void main() {
check(store.messages).isEmpty();

final newMessage = eg.streamMessage();
await store.handleEvent(eg.messageEvent(newMessage));
await store.addMessage(newMessage);
check(store.messages).deepEquals({
newMessage.id: newMessage,
});
Expand All @@ -148,7 +148,7 @@ void main() {
});

final newMessage = eg.streamMessage();
await store.handleEvent(eg.messageEvent(newMessage));
await store.addMessage(newMessage);
check(store.messages).deepEquals({
for (final m in messages) m.id: m,
newMessage.id: newMessage,
Expand All @@ -162,7 +162,7 @@ void main() {
check(store.messages).deepEquals({1: message});

final newMessage = eg.streamMessage(id: 1, content: '<p>bar</p>');
await store.handleEvent(eg.messageEvent(newMessage));
await store.addMessage(newMessage);
check(store.messages).deepEquals({1: newMessage});
});
});
Expand Down Expand Up @@ -859,7 +859,7 @@ void main() {
]);

await prepare();
await store.handleEvent(eg.messageEvent(message));
await store.addMessage(message);
}

test('smoke', () async {
Expand Down Expand Up @@ -930,7 +930,7 @@ void main() {
),
]);
await prepare();
await store.handleEvent(eg.messageEvent(message));
await store.addMessage(message);
check(store.messages[message.id]).isNotNull().poll.isNull();
});
});
Expand Down
Loading