From c6e4194c35728ca0a53896696be2785e71273430 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 28 Apr 2025 15:51:44 -0700 Subject: [PATCH 1/8] test [nfc]: Use `store.addMessage` shortcut everywhere --- test/model/message_list_test.dart | 44 ++++++++++++++--------------- test/model/message_test.dart | 10 +++---- test/widgets/message_list_test.dart | 2 +- 3 files changed, 28 insertions(+), 28 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 2c5f0711dc..f92bdfc096 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -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)); }); @@ -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); }); @@ -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)); }); @@ -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)); } @@ -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)); } @@ -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… diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 1f774e32b9..25e89b0a54 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -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, }); @@ -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, @@ -162,7 +162,7 @@ void main() { check(store.messages).deepEquals({1: message}); final newMessage = eg.streamMessage(id: 1, content: '

bar

'); - await store.handleEvent(eg.messageEvent(newMessage)); + await store.addMessage(newMessage); check(store.messages).deepEquals({1: newMessage}); }); }); @@ -859,7 +859,7 @@ void main() { ]); await prepare(); - await store.handleEvent(eg.messageEvent(message)); + await store.addMessage(message); } test('smoke', () async { @@ -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(); }); }); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 44f9a3203d..fdc2661c7b 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -1510,7 +1510,7 @@ void main() { // introduce new message final newMessage = eg.streamMessage(flags:[MessageFlag.read]); - await store.handleEvent(eg.messageEvent(newMessage)); + await store.addMessage(newMessage); await tester.pump(); // process handleEvent check(find.byType(MessageItem).evaluate()).length.equals(2); check(getAnimation(tester, message.id)) From 907074978b537168984c8e445e0378703f4a9d4b Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 28 Apr 2025 15:08:15 -0700 Subject: [PATCH 2/8] msglist test [nfc]: Shorten "controller" names in some tests These tests are about scrolling, and there's no other sort of controller in sight. So just `controller` is a good name for the scroll controller. --- test/widgets/message_list_test.dart | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index fdc2661c7b..264e11270f 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -462,14 +462,14 @@ void main() { testWidgets('scrolling changes visibility', (tester) async { await setupMessageListPage(tester, messageCount: 10); - final scrollController = findMessageListScrollController(tester)!; + final controller = findMessageListScrollController(tester)!; check(isButtonVisible(tester)).equals(false); - scrollController.jumpTo(-600); + controller.jumpTo(-600); await tester.pump(); check(isButtonVisible(tester)).equals(true); - scrollController.jumpTo(0); + controller.jumpTo(0); await tester.pump(); check(isButtonVisible(tester)).equals(false); }); @@ -478,8 +478,8 @@ void main() { await setupMessageListPage(tester, messageCount: 100); // Scroll up, to hide the button. - final scrollController = findMessageListScrollController(tester)!; - scrollController.jumpTo(-600); + final controller = findMessageListScrollController(tester)!; + controller.jumpTo(-600); await tester.pump(); check(isButtonVisible(tester)).equals(true); @@ -497,16 +497,16 @@ void main() { testWidgets('button works', (tester) async { await setupMessageListPage(tester, messageCount: 10); - final scrollController = findMessageListScrollController(tester)!; - scrollController.jumpTo(-600); + final controller = findMessageListScrollController(tester)!; + controller.jumpTo(-600); await tester.pump(); - check(scrollController.position.pixels).equals(-600); + check(controller.position.pixels).equals(-600); // Tap button. await tester.tap(find.byType(ScrollToBottomButton)); // The list scrolls to the end… await tester.pumpAndSettle(); - check(scrollController.position.pixels).equals(0); + check(controller.position.pixels).equals(0); // … and for good measure confirm the button disappeared. check(isButtonVisible(tester)).equals(false); }); From 565381633fbc2b6bd96e762c618f90e99494c523 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 30 Apr 2025 18:12:54 -0700 Subject: [PATCH 3/8] test [nfc]: Add checks-extensions for ScrollMetrics, ScrollPosition --- test/flutter_checks.dart | 12 ++++++ test/widgets/message_list_test.dart | 6 +-- test/widgets/scrolling_test.dart | 60 ++++++++++++++--------------- 3 files changed, 45 insertions(+), 33 deletions(-) diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index 328bfdd843..df2777aac6 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -142,6 +142,18 @@ extension TextEditingControllerChecks on Subject { Subject get text => has((t) => t.text, 'text'); } +extension ScrollMetricsChecks on Subject { + Subject get minScrollExtent => has((x) => x.minScrollExtent, 'minScrollExtent'); + Subject get maxScrollExtent => has((x) => x.maxScrollExtent, 'maxScrollExtent'); + Subject get pixels => has((x) => x.pixels, 'pixels'); + Subject get extentBefore => has((x) => x.extentBefore, 'extentBefore'); + Subject get extentAfter => has((x) => x.extentAfter, 'extentAfter'); +} + +extension ScrollPositionChecks on Subject { + Subject get activity => has((x) => x.activity, 'activity'); +} + extension ScrollActivityChecks on Subject { Subject get velocity => has((x) => x.velocity, 'velocity'); } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 264e11270f..68995a8197 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -500,13 +500,13 @@ void main() { final controller = findMessageListScrollController(tester)!; controller.jumpTo(-600); await tester.pump(); - check(controller.position.pixels).equals(-600); + check(controller.position).pixels.equals(-600); // Tap button. await tester.tap(find.byType(ScrollToBottomButton)); // The list scrolls to the end… await tester.pumpAndSettle(); - check(controller.position.pixels).equals(0); + check(controller.position).pixels.equals(0); // … and for good measure confirm the button disappeared. check(isButtonVisible(tester)).equals(false); }); @@ -520,7 +520,7 @@ void main() { // Scroll a long distance up, many screenfuls. controller.jumpTo(-distance); await tester.pump(); - check(controller.position.pixels).equals(-distance); + check(controller.position).pixels.equals(-distance); // Tap button. await tester.tap(find.byType(ScrollToBottomButton)); diff --git a/test/widgets/scrolling_test.dart b/test/widgets/scrolling_test.dart index 8e0afafef4..7ee90d6eab 100644 --- a/test/widgets/scrolling_test.dart +++ b/test/widgets/scrolling_test.dart @@ -124,8 +124,8 @@ void main() { // Starts out scrolled all the way to the bottom, // even though it must have taken several rounds of layout to find that. - check(controller.position.pixels) - .equals(itemHeight * numItems * (numItems + 1)/2); + check(controller.position) + .pixels.equals(itemHeight * numItems * (numItems + 1)/2); check(tester.getRect(find.text('item ${numItems-1}', skipOffstage: false))) .bottom.equals(600); }); @@ -198,30 +198,30 @@ void main() { await prepare(tester, topHeight: 300, bottomHeight: 600); await tester.drag(findBottom, Offset(0, 300)); await tester.pump(); - check(position.extentAfter).equals(300); + check(position).extentAfter.equals(300); // Start scrolling to end, from just a short distance up. position.scrollToEnd(); await tester.pump(); - check(position.extentAfter).equals(300); - check(position.activity).isA(); + check(position).extentAfter.equals(300); + check(position).activity.isA(); // The scrolling moves at a stately pace; … await tester.pump(Duration(milliseconds: 100)); - check(position.extentAfter).equals(200); + check(position).extentAfter.equals(200); await tester.pump(Duration(milliseconds: 100)); - check(position.extentAfter).equals(100); + check(position).extentAfter.equals(100); // … then upon reaching the end, … await tester.pump(Duration(milliseconds: 100)); - check(position.extentAfter).equals(0); + check(position).extentAfter.equals(0); // … goes idle on the next frame, … await tester.pump(Duration(milliseconds: 1)); - check(position.activity).isA(); + check(position).activity.isA(); // … without moving any farther. - check(position.extentAfter).equals(0); + check(position).extentAfter.equals(0); }); testWidgets('long -> bounded speed', (tester) async { @@ -231,12 +231,12 @@ void main() { await prepare(tester, topHeight: distance + 1000, bottomHeight: 300); await tester.drag(findBottom, Offset(0, distance)); await tester.pump(); - check(position.extentAfter).equals(distance); + check(position).extentAfter.equals(distance); // Start scrolling to end. position.scrollToEnd(); await tester.pump(); - check(position.activity).isA(); + check(position).activity.isA(); // Let it scroll, plotting the trajectory. final log = []; @@ -249,12 +249,12 @@ void main() { (i) => distance - referenceSpeed * i)); // Having reached the end, … - check(position.extentAfter).equals(0); + check(position).extentAfter.equals(0); // … it goes idle on the next frame, … await tester.pump(Duration(milliseconds: 1)); - check(position.activity).isA(); + check(position).activity.isA(); // … without moving any farther. - check(position.extentAfter).equals(0); + check(position).extentAfter.equals(0); }); testWidgets('starting from overscroll, just drift', (tester) async { @@ -266,33 +266,33 @@ void main() { await tester.pump(); final offset1 = position.pixels - position.maxScrollExtent; check(offset1).isGreaterThan(100 / 2); - check(position.activity).isA(); + check(position).activity.isA(); // Start drifting back into range. await tester.pump(Duration(milliseconds: 10)); final offset2 = position.pixels - position.maxScrollExtent; check(offset2)..isGreaterThan(0.0)..isLessThan(offset1); - check(position.activity).isA() + check(position).activity.isA() .velocity.isLessThan(0); // Invoke `scrollToEnd`. The motion should stop… position.scrollToEnd(); await tester.pump(); check(position.pixels - position.maxScrollExtent).equals(offset2); - check(position.activity).isA() + check(position).activity.isA() .velocity.equals(0); // … and resume drifting from there… await tester.pump(Duration(milliseconds: 10)); final offset3 = position.pixels - position.maxScrollExtent; check(offset3)..isGreaterThan(0.0)..isLessThan(offset2); - check(position.activity).isA() + check(position).activity.isA() .velocity.isLessThan(0); // … to eventually return to being in range. await tester.pump(Duration(seconds: 1)); check(position.pixels - position.maxScrollExtent).equals(0); - check(position.activity).isA(); + check(position).activity.isA(); debugDefaultTargetPlatformOverride = null; }); @@ -305,17 +305,17 @@ void main() { position.jumpTo(398); await tester.pump(); - check(position.extentAfter).equals(2); + check(position).extentAfter.equals(2); position.scrollToEnd(); await tester.pump(); - check(position.extentAfter).equals(2); + check(position).extentAfter.equals(2); // Reach the end in just 150ms, not 300ms. await tester.pump(Duration(milliseconds: 75)); - check(position.extentAfter).equals(1); + check(position).extentAfter.equals(1); await tester.pump(Duration(milliseconds: 75)); - check(position.extentAfter).equals(0); + check(position).extentAfter.equals(0); }); testWidgets('on overscroll, stop', (tester) async { @@ -325,7 +325,7 @@ void main() { // Scroll up… position.jumpTo(400); await tester.pump(); - check(position.extentAfter).equals(600); + check(position).extentAfter.equals(600); // … then invoke `scrollToEnd`… position.scrollToEnd(); @@ -334,7 +334,7 @@ void main() { // … but have the bottom sliver turn out to be shorter than it was. await prepare(tester, topHeight: 400, bottomHeight: 600, reuseController: true); - check(position.extentAfter).equals(200); + check(position).extentAfter.equals(200); // Let the scrolling animation proceed until it hits the end. int steps = 0; @@ -348,10 +348,10 @@ void main() { check(position.pixels - position.maxScrollExtent).equals(0); // … and the animation is done. Nothing further happens. - check(position.activity).isA(); + check(position).activity.isA(); await tester.pump(Duration(milliseconds: 11)); check(position.pixels - position.maxScrollExtent).equals(0); - check(position.activity).isA(); + check(position).activity.isA(); debugDefaultTargetPlatformOverride = null; }); @@ -362,7 +362,7 @@ void main() { // Scroll up… position.jumpTo(0); await tester.pump(); - check(position.extentAfter).equals(3000); + check(position).extentAfter.equals(3000); // … then invoke `scrollToEnd`… position.scrollToEnd(); @@ -371,7 +371,7 @@ void main() { // … but have the bottom sliver turn out to be longer than it was. await prepare(tester, topHeight: 1000, bottomHeight: 6000, reuseController: true); - check(position.extentAfter).equals(6000); + check(position).extentAfter.equals(6000); // Let the scrolling animation go until it stops. int steps = 0; From 231b054b289b12662d247a3822e72b14254bdc64 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 28 Apr 2025 15:11:42 -0700 Subject: [PATCH 4/8] msglist test: Generalize scroll-to-bottom tests to not assume 0 is end These were implicitly assuming that the scroll position at the bottom of the list is 0.0. That's currently true, but we'll soon let it vary. Make them a bit more general in saying what they mean. --- test/widgets/message_list_test.dart | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 68995a8197..d32764c6c5 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -462,15 +462,21 @@ void main() { testWidgets('scrolling changes visibility', (tester) async { await setupMessageListPage(tester, messageCount: 10); + // Scroll position starts at the end, so button hidden. final controller = findMessageListScrollController(tester)!; + check(controller.position).extentAfter.equals(0); check(isButtonVisible(tester)).equals(false); + // Scrolling up, button becomes visible. controller.jumpTo(-600); await tester.pump(); + check(controller.position).extentAfter.isGreaterThan(0); check(isButtonVisible(tester)).equals(true); - controller.jumpTo(0); + // Scrolling back down to end, button becomes hidden again. + controller.jumpTo(controller.position.maxScrollExtent); await tester.pump(); + check(controller.position).extentAfter.equals(0); check(isButtonVisible(tester)).equals(false); }); @@ -500,13 +506,13 @@ void main() { final controller = findMessageListScrollController(tester)!; controller.jumpTo(-600); await tester.pump(); - check(controller.position).pixels.equals(-600); + check(controller.position).extentAfter.isGreaterOrEqual(600); // Tap button. await tester.tap(find.byType(ScrollToBottomButton)); // The list scrolls to the end… await tester.pumpAndSettle(); - check(controller.position).pixels.equals(0); + check(controller.position).extentAfter.equals(0); // … and for good measure confirm the button disappeared. check(isButtonVisible(tester)).equals(false); }); From e5983aada63f3c58e80e6e12168f73ad1d5c5d7d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 29 Apr 2025 16:08:46 -0700 Subject: [PATCH 5/8] msglist: Simplify computing semanticChildCount This has already gotten out of sync with the total number of children in the list, after 5c70c76f0 added a TypingStatusWidget child while leaving this unchanged. On the other hand, its spec isn't to be the total number of children: it's to be the total number of children "that will contribute semantic information". So the spacer shouldn't count, and probably TypingStatusWidget and MarkAsReadWidget shouldn't either when they don't show anything. (Currently it effectively counts the spacer and MarkAsReadWidget, or one could retcon that to TypingStatusWidget and MarkAsReadWidget.) Further: > If the number is unknown or unbounded this should be left unset > or set to null. So it seems like when `haveOldest` is false, this should be null. Meanwhile, the exact value here seems to have little effect. I just tried the app out in TalkBack, and there are some other issues (when you scroll around, there's some glitchy behavior with focus moving to the sticky header and back, and even with the scroll position sometimes abruptly jumping back; and I couldn't find a way to navigate by a whole message or item at a time, though that might have been my inexperience with TalkBack) but it doesn't ever quote the number of list items as far as I could tell, and isn't bothered by this being a few more or less than the spec'd number. So for the moment let's do something simple: don't count any of the extra children at the end, the ones that are often or always empty. This also makes a couple of tests a bit simpler to follow. Thanks to Zixuan for raising the question. Discussion here: https://github.com/zulip/zulip-flutter/pull/1468#discussion_r2067407272 --- lib/widgets/message_list.dart | 2 +- test/widgets/message_list_test.dart | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 9d8e27757a..20df09ac34 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -641,7 +641,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat }, controller: scrollController, - semanticChildCount: length + 2, + semanticChildCount: length, // TODO(#537): what's the right value for this? center: centerSliverKey, paintOrder: SliverPaintOrder.firstIsTop, diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index d32764c6c5..bf93c1f078 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -375,7 +375,7 @@ void main() { testWidgets('basic', (tester) async { await setupMessageListPage(tester, foundOldest: false, messages: List.generate(300, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser))); - check(itemCount(tester)).equals(303); + check(itemCount(tester)).equals(301); // Fling-scroll upward... await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000); @@ -388,7 +388,7 @@ void main() { await tester.pump(Duration.zero); // Allow a frame for the response to arrive. // Now we have more messages. - check(itemCount(tester)).equals(403); + check(itemCount(tester)).equals(401); }); testWidgets('observe double-fetch glitch', (tester) async { @@ -429,7 +429,7 @@ void main() { ...List.generate(100, (i) => eg.streamMessage(id: 1302 + i)), ]); final lastRequest = connection.lastRequest; - check(itemCount(tester)).equals(404); + check(itemCount(tester)).equals(402); // Fling-scroll upward... await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000); From 271c55e06ac4f92484163bd30bf6e9cd56ba23d3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 30 Apr 2025 17:51:33 -0700 Subject: [PATCH 6/8] msglist [nfc]: Use names to clarify list-child index vs model-item index --- lib/widgets/message_list.dart | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 20df09ac34..c204b3fa2b 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -580,7 +580,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat } Widget _buildListView(BuildContext context) { - final length = model!.items.length; + final numItems = model!.items.length; const centerSliverKey = ValueKey('center sliver'); final zulipLocalizations = ZulipLocalizations.of(context); @@ -603,22 +603,24 @@ class _MessageListState extends State 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; - final index = model!.findItemWithMessageId(valueKey.value); - if (index == -1) return null; - return length - 1 - (index - 3); + final messageId = (key as ValueKey).value; + final itemIndex = model!.findItemWithMessageId(messageId); + if (itemIndex == -1) return null; + final childIndex = numItems - 1 - (itemIndex - 3); + return childIndex; }, - childCount: length + 3, - (context, i) { + childCount: numItems + 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 == 0) return const SizedBox(height: 36); - if (i == 1) return MarkAsReadWidget(narrow: widget.narrow); + if (childIndex == 1) return MarkAsReadWidget(narrow: widget.narrow); - if (i == 2) return TypingStatusWidget(narrow: widget.narrow); + if (childIndex == 2) return TypingStatusWidget(narrow: widget.narrow); - final data = model!.items[length - 1 - (i - 3)]; + final itemIndex = numItems - 1 - (childIndex - 3); + final data = model!.items[itemIndex]; return _buildItem(zulipLocalizations, data); })); @@ -641,7 +643,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat }, controller: scrollController, - semanticChildCount: length, // TODO(#537): what's the right value for this? + semanticChildCount: numItems, // TODO(#537): what's the right value for this? center: centerSliverKey, paintOrder: SliverPaintOrder.firstIsTop, From fca651bf5f14562a8a742d25c8e64385ac3ed9a7 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 18 Jan 2024 10:53:16 -0500 Subject: [PATCH 7/8] msglist: Split into back-to-back slivers Thanks to all the preparatory changes we've made in this commit series and those that came before it, this change should have only subtle effects on user-visible behavior. This therefore serves as a testing ground for all the ways that the message list's scrolling behavior can become more complicated when two (nontrivial) slivers are involved. If we find a situation where the behavior does change noticeably (beyond what's described below), that will be something to investigate and fix. In particular: * The sticky headers should behave exactly as they did before, even when the sliver boundary lies under the sticky header, thanks to several previous commit series. * On first loading a given message list, it should start out scrolled to the end, just as before. * When already scrolled to the end, the message list should stay there when a new message arrives, or a message is edited, etc., even if the (new) latest message is taller than it was. This commit adds a test to confirm that. Subtle differences include: * When scrolled up from the bottom and a new message comes in, the behavior is slightly different from before. The current exact behavior is something we probably want to change anyway, and I think the new behavior isn't particularly better or worse. * The behavior upon overscroll might be slightly different; I'm not sure. * When a new message arrives, the previously-latest message gets a fresh element in the tree, and any UI state on it is lost. This happens because it moves between one sliver-list and the other, and the `findChildIndexCallback` mechanism only works within a single sliver-list. This causes a couple of visible glitches, but they seem tolerable. This will also naturally get fixed in the next PR or two toward #82: we'll start adding new messages to the bottom sliver, rather than having them push anything from the bottom to the top sliver. Known symptoms of this are: * When the user has just marked messages as read and a new message arrives while the unread markers are fading, the marker on the previously-latest message will (if it was present) abruptly vanish while the others smoothly continue fading. We have a test for the smooth fading, and it happened to test the latest message, so this commit adjusts the test to avoid triggering this issue. * If the user had a spoiler expanded on the previously-latest message, it will reset to collapsed. --- lib/widgets/message_list.dart | 76 +++++++++++++++++++++++------ test/widgets/message_list_test.dart | 37 +++++++++++++- 2 files changed, 95 insertions(+), 18 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index c204b3fa2b..90a0762d34 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -580,11 +580,20 @@ class _MessageListState extends State with PerAccountStoreAwareStat } Widget _buildListView(BuildContext context) { - final numItems = 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] @@ -606,20 +615,59 @@ class _MessageListState extends State with PerAccountStoreAwareStat final messageId = (key as ValueKey).value; final itemIndex = model!.findItemWithMessageId(messageId); if (itemIndex == -1) return null; - final childIndex = numItems - 1 - (itemIndex - 3); + final childIndex = totalItems - 1 - (itemIndex + bottomItems); + if (childIndex < 0) return null; return childIndex; }, - childCount: numItems + 3, + 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).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 (childIndex == 0) return const SizedBox(height: 36); + if (childIndex == bottomItems + 2) return const SizedBox(height: 36); - if (childIndex == 1) return MarkAsReadWidget(narrow: widget.narrow); + if (childIndex == bottomItems + 1) return MarkAsReadWidget(narrow: widget.narrow); - if (childIndex == 2) return TypingStatusWidget(narrow: widget.narrow); + if (childIndex == bottomItems) return TypingStatusWidget(narrow: widget.narrow); - final itemIndex = numItems - 1 - (childIndex - 3); + final itemIndex = topItems + childIndex; final data = model!.items[itemIndex]; return _buildItem(zulipLocalizations, data); })); @@ -627,7 +675,7 @@ class _MessageListState extends State with PerAccountStoreAwareStat 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( @@ -643,17 +691,13 @@ class _MessageListState extends State with PerAccountStoreAwareStat }, controller: scrollController, - semanticChildCount: numItems, // TODO(#537): what's the right value for this? + semanticChildCount: totalItems, // TODO(#537): what's the right value for this? center: centerSliverKey, paintOrder: SliverPaintOrder.firstIsTop, 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, ]); } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index bf93c1f078..047a036cb5 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -453,6 +453,31 @@ void main() { }); }); + group('scroll position', () { + // The scrolling behavior is tested in more detail in the tests of + // [MessageListScrollView], in scrolling_test.dart . + + testWidgets('sticks to end upon new message', (tester) async { + await setupMessageListPage(tester, + messages: List.generate(10, (_) => eg.streamMessage(content: '

a

'))); + final controller = findMessageListScrollController(tester)!; + + // Starts at end, and with room to scroll up. + check(controller.position) + ..extentAfter.equals(0) + ..extentBefore.isGreaterThan(0); + final oldPosition = controller.position.pixels; + + // On new message, position remains at end… + await store.addMessage(eg.streamMessage(content: '

a

b

')); + await tester.pump(); + check(controller.position) + ..extentAfter.equals(0) + // … even though that means a bigger number now. + ..pixels.isGreaterThan(oldPosition); + }); + }); + group('ScrollToBottomButton interactions', () { bool isButtonVisible(WidgetTester tester) { return tester.any(find.descendant( @@ -1490,8 +1515,15 @@ void main() { // as the number of items changes in MessageList. See // `findChildIndexCallback` passed into [SliverStickyHeaderList] // at [_MessageListState._buildListView]. + + // TODO(#82): Cut paddingMessage. It's there to paper over a glitch: + // the _UnreadMarker animation *does* get interrupted in the case where + // the message gets pushed from one sliver to the other. See: + // https://github.com/zulip/zulip-flutter/pull/1436#issuecomment-2756738779 + // That case will no longer exist when #82 is complete. final message = eg.streamMessage(flags: []); - await setupMessageListPage(tester, messages: [message]); + final paddingMessage = eg.streamMessage(); + await setupMessageListPage(tester, messages: [message, paddingMessage]); check(getAnimation(tester, message.id)) ..value.equals(1.0) ..status.equals(AnimationStatus.dismissed); @@ -1515,10 +1547,11 @@ void main() { ..status.equals(AnimationStatus.forward); // introduce new message + check(find.byType(MessageItem)).findsExactly(2); final newMessage = eg.streamMessage(flags:[MessageFlag.read]); await store.addMessage(newMessage); await tester.pump(); // process handleEvent - check(find.byType(MessageItem).evaluate()).length.equals(2); + check(find.byType(MessageItem)).findsExactly(3); check(getAnimation(tester, message.id)) ..value.isGreaterThan(0.0) ..value.isLessThan(1.0) From 52d69092a0ece6fd556ff770e625ea7ee434ab4f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 25 Mar 2025 13:56:55 -0700 Subject: [PATCH 8/8] scroll: Show start of latest message if long, instead of end This makes our first payoff in actual UX from having the message list split into two back-to-back slivers! With this change, if you open a message list and the latest message is very tall, the list starts out scrolled so that you can see the top of that latest message -- plus a bit of context above it (25% of the viewport's height). Previously the list would always start out scrolled to the end, so you'd have to scroll up in order to read even the one latest message from the beginning. In addition to a small UX improvement now, this makes a preview of behavior we'll want to have when the bottom sliver starts at the first unread message, and may have many messages after that. This new behavior is nice already with one message, if the message happens to be very tall; but it'll become critical when the bottom sliver is routinely many screenfuls tall. --- lib/widgets/scrolling.dart | 7 ++- test/widgets/scrolling_test.dart | 79 ++++++++++++++++++++++++-------- 2 files changed, 65 insertions(+), 21 deletions(-) diff --git a/lib/widgets/scrolling.dart b/lib/widgets/scrolling.dart index 5bccc680a5..aac36bd8d6 100644 --- a/lib/widgets/scrolling.dart +++ b/lib/widgets/scrolling.dart @@ -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; diff --git a/test/widgets/scrolling_test.dart b/test/widgets/scrolling_test.dart index 7ee90d6eab..cfcb5870eb 100644 --- a/test/widgets/scrolling_test.dart +++ b/test/widgets/scrolling_test.dart @@ -64,20 +64,58 @@ void main() { }); testWidgets('short/long -> scrolls to ends and no farther', (tester) async { - // Starts out scrolled to bottom. + // Starts out scrolled to top (to show top of the bottom sliver). await prepare(tester, topHeight: 100, bottomHeight: 800); - check(tester.getRect(findBottom)).bottom.equals(600); + check(tester.getRect(findTop)).top.equals(0); + check(tester.getRect(findBottom)).bottom.equals(900); - // Try scrolling down (by dragging up); doesn't move. - await tester.drag(findBottom, Offset(0, -100)); + // Try scrolling up (by dragging down); doesn't move. + await tester.drag(findBottom, Offset(0, 100)); await tester.pump(); - check(tester.getRect(findBottom)).bottom.equals(600); + check(tester.getRect(findBottom)).bottom.equals(900); - // Try scrolling up (by dragging down); moves only as far as top of list. - await tester.drag(findBottom, Offset(0, 400)); + // Try scrolling down (by dragging up); moves only as far as bottom of list. + await tester.drag(findBottom, Offset(0, -400)); await tester.pump(); - check(tester.getRect(findBottom)).bottom.equals(900); + check(tester.getRect(findBottom)).bottom.equals(600); + }); + + testWidgets('starts by showing top of bottom sliver, long/long', (tester) async { + // Both slivers are long; the bottom sliver gets 75% of the viewport. + await prepare(tester, topHeight: 1000, bottomHeight: 3000); + check(tester.getRect(findBottom)).top.equals(150); + }); + + testWidgets('starts by showing top of bottom sliver, short/long', (tester) async { + // The top sliver is shorter than 25% of the viewport. + // It's shown in full, and the bottom sliver gets the rest (so >75%). + await prepare(tester, topHeight: 50, bottomHeight: 3000); check(tester.getRect(findTop)).top.equals(0); + check(tester.getRect(findBottom)).top.equals(50); + }); + + testWidgets('starts by showing top of bottom sliver, short/medium', (tester) async { + // The whole list fits in the viewport. It's pinned to the bottom, + // even when that gives the bottom sliver more than 75%. + await prepare(tester, topHeight: 50, bottomHeight: 500); + check(tester.getRect(findTop))..top.equals(50)..bottom.equals(100); + check(tester.getRect(findBottom)).bottom.equals(600); + }); + + testWidgets('starts by showing top of bottom sliver, medium/short', (tester) async { + // The whole list fits in the viewport. It's pinned to the bottom, + // even when that gives the top sliver more than 25%. + await prepare(tester, topHeight: 300, bottomHeight: 100); + check(tester.getRect(findTop))..top.equals(200)..bottom.equals(500); + check(tester.getRect(findBottom)).bottom.equals(600); + }); + + testWidgets('starts by showing top of bottom sliver, long/short', (tester) async { + // The bottom sliver is shorter than 75% of the viewport. + // It's shown in full, and the top sliver gets the rest (so >25%). + await prepare(tester, topHeight: 1000, bottomHeight: 300); + check(tester.getRect(findTop)).bottom.equals(300); + check(tester.getRect(findBottom)).bottom.equals(600); }); testWidgets('short/short -> starts at bottom, immediately without animation', (tester) async { @@ -91,20 +129,20 @@ void main() { check(ys).deepEquals(List.generate(10, (_) => 0.0)); }); - testWidgets('short/long -> starts at bottom, immediately without animation', (tester) async { + testWidgets('short/long -> starts at desired start, immediately without animation', (tester) async { await prepare(tester, topHeight: 100, bottomHeight: 800); final ys = []; for (int i = 0; i < 10; i++) { - ys.add(tester.getRect(findBottom).bottom - 600); + ys.add(tester.getRect(findTop).top); await tester.pump(Duration(milliseconds: 15)); } check(ys).deepEquals(List.generate(10, (_) => 0.0)); }); - testWidgets('starts at bottom, even when bottom underestimated at first', (tester) async { + testWidgets('starts at desired start, even when bottom underestimated at first', (tester) async { const numItems = 10; - const itemHeight = 300.0; + const itemHeight = 20.0; // A list where the bottom sliver takes several rounds of layout // to see how long it really is. @@ -112,22 +150,25 @@ void main() { await tester.pumpWidget(Directionality(textDirection: TextDirection.ltr, child: MessageListScrollView( controller: controller, + // The tiny cacheExtent causes each layout round to only reach + // the first item it expects will go beyond the viewport. + cacheExtent: 1.0, // in (logical) pixels! center: const ValueKey('center'), slivers: [ SliverToBoxAdapter( - child: SizedBox(height: 100, child: Text('top'))), + child: SizedBox(height: 300, child: Text('top'))), SliverList.list(key: const ValueKey('center'), children: List.generate(numItems, (i) => SizedBox(height: (i+1) * itemHeight, child: Text('item $i')))), ]))); await tester.pump(); - // Starts out scrolled all the way to the bottom, - // even though it must have taken several rounds of layout to find that. - check(controller.position) - .pixels.equals(itemHeight * numItems * (numItems + 1)/2); - check(tester.getRect(find.text('item ${numItems-1}', skipOffstage: false))) - .bottom.equals(600); + // Starts out with the bottom sliver occupying 75% of the viewport… + check(controller.position).pixels.equals(450); + // … even though it has more height than that. + check(tester.getRect(find.text('item 6'))).bottom.isGreaterThan(600); + // (And even though on the first round of layout, it would have looked + // much shorter so that the view would have tried to scroll to its end.) }); testWidgets('stick to end of list when it grows', (tester) async {