From 31bd0707b7da4885cf265951a20371b19c4003ed Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 5 May 2025 16:48:49 -0700 Subject: [PATCH 1/8] 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. --- test/model/message_list_test.dart | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index bb85556ae9..9357beb19d 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1,6 +1,7 @@ import 'dart:convert'; import 'package:checks/checks.dart'; +import 'package:flutter/foundation.dart'; import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; import 'package:zulip/api/backoff.dart'; @@ -27,6 +28,24 @@ const newestResult = eg.newestGetMessagesResult; const olderResult = eg.olderGetMessagesResult; void main() { + // Arrange for errors caught within the Flutter framework to be printed + // unconditionally, rather than throttled as they normally are in an app. + // + // When using `testWidgets` from flutter_test, this is done automatically; + // compare the [FlutterError.dumpErrorToConsole] call sites, + // and [FlutterError.onError=] and [debugPrint=] call sites, in flutter_test. + // + // This test file is unusual in needing this manual arrangement; it's needed + // because these aren't widget tests, and yet do have some failures arise as + // exceptions that get caught by the framework: namely, when [checkInvariants] + // throws from within an `addListener` callback. Those exceptions get caught + // by [ChangeNotifier.notifyListeners] and reported there through + // [FlutterError.reportError]. + debugPrint = debugPrintSynchronously; + FlutterError.onError = (details) { + FlutterError.dumpErrorToConsole(details, forceReport: true); + }; + // These variables are the common state operated on by each test. // Each test case calls [prepare] to initialize them. late Subscription subscription; From f31ab3e4c5e0c3412556981906d0a614d4b8abfa Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 5 May 2025 19:39:30 -0700 Subject: [PATCH 2/8] msglist test [nfc]: Use checkHasMessages/MessageIds helpers throughout --- test/model/message_list_test.dart | 72 +++++++++++++------------------ 1 file changed, 31 insertions(+), 41 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 9357beb19d..8d47d61ff7 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -116,6 +116,14 @@ void main() { }); } + void checkHasMessageIds(Iterable messageIds) { + check(model.messages.map((m) => m.id)).deepEquals(messageIds); + } + + void checkHasMessages(Iterable messages) { + checkHasMessageIds(messages.map((e) => e.id)); + } + group('fetchInitial', () { final someChannel = eg.stream(); const someTopic = 'some topic'; @@ -439,10 +447,6 @@ void main() { await setVisibility(policy); } - void checkHasMessageIds(Iterable messageIds) { - check(model.messages.map((m) => m.id)).deepEquals(messageIds); - } - test('mute a visible topic', () async { await prepare(narrow: const CombinedFeedNarrow()); await prepareMutes(); @@ -643,11 +647,11 @@ void main() { check(model).messages.length.equals(30); await store.handleEvent(eg.deleteMessageEvent(messagesToDelete)); checkNotifiedOnce(); - check(model.messages.map((message) => message.id)).deepEquals([ + checkHasMessages([ ...messages.sublist(0, 2), ...messages.sublist(5, 10), ...messages.sublist(15), - ].map((message) => message.id)); + ]); }); }); @@ -750,10 +754,6 @@ void main() { final stream = eg.stream(); final otherStream = eg.stream(); - void checkHasMessages(Iterable messages) { - check(model.messages.map((e) => e.id)).deepEquals(messages.map((e) => e.id)); - } - Future prepareNarrow(Narrow narrow, List? messages) async { await prepare(narrow: narrow); for (final streamToAdd in [stream, otherStream]) { @@ -1457,8 +1457,7 @@ void main() { eg.dmMessage( id: 205, from: eg.otherUser, to: [eg.selfUser]), ]); final expected = []; - check(model.messages.map((m) => m.id)) - .deepEquals(expected..addAll([201, 203, 205])); + checkHasMessageIds(expected..addAll([201, 203, 205])); // … and on fetchOlder… connection.prepare(json: olderResult( @@ -1471,34 +1470,33 @@ void main() { ]).toJson()); await model.fetchOlder(); checkNotified(count: 2); - check(model.messages.map((m) => m.id)) - .deepEquals(expected..insertAll(0, [101, 103, 105])); + checkHasMessageIds(expected..insertAll(0, [101, 103, 105])); // … and on MessageEvent. await store.addMessage( eg.streamMessage(id: 301, stream: stream1, topic: 'A')); checkNotifiedOnce(); - check(model.messages.map((m) => m.id)).deepEquals(expected..add(301)); + checkHasMessageIds(expected..add(301)); await store.addMessage( eg.streamMessage(id: 302, stream: stream1, topic: 'B')); checkNotNotified(); - check(model.messages.map((m) => m.id)).deepEquals(expected); + checkHasMessageIds(expected); await store.addMessage( eg.streamMessage(id: 303, stream: stream2, topic: 'C')); checkNotifiedOnce(); - check(model.messages.map((m) => m.id)).deepEquals(expected..add(303)); + checkHasMessageIds(expected..add(303)); await store.addMessage( eg.streamMessage(id: 304, stream: stream2, topic: 'D')); checkNotNotified(); - check(model.messages.map((m) => m.id)).deepEquals(expected); + checkHasMessageIds(expected); 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)); + checkHasMessageIds(expected..add(305)); }); test('in ChannelNarrow', () async { @@ -1516,8 +1514,7 @@ void main() { eg.streamMessage(id: 203, stream: stream, topic: 'C'), ]); final expected = []; - check(model.messages.map((m) => m.id)) - .deepEquals(expected..addAll([201, 202])); + checkHasMessageIds(expected..addAll([201, 202])); // … and on fetchOlder… connection.prepare(json: olderResult( @@ -1528,24 +1525,23 @@ void main() { ]).toJson()); await model.fetchOlder(); checkNotified(count: 2); - check(model.messages.map((m) => m.id)) - .deepEquals(expected..insertAll(0, [101, 102])); + checkHasMessageIds(expected..insertAll(0, [101, 102])); // … and on MessageEvent. await store.addMessage( eg.streamMessage(id: 301, stream: stream, topic: 'A')); checkNotifiedOnce(); - check(model.messages.map((m) => m.id)).deepEquals(expected..add(301)); + checkHasMessageIds(expected..add(301)); await store.addMessage( eg.streamMessage(id: 302, stream: stream, topic: 'B')); checkNotifiedOnce(); - check(model.messages.map((m) => m.id)).deepEquals(expected..add(302)); + checkHasMessageIds(expected..add(302)); await store.addMessage( eg.streamMessage(id: 303, stream: stream, topic: 'C')); checkNotNotified(); - check(model.messages.map((m) => m.id)).deepEquals(expected); + checkHasMessageIds(expected); }); test('in TopicNarrow', () async { @@ -1560,8 +1556,7 @@ void main() { eg.streamMessage(id: 201, stream: stream, topic: 'A'), ]); final expected = []; - check(model.messages.map((m) => m.id)) - .deepEquals(expected..addAll([201])); + checkHasMessageIds(expected..addAll([201])); // … and on fetchOlder… connection.prepare(json: olderResult( @@ -1570,14 +1565,13 @@ void main() { ]).toJson()); await model.fetchOlder(); checkNotified(count: 2); - check(model.messages.map((m) => m.id)) - .deepEquals(expected..insertAll(0, [101])); + checkHasMessageIds(expected..insertAll(0, [101])); // … and on MessageEvent. await store.addMessage( eg.streamMessage(id: 301, stream: stream, topic: 'A')); checkNotifiedOnce(); - check(model.messages.map((m) => m.id)).deepEquals(expected..add(301)); + checkHasMessageIds(expected..add(301)); }); test('in MentionsNarrow', () async { @@ -1600,23 +1594,21 @@ void main() { // Check filtering on fetchInitial… await prepareMessages(foundOldest: false, messages: getMessages(201)); final expected = []; - check(model.messages.map((m) => m.id)) - .deepEquals(expected..addAll([201, 202, 203])); + checkHasMessageIds(expected..addAll([201, 202, 203])); // … and on fetchOlder… connection.prepare(json: olderResult( anchor: 201, foundOldest: true, messages: getMessages(101)).toJson()); await model.fetchOlder(); checkNotified(count: 2); - check(model.messages.map((m) => m.id)) - .deepEquals(expected..insertAll(0, [101, 102, 103])); + checkHasMessageIds(expected..insertAll(0, [101, 102, 103])); // … and on MessageEvent. final messages = getMessages(301); for (var i = 0; i < 3; i += 1) { await store.addMessage(messages[i]); checkNotifiedOnce(); - check(model.messages.map((m) => m.id)).deepEquals(expected..add(301 + i)); + checkHasMessageIds(expected..add(301 + i)); } }); @@ -1638,23 +1630,21 @@ void main() { // Check filtering on fetchInitial… await prepareMessages(foundOldest: false, messages: getMessages(201)); final expected = []; - check(model.messages.map((m) => m.id)) - .deepEquals(expected..addAll([201, 202])); + checkHasMessageIds(expected..addAll([201, 202])); // … and on fetchOlder… connection.prepare(json: olderResult( anchor: 201, foundOldest: true, messages: getMessages(101)).toJson()); await model.fetchOlder(); checkNotified(count: 2); - check(model.messages.map((m) => m.id)) - .deepEquals(expected..insertAll(0, [101, 102])); + checkHasMessageIds(expected..insertAll(0, [101, 102])); // … and on MessageEvent. final messages = getMessages(301); for (var i = 0; i < 2; i += 1) { await store.addMessage(messages[i]); checkNotifiedOnce(); - check(model.messages.map((m) => m.id)).deepEquals(expected..add(301 + i)); + checkHasMessageIds(expected..add(301 + i)); } }); }); From f4c35e19fc0a023b7979adc94d4e9957eea6c902 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 16:49:20 -0700 Subject: [PATCH 3/8] msglist [nfc]: Expand doc on MessageListView.messages --- lib/model/message_list.dart | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 2f8777d3ed..fd171938c4 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -70,7 +70,11 @@ mixin _MessageSequence { /// A sequence number for invalidating stale fetches. int generation = 0; - /// The messages. + /// The known messages in the list. + /// + /// This may or may not represent all the message history that + /// conceptually belongs in this message list. + /// That information is expressed in [fetched] and [haveOldest]. /// /// See also [contents] and [items]. final List messages = []; From 4a9a2b118d90401ac680869742e82163d023d966 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 16:58:09 -0700 Subject: [PATCH 4/8] 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. --- lib/model/message_list.dart | 9 +++++++++ lib/widgets/message_list.dart | 5 ++--- test/model/message_list_test.dart | 5 +++++ 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index fd171938c4..85881f4dca 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -137,8 +137,17 @@ mixin _MessageSequence { /// This information is completely derived from [messages] and /// the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown]. /// It exists as an optimization, to memoize that computation. + /// + /// See also [middleItem], an index which divides this list + /// into a top slice and a bottom slice. final QueueList items = QueueList(); + /// An index into [items] dividing it into a top slice and a bottom slice. + /// + /// The indices 0 to before [middleItem] are the top slice of [items], + /// and the indices from [middleItem] to the end are the bottom slice. + int get middleItem => items.isEmpty ? 0 : items.length - 1; + int _findMessageWithId(int messageId) { return binarySearchByKey(messages, messageId, (message, messageId) => message.id.compareTo(messageId)); diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 3e8850f5cc..60c08dfbca 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -573,10 +573,9 @@ class _MessageListState extends State with PerAccountStoreAwareStat // 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; + final topItems = model.middleItem; + final bottomItems = totalItems - topItems; // 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. diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 8d47d61ff7..bf626d77d0 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1997,6 +1997,10 @@ void checkInvariants(MessageListView model) { }); } check(model.items).length.equals(i); + + check(model).middleItem + ..isGreaterOrEqual(0) + ..isLessOrEqual(model.items.length); } extension MessageListRecipientHeaderItemChecks on Subject { @@ -2024,6 +2028,7 @@ extension MessageListViewChecks on Subject { Subject> get messages => has((x) => x.messages, 'messages'); Subject> get contents => has((x) => x.contents, 'contents'); Subject> get items => has((x) => x.items, 'items'); + Subject get middleItem => has((x) => x.middleItem, 'middleItem'); Subject get fetched => has((x) => x.fetched, 'fetched'); Subject get haveOldest => has((x) => x.haveOldest, 'haveOldest'); Subject get fetchingOlder => has((x) => x.fetchingOlder, 'fetchingOlder'); From 7401fd7438350c7102e5ba0e216df11d8afc3ff4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 5 May 2025 17:00:13 -0700 Subject: [PATCH 5/8] msglist: Always place the sliver boundary at a message 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. --- lib/model/message_list.dart | 23 ++++++++++++++++++++++- test/model/message_list_test.dart | 3 +++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 85881f4dca..b7ca3ec952 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -146,7 +146,28 @@ mixin _MessageSequence { /// /// The indices 0 to before [middleItem] are the top slice of [items], /// and the indices from [middleItem] to the end are the bottom slice. - int get middleItem => items.isEmpty ? 0 : items.length - 1; + /// + /// If the bottom slice is not empty, + /// then its first item is a [MessageListMessageItem]. + int get middleItem { + switch (items.lastOrNull) { + case null: + return 0; + + case MessageListHistoryStartItem(): + assert(items.length == 1); + return items.length; + + case MessageListMessageItem(): + return items.length - 1; + + case MessageListLoadingItem(direction: MessageListDirection.older): + case MessageListRecipientHeaderItem(): + case MessageListDateSeparatorItem(): + assert(false, "unexpected type of last item"); + return items.length - 1; + } + } int _findMessageWithId(int messageId) { return binarySearchByKey(messages, messageId, diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index bf626d77d0..d69cbcf58e 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -2001,6 +2001,9 @@ void checkInvariants(MessageListView model) { check(model).middleItem ..isGreaterOrEqual(0) ..isLessOrEqual(model.items.length); + if (model.middleItem < model.items.length) { + check(model.items[model.middleItem]).isA(); + } } extension MessageListRecipientHeaderItemChecks on Subject { From 0aec5c11a0e56c193a4de7b7f34db95feace71e3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 5 May 2025 17:01:46 -0700 Subject: [PATCH 6/8] msglist [nfc]: Define sliver boundary by messages 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. --- lib/model/message_list.dart | 18 ++++++++++++++++-- test/model/message_list_test.dart | 12 ++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index b7ca3ec952..338cf6b69e 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -76,9 +76,20 @@ mixin _MessageSequence { /// conceptually belongs in this message list. /// That information is expressed in [fetched] and [haveOldest]. /// + /// See also [middleMessage], an index which divides this list + /// into a top slice and a bottom slice. + /// /// See also [contents] and [items]. final List messages = []; + /// An index into [messages] dividing it into a top slice and a bottom slice. + /// + /// The indices 0 to before [middleMessage] are the top slice of [messages], + /// and the indices from [middleMessage] to the end are the bottom slice. + /// + /// The corresponding item index is [middleItem]. + int get middleMessage => messages.isEmpty ? 0 : messages.length - 1; + /// Whether [messages] and [items] represent the results of a fetch. /// /// This allows the UI to distinguish "still working on fetching messages" @@ -147,8 +158,11 @@ mixin _MessageSequence { /// The indices 0 to before [middleItem] are the top slice of [items], /// and the indices from [middleItem] to the end are the bottom slice. /// - /// If the bottom slice is not empty, - /// then its first item is a [MessageListMessageItem]. + /// The top and bottom slices of [items] correspond to + /// the top and bottom slices of [messages] respectively. + /// Either the bottom slices of both [items] and [messages] are empty, + /// or the first item in the bottom slice of [items] is a [MessageListMessageItem] + /// for the first message in the bottom slice of [messages]. int get middleItem { switch (items.lastOrNull) { case null: diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index d69cbcf58e..94446ca241 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1960,6 +1960,10 @@ void checkInvariants(MessageListView model) { check(isSortedWithoutDuplicates(model.messages.map((m) => m.id).toList())) .isTrue(); + check(model).middleMessage + ..isGreaterOrEqual(0) + ..isLessOrEqual(model.messages.length); + check(model).contents.length.equals(model.messages.length); for (int i = 0; i < model.contents.length; i++) { final poll = model.messages[i].poll; @@ -2001,8 +2005,11 @@ void checkInvariants(MessageListView model) { check(model).middleItem ..isGreaterOrEqual(0) ..isLessOrEqual(model.items.length); - if (model.middleItem < model.items.length) { - check(model.items[model.middleItem]).isA(); + if (model.middleItem == model.items.length) { + check(model.middleMessage).equals(model.messages.length); + } else { + check(model.items[model.middleItem]).isA() + .message.identicalTo(model.messages[model.middleMessage]); } } @@ -2029,6 +2036,7 @@ extension MessageListViewChecks on Subject { Subject get store => has((x) => x.store, 'store'); Subject get narrow => has((x) => x.narrow, 'narrow'); Subject> get messages => has((x) => x.messages, 'messages'); + Subject get middleMessage => has((x) => x.middleMessage, 'middleMessage'); Subject> get contents => has((x) => x.contents, 'contents'); Subject> get items => has((x) => x.items, 'items'); Subject get middleItem => has((x) => x.middleItem, 'middleItem'); From 489f48a71b0c88847a8c014536fcf46100d48b31 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 17:23:32 -0700 Subject: [PATCH 7/8] msglist [nfc]: Maintain middleItem as a field 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. --- lib/model/message_list.dart | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 338cf6b69e..0b6072df6c 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -163,25 +163,7 @@ mixin _MessageSequence { /// Either the bottom slices of both [items] and [messages] are empty, /// or the first item in the bottom slice of [items] is a [MessageListMessageItem] /// for the first message in the bottom slice of [messages]. - int get middleItem { - switch (items.lastOrNull) { - case null: - return 0; - - case MessageListHistoryStartItem(): - assert(items.length == 1); - return items.length; - - case MessageListMessageItem(): - return items.length - 1; - - case MessageListLoadingItem(direction: MessageListDirection.older): - case MessageListRecipientHeaderItem(): - case MessageListDateSeparatorItem(): - assert(false, "unexpected type of last item"); - return items.length - 1; - } - } + int middleItem = 0; int _findMessageWithId(int messageId) { return binarySearchByKey(messages, messageId, @@ -313,6 +295,7 @@ mixin _MessageSequence { _fetchOlderCooldownBackoffMachine = null; contents.clear(); items.clear(); + middleItem = 0; } /// Redo all computations from scratch, based on [messages]. @@ -352,6 +335,7 @@ mixin _MessageSequence { canShareSender = (prevMessageItem.message.senderId == message.senderId); } } + if (index == middleMessage) middleItem = items.length; items.add(MessageListMessageItem(message, content, showSender: !canShareSender, isLastInBlock: true)); } @@ -362,6 +346,7 @@ mixin _MessageSequence { for (var i = 0; i < messages.length; i++) { _processMessage(i); } + if (middleMessage == messages.length) middleItem = items.length; } } From c9ebf4c74eb53791f791d083b98ad83c84121e78 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 1 May 2025 17:13:28 -0700 Subject: [PATCH 8/8] 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 `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 fca651bf5, where a test had had to be weakened slightly because messages began to get moved between slivers. --- lib/model/message_list.dart | 27 +++- test/model/message_list_test.dart | 239 ++++++++++++++++++++++++++++ test/widgets/message_list_test.dart | 68 +++++--- 3 files changed, 311 insertions(+), 23 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 0b6072df6c..f2a45b78aa 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -88,7 +88,7 @@ mixin _MessageSequence { /// and the indices from [middleMessage] to the end are the bottom slice. /// /// The corresponding item index is [middleItem]. - int get middleMessage => messages.isEmpty ? 0 : messages.length - 1; + int middleMessage = 0; /// Whether [messages] and [items] represent the results of a fetch. /// @@ -232,6 +232,7 @@ mixin _MessageSequence { candidate++; assert(contents.length == messages.length); while (candidate < messages.length) { + if (candidate == middleMessage) middleMessage = target; if (test(messages[candidate])) { candidate++; continue; @@ -240,6 +241,7 @@ mixin _MessageSequence { contents[target] = contents[candidate]; target++; candidate++; } + if (candidate == middleMessage) middleMessage = target; messages.length = target; contents.length = target; assert(contents.length == messages.length); @@ -262,6 +264,13 @@ mixin _MessageSequence { } if (messagesToRemoveById.isEmpty) return false; + if (middleMessage == messages.length) { + middleMessage -= messagesToRemoveById.length; + } else { + final middleMessageId = messages[middleMessage].id; + middleMessage -= messagesToRemoveById + .where((id) => id < middleMessageId).length; + } assert(contents.length == messages.length); messages.removeWhere((message) => messagesToRemoveById.contains(message.id)); contents.removeWhere((content) => contentToRemove.contains(content)); @@ -276,11 +285,15 @@ mixin _MessageSequence { // On a Pixel 5, a batch of 100 messages takes ~15-20ms in _insertAllMessages. // (Before that, ~2-5ms in jsonDecode and 0ms in fromJson, // so skip worrying about those steps.) + final oldLength = messages.length; assert(contents.length == messages.length); messages.insertAll(index, toInsert); contents.insertAll(index, toInsert.map( (message) => _parseMessageContent(message))); assert(contents.length == messages.length); + if (index <= middleMessage) { + middleMessage += messages.length - oldLength; + } _reprocessAll(); } @@ -288,6 +301,7 @@ mixin _MessageSequence { void _reset() { generation += 1; messages.clear(); + middleMessage = 0; _fetched = false; _haveOldest = false; _fetchingOlder = false; @@ -486,13 +500,18 @@ class MessageListView with ChangeNotifier, _MessageSequence { allowEmptyTopicName: true, ); if (this.generation > generation) return; + _adjustNarrowForTopicPermalink(result.messages.firstOrNull); + store.reconcileMessages(result.messages); store.recentSenders.handleMessages(result.messages); // TODO(#824) + + // We'll make the bottom slice start at the last visible message, if any. for (final message in result.messages) { - if (_messageVisible(message)) { - _addMessage(message); - } + if (!_messageVisible(message)) continue; + middleMessage = messages.length; + _addMessage(message); + // Now [middleMessage] is the last message (the one just added). } _fetched = true; _haveOldest = result.foundOldest; diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 94446ca241..ac41d771ec 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1,6 +1,7 @@ import 'dart:convert'; import 'package:checks/checks.dart'; +import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; @@ -1649,6 +1650,214 @@ void main() { }); }); + group('middleMessage maintained', () { + // In [checkInvariants] we verify that messages don't move from the + // top to the bottom slice or vice versa. + // Most of these test cases rely on that for all the checks they need. + + test('on fetchInitial empty', () async { + await prepare(narrow: const CombinedFeedNarrow()); + await prepareMessages(foundOldest: true, messages: []); + check(model)..messages.isEmpty() + ..middleMessage.equals(0); + }); + + test('on fetchInitial empty due to muting', () async { + await prepare(narrow: const CombinedFeedNarrow()); + final stream = eg.stream(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream, isMuted: true)); + await prepareMessages(foundOldest: true, messages: [ + eg.streamMessage(stream: stream), + ]); + check(model)..messages.isEmpty() + ..middleMessage.equals(0); + }); + + test('on fetchInitial not empty', () async { + await prepare(narrow: const CombinedFeedNarrow()); + final stream1 = eg.stream(); + final stream2 = eg.stream(); + await store.addStreams([stream1, stream2]); + await store.addSubscription(eg.subscription(stream1)); + await store.addSubscription(eg.subscription(stream2, isMuted: true)); + final messages = [ + eg.streamMessage(stream: stream1), eg.streamMessage(stream: stream2), + eg.streamMessage(stream: stream1), eg.streamMessage(stream: stream2), + eg.streamMessage(stream: stream1), eg.streamMessage(stream: stream2), + eg.streamMessage(stream: stream1), eg.streamMessage(stream: stream2), + eg.streamMessage(stream: stream1), eg.streamMessage(stream: stream2), + ]; + await prepareMessages(foundOldest: true, messages: messages); + // The anchor message is the last visible message… + check(model) + ..messages.length.equals(5) + ..middleMessage.equals(model.messages.length - 1) + // … even though that's not the last message that was in the response. + ..messages[model.middleMessage].id + .equals(messages[messages.length - 2].id); + }); + + /// Like [prepareMessages], but arrange for the given top and bottom slices. + Future prepareMessageSplit(List top, List bottom, { + bool foundOldest = true, + }) async { + assert(bottom.isNotEmpty); // could handle this too if necessary + await prepareMessages(foundOldest: foundOldest, messages: [ + ...top, + bottom.first, + ]); + if (bottom.length > 1) { + await store.addMessages(bottom.skip(1)); + checkNotifiedOnce(); + } + check(model) + ..messages.length.equals(top.length + bottom.length) + ..middleMessage.equals(top.length); + } + + test('on fetchOlder', () async { + await prepare(narrow: const CombinedFeedNarrow()); + final stream = eg.stream(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await prepareMessageSplit(foundOldest: false, + [eg.streamMessage(id: 100, stream: stream)], + [eg.streamMessage(id: 101, stream: stream)]); + + connection.prepare(json: olderResult(anchor: 100, foundOldest: true, + messages: List.generate(5, (i) => + eg.streamMessage(id: 95 + i, stream: stream))).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + }); + + test('on fetchOlder, from top empty', () async { + await prepare(narrow: const CombinedFeedNarrow()); + final stream = eg.stream(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await prepareMessageSplit(foundOldest: false, + [], [eg.streamMessage(id: 100, stream: stream)]); + + connection.prepare(json: olderResult(anchor: 100, foundOldest: true, + messages: List.generate(5, (i) => + eg.streamMessage(id: 95 + i, stream: stream))).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + // The messages from fetchOlder should go in the top sliver, always. + check(model).middleMessage.equals(5); + }); + + test('on MessageEvent', () async { + await prepare(narrow: const CombinedFeedNarrow()); + final stream = eg.stream(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await prepareMessageSplit(foundOldest: false, + [eg.streamMessage(stream: stream)], + [eg.streamMessage(stream: stream)]); + + await store.addMessage(eg.streamMessage(stream: stream)); + checkNotifiedOnce(); + }); + + test('on messages muted, including anchor', () async { + await prepare(narrow: const CombinedFeedNarrow()); + final stream = eg.stream(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await prepareMessageSplit([ + eg.streamMessage(stream: stream, topic: 'foo'), + eg.streamMessage(stream: stream, topic: 'bar'), + ], [ + eg.streamMessage(stream: stream, topic: 'bar'), + eg.streamMessage(stream: stream, topic: 'foo'), + ]); + + await store.handleEvent(eg.userTopicEvent( + stream.streamId, 'bar', UserTopicVisibilityPolicy.muted)); + checkNotifiedOnce(); + }); + + test('on messages muted, not including anchor', () async { + await prepare(narrow: const CombinedFeedNarrow()); + final stream = eg.stream(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await prepareMessageSplit([ + eg.streamMessage(stream: stream, topic: 'foo'), + eg.streamMessage(stream: stream, topic: 'bar'), + ], [ + eg.streamMessage(stream: stream, topic: 'foo'), + ]); + + await store.handleEvent(eg.userTopicEvent( + stream.streamId, 'bar', UserTopicVisibilityPolicy.muted)); + checkNotifiedOnce(); + }); + + test('on messages muted, bottom empty', () async { + await prepare(narrow: const CombinedFeedNarrow()); + final stream = eg.stream(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + await prepareMessageSplit([ + eg.streamMessage(stream: stream, topic: 'foo'), + eg.streamMessage(stream: stream, topic: 'bar'), + ], [ + eg.streamMessage(stream: stream, topic: 'third'), + ]); + + await store.handleEvent(eg.deleteMessageEvent([ + model.messages.last as StreamMessage])); + checkNotifiedOnce(); + check(model).middleMessage.equals(model.messages.length); + + await store.handleEvent(eg.userTopicEvent( + stream.streamId, 'bar', UserTopicVisibilityPolicy.muted)); + checkNotifiedOnce(); + }); + + test('on messages deleted', () async { + await prepare(narrow: const CombinedFeedNarrow()); + final stream = eg.stream(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + final messages = [ + eg.streamMessage(id: 1, stream: stream), + eg.streamMessage(id: 2, stream: stream), + eg.streamMessage(id: 3, stream: stream), + eg.streamMessage(id: 4, stream: stream), + ]; + await prepareMessageSplit(messages.sublist(0, 2), messages.sublist(2)); + + await store.handleEvent(eg.deleteMessageEvent(messages.sublist(1, 3))); + checkNotifiedOnce(); + }); + + test('on messages deleted, bottom empty', () async { + await prepare(narrow: const CombinedFeedNarrow()); + final stream = eg.stream(); + await store.addStream(stream); + await store.addSubscription(eg.subscription(stream)); + final messages = [ + eg.streamMessage(id: 1, stream: stream), + eg.streamMessage(id: 2, stream: stream), + eg.streamMessage(id: 3, stream: stream), + eg.streamMessage(id: 4, stream: stream), + ]; + await prepareMessageSplit(messages.sublist(0, 3), messages.sublist(3)); + + await store.handleEvent(eg.deleteMessageEvent(messages.sublist(3))); + checkNotifiedOnce(); + check(model).middleMessage.equals(model.messages.length); + + await store.handleEvent(eg.deleteMessageEvent(messages.sublist(1, 2))); + checkNotifiedOnce(); + }); + }); + group('handle content parsing into subclasses of ZulipMessageContent', () { test('ZulipContent', () async { final stream = eg.stream(); @@ -1922,6 +2131,10 @@ void main() { }); } +MessageListView? _lastModel; +List? _lastMessages; +int? _lastMiddleMessage; + void checkInvariants(MessageListView model) { if (!model.fetched) { check(model) @@ -1964,6 +2177,21 @@ void checkInvariants(MessageListView model) { ..isGreaterOrEqual(0) ..isLessOrEqual(model.messages.length); + if (identical(model, _lastModel) + && model.generation == _lastModel!.generation) { + // All messages that were present, and still are, should be on the same side + // of `middleMessage` (still top or bottom slice respectively) as they were. + _checkNoIntersection(ListSlice(model.messages, 0, model.middleMessage), + ListSlice(_lastMessages!, _lastMiddleMessage!, _lastMessages!.length), + because: 'messages moved from bottom slice to top slice'); + _checkNoIntersection(ListSlice(_lastMessages!, 0, _lastMiddleMessage!), + ListSlice(model.messages, model.middleMessage, model.messages.length), + because: 'messages moved from top slice to bottom slice'); + } + _lastModel = model; + _lastMessages = model.messages.toList(); + _lastMiddleMessage = model.middleMessage; + check(model).contents.length.equals(model.messages.length); for (int i = 0; i < model.contents.length; i++) { final poll = model.messages[i].poll; @@ -2013,6 +2241,17 @@ void checkInvariants(MessageListView model) { } } +void _checkNoIntersection(List xs, List ys, {String? because}) { + // Both lists are sorted by ID. As an optimization, bet on all or nearly all + // of the first list having smaller IDs than all or nearly all of the other. + if (xs.isEmpty || ys.isEmpty) return; + if (xs.last.id < ys.first.id) return; + final yCandidates = Set.of(ys.takeWhile((m) => m.id <= xs.last.id)); + final intersection = xs.reversed.takeWhile((m) => ys.first.id <= m.id) + .where(yCandidates.contains); + check(intersection, because: because).isEmpty(); +} + extension MessageListRecipientHeaderItemChecks on Subject { Subject get message => has((x) => x.message, 'message'); } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index f4de7b54ae..5f03a35c9b 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -460,23 +460,61 @@ void main() { // [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

'))); + await setupMessageListPage(tester, messages: List.generate(10, + (i) => eg.streamMessage(content: '

message $i

'))); final controller = findMessageListScrollController(tester)!; + final findMiddleMessage = find.text('message 5'); - // Starts at end, and with room to scroll up. - check(controller.position) - ..extentAfter.equals(0) - ..extentBefore.isGreaterThan(0); - final oldPosition = controller.position.pixels; + // Started out scrolled to the bottom. + check(controller.position).extentAfter.equals(0); + final scrollPixels = controller.position.pixels; + + // Note the position of some mid-screen message. + final messageRect = tester.getRect(findMiddleMessage); + check(messageRect)..top.isGreaterThan(0)..bottom.isLessThan(600); - // On new message, position remains at end… + // When a new message arrives, the existing message moves up… await store.addMessage(eg.streamMessage(content: '

a

b

')); await tester.pump(); + check(tester.getRect(findMiddleMessage)) + ..top.isLessThan(messageRect.top) + ..height.isCloseTo(messageRect.height, Tolerance().distance); + // … because the position remains at the end… check(controller.position) ..extentAfter.equals(0) // … even though that means a bigger number now. - ..pixels.isGreaterThan(oldPosition); + ..pixels.isGreaterThan(scrollPixels); + }); + + testWidgets('preserves visible messages upon new message, when not at end', (tester) async { + await setupMessageListPage(tester, messages: List.generate(10, + (i) => eg.streamMessage(content: '

message $i

'))); + final controller = findMessageListScrollController(tester)!; + final findMiddleMessage = find.text('message 5'); + + // Started at bottom. Scroll up a bit. + check(controller.position).extentAfter.equals(0); + controller.position.jumpTo(controller.position.pixels - 100); + await tester.pump(); + check(controller.position).extentAfter.equals(100); + final scrollPixels = controller.position.pixels; + + // Note the position of some mid-screen message. + final messageRect = tester.getRect(findMiddleMessage); + check(messageRect)..top.isGreaterThan(0)..bottom.isLessThan(600); + + // When a new message arrives, the existing message doesn't shift… + await store.addMessage(eg.streamMessage(content: '

a

b

')); + await tester.pump(); + check(tester.getRect(findMiddleMessage)).equals(messageRect); + // … because the scroll position value remained the same… + check(controller.position) + ..pixels.equals(scrollPixels) + // … even though there's now more content off screen below. + // (This last check relies on the fact that the old extentAfter is small, + // less than cacheExtent, so that the new content is only barely offscreen, + // it gets built, and the new extentAfter reflects it.) + ..extentAfter.isGreaterThan(100); }); }); @@ -1517,15 +1555,8 @@ 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: []); - final paddingMessage = eg.streamMessage(); - await setupMessageListPage(tester, messages: [message, paddingMessage]); + await setupMessageListPage(tester, messages: [message]); check(getAnimation(tester, message.id)) ..value.equals(1.0) ..status.equals(AnimationStatus.dismissed); @@ -1549,11 +1580,10 @@ 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)).findsExactly(3); + check(find.byType(MessageItem)).findsExactly(2); check(getAnimation(tester, message.id)) ..value.isGreaterThan(0.0) ..value.isLessThan(1.0)