From 9e2d9e5f79f4a80085f03f14a74d204287a1d32e Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 22 Apr 2025 17:16:35 -0400 Subject: [PATCH 1/6] msglist [nfc]: Extract _SenderRow widget --- lib/widgets/message_list.dart | 52 ++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index c9e20ce87a..7670c6e8ca 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -1338,14 +1338,13 @@ String formatHeaderDate( } } -/// A Zulip message, showing the sender's name and avatar if specified. -// Design referenced from: -// - https://github.com/zulip/zulip-mobile/issues/5511 -// - https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538%3A20849&mode=dev -class MessageWithPossibleSender extends StatelessWidget { - const MessageWithPossibleSender({super.key, required this.item}); +// TODO(i18n): web seems to ignore locale in formatting time, but we could do better +final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US'); - final MessageListMessageItem item; +class _SenderRow extends StatelessWidget { + const _SenderRow({required this.message}); + + final Message message; @override Widget build(BuildContext context) { @@ -1353,14 +1352,12 @@ class MessageWithPossibleSender extends StatelessWidget { final messageListTheme = MessageListTheme.of(context); final designVariables = DesignVariables.of(context); - final message = item.message; final sender = store.getUser(message.senderId); - - Widget? senderRow; - if (item.showSender) { - final time = _kMessageTimestampFormat - .format(DateTime.fromMillisecondsSinceEpoch(1000 * message.timestamp)); - senderRow = Row( + final time = _kMessageTimestampFormat + .format(DateTime.fromMillisecondsSinceEpoch(1000 * message.timestamp)); + return Padding( + padding: const EdgeInsets.fromLTRB(16, 2, 16, 0), + child: Row( mainAxisAlignment: MainAxisAlignment.spaceBetween, crossAxisAlignment: CrossAxisAlignment.baseline, textBaseline: localizedTextBaseline(context), @@ -1400,8 +1397,23 @@ class MessageWithPossibleSender extends StatelessWidget { height: (18 / 16), fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')], ).merge(weightVariableTextStyle(context))), - ]); - } + ])); + } +} + +/// A Zulip message, showing the sender's name and avatar if specified. +// Design referenced from: +// - https://github.com/zulip/zulip-mobile/issues/5511 +// - https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538%3A20849&mode=dev +class MessageWithPossibleSender extends StatelessWidget { + const MessageWithPossibleSender({super.key, required this.item}); + + final MessageListMessageItem item; + + @override + Widget build(BuildContext context) { + final designVariables = DesignVariables.of(context); + final message = item.message; final localizations = ZulipLocalizations.of(context); String? editStateText; @@ -1430,9 +1442,8 @@ class MessageWithPossibleSender extends StatelessWidget { child: Padding( padding: const EdgeInsets.symmetric(vertical: 4), child: Column(children: [ - if (senderRow != null) - Padding(padding: const EdgeInsets.fromLTRB(16, 2, 16, 0), - child: senderRow), + if (item.showSender) + _SenderRow(message: message), Row( crossAxisAlignment: CrossAxisAlignment.baseline, textBaseline: localizedTextBaseline(context), @@ -1460,6 +1471,3 @@ class MessageWithPossibleSender extends StatelessWidget { ]))); } } - -// TODO(i18n): web seems to ignore locale in formatting time, but we could do better -final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US'); From 2d7d83dfd20c1403b53c6f24bd9fb9a85a90ad11 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 27 Mar 2025 17:06:27 -0400 Subject: [PATCH 2/6] msglist [nfc]: Add showTimestamp to _SenderRow widget This is for the upcoming local-echo feature, to hide the timestamp. There isn't a Figma design for messages without a timestamp. --- lib/widgets/message_list.dart | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 7670c6e8ca..4605820c07 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -1342,9 +1342,10 @@ String formatHeaderDate( final _kMessageTimestampFormat = DateFormat('h:mm aa', 'en_US'); class _SenderRow extends StatelessWidget { - const _SenderRow({required this.message}); + const _SenderRow({required this.message, required this.showTimestamp}); final Message message; + final bool showTimestamp; @override Widget build(BuildContext context) { @@ -1389,14 +1390,16 @@ class _SenderRow extends StatelessWidget { ), ], ]))), - const SizedBox(width: 4), - Text(time, - style: TextStyle( - color: messageListTheme.labelTime, - fontSize: 16, - height: (18 / 16), - fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')], - ).merge(weightVariableTextStyle(context))), + if (showTimestamp) ...[ + const SizedBox(width: 4), + Text(time, + style: TextStyle( + color: messageListTheme.labelTime, + fontSize: 16, + height: (18 / 16), + fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')], + ).merge(weightVariableTextStyle(context))), + ], ])); } } @@ -1443,7 +1446,7 @@ class MessageWithPossibleSender extends StatelessWidget { padding: const EdgeInsets.symmetric(vertical: 4), child: Column(children: [ if (item.showSender) - _SenderRow(message: message), + _SenderRow(message: message, showTimestamp: true), Row( crossAxisAlignment: CrossAxisAlignment.baseline, textBaseline: localizedTextBaseline(context), From fb6b53bc69c0ad329cc2779b85f82235d75138d8 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 22 Apr 2025 17:29:52 -0400 Subject: [PATCH 3/6] api [nfc]: Extract Conversation.isSameAs This will make it easier to support comparing the conversations between subclasses of MessageBase. The message list tests on displayRecipient are now mostly exercising the logic on Conversation.isSameAs, which makes it reasonable to move the tests. Keep them here for now since this logic is more relevant to message lists than it is to the rest of the app. Dropped the comment on _equalIdSequences since it is now private in the short body of DmConversation. --- lib/api/model/model.dart | 27 ++++++++++++++++++++++++++- lib/model/message_list.dart | 31 +------------------------------ 2 files changed, 27 insertions(+), 31 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 90769e815b..4c12403b85 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -614,7 +614,10 @@ extension type const TopicName(String _value) { /// Different from [MessageDestination], this information comes from /// [getMessages] or [getEvents], identifying the conversation that contains a /// message. -sealed class Conversation {} +sealed class Conversation { + /// Whether [this] and [other] refer to the same Zulip conversation. + bool isSameAs(Conversation other); +} /// The conversation a stream message is in. @JsonSerializable(fieldRename: FieldRename.snake, createToJson: false) @@ -640,6 +643,13 @@ class StreamConversation extends Conversation { factory StreamConversation.fromJson(Map json) => _$StreamConversationFromJson(json); + + @override + bool isSameAs(Conversation other) { + return other is StreamConversation + && streamId == other.streamId + && topic.isSameAs(other.topic); + } } /// The conversation a DM message is in. @@ -653,6 +663,21 @@ class DmConversation extends Conversation { DmConversation({required this.allRecipientIds}) : assert(isSortedWithoutDuplicates(allRecipientIds.toList())); + + 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; + } + + @override + bool isSameAs(Conversation other) { + if (other is! DmConversation) return false; + return _equalIdSequences(allRecipientIds, other.allRecipientIds); + } } /// A message or message-like object, for showing in a message list. diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 58a0e1bb95..97dc07c77b 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -351,26 +351,7 @@ mixin _MessageSequence { @visibleForTesting bool haveSameRecipient(Message prevMessage, Message message) { - if (prevMessage is StreamMessage && message is StreamMessage) { - if (prevMessage.streamId != message.streamId) return false; - if (prevMessage.topic.canonicalize() != message.topic.canonicalize()) return false; - } else if (prevMessage is DmMessage && message is DmMessage) { - if (!_equalIdSequences(prevMessage.allRecipientIds, message.allRecipientIds)) { - return false; - } - } else { - return false; - } - return true; - - // switch ((prevMessage, message)) { - // case (StreamMessage(), StreamMessage()): - // // TODO(dart-3): this doesn't type-narrow prevMessage and message - // case (DmMessage(), DmMessage()): - // // … - // default: - // return false; - // } + return prevMessage.conversation.isSameAs(message.conversation); } @visibleForTesting @@ -382,16 +363,6 @@ bool messagesSameDay(Message prevMessage, Message message) { 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; From 7a47731b0a6844a88485d876ee76d04348975472 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 4 Apr 2025 17:37:46 -0400 Subject: [PATCH 4/6] msglist [nfc]: Extract MessageListMessageBaseItem This is NFC because MessageListMessageItem is still the only subclass of it. --- lib/model/message_list.dart | 46 ++++++++++++++++++++----------- test/model/message_list_test.dart | 8 ++++-- 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 97dc07c77b..2e4c4fb600 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -35,18 +35,31 @@ class MessageListDateSeparatorItem extends MessageListItem { MessageListDateSeparatorItem(this.message); } -/// A message to show in the message list. -class MessageListMessageItem extends MessageListItem { - final Message message; - ZulipMessageContent content; +/// A [MessageBase] to show in the message list. +sealed class MessageListMessageBaseItem extends MessageListItem { + MessageBase get message; + ZulipMessageContent get content; bool showSender; bool isLastInBlock; + MessageListMessageBaseItem({ + required this.showSender, + required this.isLastInBlock, + }); +} + +/// A [Message] to show in the message list. +class MessageListMessageItem extends MessageListMessageBaseItem { + @override + final Message message; + @override + ZulipMessageContent content; + MessageListMessageItem( this.message, this.content, { - required this.showSender, - required this.isLastInBlock, + required super.showSender, + required super.isLastInBlock, }); } @@ -350,12 +363,12 @@ mixin _MessageSequence { } @visibleForTesting -bool haveSameRecipient(Message prevMessage, Message message) { +bool haveSameRecipient(MessageBase prevMessage, MessageBase message) { return prevMessage.conversation.isSameAs(message.conversation); } @visibleForTesting -bool messagesSameDay(Message prevMessage, Message message) { +bool messagesSameDay(MessageBase prevMessage, MessageBase message) { // 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); @@ -410,19 +423,20 @@ class MessageListView with ChangeNotifier, _MessageSequence { /// one way or another. /// /// See also [_allMessagesVisible]. - bool _messageVisible(Message message) { + bool _messageVisible(MessageBase message) { switch (narrow) { case CombinedFeedNarrow(): - return switch (message) { - StreamMessage() => - store.isTopicVisible(message.streamId, message.topic), - DmMessage() => true, + return switch (message.conversation) { + StreamConversation(:final streamId, :final topic) => + store.isTopicVisible(streamId, topic), + DmConversation() => true, }; case ChannelNarrow(:final streamId): - assert(message is StreamMessage && message.streamId == streamId); - if (message is! StreamMessage) return false; - return store.isTopicVisibleInStream(streamId, message.topic); + assert(message is MessageBase + && message.conversation.streamId == streamId); + if (message is! MessageBase) return false; + return store.isTopicVisibleInStream(streamId, message.conversation.topic); case TopicNarrow(): case DmNarrow(): diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index f92bdfc096..04bf5c3bde 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -2001,13 +2001,17 @@ extension MessageListDateSeparatorItemChecks on Subject get message => has((x) => x.message, 'message'); } -extension MessageListMessageItemChecks on Subject { - Subject get message => has((x) => x.message, 'message'); +extension MessageListMessageBaseItemChecks on Subject { + Subject get message => has((x) => x.message, 'message'); Subject get content => has((x) => x.content, 'content'); Subject get showSender => has((x) => x.showSender, 'showSender'); Subject get isLastInBlock => has((x) => x.isLastInBlock, 'isLastInBlock'); } +extension MessageListMessageItemChecks on Subject { + Subject get message => has((x) => x.message, 'message'); +} + extension MessageListViewChecks on Subject { Subject get store => has((x) => x.store, 'store'); Subject get narrow => has((x) => x.narrow, 'narrow'); From 88fba8314b7ba0edfd6a8ac0bc718934f0cb331a Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 7 Apr 2025 15:37:23 -0400 Subject: [PATCH 5/6] msglist [nfc]: Let MessageItem take a MessageListMessageBaseItem This will make MessageItem compatible with other future subclasses of MessageListMessageBaseItem, in particular MessageListOutboxMessageItem, which do not need unread markers. --- lib/widgets/message_list.dart | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 4605820c07..ae7c51b67c 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -992,25 +992,32 @@ class MessageItem extends StatelessWidget { this.trailingWhitespace, }); - final MessageListMessageItem item; + final MessageListMessageBaseItem item; final Widget header; final double? trailingWhitespace; @override Widget build(BuildContext context) { - final message = item.message; final messageListTheme = MessageListTheme.of(context); + + final item = this.item; + Widget child = ColoredBox( + color: messageListTheme.bgMessageRegular, + child: Column(children: [ + switch (item) { + MessageListMessageItem() => MessageWithPossibleSender(item: item), + }, + if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!), + ])); + if (item case MessageListMessageItem(:final message)) { + child = _UnreadMarker( + isRead: message.flags.contains(MessageFlag.read), + child: child); + } return StickyHeaderItem( allowOverflow: !item.isLastInBlock, header: header, - child: _UnreadMarker( - isRead: message.flags.contains(MessageFlag.read), - child: ColoredBox( - color: messageListTheme.bgMessageRegular, - child: Column(children: [ - MessageWithPossibleSender(item: item), - if (trailingWhitespace != null && item.isLastInBlock) SizedBox(height: trailingWhitespace!), - ])))); + child: child); } } From 1e8f2c257286fab7d9778d589179ce069d3d0c46 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 8 Apr 2025 16:15:23 -0400 Subject: [PATCH 6/6] msglist test [nfc]: Add MessageEvent test group --- test/model/message_list_test.dart | 56 ++++++++++++++++--------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 04bf5c3bde..4aedea1d03 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -360,37 +360,39 @@ void main() { }); }); - test('MessageEvent', () async { - final stream = eg.stream(); - await prepare(narrow: ChannelNarrow(stream.streamId)); - await prepareMessages(foundOldest: true, messages: - List.generate(30, (i) => eg.streamMessage(stream: stream))); + group('MessageEvent', () { + test('in narrow', () async { + final stream = eg.stream(); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream))); - check(model).messages.length.equals(30); - await store.addMessage(eg.streamMessage(stream: stream)); - checkNotifiedOnce(); - check(model).messages.length.equals(31); - }); + check(model).messages.length.equals(30); + await store.addMessage(eg.streamMessage(stream: stream)); + checkNotifiedOnce(); + check(model).messages.length.equals(31); + }); - test('MessageEvent, not in narrow', () async { - final stream = eg.stream(); - await prepare(narrow: ChannelNarrow(stream.streamId)); - await prepareMessages(foundOldest: true, messages: - List.generate(30, (i) => eg.streamMessage(stream: stream))); + test('not in narrow', () async { + final stream = eg.stream(); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await prepareMessages(foundOldest: true, messages: + List.generate(30, (i) => eg.streamMessage(stream: stream))); - check(model).messages.length.equals(30); - final otherStream = eg.stream(); - await store.addMessage(eg.streamMessage(stream: otherStream)); - checkNotNotified(); - check(model).messages.length.equals(30); - }); + check(model).messages.length.equals(30); + final otherStream = eg.stream(); + await store.addMessage(eg.streamMessage(stream: otherStream)); + checkNotNotified(); + check(model).messages.length.equals(30); + }); - test('MessageEvent, before fetch', () async { - final stream = eg.stream(); - await prepare(narrow: ChannelNarrow(stream.streamId)); - await store.addMessage(eg.streamMessage(stream: stream)); - checkNotNotified(); - check(model).fetched.isFalse(); + test('before fetch', () async { + final stream = eg.stream(); + await prepare(narrow: ChannelNarrow(stream.streamId)); + await store.addMessage(eg.streamMessage(stream: stream)); + checkNotNotified(); + check(model).fetched.isFalse(); + }); }); group('UserTopicEvent', () {