From ddd08decf785ae1971125163087c84cae8399c95 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 31 Aug 2023 14:14:22 -0700 Subject: [PATCH 1/7] msglist [nfc]: Let StreamTopicRecipientHeader handle looking in store --- lib/widgets/message_list.dart | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index f4b4718f7f..80384c7999 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -332,15 +332,10 @@ class RecipientHeader extends StatelessWidget { Widget build(BuildContext context) { // TODO recipient headings depend on narrow final message = this.message; - switch (message) { - case StreamMessage(): - final store = PerAccountStoreWidget.of(context); - final subscription = store.subscriptions[message.streamId]; - return StreamTopicRecipientHeader( - message: message, streamColor: colorForStream(subscription)); - case DmMessage(): - return DmRecipientHeader(message: message); - } + return switch (message) { + StreamMessage() => StreamTopicRecipientHeader(message: message), + DmMessage() => DmRecipientHeader(message: message), + }; } } @@ -421,14 +416,16 @@ Color colorForStream(Subscription? subscription) { } class StreamTopicRecipientHeader extends StatelessWidget { - const StreamTopicRecipientHeader( - {super.key, required this.message, required this.streamColor}); + const StreamTopicRecipientHeader({super.key, required this.message}); final StreamMessage message; - final Color streamColor; @override Widget build(BuildContext context) { + final store = PerAccountStoreWidget.of(context); + final subscription = store.subscriptions[message.streamId]; + final streamColor = colorForStream(subscription); + final streamName = message.displayRecipient; // TODO get from stream data final topic = message.subject; final contrastingColor = From b6e3718c0b4d0762254f6cc1002c195330718350 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 31 Aug 2023 14:16:00 -0700 Subject: [PATCH 2/7] msglist: Get stream name for recipient header from stream data --- lib/widgets/message_list.dart | 9 ++++++--- test/widgets/message_list_test.dart | 28 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 80384c7999..5c304f545e 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -423,15 +423,18 @@ class StreamTopicRecipientHeader extends StatelessWidget { @override Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); - final subscription = store.subscriptions[message.streamId]; - final streamColor = colorForStream(subscription); - final streamName = message.displayRecipient; // TODO get from stream data + final stream = store.streams[message.streamId]; + final streamName = stream?.name ?? message.displayRecipient; // TODO(log) if missing final topic = message.subject; + + final subscription = store.subscriptions[message.streamId]; + final streamColor = colorForStream(subscription); final contrastingColor = ThemeData.estimateBrightnessForColor(streamColor) == Brightness.dark ? Colors.white : Colors.black; + return GestureDetector( onTap: () => Navigator.push(context, MessageListPage.buildRoute(context: context, diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 60b26bcd0a..149f00e26b 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -17,6 +17,7 @@ import '../example_data.dart' as eg; import '../model/binding.dart'; import '../model/message_list_test.dart'; import '../model/test_store.dart'; +import '../stdlib_checks.dart'; import '../test_images.dart'; import 'content_checks.dart'; @@ -182,6 +183,33 @@ void main() { }); group('recipient headers', () { + testWidgets('show stream name from message when stream unknown', (tester) async { + // This can perfectly well happen, because message fetches can race + // with events. + final stream = eg.stream(name: 'stream name'); + await setupMessageListPage(tester, messages: [ + eg.streamMessage(stream: stream), + ]); + await tester.pump(); + tester.widget(find.text('stream name')); + }); + + testWidgets('show stream name from stream data when known', (tester) async { + final stream = eg.stream(name: 'old stream name'); + await setupMessageListPage(tester, messages: [ + eg.streamMessage(stream: stream), + ]); + // TODO(#182) this test would be more realistic using a StreamUpdateEvent + store.handleEvent(StreamCreateEvent(id: stream.streamId, streams: [ + ZulipStream.fromJson({ + ...(deepToJson(stream) as Map), + 'name': 'new stream name', + }), + ])); + await tester.pump(); + tester.widget(find.text('new stream name')); + }); + testWidgets('show names on DMs', (tester) async { await setupMessageListPage(tester, messages: [ eg.dmMessage(from: eg.selfUser, to: []), From 3c9ee7ea2dc6f25263018395e34c2fcead81fc1a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 31 Aug 2023 14:19:21 -0700 Subject: [PATCH 3/7] msglist [nfc]: Skip redundant store lookup in DmRecipientHeader This was initially written when we didn't depend on the store in the actual build phase. But now we do, so we have it around at this point and might as well use it. --- lib/widgets/message_list.dart | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 5c304f545e..1c2226bfda 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -491,12 +491,9 @@ class DmRecipientHeader extends StatelessWidget { return GestureDetector( behavior: HitTestBehavior.translucent, - onTap: () { - final store = PerAccountStoreWidget.of(context); - final narrow = DmNarrow.ofMessage(message, selfUserId: store.account.userId); - Navigator.push(context, - MessageListPage.buildRoute(context: context, narrow: narrow)); - }, + onTap: () => Navigator.push(context, + MessageListPage.buildRoute(context: context, + narrow: DmNarrow.ofMessage(message, selfUserId: store.account.userId))), child: DecoratedBox( decoration: BoxDecoration( color: Colors.white, From ecfa4c7bc8b66791cd81873ead8fea6c6acd9c48 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 1 Sep 2023 09:09:59 -0700 Subject: [PATCH 4/7] msglist [nfc]: Use switch statement in itemBuilder, vs switch expression This makes room to add a bit more complexity to some of the cases. --- lib/widgets/message_list.dart | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 1c2226bfda..09ad4b9514 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -270,22 +270,22 @@ class _MessageListState extends State with PerAccountStoreAwareStat reverse: true, itemBuilder: (context, i) { final data = model!.items[length - 1 - i]; - return switch (data) { - MessageListHistoryStartItem() => - const Center( + switch (data) { + case MessageListHistoryStartItem(): + return const Center( child: Padding( padding: EdgeInsets.symmetric(vertical: 16.0), - child: Text("No earlier messages."))), // TODO use an icon - MessageListLoadingItem() => - const Center( + child: Text("No earlier messages."))); // TODO use an icon + case MessageListLoadingItem(): + return const Center( child: Padding( padding: EdgeInsets.symmetric(vertical: 16.0), - child: CircularProgressIndicator())), // TODO perhaps a different indicator - MessageListMessageItem() => - MessageItem( + child: CircularProgressIndicator())); // TODO perhaps a different indicator + case MessageListMessageItem(): + return MessageItem( trailing: i == 0 ? const SizedBox(height: 8) : const SizedBox(height: 11), - item: data), - }; + item: data); + } }); } } From 7ad6177f43b2df8dc6a631ff848cdc4b207b2a5f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 1 Sep 2023 12:50:58 -0700 Subject: [PATCH 5/7] msglist [nfc]: Make explicit that we don't have loading indicators at bottom We're already relying on this assumption in the code to add a new message, in _processMessage. We'll add the needed complications there when they become needed. --- lib/model/message_list.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index e998dd1917..93155056b0 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -27,14 +27,15 @@ class MessageListMessageItem extends MessageListItem { MessageListMessageItem(this.message, this.content); } -/// Indicates the app is loading more messages at the top or bottom. +/// Indicates the app is loading more messages at the top. +// TODO(#80): or loading at the bottom, by adding a [MessageListDirection.newer] class MessageListLoadingItem extends MessageListItem { final MessageListDirection direction; const MessageListLoadingItem(this.direction); } -enum MessageListDirection { older, newer } +enum MessageListDirection { older } /// Indicates we've reached the oldest message in the narrow. class MessageListHistoryStartItem extends MessageListItem { @@ -102,7 +103,6 @@ mixin _MessageSequence { case MessageListLoadingItem(): switch (item.direction) { case MessageListDirection.older: return -1; - case MessageListDirection.newer: return 1; } case MessageListMessageItem(:var message): return message.id.compareTo(messageId); } From 0207e2cf5b3ec9ddc045cc6078bb0df7aee99508 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 14 Jul 2023 19:32:28 -0700 Subject: [PATCH 6/7] msglist [nfc]: Introduce MessageListRecipientHeaderItem Instead of showing recipient headers as part of each MessageListMessageItem, they're now their own message-list items. At this stage, we still show one for each message. --- lib/model/message_list.dart | 19 ++++++++++++++++--- lib/widgets/message_list.dart | 16 +++++++++++----- test/model/message_list_test.dart | 12 ++++++++++-- test/widgets/message_list_test.dart | 12 ++++++------ 4 files changed, 43 insertions(+), 16 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 93155056b0..71b875f6f3 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -19,12 +19,19 @@ sealed class MessageListItem { const MessageListItem(); } +class MessageListRecipientHeaderItem extends MessageListItem { + final Message message; + + MessageListRecipientHeaderItem(this.message); +} + /// A message to show in the message list. class MessageListMessageItem extends MessageListItem { final Message message; final ZulipContent content; + final bool isLastInBlock; - MessageListMessageItem(this.message, this.content); + MessageListMessageItem(this.message, this.content, {required this.isLastInBlock}); } /// Indicates the app is loading more messages at the top. @@ -104,6 +111,8 @@ mixin _MessageSequence { switch (item.direction) { case MessageListDirection.older: return -1; } + case MessageListRecipientHeaderItem(:var message): + return (message.id <= messageId) ? -1 : 1; case MessageListMessageItem(:var message): return message.id.compareTo(messageId); } } @@ -118,7 +127,7 @@ mixin _MessageSequence { assert(itemIndex > -1 && items[itemIndex] is MessageListMessageItem && identical((items[itemIndex] as MessageListMessageItem).message, message)); - items[itemIndex] = MessageListMessageItem(message, content); + items[itemIndex] = MessageListMessageItem(isLastInBlock: true, message, content); } /// Append [message] to [messages], and update derived data accordingly. @@ -163,7 +172,11 @@ mixin _MessageSequence { // This will get more complicated to handle the ways that messages interact // with the display of neighboring messages: sender headings #175, // recipient headings #174, and date separators #173. - items.add(MessageListMessageItem(messages[index], contents[index])); + items.add(MessageListRecipientHeaderItem(messages[index])); + items.add(MessageListMessageItem( + isLastInBlock: true, + messages[index], contents[index], + )); } /// Update [items] to include markers at start and end as appropriate. diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 09ad4b9514..bfdb03b97a 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -281,6 +281,10 @@ class _MessageListState extends State with PerAccountStoreAwareStat child: Padding( padding: EdgeInsets.symmetric(vertical: 16.0), child: CircularProgressIndicator())); // TODO perhaps a different indicator + case MessageListRecipientHeaderItem(): + final header = RecipientHeader(message: data.message); + return StickyHeaderItem(allowOverflow: true, + header: header, child: header); case MessageListMessageItem(): return MessageItem( trailing: i == 0 ? const SizedBox(height: 8) : const SizedBox(height: 11), @@ -376,17 +380,19 @@ class MessageItem extends StatelessWidget { // right than at bottom and in the recipient header: black 10% alpha, // vs. 88% lightness. Assume that's an accident. shape: Border( - left: recipientBorder, bottom: restBorder, right: restBorder)); + left: recipientBorder, + right: restBorder, + bottom: item.isLastInBlock ? restBorder : BorderSide.none, + )); - final recipientHeader = RecipientHeader(message: message); return StickyHeaderItem( - header: recipientHeader, + allowOverflow: !item.isLastInBlock, + header: RecipientHeader(message: message), child: Column(children: [ - recipientHeader, DecoratedBox( decoration: borderDecoration, child: MessageWithSender(message: message, content: item.content)), - if (trailing != null) trailing!, + if (trailing != null && item.isLastInBlock) trailing!, ])); // Web handles the left-side recipient marker in a funky way: diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 4a2fc3b0a9..1d7dcb349f 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -486,7 +486,7 @@ void checkInvariants(MessageListView model) { check(model).items.length.equals( ((model.haveOldest || model.fetchingOlder) ? 1 : 0) - + model.messages.length); + + 2 * model.messages.length); int i = 0; if (model.haveOldest) { check(model.items[i++]).isA(); @@ -495,15 +495,23 @@ void checkInvariants(MessageListView model) { check(model.items[i++]).isA(); } for (int j = 0; j < model.messages.length; j++) { + check(model.items[i++]).isA() + .message.identicalTo(model.messages[j]); check(model.items[i++]).isA() ..message.identicalTo(model.messages[j]) - ..content.identicalTo(model.contents[j]); + ..content.identicalTo(model.contents[j]) + ..isLastInBlock.isTrue(); } } +extension MessageListRecipientHeaderItemChecks on Subject { + Subject get message => has((x) => x.message, 'message'); +} + extension MessageListMessageItemChecks on Subject { Subject get message => has((x) => x.message, 'message'); Subject get content => has((x) => x.content, 'content'); + Subject get isLastInBlock => has((x) => x.isLastInBlock, 'isLastInBlock'); } extension MessageListViewChecks on Subject { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 149f00e26b..37c8fde6fb 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -70,7 +70,7 @@ void main() { testWidgets('basic', (tester) async { await setupMessageListPage(tester, foundOldest: false, messages: List.generate(100, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser))); - check(itemCount(tester)).equals(100); + check(itemCount(tester)).equals(200); // Fling-scroll upward... await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000); @@ -83,13 +83,13 @@ void main() { await tester.pump(Duration.zero); // Allow a frame for the response to arrive. // Now we have more messages. - check(itemCount(tester)).equals(200); + check(itemCount(tester)).equals(400); }); testWidgets('observe double-fetch glitch', (tester) async { await setupMessageListPage(tester, foundOldest: false, messages: List.generate(100, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser))); - check(itemCount(tester)).equals(100); + check(itemCount(tester)).equals(200); // Fling-scroll upward... await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000); @@ -101,10 +101,10 @@ void main() { for (int i = 0; i < 30; i++) { // Find the point in the fling where the fetch starts. await tester.pump(const Duration(milliseconds: 100)); - if (itemCount(tester)! > 100) break; // The loading indicator appeared. + if (itemCount(tester)! > 200) break; // The loading indicator appeared. } await tester.pump(Duration.zero); // Allow a frame for the response to arrive. - check(itemCount(tester)).equals(200); + check(itemCount(tester)).equals(400); // On the next frame, we promptly fetch *another* batch. // This is a glitch and it'd be nicer if we didn't. @@ -112,7 +112,7 @@ void main() { messages: List.generate(100, (i) => eg.streamMessage(id: 750 + i, sender: eg.selfUser))).toJson()); await tester.pump(const Duration(milliseconds: 1)); await tester.pump(Duration.zero); - check(itemCount(tester)).equals(300); + check(itemCount(tester)).equals(600); }, skip: true); // TODO this still reproduces manually, still needs debugging, // but has become harder to reproduce in a test. }); From c5e70275eba836fbc493f5b92e23b5a99504dcad Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 1 Sep 2023 14:32:55 -0700 Subject: [PATCH 7/7] msglist: Share recipient headers across messages Fixes: #174 --- lib/model/message_list.dart | 75 +++++++++-- test/model/message_list_test.dart | 191 +++++++++++++++++++++++++++- test/widgets/message_list_test.dart | 12 +- 3 files changed, 256 insertions(+), 22 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 71b875f6f3..9cf6eb3181 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -28,8 +28,8 @@ class MessageListRecipientHeaderItem extends MessageListItem { /// A message to show in the message list. class MessageListMessageItem extends MessageListItem { final Message message; - final ZulipContent content; - final bool isLastInBlock; + ZulipContent content; + bool isLastInBlock; MessageListMessageItem(this.message, this.content, {required this.isLastInBlock}); } @@ -127,7 +127,7 @@ mixin _MessageSequence { assert(itemIndex > -1 && items[itemIndex] is MessageListMessageItem && identical((items[itemIndex] as MessageListMessageItem).message, message)); - items[itemIndex] = MessageListMessageItem(isLastInBlock: true, message, content); + (items[itemIndex] as MessageListMessageItem).content = content; } /// Append [message] to [messages], and update derived data accordingly. @@ -170,13 +170,20 @@ mixin _MessageSequence { /// This message must already have been parsed and reflected in [contents]. void _processMessage(int index) { // This will get more complicated to handle the ways that messages interact - // with the display of neighboring messages: sender headings #175, - // recipient headings #174, and date separators #173. - items.add(MessageListRecipientHeaderItem(messages[index])); - items.add(MessageListMessageItem( - isLastInBlock: true, - messages[index], contents[index], - )); + // with the display of neighboring messages: sender headings #175 + // and date separators #173. + final message = messages[index]; + final content = contents[index]; + if (index > 0 && canShareRecipientHeader(messages[index - 1], message)) { + assert(items.last is MessageListMessageItem); + final prevMessageItem = items.last as MessageListMessageItem; + assert(identical(prevMessageItem.message, messages[index - 1])); + assert(prevMessageItem.isLastInBlock); + prevMessageItem.isLastInBlock = false; + } else { + items.add(MessageListRecipientHeaderItem(message)); + } + items.add(MessageListMessageItem(message, content, isLastInBlock: true)); } /// Update [items] to include markers at start and end as appropriate. @@ -210,6 +217,53 @@ mixin _MessageSequence { } } +@visibleForTesting +bool canShareRecipientHeader(Message prevMessage, Message message) { + if (prevMessage is StreamMessage && message is StreamMessage) { + if (prevMessage.streamId != message.streamId) return false; + if (prevMessage.subject != message.subject) return false; + } else if (prevMessage is DmMessage && message is DmMessage) { + if (!_equalIdSequences(prevMessage.allRecipientIds, message.allRecipientIds)) { + return false; + } + } else { + return false; + } + + // switch ((prevMessage, message)) { + // case (StreamMessage(), StreamMessage()): + // // TODO(dart-3): this doesn't type-narrow prevMessage and message + // case (DmMessage(), DmMessage()): + // // … + // default: + // return false; + // } + + // TODO memoize [DateTime]s... also use memoized for showing date/time in msglist + final prevTime = DateTime.fromMillisecondsSinceEpoch(prevMessage.timestamp * 1000); + final time = DateTime.fromMillisecondsSinceEpoch(message.timestamp * 1000); + if (!_sameDay(prevTime, time)) return false; + + return true; +} + +// Intended for [Message.allRecipientIds]. Assumes efficient `length`. +bool _equalIdSequences(Iterable xs, Iterable ys) { + if (xs.length != ys.length) return false; + final xs_ = xs.iterator; final ys_ = ys.iterator; + while (xs_.moveNext() && ys_.moveNext()) { + if (xs_.current != ys_.current) return false; + } + return true; +} + +bool _sameDay(DateTime date1, DateTime date2) { + if (date1.year != date2.year) return false; + if (date1.month != date2.month) return false; + if (date1.day != date2.day) return false; + return true; +} + /// A view-model for a message list. /// /// The owner of one of these objects must call [dispose] when the object @@ -343,6 +397,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { /// were changed, and ignores any changes to its stream or topic. /// /// TODO(#150): Handle message moves. + // NB that when handling message moves (#150), recipient headers may need updating. void maybeUpdateMessage(UpdateMessageEvent event) { final idx = _findMessageWithId(event.messageId); if (idx == -1) { diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 1d7dcb349f..8e1f1f0dbc 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -461,6 +461,183 @@ void main() async { check(model).messages.length.equals(31); check(model.contents[0]).equalsNode(correctContent); }); + + test('recipient headers are maintained consistently', () async { + // This tests the code that maintains the invariant that recipient headers + // are present just where [canShareRecipientHeader] requires them. + // In [checkInvariants] we check the current state against that invariant, + // so here we just need to exercise that code through all the relevant cases. + // Each [checkNotifiedOnce] call ensures there's been a [checkInvariants] call + // (in the listener that increments [notifiedCount]). + // + // A separate unit test covers [canShareRecipientHeader] itself. + // So this test just needs messages that can share, and messages that can't, + // and doesn't need to exercise the different reasons that messages can't. + + const timestamp = 1693602618; + final stream = eg.stream(); + Message streamMessage(int id) => + eg.streamMessage(id: id, stream: stream, topic: 'foo', timestamp: timestamp); + Message dmMessage(int id) => + eg.dmMessage(id: id, from: eg.selfUser, to: [], timestamp: timestamp); + + // First, test fetchInitial, where some headers are needed and others not. + prepare(); + connection.prepare(json: newestResult( + foundOldest: false, + messages: [streamMessage(10), streamMessage(11), dmMessage(12)], + ).toJson()); + await model.fetchInitial(); + checkNotifiedOnce(); + + // Then fetchOlder, where a header is needed in between… + connection.prepare(json: olderResult( + anchor: model.messages[0].id, + foundOldest: false, + messages: [streamMessage(7), streamMessage(8), dmMessage(9)], + ).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + + // … and fetchOlder where there's no header in between. + connection.prepare(json: olderResult( + anchor: model.messages[0].id, + foundOldest: false, + messages: [streamMessage(6)], + ).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + + // Then test maybeAddMessage, where a new header is needed… + model.maybeAddMessage(streamMessage(13)); + checkNotifiedOnce(); + + // … and where it's not. + model.maybeAddMessage(streamMessage(14)); + checkNotifiedOnce(); + + // Then test maybeUpdateMessage, where a header is and remains needed… + UpdateMessageEvent updateEvent(Message message) => UpdateMessageEvent( + id: 1, messageId: message.id, messageIds: [message.id], + flags: message.flags, + renderedContent: '${message.content}

edited

', + ); + model.maybeUpdateMessage(updateEvent(model.messages.first)); + checkNotifiedOnce(); + model.maybeUpdateMessage(updateEvent(model.messages[model.messages.length - 2])); + checkNotifiedOnce(); + + // … and where it's not. + model.maybeUpdateMessage(updateEvent(model.messages.last)); + checkNotifiedOnce(); + + // Then test reassemble. + model.reassemble(); + checkNotifiedOnce(); + + // Have a new fetchOlder reach the oldest, so that a history-start marker appears… + connection.prepare(json: olderResult( + anchor: model.messages[0].id, + foundOldest: true, + messages: [streamMessage(5)], + ).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + + // … and then test reassemble again. + model.reassemble(); + checkNotifiedOnce(); + }); + + group('canShareRecipientHeader', () { + test('stream messages vs DMs, no share', () { + final dmMessage = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]); + final streamMessage = eg.streamMessage(timestamp: dmMessage.timestamp); + check(canShareRecipientHeader(streamMessage, dmMessage)).isFalse(); + check(canShareRecipientHeader(dmMessage, streamMessage)).isFalse(); + }); + + test('stream messages of same day share just if same stream/topic', () { + final stream0 = eg.stream(streamId: 123); + final stream1 = eg.stream(streamId: 234); + final messageAB = eg.streamMessage(stream: stream0, topic: 'foo'); + final messageXB = eg.streamMessage(stream: stream1, topic: 'foo', timestamp: messageAB.timestamp); + final messageAX = eg.streamMessage(stream: stream0, topic: 'bar', timestamp: messageAB.timestamp); + check(canShareRecipientHeader(messageAB, messageAB)).isTrue(); + check(canShareRecipientHeader(messageAB, messageXB)).isFalse(); + check(canShareRecipientHeader(messageXB, messageAB)).isFalse(); + check(canShareRecipientHeader(messageAB, messageAX)).isFalse(); + check(canShareRecipientHeader(messageAX, messageAB)).isFalse(); + check(canShareRecipientHeader(messageAX, messageXB)).isFalse(); + check(canShareRecipientHeader(messageXB, messageAX)).isFalse(); + }); + + test('DMs of same day share just if same recipients', () { + final message0 = eg.dmMessage(from: eg.selfUser, to: []); + final message01 = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser], timestamp: message0.timestamp); + final message10 = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], timestamp: message0.timestamp); + final message02 = eg.dmMessage(from: eg.selfUser, to: [eg.thirdUser], timestamp: message0.timestamp); + final message20 = eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser], timestamp: message0.timestamp); + final message012 = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser, eg.thirdUser], timestamp: message0.timestamp); + final message102 = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser, eg.thirdUser], timestamp: message0.timestamp); + final message201 = eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser, eg.otherUser], timestamp: message0.timestamp); + final groups = [[message0], [message01, message10], + [message02, message20], [message012, message102, message201]]; + for (int i0 = 0; i0 < groups.length; i0++) { + for (int i1 = 0; i1 < groups.length; i1++) { + for (int j0 = 0; j0 < groups[i0].length; j0++) { + for (int j1 = 0; j1 < groups[i1].length; j1++) { + final message0 = groups[i0][j0]; + final message1 = groups[i1][j1]; + check( + because: 'recipients ${message0.allRecipientIds} vs ${message1.allRecipientIds}', + canShareRecipientHeader(message0, message1), + ).equals(i0 == i1); + } + } + } + } + }); + + test('messages to same recipient share just if same day', () { + // These timestamps will differ depending on the timezone of the + // environment where the tests are run, in order to give the same results + // in the code under test which is also based on the ambient timezone. + // TODO(dart): It'd be great if tests could control the ambient timezone, + // so as to exercise cases like where local time falls back across midnight. + int timestampFromLocalTime(String date) => DateTime.parse(date).millisecondsSinceEpoch ~/ 1000; + + const t111a = '2021-01-01 00:00:00'; + const t111b = '2021-01-01 12:00:00'; + const t111c = '2021-01-01 23:59:58'; + const t111d = '2021-01-01 23:59:59'; + const t112a = '2021-01-02 00:00:00'; + const t112b = '2021-01-02 00:00:01'; + const t121 = '2021-02-01 00:00:00'; + const t211 = '2022-01-01 00:00:00'; + final groups = [[t111a, t111b, t111c, t111d], [t112a, t112b], [t121], [t211]]; + + final stream = eg.stream(); + for (int i0 = 0; i0 < groups.length; i0++) { + for (int i1 = i0; i1 < groups.length; i1++) { + for (int j0 = 0; j0 < groups[i0].length; j0++) { + for (int j1 = (i0 == i1) ? j0 : 0; j1 < groups[i1].length; j1++) { + final time0 = groups[i0][j0]; + final time1 = groups[i1][j1]; + check(because: 'times $time0, $time1', canShareRecipientHeader( + eg.streamMessage(stream: stream, topic: 'foo', timestamp: timestampFromLocalTime(time0)), + eg.streamMessage(stream: stream, topic: 'foo', timestamp: timestampFromLocalTime(time1)), + )).equals(i0 == i1); + check(because: 'times $time0, $time1', canShareRecipientHeader( + eg.dmMessage(from: eg.selfUser, to: [], timestamp: timestampFromLocalTime(time0)), + eg.dmMessage(from: eg.selfUser, to: [], timestamp: timestampFromLocalTime(time1)), + )).equals(i0 == i1); + } + } + } + } + }); + }); } void checkInvariants(MessageListView model) { @@ -484,9 +661,6 @@ void checkInvariants(MessageListView model) { .equalsNode(parseContent(model.messages[i].content)); } - check(model).items.length.equals( - ((model.haveOldest || model.fetchingOlder) ? 1 : 0) - + 2 * model.messages.length); int i = 0; if (model.haveOldest) { check(model.items[i++]).isA(); @@ -495,13 +669,18 @@ void checkInvariants(MessageListView model) { check(model.items[i++]).isA(); } for (int j = 0; j < model.messages.length; j++) { - check(model.items[i++]).isA() - .message.identicalTo(model.messages[j]); + if (j == 0 + || !canShareRecipientHeader(model.messages[j-1], model.messages[j])) { + check(model.items[i++]).isA() + .message.identicalTo(model.messages[j]); + } check(model.items[i++]).isA() ..message.identicalTo(model.messages[j]) ..content.identicalTo(model.contents[j]) - ..isLastInBlock.isTrue(); + ..isLastInBlock.equals( + i == model.items.length || model.items[i] is! MessageListMessageItem); } + check(model.items).length.equals(i); } extension MessageListRecipientHeaderItemChecks on Subject { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 37c8fde6fb..f59cbc210b 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -70,7 +70,7 @@ void main() { testWidgets('basic', (tester) async { await setupMessageListPage(tester, foundOldest: false, messages: List.generate(100, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser))); - check(itemCount(tester)).equals(200); + check(itemCount(tester)).equals(101); // Fling-scroll upward... await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000); @@ -83,13 +83,13 @@ void main() { await tester.pump(Duration.zero); // Allow a frame for the response to arrive. // Now we have more messages. - check(itemCount(tester)).equals(400); + check(itemCount(tester)).equals(201); }); testWidgets('observe double-fetch glitch', (tester) async { await setupMessageListPage(tester, foundOldest: false, messages: List.generate(100, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser))); - check(itemCount(tester)).equals(200); + check(itemCount(tester)).equals(101); // Fling-scroll upward... await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000); @@ -101,10 +101,10 @@ void main() { for (int i = 0; i < 30; i++) { // Find the point in the fling where the fetch starts. await tester.pump(const Duration(milliseconds: 100)); - if (itemCount(tester)! > 200) break; // The loading indicator appeared. + if (itemCount(tester)! > 101) break; // The loading indicator appeared. } await tester.pump(Duration.zero); // Allow a frame for the response to arrive. - check(itemCount(tester)).equals(400); + check(itemCount(tester)).equals(201); // On the next frame, we promptly fetch *another* batch. // This is a glitch and it'd be nicer if we didn't. @@ -112,7 +112,7 @@ void main() { messages: List.generate(100, (i) => eg.streamMessage(id: 750 + i, sender: eg.selfUser))).toJson()); await tester.pump(const Duration(milliseconds: 1)); await tester.pump(Duration.zero); - check(itemCount(tester)).equals(600); + check(itemCount(tester)).equals(301); }, skip: true); // TODO this still reproduces manually, still needs debugging, // but has become harder to reproduce in a test. });