diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 2f8777d3ed..f2a45b78aa 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -70,11 +70,26 @@ 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 [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 middleMessage = 0; + /// Whether [messages] and [items] represent the results of a fetch. /// /// This allows the UI to distinguish "still working on fetching messages" @@ -133,8 +148,23 @@ 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. + /// + /// 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 middleItem = 0; + int _findMessageWithId(int messageId) { return binarySearchByKey(messages, messageId, (message, messageId) => message.id.compareTo(messageId)); @@ -202,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; @@ -210,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); @@ -232,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)); @@ -246,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(); } @@ -258,6 +301,7 @@ mixin _MessageSequence { void _reset() { generation += 1; messages.clear(); + middleMessage = 0; _fetched = false; _haveOldest = false; _fetchingOlder = false; @@ -265,6 +309,7 @@ mixin _MessageSequence { _fetchOlderCooldownBackoffMachine = null; contents.clear(); items.clear(); + middleItem = 0; } /// Redo all computations from scratch, based on [messages]. @@ -304,6 +349,7 @@ mixin _MessageSequence { canShareSender = (prevMessageItem.message.senderId == message.senderId); } } + if (index == middleMessage) middleItem = items.length; items.add(MessageListMessageItem(message, content, showSender: !canShareSender, isLastInBlock: true)); } @@ -314,6 +360,7 @@ mixin _MessageSequence { for (var i = 0; i < messages.length; i++) { _processMessage(i); } + if (middleMessage == messages.length) middleItem = items.length; } } @@ -453,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/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 bb85556ae9..ac41d771ec 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1,6 +1,8 @@ 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'; import 'package:zulip/api/backoff.dart'; @@ -27,6 +29,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; @@ -97,6 +117,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'; @@ -420,10 +448,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(); @@ -624,11 +648,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)); + ]); }); }); @@ -731,10 +755,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]) { @@ -1438,8 +1458,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( @@ -1452,34 +1471,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 { @@ -1497,8 +1515,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( @@ -1509,24 +1526,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 { @@ -1541,8 +1557,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( @@ -1551,14 +1566,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 { @@ -1581,23 +1595,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)); } }); @@ -1619,24 +1631,230 @@ 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)); + } + }); + }); + + 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(); }); }); @@ -1913,6 +2131,10 @@ void main() { }); } +MessageListView? _lastModel; +List? _lastMessages; +int? _lastMiddleMessage; + void checkInvariants(MessageListView model) { if (!model.fetched) { check(model) @@ -1951,6 +2173,25 @@ void checkInvariants(MessageListView model) { check(isSortedWithoutDuplicates(model.messages.map((m) => m.id).toList())) .isTrue(); + check(model).middleMessage + ..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; @@ -1988,6 +2229,27 @@ void checkInvariants(MessageListView model) { }); } check(model.items).length.equals(i); + + check(model).middleItem + ..isGreaterOrEqual(0) + ..isLessOrEqual(model.items.length); + 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]); + } +} + +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 { @@ -2013,8 +2275,10 @@ 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'); Subject get fetched => has((x) => x.fetched, 'fetched'); Subject get haveOldest => has((x) => x.haveOldest, 'haveOldest'); Subject get fetchingOlder => has((x) => x.fetchingOlder, 'fetchingOlder'); 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)