diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index f9f65e0aab..9044c06b12 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -25,6 +25,12 @@ class MessageListRecipientHeaderItem extends MessageListItem { MessageListRecipientHeaderItem(this.message); } +class MessageListDateSeparatorItem extends MessageListItem { + final Message message; + + MessageListDateSeparatorItem(this.message); +} + /// A message to show in the message list. class MessageListMessageItem extends MessageListItem { final Message message; @@ -118,6 +124,7 @@ mixin _MessageSequence { case MessageListDirection.older: return -1; } case MessageListRecipientHeaderItem(:var message): + case MessageListDateSeparatorItem(:var message): return (message.id <= messageId) ? -1 : 1; case MessageListMessageItem(:var message): return message.id.compareTo(messageId); } @@ -181,16 +188,19 @@ mixin _MessageSequence { final message = messages[index]; final content = contents[index]; bool canShareSender; - if (index > 0 && canShareRecipientHeader(messages[index - 1], message)) { + if (index == 0 || !haveSameRecipient(messages[index - 1], message)) { + items.add(MessageListRecipientHeaderItem(message)); + canShareSender = false; + } else if (!messagesSameDay(messages[index - 1], message)) { + items.add(MessageListDateSeparatorItem(message)); + canShareSender = false; + } else { assert(items.last is MessageListMessageItem); final prevMessageItem = items.last as MessageListMessageItem; assert(identical(prevMessageItem.message, messages[index - 1])); assert(prevMessageItem.isLastInBlock); prevMessageItem.isLastInBlock = false; canShareSender = (prevMessageItem.message.senderId == message.senderId); - } else { - items.add(MessageListRecipientHeaderItem(message)); - canShareSender = false; } items.add(MessageListMessageItem(message, content, showSender: !canShareSender, isLastInBlock: true)); @@ -228,7 +238,7 @@ mixin _MessageSequence { } @visibleForTesting -bool canShareRecipientHeader(Message prevMessage, Message message) { +bool haveSameRecipient(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; @@ -239,6 +249,7 @@ bool canShareRecipientHeader(Message prevMessage, Message message) { } else { return false; } + return true; // switch ((prevMessage, message)) { // case (StreamMessage(), StreamMessage()): @@ -248,12 +259,14 @@ bool canShareRecipientHeader(Message prevMessage, Message message) { // default: // return false; // } +} +@visibleForTesting +bool messagesSameDay(Message prevMessage, Message 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); if (!_sameDay(prevTime, time)) return false; - return true; } diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 6a644d2493..090465f14a 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -344,6 +344,11 @@ class _MessageListState extends State with PerAccountStoreAwareStat final header = RecipientHeader(message: data.message, narrow: widget.narrow); return StickyHeaderItem(allowOverflow: true, header: header, child: header); + case MessageListDateSeparatorItem(): + final header = RecipientHeader(message: data.message, narrow: widget.narrow); + return StickyHeaderItem(allowOverflow: true, + header: header, + child: DateSeparator(message: data.message)); case MessageListMessageItem(): final header = RecipientHeader(message: data.message, narrow: widget.narrow); return MessageItem( @@ -463,6 +468,50 @@ class RecipientHeader extends StatelessWidget { } } +class DateSeparator extends StatelessWidget { + const DateSeparator({super.key, required this.message}); + + final Message message; + + // This color matches recipient headers. TODO(design) is that what we want? + static final _textColor = Colors.black.withOpacity(0.4); + + @override + Widget build(BuildContext context) { + // This makes the small-caps text vertically centered, + // to align with the vertically centered divider lines. + const textBottomPadding = 2.0; + + return ColoredBox(color: Colors.white, + child: Padding( + padding: const EdgeInsets.symmetric(vertical: 8, horizontal: 2), + child: Row(children: [ + const Expanded( + child: SizedBox(height: 0, + child: DecoratedBox( + decoration: BoxDecoration( + border: Border( + bottom: BorderSide( + width: 0, + color: Colors.black)))))), + Padding(padding: const EdgeInsets.fromLTRB(2, 0, 2, textBottomPadding), + child: DateText( + color: _textColor, + fontSize: 16, + height: (16 / 16), + timestamp: message.timestamp)), + const SizedBox(height: 0, width: 12, + child: DecoratedBox( + decoration: BoxDecoration( + border: Border( + bottom: BorderSide( + width: 0, + color: Colors.black))))) + ])), + ); + } +} + class MessageItem extends StatelessWidget { const MessageItem({ super.key, @@ -708,26 +757,50 @@ class RecipientHeaderDate extends StatelessWidget { @override Widget build(BuildContext context) { - final zulipLocalizations = ZulipLocalizations.of(context); return Padding( padding: const EdgeInsets.fromLTRB(10, 0, 16, 0), - child: Text( - style: TextStyle( - color: color, - fontFamily: 'Source Sans 3', - fontSize: 16, - // In Figma this has a line-height of 19, but using 18 - // here to match the stream/topic text widgets helps - // to align all the text to the same baseline. - height: (18 / 16), - // This is equivalent to css `all-small-caps`, see: - // https://developer.mozilla.org/en-US/docs/Web/CSS/font-variant-caps#all-small-caps - fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')], - ).merge(weightVariableTextStyle(context)), - formatHeaderDate( - zulipLocalizations, - DateTime.fromMillisecondsSinceEpoch(message.timestamp * 1000), - now: DateTime.now()))); + child: DateText( + color: color, + fontSize: 16, + // In Figma this has a line-height of 19, but using 18 + // here to match the stream/topic text widgets helps + // to align all the text to the same baseline. + height: (18 / 16), + timestamp: message.timestamp)); + } +} + +class DateText extends StatelessWidget { + const DateText({ + super.key, + required this.color, + required this.fontSize, + required this.height, + required this.timestamp, + }); + + final Color color; + final double fontSize; + final double height; + final int timestamp; + + @override + Widget build(BuildContext context) { + final zulipLocalizations = ZulipLocalizations.of(context); + return Text( + style: TextStyle( + color: color, + fontFamily: 'Source Sans 3', + fontSize: fontSize, + height: height, + // This is equivalent to css `all-small-caps`, see: + // https://developer.mozilla.org/en-US/docs/Web/CSS/font-variant-caps#all-small-caps + fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')], + ).merge(weightVariableTextStyle(context)), + formatHeaderDate( + zulipLocalizations, + DateTime.fromMillisecondsSinceEpoch(timestamp * 1000), + now: DateTime.now())); } } diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 4da42e3ca1..c206debb78 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -810,38 +810,38 @@ void main() async { ]); }); - group('canShareRecipientHeader', () { - test('stream messages vs DMs, no share', () { + group('haveSameRecipient', () { + test('stream messages vs DMs, no match', () { 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(); + final streamMessage = eg.streamMessage(); + check(haveSameRecipient(streamMessage, dmMessage)).isFalse(); + check(haveSameRecipient(dmMessage, streamMessage)).isFalse(); }); - test('stream messages of same day share just if same stream/topic', () { + test('stream messages match 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(); + final messageXB = eg.streamMessage(stream: stream1, topic: 'foo'); + final messageAX = eg.streamMessage(stream: stream0, topic: 'bar'); + check(haveSameRecipient(messageAB, messageAB)).isTrue(); + check(haveSameRecipient(messageAB, messageXB)).isFalse(); + check(haveSameRecipient(messageXB, messageAB)).isFalse(); + check(haveSameRecipient(messageAB, messageAX)).isFalse(); + check(haveSameRecipient(messageAX, messageAB)).isFalse(); + check(haveSameRecipient(messageAX, messageXB)).isFalse(); + check(haveSameRecipient(messageXB, messageAX)).isFalse(); }); - test('DMs of same day share just if same recipients', () { + test('DMs match 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 message01 = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]); + final message10 = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); + final message02 = eg.dmMessage(from: eg.selfUser, to: [eg.thirdUser]); + final message20 = eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser]); + final message012 = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser, eg.thirdUser]); + final message102 = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser, eg.thirdUser]); + final message201 = eg.dmMessage(from: eg.thirdUser, to: [eg.selfUser, eg.otherUser]); final groups = [[message0], [message01, message10], [message02, message20], [message012, message102, message201]]; for (int i0 = 0; i0 < groups.length; i0++) { @@ -852,52 +852,52 @@ void main() async { final message1 = groups[i1][j1]; check( because: 'recipients ${message0.allRecipientIds} vs ${message1.allRecipientIds}', - canShareRecipientHeader(message0, message1), + haveSameRecipient(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); - } + test('messagesSameDay', () { + // 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', messagesSameDay( + 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', messagesSameDay( + eg.dmMessage(from: eg.selfUser, to: [], timestamp: timestampFromLocalTime(time0)), + eg.dmMessage(from: eg.selfUser, to: [], timestamp: timestampFromLocalTime(time1)), + )).equals(i0 == i1); } } } - }); + } }); } @@ -948,10 +948,13 @@ void checkInvariants(MessageListView model) { for (int j = 0; j < model.messages.length; j++) { bool isFirstInBlock = false; if (j == 0 - || !canShareRecipientHeader(model.messages[j-1], model.messages[j])) { + || !haveSameRecipient(model.messages[j-1], model.messages[j])) { check(model.items[i++]).isA() .message.identicalTo(model.messages[j]); isFirstInBlock = true; + } else if (!messagesSameDay(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]) @@ -968,6 +971,10 @@ extension MessageListRecipientHeaderItemChecks on Subject get message => has((x) => x.message, 'message'); } +extension MessageListDateSeparatorItemChecks 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');