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

PIG208 · 2025-04-17T19:35:15Z

This is a prep PR for #1453, toward #1441.

chrisbobbe

Thanks, this looks good! Small comments below.

chrisbobbe · 2025-04-21T20:52:19Z

lib/widgets/message_list.dart


-  final MessageListMessageItem item;
+class _SenderRow extends StatelessWidget {
+  const _SenderRow({required this.message, required this.showTimestamp});


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

chrisbobbe · 2025-04-21T20:53:19Z

lib/model/message_list.dart

@@ -36,17 +36,29 @@ class MessageListDateSeparatorItem extends MessageListItem {
 }

 /// A message to show in the message list.
-class MessageListMessageItem extends MessageListItem {


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"

chrisbobbe · 2025-04-21T20:59:03Z

lib/model/message_list.dart

+  final conversation = message.conversation;
+  if (prevConversation is StreamConversation && conversation is StreamConversation) {
+    if (prevConversation.streamId != conversation.streamId) return false;
+    if (prevConversation.topic.canonicalize() != conversation.topic.canonicalize()) return false;


It looks like the existing code could have been using TopicName.isSameAs; switching to that method could be a small independent improvement.

In fact, I wonder if Conversation could declare an isSameAs method, for its subclasses to implement.

Good idea! I think we can go further and also pull out MessageBase.inSameConversationAs to get rid of haveSameRecipient altogether, moving its tests to test/api/model/model_test.dart.

One worry though, is that haveSameRecipient's meaning is quite local to the message list. We wouldn't use Conversation.isSameAs or MessageBase.inSameConversationAs elsewhere in the app, at least right now.

PIG208 · 2025-04-22T22:31:03Z

Thanks for the review! Updated the PR.

chrisbobbe

Thanks! LGTM; marking for Greg's review.

chrisbobbe · 2025-04-23T23:04:47Z

lib/api/model/model.dart

@@ -596,7 +596,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 {


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'

gnprice

Thanks both!

Generally these refactors look fine. A couple of them I'm not totally convinced of, so the motivation may become clearer with the PR that uses them. Otherwise, just small comments below.

gnprice · 2025-04-25T01:25:48Z

lib/model/message_list.dart

+  /// Returns whether an item has been appended or not.
+  ///
+  /// The caller must append a [MessageListMessageBaseItem] after this.
+  bool _maybeAppendAuxillaryItem(MessageBase message, {


nit: spelling, "auxiliary"

Suggested change

bool _maybeAppendAuxillaryItem(MessageBase message, {

bool _maybeAppendAuxiliaryItem(MessageBase message, {

gnprice · 2025-04-25T01:36:28Z

lib/widgets/message_list.dart

-          if (senderRow != null)
-            Padding(padding: const EdgeInsets.fromLTRB(16, 2, 16, 0),
-              child: senderRow),
+          if (item.showSender) _SenderRow(message: message, showTimestamp: true),


nit: this is a major subcomponent, so good to give it a bit more visual prominence:

Suggested change

if (item.showSender) _SenderRow(message: message, showTimestamp: true),

if (item.showSender)

_SenderRow(message: message, showTimestamp: true),

gnprice · 2025-04-25T01:48:36Z

lib/widgets/message_list.dart

+                height: (18 / 16),
+                fontFeatures: const [FontFeature.enable('c2sc'), FontFeature.enable('smcp')],
+              ).merge(weightVariableTextStyle(context))),
+          ]


gnprice · 2025-04-25T04:01:29Z

lib/api/model/model.dart

+    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;


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.

gnprice · 2025-04-25T04:04:16Z

lib/api/model/model.dart

+  /// Whether [this] and [other] have the same canonical form.
+  bool isSameAs(Conversation other);


Suggested change

/// Whether [this] and [other] have the same canonical form.

bool isSameAs(Conversation other);

/// Whether [this] and [other] refer to the same Zulip conversation.

bool isSameAs(Conversation other);

I'd think of a "canonical form" as a strategy for implementing this sort of method, and the intended semantics as something a bit more abstract.

gnprice · 2025-04-25T04:05:50Z

lib/model/message_list.dart

@@ -36,17 +36,29 @@ class MessageListDateSeparatorItem extends MessageListItem {
 }

 /// A message to show in the message list.


doc should get updated

gnprice · 2025-04-25T04:09:21Z

lib/widgets/message_list.dart

-  final MessageListMessageItem item;
+  final MessageListMessageBaseItem item;


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.

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.

Also removed a stale comment that refers to resolved issues (zulip#173 and zulip#175). We will reuse this helper when handling outbox messages.

This is for the upcoming local-echo feature, to hide the timestamp. There isn't a Figma design for messages without a timestamp.

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.

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

This will make MessageItem compatible with other future subclasses of MessageListMessageBaseItem, in particular MessageListOutboxMessageItem, which do not need unread markers.

PIG208 · 2025-04-25T17:26:11Z

Thanks for the review! Updated the PR.

PIG208 assigned chrisbobbe Apr 17, 2025

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Apr 17, 2025

chrisbobbe reviewed Apr 22, 2025

View reviewed changes

PIG208 force-pushed the pr-echo-6 branch from 8eb031a to aae5a01 Compare April 22, 2025 22:29

chrisbobbe approved these changes Apr 23, 2025

View reviewed changes

chrisbobbe assigned gnprice and unassigned chrisbobbe Apr 23, 2025

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Apr 23, 2025

chrisbobbe requested a review from gnprice April 23, 2025 23:15

PIG208 force-pushed the pr-echo-6 branch from aae5a01 to 81242c6 Compare April 24, 2025 00:58

gnprice reviewed Apr 25, 2025

View reviewed changes

PIG208 added 7 commits April 25, 2025 13:12

msglist [nfc]: Extract _maybeAppendAuxiliaryItem

7489c08

Also removed a stale comment that refers to resolved issues (zulip#173 and zulip#175). We will reuse this helper when handling outbox messages.

msglist [nfc]: Extract _SenderRow widget

8947d60

msglist [nfc]: Add showTimestamp to _SenderRow widget

e7ff388

This is for the upcoming local-echo feature, to hide the timestamp. There isn't a Figma design for messages without a timestamp.

msglist [nfc]: Extract MessageListMessageBaseItem

46fcfc4

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

msglist [nfc]: Let MessageItem take a MessageListMessageBaseItem

04a3224

This will make MessageItem compatible with other future subclasses of MessageListMessageBaseItem, in particular MessageListOutboxMessageItem, which do not need unread markers.

msglist test [nfc]: Add MessageEvent test group

429a1fb

PIG208 force-pushed the pr-echo-6 branch from 81242c6 to 429a1fb Compare April 25, 2025 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

PIG208 commented Apr 17, 2025 •

edited

Loading

chrisbobbe left a comment

chrisbobbe Apr 21, 2025

chrisbobbe Apr 21, 2025

chrisbobbe Apr 21, 2025

chrisbobbe Apr 21, 2025

PIG208 Apr 22, 2025 •

edited

Loading

PIG208 commented Apr 22, 2025

chrisbobbe left a comment

chrisbobbe Apr 23, 2025

gnprice left a comment

gnprice Apr 25, 2025

gnprice Apr 25, 2025

gnprice Apr 25, 2025

gnprice Apr 25, 2025

gnprice Apr 25, 2025

gnprice Apr 25, 2025

gnprice Apr 25, 2025

gnprice Apr 25, 2025

PIG208 commented Apr 25, 2025

	bool _maybeAppendAuxillaryItem(MessageBase message, {
	bool _maybeAppendAuxiliaryItem(MessageBase message, {

	if (item.showSender) _SenderRow(message: message, showTimestamp: true),
	if (item.showSender)
	_SenderRow(message: message, showTimestamp: true),

		/// Whether [this] and [other] have the same canonical form.
		bool isSameAs(Conversation other);

		@@ -36,17 +36,29 @@ class MessageListDateSeparatorItem extends MessageListItem {
		}

		/// A message to show in the message list.

		final MessageListMessageItem item;
		final MessageListMessageBaseItem item;

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

Are you sure you want to change the base?

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

Conversation

PIG208 commented Apr 17, 2025 • edited Loading

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Apr 22, 2025 • edited Loading

Choose a reason for hiding this comment

PIG208 commented Apr 22, 2025

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Apr 25, 2025

PIG208 commented Apr 17, 2025 •

edited

Loading

PIG208 Apr 22, 2025 •

edited

Loading