Skip to content

Add date separators #469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 5, 2024
Merged

Add date separators #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f0f27c
msglist [nfc]: Factor a DateText widget out from RecipientHeaderDate
gnprice Jan 4, 2024
69767fb
msglist [nfc]: Split haveSameRecipient from messagesSameDay
gnprice Jan 4, 2024
fe4655f
msglist: Show date separators
gnprice Dec 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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;
Expand All @@ -239,6 +249,7 @@ bool canShareRecipientHeader(Message prevMessage, Message message) {
} else {
return false;
}
return true;

// switch ((prevMessage, message)) {
// case (StreamMessage(), StreamMessage()):
Expand All @@ -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;
}

Expand Down
109 changes: 91 additions & 18 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,11 @@ class _MessageListState extends State<MessageList> 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(
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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()));
}
}

Expand Down
127 changes: 67 additions & 60 deletions test/model/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Expand All @@ -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);
}
}
}
});
}
});
}

Expand Down Expand Up @@ -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<MessageListRecipientHeaderItem>()
.message.identicalTo(model.messages[j]);
isFirstInBlock = true;
} else if (!messagesSameDay(model.messages[j-1], model.messages[j])) {
check(model.items[i++]).isA<MessageListDateSeparatorItem>()
.message.identicalTo(model.messages[j]);
}
check(model.items[i++]).isA<MessageListMessageItem>()
..message.identicalTo(model.messages[j])
Expand All @@ -968,6 +971,10 @@ extension MessageListRecipientHeaderItemChecks on Subject<MessageListRecipientHe
Subject<Message> get message => has((x) => x.message, 'message');
}

extension MessageListDateSeparatorItemChecks on Subject<MessageListDateSeparatorItem> {
Subject<Message> get message => has((x) => x.message, 'message');
}

extension MessageListMessageItemChecks on Subject<MessageListMessageItem> {
Subject<Message> get message => has((x) => x.message, 'message');
Subject<ZulipContent> get content => has((x) => x.content, 'content');
Expand Down