Skip to content

local echo (6/n): Prepare message list for outbox message support #1475

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 6 commits into from
May 7, 2025
Merged

local echo (6/n): Prepare message list for outbox message support #1475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9e2d9e5
msglist [nfc]: Extract _SenderRow widget
PIG208 Apr 22, 2025
2d7d83d
msglist [nfc]: Add showTimestamp to _SenderRow widget
PIG208 Mar 27, 2025
fb6b53b
api [nfc]: Extract Conversation.isSameAs
PIG208 Apr 22, 2025
7a47731
msglist [nfc]: Extract MessageListMessageBaseItem
PIG208 Apr 4, 2025
88fba83
msglist [nfc]: Let MessageItem take a MessageListMessageBaseItem
PIG208 Apr 7, 2025
1e8f2c2
msglist test [nfc]: Add MessageEvent test group
PIG208 Apr 8, 2025
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
27 changes: 26 additions & 1 deletion lib/api/model/model.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 then it is to the rest of the app.

Commit-message nit: 'than it is'

/// 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)
Expand All @@ -640,6 +643,13 @@ class StreamConversation extends Conversation {

factory StreamConversation.fromJson(Map<String, dynamic> 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.
Expand All @@ -653,6 +663,21 @@ class DmConversation extends Conversation {

DmConversation({required this.allRecipientIds})
: assert(isSortedWithoutDuplicates(allRecipientIds.toList()));

bool _equalIdSequences(Iterable<int> xs, Iterable<int> 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;
Comment on lines +668 to +673
Copy link
Member

Choose a reason for hiding this comment

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

This is best kept as its own method with its own name. It's written to a low-level API, but has a meaning that can be described very crisply and abstractly.

}

@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.
Expand Down
77 changes: 31 additions & 46 deletions lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,31 @@ class MessageListDateSeparatorItem extends MessageListItem {
MessageListDateSeparatorItem(this.message);
}

/// A message to show in the message list.
class MessageListMessageItem extends MessageListItem {
Copy link
Collaborator

Choose a reason for hiding this comment

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

msglist [nfc]: Extract MessageListMessageBaseItem

This is an NFC because MessageListMessageItem is still the only
subclass of it.

Commit-message nit: "NFC" or "an NFC change"

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,
});
}

Expand Down Expand Up @@ -350,48 +363,19 @@ 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;
// }
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);
if (!_sameDay(prevTime, time)) return false;
return true;
}

// Intended for [Message.allRecipientIds]. Assumes efficient `length`.
bool _equalIdSequences(Iterable<int> xs, Iterable<int> 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;
Expand Down Expand Up @@ -439,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<StreamConversation>
&& message.conversation.streamId == streamId);
if (message is! MessageBase<StreamConversation>) return false;
return store.isTopicVisibleInStream(streamId, message.conversation.topic);

case TopicNarrow():
case DmNarrow():
Expand Down
98 changes: 58 additions & 40 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,25 +992,32 @@ class MessageItem extends StatelessWidget {
this.trailingWhitespace,
});

final MessageListMessageItem item;
final MessageListMessageBaseItem item;
Copy link
Member

Choose a reason for hiding this comment

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

nit:

msglist [nfc]: Handle MessageListMessageBaseItem with MessageItem

Can expand slightly to clarify:

msglist [nfc]: Handle MessageListMessageBaseItem with MessageItem widget

Otherwise, when looking at the commit message before rereading the code, these two classes sound like the same kind of thing, and then their relationship is confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Or I think better yet:

msglist [nfc]: Let MessageItem widget take a MessageListMessageBaseItem

That basically specifies that we're talking about a field on the widget, vs. the wide range of other things that "handle" could mean.

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);
}
}

Expand Down Expand Up @@ -1338,29 +1345,27 @@ 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, required this.showTimestamp});
Copy link
Collaborator

Choose a reason for hiding this comment

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

msglist [nfc]: Extract _SenderRow widget

How about splitting this commit into two:

  • one for minimally extracting the new helper widget
  • one that adds showTimestamp, noting in the commit message that it's for the upcoming local-echo feature, and that the false case doesn't have a Figma design


final Message message;
final bool showTimestamp;

@override
Widget build(BuildContext context) {
final store = PerAccountStoreWidget.of(context);
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),
Expand Down Expand Up @@ -1392,16 +1397,33 @@ class MessageWithPossibleSender 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))),
],
]));
}
}

/// 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;
Expand Down Expand Up @@ -1430,9 +1452,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, showTimestamp: true),
Row(
crossAxisAlignment: CrossAxisAlignment.baseline,
textBaseline: localizedTextBaseline(context),
Expand Down Expand Up @@ -1460,6 +1481,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');
Loading