Skip to content

msglist: Show sender only when needed #303

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 4 commits into from
Sep 12, 2023
Merged

msglist: Show sender only when needed #303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c8cf27
msglist [nfc]: Have MessageWithSender take the whole message-item object
gnprice Aug 15, 2023
c5ed648
msglist [nfc]: Rename to MessageWithPossibleSender for generality
gnprice Jul 15, 2023
b3f7aff
msglist [nfc]: Add showSender flag, always true for now
gnprice Jul 15, 2023
77ce192
msglist: Show sender only when needed
gnprice Jul 15, 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
17 changes: 14 additions & 3 deletions lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,15 @@ class MessageListRecipientHeaderItem extends MessageListItem {
class MessageListMessageItem extends MessageListItem {
final Message message;
ZulipContent content;
bool showSender;
bool isLastInBlock;

MessageListMessageItem(this.message, this.content, {required this.isLastInBlock});
MessageListMessageItem(
this.message,
this.content, {
required this.showSender,
required this.isLastInBlock,
});
}

/// Indicates the app is loading more messages at the top.
Expand Down Expand Up @@ -174,16 +180,20 @@ mixin _MessageSequence {
// and date separators #173.
final message = messages[index];
final content = contents[index];
bool canShareSender;
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;
canShareSender = (prevMessageItem.message.senderId == message.senderId);
} else {
items.add(MessageListRecipientHeaderItem(message));
canShareSender = false;
}
items.add(MessageListMessageItem(message, content, isLastInBlock: true));
items.add(MessageListMessageItem(message, content,
showSender: !canShareSender, isLastInBlock: true));
}

/// Update [items] to include markers at start and end as appropriate.
Expand Down Expand Up @@ -397,7 +407,8 @@ 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.
// NB that when handling message moves (#150), recipient headers
// may need updating, and consequently showSender too.
void maybeUpdateMessage(UpdateMessageEvent event) {
final idx = _findMessageWithId(event.messageId);
if (idx == -1) {
Expand Down
51 changes: 27 additions & 24 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import 'package:flutter/material.dart';
import 'package:intl/intl.dart';

import '../api/model/model.dart';
import '../model/content.dart';
import '../model/message_list.dart';
import '../model/narrow.dart';
import '../model/store.dart';
Expand Down Expand Up @@ -391,7 +390,7 @@ class MessageItem extends StatelessWidget {
child: Column(children: [
DecoratedBox(
decoration: borderDecoration,
child: MessageWithSender(message: message, content: item.content)),
child: MessageWithPossibleSender(item: item)),
if (trailing != null && item.isLastInBlock) trailing!,
]));

Expand Down Expand Up @@ -562,16 +561,15 @@ class RecipientHeaderChevronContainer extends StatelessWidget {
}
}

/// A Zulip message, showing the sender's name and avatar.
class MessageWithSender extends StatelessWidget {
const MessageWithSender(
{super.key, required this.message, required this.content});
/// A Zulip message, showing the sender's name and avatar if specified.
class MessageWithPossibleSender extends StatelessWidget {
const MessageWithPossibleSender({super.key, required this.item});

final Message message;
final ZulipContent content;
final MessageListMessageItem item;

@override
Widget build(BuildContext context) {
final message = item.message;
final time = _kMessageTimestampFormat
.format(DateTime.fromMillisecondsSinceEpoch(1000 * message.timestamp));

Expand All @@ -582,26 +580,31 @@ class MessageWithSender extends StatelessWidget {
child: Padding(
padding: const EdgeInsets.only(top: 2, bottom: 3, left: 8, right: 8),
child: Row(crossAxisAlignment: CrossAxisAlignment.start, children: [
Padding(
padding: const EdgeInsets.fromLTRB(3, 6, 11, 0),
child: GestureDetector(
onTap: () => Navigator.push(context,
ProfilePage.buildRoute(context: context,
userId: message.senderId)),
child: Avatar(userId: message.senderId, size: 35, borderRadius: 4))),
item.showSender
? Padding(
padding: const EdgeInsets.fromLTRB(3, 6, 11, 0),
child: GestureDetector(
onTap: () => Navigator.push(context,
ProfilePage.buildRoute(context: context,
userId: message.senderId)),
child: Avatar(size: 35, borderRadius: 4,
userId: message.senderId)))
: const SizedBox(width: 3 + 35 + 11),
Expanded(
child: Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
children: [
const SizedBox(height: 3),
GestureDetector(
onTap: () => Navigator.push(context,
ProfilePage.buildRoute(context: context,
userId: message.senderId)),
child: Text(message.senderFullName, // TODO get from user data
style: const TextStyle(fontWeight: FontWeight.bold))),
const SizedBox(height: 4),
MessageContent(message: message, content: content),
if (item.showSender) ...[
const SizedBox(height: 3),
GestureDetector(
onTap: () => Navigator.push(context,
ProfilePage.buildRoute(context: context,
userId: message.senderId)),
child: Text(message.senderFullName, // TODO get from user data
style: const TextStyle(fontWeight: FontWeight.bold))),
const SizedBox(height: 4),
],
MessageContent(message: message, content: item.content),
])),
Container(
width: 80,
Expand Down
42 changes: 42 additions & 0 deletions test/model/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,43 @@ void main() async {
checkNotifiedOnce();
});

test('showSender is maintained correctly', () async {
// TODO(#150): This will get more complicated with message moves.
// Until then, we always compute this sequentially from oldest to newest.
// So we just need to exercise the different cases of the logic for
// whether the sender should be shown, but the difference between
// fetchInitial and maybeAddMessage etc. doesn't matter.

const timestamp = 1693602618;
final stream = eg.stream();
Message streamMessage(int id, User sender) =>
eg.streamMessage(id: id, sender: sender,
stream: stream, topic: 'foo', timestamp: timestamp);
Message dmMessage(int id, User sender) =>
eg.dmMessage(id: id, from: sender, timestamp: timestamp,
to: [sender.userId == eg.selfUser.userId ? eg.otherUser : eg.selfUser]);

prepare();
await prepareMessages(foundOldest: true, messages: [
streamMessage(1, eg.selfUser), // first message, so show sender
streamMessage(2, eg.selfUser), // hide sender
streamMessage(3, eg.otherUser), // no recipient header, but new sender
dmMessage(4, eg.otherUser), // same sender, but recipient header
]);

// We check showSender has the right values in [checkInvariants],
// but to make this test explicit:
check(model.items).deepEquals([
it()..isA<MessageListHistoryStartItem>(),
it()..isA<MessageListRecipientHeaderItem>(),
it()..isA<MessageListMessageItem>().showSender.isTrue(),
it()..isA<MessageListMessageItem>().showSender.isFalse(),
it()..isA<MessageListMessageItem>().showSender.isTrue(),
it()..isA<MessageListRecipientHeaderItem>(),
it()..isA<MessageListMessageItem>().showSender.isTrue(),
]);
});

group('canShareRecipientHeader', () {
test('stream messages vs DMs, no share', () {
final dmMessage = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]);
Expand Down Expand Up @@ -669,14 +706,18 @@ void checkInvariants(MessageListView model) {
check(model.items[i++]).isA<MessageListLoadingItem>();
}
for (int j = 0; j < model.messages.length; j++) {
bool isFirstInBlock = false;
if (j == 0
|| !canShareRecipientHeader(model.messages[j-1], model.messages[j])) {
check(model.items[i++]).isA<MessageListRecipientHeaderItem>()
.message.identicalTo(model.messages[j]);
isFirstInBlock = true;
}
check(model.items[i++]).isA<MessageListMessageItem>()
..message.identicalTo(model.messages[j])
..content.identicalTo(model.contents[j])
..showSender.equals(
isFirstInBlock || model.messages[j].senderId != model.messages[j-1].senderId)
..isLastInBlock.equals(
i == model.items.length || model.items[i] is! MessageListMessageItem);
}
Expand All @@ -690,6 +731,7 @@ extension MessageListRecipientHeaderItemChecks on Subject<MessageListRecipientHe
extension MessageListMessageItemChecks on Subject<MessageListMessageItem> {
Subject<Message> get message => has((x) => x.message, 'message');
Subject<ZulipContent> get content => has((x) => x.content, 'content');
Subject<bool> get showSender => has((x) => x.showSender, 'showSender');
Subject<bool> get isLastInBlock => has((x) => x.isLastInBlock, 'isLastInBlock');
}

Expand Down
10 changes: 5 additions & 5 deletions test/widgets/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ 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(101);
messages: List.generate(200, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser)));
check(itemCount(tester)).equals(201);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, interesting: I guess when there are a lot of consecutive short messages with the same sender and recipient, we now save a lot of vertical space, so this test needs more messages to cover as much vertical space as it wants to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, exactly. Without this change, these messages take up so little vertical space that the message list promptly decides it needs to fetch more older messages, disrupting the sequence this test was going for.


// Fling-scroll upward...
await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000);
Expand All @@ -83,7 +83,7 @@ void main() {
await tester.pump(Duration.zero); // Allow a frame for the response to arrive.

// Now we have more messages.
check(itemCount(tester)).equals(201);
check(itemCount(tester)).equals(301);
});

testWidgets('observe double-fetch glitch', (tester) async {
Expand Down Expand Up @@ -254,7 +254,7 @@ void main() {
});
});

group('MessageWithSender', () {
group('MessageWithPossibleSender', () {
testWidgets('Updates avatar on RealmUserUpdateEvent', (tester) async {
addTearDown(testBinding.reset);

Expand All @@ -263,7 +263,7 @@ void main() {
RealmContentNetworkImage? findAvatarImageWidget(WidgetTester tester) {
return tester.widgetList<RealmContentNetworkImage>(
find.descendant(
of: find.byType(MessageWithSender),
of: find.byType(MessageWithPossibleSender),
matching: find.byType(RealmContentNetworkImage))).firstOrNull;
}

Expand Down