From 491de2eb89cd9acc1cca9bcf6bad6136ac79951a Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 1 Apr 2025 17:55:04 -0400 Subject: [PATCH 1/8] api [nfc]: Make Message.fromJson a plain static method Message will become generic later, which does not support generic factory methods. We will add a comment explaining it then. --- lib/api/model/events.dart | 2 +- lib/api/model/model.dart | 2 +- lib/api/route/messages.dart | 8 ++++++++ lib/api/route/messages.g.dart | 5 +---- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/api/model/events.dart b/lib/api/model/events.dart index df15295894..0479b0428f 100644 --- a/lib/api/model/events.dart +++ b/lib/api/model/events.dart @@ -679,7 +679,7 @@ class MessageEvent extends Event { // events and in the get-messages results is that `matchContent` and // `matchTopic` are absent here. Already [Message.matchContent] and // [Message.matchTopic] are optional, so no action is needed on that. - @JsonKey(readValue: _readMessageValue, includeToJson: false) + @JsonKey(readValue: _readMessageValue, fromJson: Message.fromJson, includeToJson: false) final Message message; // When present, this equals the "local_id" parameter diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 5c94626584..15bb7e8748 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -619,7 +619,7 @@ sealed class Message { required this.matchTopic, }); - factory Message.fromJson(Map json) { + static Message fromJson(Map json) { final type = json['type'] as String; if (type == 'stream') return StreamMessage.fromJson(json); if (type == 'private') return DmMessage.fromJson(json); diff --git a/lib/api/route/messages.dart b/lib/api/route/messages.dart index dcd7998ee8..6a42158b75 100644 --- a/lib/api/route/messages.dart +++ b/lib/api/route/messages.dart @@ -67,6 +67,7 @@ Future getMessage(ApiConnection connection, { @JsonSerializable(fieldRename: FieldRename.snake) class GetMessageResult { // final String rawContent; // deprecated; ignore + @JsonKey(fromJson: Message.fromJson) final Message message; GetMessageResult({ @@ -138,6 +139,7 @@ class GetMessagesResult { final bool foundOldest; final bool foundAnchor; final bool historyLimited; + @JsonKey(fromJson: _messagesFromJson) final List messages; GetMessagesResult({ @@ -149,6 +151,12 @@ class GetMessagesResult { required this.messages, }); + static List _messagesFromJson(Object json) { + return (json as List) + .map((e) => Message.fromJson(e as Map)) + .toList(); + } + factory GetMessagesResult.fromJson(Map json) => _$GetMessagesResultFromJson(json); diff --git a/lib/api/route/messages.g.dart b/lib/api/route/messages.g.dart index 4cfa014c47..21729f04da 100644 --- a/lib/api/route/messages.g.dart +++ b/lib/api/route/messages.g.dart @@ -23,10 +23,7 @@ GetMessagesResult _$GetMessagesResultFromJson(Map json) => foundOldest: json['found_oldest'] as bool, foundAnchor: json['found_anchor'] as bool, historyLimited: json['history_limited'] as bool, - messages: - (json['messages'] as List) - .map((e) => Message.fromJson(e as Map)) - .toList(), + messages: GetMessagesResult._messagesFromJson(json['messages'] as Object), ); Map _$GetMessagesResultToJson(GetMessagesResult instance) => From 59bd6886179ab02e281c4f703b08d0964ff638b7 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 3 Apr 2025 17:10:04 -0400 Subject: [PATCH 2/8] api: Drop DmRecipient This is unused in app code. This is mostly NFC except that we will ignore fields other than "id" from the list of objects from "display_recipient" on message objects from the server. --- lib/api/model/model.dart | 77 ++++++++-------------------------- lib/api/model/model.g.dart | 21 ++-------- test/api/model/model_test.dart | 23 ++-------- 3 files changed, 24 insertions(+), 97 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 15bb7e8748..ee8e3310fc 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -765,78 +765,35 @@ class StreamMessage extends Message { Map toJson() => _$StreamMessageToJson(this); } -@JsonSerializable(fieldRename: FieldRename.snake) -class DmRecipient { - final int id; - final String email; - final String fullName; - - // final String? shortName; // obsolete, ignore - // final bool? isMirrorDummy; // obsolete, ignore - - DmRecipient({required this.id, required this.email, required this.fullName}); - - factory DmRecipient.fromJson(Map json) => - _$DmRecipientFromJson(json); - - Map toJson() => _$DmRecipientToJson(this); - - @override - String toString() => 'DmRecipient(id: $id, email: $email, fullName: $fullName)'; - - @override - bool operator ==(Object other) { - if (other is! DmRecipient) return false; - return other.id == id && other.email == email && other.fullName == fullName; - } - - @override - int get hashCode => Object.hash('DmRecipient', id, email, fullName); -} - -class DmRecipientListConverter extends JsonConverter, List> { - const DmRecipientListConverter(); - - @override - List fromJson(List json) { - return json.map((e) => DmRecipient.fromJson(e as Map)) - .toList(growable: false) - ..sort((a, b) => a.id.compareTo(b.id)); - } - - @override - List toJson(List object) => object; -} - @JsonSerializable(fieldRename: FieldRename.snake) class DmMessage extends Message { @override @JsonKey(includeToJson: true) String get type => 'private'; - /// The `display_recipient` from the server, sorted by user ID numerically. + /// The user IDs of all users in the thread, sorted numerically, as in + /// `display_recipient` from the server. + /// + /// The other fields on `display_recipient` are ignored and won't roundtrip. /// /// This lists the sender as well as all (other) recipients, and it /// lists each user just once. In particular the self-user is always /// included. - /// - /// Note the data here is not updated on changes to the users, so everything - /// other than the user IDs may be stale. - /// Consider using [allRecipientIds] instead, and getting user details - /// from the store. // TODO(server): Document that it's all users. That statement is based on // reverse-engineering notes in zulip-mobile:src/api/modelTypes.js at PmMessage. - @DmRecipientListConverter() - final List displayRecipient; + @JsonKey(name: 'display_recipient', fromJson: _allRecipientIdsFromJson, toJson: _allRecipientIdsToJson) + final List allRecipientIds; + + static List _allRecipientIdsFromJson(Object? json) { + return (json as List).map( + (element) => ((element as Map)['id'] as num).toInt() + ).toList(growable: false) + ..sort(); + } - /// The user IDs of all users in the thread, sorted numerically. - /// - /// This lists the sender as well as all (other) recipients, and it - /// lists each user just once. In particular the self-user is always - /// included. - /// - /// This is a result of [List.map], so it has an efficient `length`. - Iterable get allRecipientIds => displayRecipient.map((e) => e.id); + static List> _allRecipientIdsToJson(List allRecipientIds) { + return allRecipientIds.map((element) => {'id': element}).toList(); + } DmMessage({ required super.client, @@ -856,7 +813,7 @@ class DmMessage extends Message { required super.flags, required super.matchContent, required super.matchTopic, - required this.displayRecipient, + required this.allRecipientIds, }); factory DmMessage.fromJson(Map json) => diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart index 32c8eeb0e7..4bbd0f69f4 100644 --- a/lib/api/model/model.g.dart +++ b/lib/api/model/model.g.dart @@ -319,19 +319,6 @@ const _$MessageEditStateEnumMap = { MessageEditState.moved: 'moved', }; -DmRecipient _$DmRecipientFromJson(Map json) => DmRecipient( - id: (json['id'] as num).toInt(), - email: json['email'] as String, - fullName: json['full_name'] as String, -); - -Map _$DmRecipientToJson(DmRecipient instance) => - { - 'id': instance.id, - 'email': instance.email, - 'full_name': instance.fullName, - }; - DmMessage _$DmMessageFromJson(Map json) => DmMessage( client: json['client'] as String, content: json['content'] as String, @@ -352,8 +339,8 @@ DmMessage _$DmMessageFromJson(Map json) => DmMessage( flags: Message._flagsFromJson(json['flags']), matchContent: json['match_content'] as String?, matchTopic: json['match_subject'] as String?, - displayRecipient: const DmRecipientListConverter().fromJson( - json['display_recipient'] as List, + allRecipientIds: DmMessage._allRecipientIdsFromJson( + json['display_recipient'], ), )..poll = Poll.fromJson(Message._readPoll(json, 'submessages')); @@ -377,8 +364,8 @@ Map _$DmMessageToJson(DmMessage instance) => { 'match_content': instance.matchContent, 'match_subject': instance.matchTopic, 'type': instance.type, - 'display_recipient': const DmRecipientListConverter().toJson( - instance.displayRecipient, + 'display_recipient': DmMessage._allRecipientIdsToJson( + instance.allRecipientIds, ), }; diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index 95737c173f..b1552deb5b 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -172,9 +172,9 @@ void main() { return DmMessage.fromJson({ ...baseJson, ...specialJson }); } - Iterable asRecipients(Iterable users) { + List> asRecipients(Iterable users) { return users.map((u) => - DmRecipient(id: u.userId, email: u.email, fullName: u.fullName)); + {'id': u.userId, 'email': u.email, 'full_name': u.fullName}).toList(); } Map withRecipients(Iterable recipients) { @@ -183,7 +183,7 @@ void main() { 'sender_id': from.userId, 'sender_email': from.email, 'sender_full_name': from.fullName, - 'display_recipient': asRecipients(recipients).map((r) => r.toJson()).toList(), + 'display_recipient': asRecipients(recipients), }; } @@ -191,23 +191,6 @@ void main() { User user3 = eg.user(userId: 3); User user11 = eg.user(userId: 11); - test('displayRecipient', () { - check(parse(withRecipients([user2])).displayRecipient) - .deepEquals(asRecipients([user2])); - - check(parse(withRecipients([user2, user3])).displayRecipient) - .deepEquals(asRecipients([user2, user3])); - check(parse(withRecipients([user3, user2])).displayRecipient) - .deepEquals(asRecipients([user2, user3])); - - check(parse(withRecipients([user2, user3, user11])).displayRecipient) - .deepEquals(asRecipients([user2, user3, user11])); - check(parse(withRecipients([user3, user11, user2])).displayRecipient) - .deepEquals(asRecipients([user2, user3, user11])); - check(parse(withRecipients([user11, user2, user3])).displayRecipient) - .deepEquals(asRecipients([user2, user3, user11])); - }); - test('allRecipientIds', () { check(parse(withRecipients([user2])).allRecipientIds) .deepEquals([2]); From c17ce3e2eb5adf2b6dd827a0eee8dbafa46c29e1 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 27 Mar 2025 18:19:12 -0400 Subject: [PATCH 3/8] api [nfc]: Pull out Conversation This will help support outbox messages in MessageListView later. We extracted Conversation instead of using MessageDestination because they are foundamentally different. Conversation is the identifier for the conversation that contains the message from, for example, get-messages or message events, but MessageDestination is specifically for send-message. --- lib/api/model/model.dart | 95 ++++++++++++++++++++------ lib/api/model/model.g.dart | 111 ++++++++++++++++--------------- lib/model/message.dart | 8 +-- lib/model/narrow.dart | 1 + test/api/model/model_checks.dart | 7 +- 5 files changed, 144 insertions(+), 78 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index ee8e3310fc..785ded6f9c 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -1,5 +1,6 @@ import 'package:json_annotation/json_annotation.dart'; +import '../../model/algorithms.dart'; import 'events.dart'; import 'initial_snapshot.dart'; import 'reaction.dart'; @@ -531,6 +532,52 @@ String? tryParseEmojiCodeToUnicode(String emojiCode) { } } +/// As in [StreamMessage.conversation] and [DmMessage.conversation]. +/// +/// Different from [MessageDestination], this information comes from +/// [getMessages] or [getEvents], identifying the conversation that contains a +/// message. +sealed class Conversation {} + +/// The conversation a stream message is in. +@JsonSerializable(fieldRename: FieldRename.snake, createToJson: false) +class StreamConversation extends Conversation { + int streamId; + + @JsonKey(name: 'subject') + TopicName topic; + + /// The name of the channel with ID [streamId] when the message was sent. + /// + /// The primary reference for the name of the channel is + /// the client's data structures about channels, at [streamId]. + /// This value may be used as a fallback when the channel is unknown. + /// + /// This is non-null when found in a [StreamMessage] object in the API, + /// but may become null in the client's data structures, + /// e.g. if the message gets moved between channels. + @JsonKey(required: true, disallowNullValue: true) + String? displayRecipient; + + StreamConversation(this.streamId, this.topic, {required this.displayRecipient}); + + factory StreamConversation.fromJson(Map json) => + _$StreamConversationFromJson(json); +} + +/// The conversation a DM message is in. +class DmConversation extends Conversation { + /// The user IDs of all users in the conversation, sorted numerically. + /// + /// This lists the sender as well as all (other) recipients, and it + /// lists each user just once. In particular the self-user is always + /// included. + final List allRecipientIds; + + DmConversation({required this.allRecipientIds}) + : assert(isSortedWithoutDuplicates(allRecipientIds.toList())); +} + /// As in the get-messages response. /// /// https://zulip.com/api/get-messages#response @@ -720,20 +767,25 @@ class StreamMessage extends Message { @JsonKey(includeToJson: true) String get type => 'stream'; - // This is not nullable API-wise, but if the message moves across channels, - // [displayRecipient] still refers to the original channel and it has to be - // invalidated. - @JsonKey(required: true, disallowNullValue: true) - String? displayRecipient; - - int streamId; + @JsonKey(includeToJson: true) + int get streamId => conversation.streamId; // The topic/subject is documented to be present on DMs too, just empty. // We ignore it on DMs; if a future server introduces distinct topics in DMs, // that will need new UI that we'll design then as part of that feature, // and ignoring the topics seems as good a fallback behavior as any. - @JsonKey(name: 'subject') - TopicName topic; + @JsonKey(name: 'subject', includeToJson: true) + TopicName get topic => conversation.topic; + + @JsonKey(includeToJson: true) + String? get displayRecipient => conversation.displayRecipient; + + @JsonKey(readValue: _readConversation, includeToJson: false) + StreamConversation conversation; + + static Map _readConversation(Map json, String key) { + return json as Map; + } StreamMessage({ required super.client, @@ -753,9 +805,7 @@ class StreamMessage extends Message { required super.flags, required super.matchContent, required super.matchTopic, - required this.displayRecipient, - required this.streamId, - required this.topic, + required this.conversation, }); factory StreamMessage.fromJson(Map json) => @@ -781,20 +831,23 @@ class DmMessage extends Message { /// included. // TODO(server): Document that it's all users. That statement is based on // reverse-engineering notes in zulip-mobile:src/api/modelTypes.js at PmMessage. - @JsonKey(name: 'display_recipient', fromJson: _allRecipientIdsFromJson, toJson: _allRecipientIdsToJson) - final List allRecipientIds; + @JsonKey(name: 'display_recipient', toJson: _allRecipientIdsToJson, includeToJson: true) + List get allRecipientIds => conversation.allRecipientIds; - static List _allRecipientIdsFromJson(Object? json) { - return (json as List).map( - (element) => ((element as Map)['id'] as num).toInt() - ).toList(growable: false) - ..sort(); - } + @JsonKey(name: 'display_recipient', fromJson: _conversationFromJson, includeToJson: false) + final DmConversation conversation; static List> _allRecipientIdsToJson(List allRecipientIds) { return allRecipientIds.map((element) => {'id': element}).toList(); } + static DmConversation _conversationFromJson(List json) { + return DmConversation(allRecipientIds: json.map( + (element) => ((element as Map)['id'] as num).toInt() + ).toList(growable: false) + ..sort()); + } + DmMessage({ required super.client, required super.content, @@ -813,7 +866,7 @@ class DmMessage extends Message { required super.flags, required super.matchContent, required super.matchTopic, - required this.allRecipientIds, + required this.conversation, }); factory DmMessage.fromJson(Map json) => diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart index 4bbd0f69f4..c928be4e19 100644 --- a/lib/api/model/model.g.dart +++ b/lib/api/model/model.g.dart @@ -254,64 +254,71 @@ Map _$SubscriptionToJson(Subscription instance) => 'color': instance.color, }; -StreamMessage _$StreamMessageFromJson(Map json) { +StreamConversation _$StreamConversationFromJson(Map json) { $checkKeys( json, requiredKeys: const ['display_recipient'], disallowNullValues: const ['display_recipient'], ); - return StreamMessage( - client: json['client'] as String, - content: json['content'] as String, - contentType: json['content_type'] as String, - editState: Message._messageEditStateFromJson( - MessageEditState._readFromMessage(json, 'edit_state'), - ), - id: (json['id'] as num).toInt(), - isMeMessage: json['is_me_message'] as bool, - lastEditTimestamp: (json['last_edit_timestamp'] as num?)?.toInt(), - reactions: Message._reactionsFromJson(json['reactions']), - recipientId: (json['recipient_id'] as num).toInt(), - senderEmail: json['sender_email'] as String, - senderFullName: json['sender_full_name'] as String, - senderId: (json['sender_id'] as num).toInt(), - senderRealmStr: json['sender_realm_str'] as String, - timestamp: (json['timestamp'] as num).toInt(), - flags: Message._flagsFromJson(json['flags']), - matchContent: json['match_content'] as String?, - matchTopic: json['match_subject'] as String?, + return StreamConversation( + (json['stream_id'] as num).toInt(), + TopicName.fromJson(json['subject'] as String), displayRecipient: json['display_recipient'] as String?, - streamId: (json['stream_id'] as num).toInt(), - topic: TopicName.fromJson(json['subject'] as String), - )..poll = Poll.fromJson(Message._readPoll(json, 'submessages')); + ); } -Map _$StreamMessageToJson( - StreamMessage instance, -) => { - 'client': instance.client, - 'content': instance.content, - 'content_type': instance.contentType, - 'edit_state': _$MessageEditStateEnumMap[instance.editState]!, - 'id': instance.id, - 'is_me_message': instance.isMeMessage, - 'last_edit_timestamp': instance.lastEditTimestamp, - 'reactions': Message._reactionsToJson(instance.reactions), - 'recipient_id': instance.recipientId, - 'sender_email': instance.senderEmail, - 'sender_full_name': instance.senderFullName, - 'sender_id': instance.senderId, - 'sender_realm_str': instance.senderRealmStr, - 'submessages': Poll.toJson(instance.poll), - 'timestamp': instance.timestamp, - 'flags': instance.flags, - 'match_content': instance.matchContent, - 'match_subject': instance.matchTopic, - 'type': instance.type, - if (instance.displayRecipient case final value?) 'display_recipient': value, - 'stream_id': instance.streamId, - 'subject': instance.topic, -}; +StreamMessage _$StreamMessageFromJson(Map json) => + StreamMessage( + client: json['client'] as String, + content: json['content'] as String, + contentType: json['content_type'] as String, + editState: Message._messageEditStateFromJson( + MessageEditState._readFromMessage(json, 'edit_state'), + ), + id: (json['id'] as num).toInt(), + isMeMessage: json['is_me_message'] as bool, + lastEditTimestamp: (json['last_edit_timestamp'] as num?)?.toInt(), + reactions: Message._reactionsFromJson(json['reactions']), + recipientId: (json['recipient_id'] as num).toInt(), + senderEmail: json['sender_email'] as String, + senderFullName: json['sender_full_name'] as String, + senderId: (json['sender_id'] as num).toInt(), + senderRealmStr: json['sender_realm_str'] as String, + timestamp: (json['timestamp'] as num).toInt(), + flags: Message._flagsFromJson(json['flags']), + matchContent: json['match_content'] as String?, + matchTopic: json['match_subject'] as String?, + conversation: StreamConversation.fromJson( + StreamMessage._readConversation(json, 'conversation') + as Map, + ), + )..poll = Poll.fromJson(Message._readPoll(json, 'submessages')); + +Map _$StreamMessageToJson(StreamMessage instance) => + { + 'client': instance.client, + 'content': instance.content, + 'content_type': instance.contentType, + 'edit_state': _$MessageEditStateEnumMap[instance.editState]!, + 'id': instance.id, + 'is_me_message': instance.isMeMessage, + 'last_edit_timestamp': instance.lastEditTimestamp, + 'reactions': Message._reactionsToJson(instance.reactions), + 'recipient_id': instance.recipientId, + 'sender_email': instance.senderEmail, + 'sender_full_name': instance.senderFullName, + 'sender_id': instance.senderId, + 'sender_realm_str': instance.senderRealmStr, + 'submessages': Poll.toJson(instance.poll), + 'timestamp': instance.timestamp, + 'flags': instance.flags, + 'match_content': instance.matchContent, + 'match_subject': instance.matchTopic, + 'type': instance.type, + 'stream_id': instance.streamId, + 'subject': instance.topic, + 'display_recipient': instance.displayRecipient, + }; const _$MessageEditStateEnumMap = { MessageEditState.none: 'none', @@ -339,8 +346,8 @@ DmMessage _$DmMessageFromJson(Map json) => DmMessage( flags: Message._flagsFromJson(json['flags']), matchContent: json['match_content'] as String?, matchTopic: json['match_subject'] as String?, - allRecipientIds: DmMessage._allRecipientIdsFromJson( - json['display_recipient'], + conversation: DmMessage._conversationFromJson( + json['display_recipient'] as List, ), )..poll = Poll.fromJson(Message._readPoll(json, 'submessages')); diff --git a/lib/model/message.dart b/lib/model/message.dart index 83c62f0656..962d79eeab 100644 --- a/lib/model/message.dart +++ b/lib/model/message.dart @@ -211,14 +211,14 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore { } if (newStreamId != origStreamId) { - message.streamId = newStreamId; - // See [StreamMessage.displayRecipient] on why the invalidation is + message.conversation.streamId = newStreamId; + // See [StreamConversation.displayRecipient] on why the invalidation is // needed. - message.displayRecipient = null; + message.conversation.displayRecipient = null; } if (newTopic != origTopic) { - message.topic = newTopic; + message.conversation.topic = newTopic; } if (!wasResolveOrUnresolve diff --git a/lib/model/narrow.dart b/lib/model/narrow.dart index 25b14ff980..c5bbbde599 100644 --- a/lib/model/narrow.dart +++ b/lib/model/narrow.dart @@ -235,6 +235,7 @@ class DmNarrow extends Narrow implements SendableNarrow { /// See also: /// * [otherRecipientIds], an alternate way of identifying the conversation. /// * [DmMessage.allRecipientIds], which provides this same format. + /// * [DmConversation.allRecipientIds], which also provides this same format. final List allRecipientIds; /// The user ID of the self-user. diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 77995466ce..078eedf102 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -24,6 +24,10 @@ extension UserChecks on Subject { extension ZulipStreamChecks on Subject { } +extension StreamConversationChecks on Subject { + Subject get displayRecipient => has((x) => x.displayRecipient, 'displayRecipient'); +} + extension MessageChecks on Subject { Subject get client => has((e) => e.client, 'client'); Subject get content => has((e) => e.content, 'content'); @@ -52,9 +56,10 @@ extension TopicNameChecks on Subject { } extension StreamMessageChecks on Subject { - Subject get displayRecipient => has((e) => e.displayRecipient, 'displayRecipient'); Subject get streamId => has((e) => e.streamId, 'streamId'); Subject get topic => has((e) => e.topic, 'topic'); + Subject get displayRecipient => has((e) => e.displayRecipient, 'displayRecipient'); + Subject get conversation => has((e) => e.conversation, 'conversation'); } extension ReactionsChecks on Subject { From 41a5ee3b82e232d80ba193c8e74fe9580d686cec Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 27 Mar 2025 18:19:12 -0400 Subject: [PATCH 4/8] api [nfc]: Place TopicName before Conversation --- lib/api/model/model.dart | 118 +++++++++++++++---------------- test/api/model/model_checks.dart | 10 +-- 2 files changed, 64 insertions(+), 64 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 785ded6f9c..c389780f51 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -532,6 +532,65 @@ String? tryParseEmojiCodeToUnicode(String emojiCode) { } } +/// The name of a Zulip topic. +// TODO(dart): Can we forbid calling Object members on this extension type? +// (The lack of "implements Object" ought to do that, but doesn't.) +// In particular an interpolation "foo > $topic" is a bug we'd like to catch. +// TODO(dart): Can we forbid using this extension type as a key in a Map? +// (The lack of "implements Object" arguably should do that, but doesn't.) +// Using as a Map key is almost certainly a bug because it won't case-fold; +// see for example #739, #980, #1205. +extension type const TopicName(String _value) { + /// The canonical form of the resolved-topic prefix. + // This is RESOLVED_TOPIC_PREFIX in web: + // https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts + static const resolvedTopicPrefix = '✔ '; + + /// Pattern for an arbitrary resolved-topic prefix. + /// + /// These always begin with [resolvedTopicPrefix] + /// but can be weird and go on longer, like "✔ ✔✔ ". + // This is RESOLVED_TOPIC_PREFIX_RE in web: + // https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts#L4-L12 + static final resolvedTopicPrefixRegexp = RegExp(r'^✔ [ ✔]*'); + + /// The string this topic is identified by in the Zulip API. + /// + /// This should be used in constructing HTTP requests to the server, + /// but rarely for other purposes. See [displayName] and [canonicalize]. + String get apiName => _value; + + /// The string this topic is displayed as to the user in our UI. + /// + /// At the moment this always equals [apiName]. + /// In the future this will become null for the "general chat" topic (#1250), + /// so that UI code can identify when it needs to represent the topic + /// specially in the way prescribed for "general chat". + // TODO(#1250) carry out that plan + String get displayName => _value; + + /// The key to use for "same topic as" comparisons. + String canonicalize() => apiName.toLowerCase(); + + /// Whether the topic starts with [resolvedTopicPrefix]. + bool get isResolved => _value.startsWith(resolvedTopicPrefix); + + /// This [TopicName] plus the [resolvedTopicPrefix] prefix. + TopicName resolve() => TopicName(resolvedTopicPrefix + _value); + + /// A [TopicName] with [resolvedTopicPrefixRegexp] stripped if present. + TopicName unresolve() => + TopicName(_value.replaceFirst(resolvedTopicPrefixRegexp, '')); + + /// Whether [this] and [other] have the same canonical form, + /// using [canonicalize]. + bool isSameAs(TopicName other) => canonicalize() == other.canonicalize(); + + TopicName.fromJson(this._value); + + String toJson() => apiName; +} + /// As in [StreamMessage.conversation] and [DmMessage.conversation]. /// /// Different from [MessageDestination], this information comes from @@ -702,65 +761,6 @@ enum MessageFlag { String toJson() => _$MessageFlagEnumMap[this]!; } -/// The name of a Zulip topic. -// TODO(dart): Can we forbid calling Object members on this extension type? -// (The lack of "implements Object" ought to do that, but doesn't.) -// In particular an interpolation "foo > $topic" is a bug we'd like to catch. -// TODO(dart): Can we forbid using this extension type as a key in a Map? -// (The lack of "implements Object" arguably should do that, but doesn't.) -// Using as a Map key is almost certainly a bug because it won't case-fold; -// see for example #739, #980, #1205. -extension type const TopicName(String _value) { - /// The canonical form of the resolved-topic prefix. - // This is RESOLVED_TOPIC_PREFIX in web: - // https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts - static const resolvedTopicPrefix = '✔ '; - - /// Pattern for an arbitrary resolved-topic prefix. - /// - /// These always begin with [resolvedTopicPrefix] - /// but can be weird and go on longer, like "✔ ✔✔ ". - // This is RESOLVED_TOPIC_PREFIX_RE in web: - // https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts#L4-L12 - static final resolvedTopicPrefixRegexp = RegExp(r'^✔ [ ✔]*'); - - /// The string this topic is identified by in the Zulip API. - /// - /// This should be used in constructing HTTP requests to the server, - /// but rarely for other purposes. See [displayName] and [canonicalize]. - String get apiName => _value; - - /// The string this topic is displayed as to the user in our UI. - /// - /// At the moment this always equals [apiName]. - /// In the future this will become null for the "general chat" topic (#1250), - /// so that UI code can identify when it needs to represent the topic - /// specially in the way prescribed for "general chat". - // TODO(#1250) carry out that plan - String get displayName => _value; - - /// The key to use for "same topic as" comparisons. - String canonicalize() => apiName.toLowerCase(); - - /// Whether the topic starts with [resolvedTopicPrefix]. - bool get isResolved => _value.startsWith(resolvedTopicPrefix); - - /// This [TopicName] plus the [resolvedTopicPrefix] prefix. - TopicName resolve() => TopicName(resolvedTopicPrefix + _value); - - /// A [TopicName] with [resolvedTopicPrefixRegexp] stripped if present. - TopicName unresolve() => - TopicName(_value.replaceFirst(resolvedTopicPrefixRegexp, '')); - - /// Whether [this] and [other] have the same canonical form, - /// using [canonicalize]. - bool isSameAs(TopicName other) => canonicalize() == other.canonicalize(); - - TopicName.fromJson(this._value); - - String toJson() => apiName; -} - @JsonSerializable(fieldRename: FieldRename.snake) class StreamMessage extends Message { @override diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 078eedf102..260c1ec6de 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -24,6 +24,11 @@ extension UserChecks on Subject { extension ZulipStreamChecks on Subject { } +extension TopicNameChecks on Subject { + Subject get apiName => has((x) => x.apiName, 'apiName'); + Subject get displayName => has((x) => x.displayName, 'displayName'); +} + extension StreamConversationChecks on Subject { Subject get displayRecipient => has((x) => x.displayRecipient, 'displayRecipient'); } @@ -50,11 +55,6 @@ extension MessageChecks on Subject { Subject get matchTopic => has((e) => e.matchTopic, 'matchTopic'); } -extension TopicNameChecks on Subject { - Subject get apiName => has((x) => x.apiName, 'apiName'); - Subject get displayName => has((x) => x.displayName, 'displayName'); -} - extension StreamMessageChecks on Subject { Subject get streamId => has((e) => e.streamId, 'streamId'); Subject get topic => has((e) => e.topic, 'topic'); From f136d8e4f8bc383edebf938939832818e4a51f2b Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 27 Mar 2025 18:19:12 -0400 Subject: [PATCH 5/8] api [nfc]: Pull out MessageBase, make Message generic See also CZO discussion on the design of this, and the drawbacks of the alternatives: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/A.20new.20variant.20of.20Zulip.20messages.20-.20inheritance.20structure/with/2141288 This will help support outbox messages in MessageListView later. Needing to specify the element type of `List`s in some tests (or in some cases, the type of a variable declared) is a side effect of making `StreamMessage` and `DmMessage` extend `Message` with different `T`'s. When both types of messages appear in the same `List`, the upper bound is `Object`, instead of the more specific `Message`. See "least-upper-bound" tagged issues for reference: https://github.com/dart-lang/language/issues?q=state%3Aopen%20label%3A%22least-upper-bound%22 --- lib/api/model/model.dart | 44 +++++++++++++++++++++++------ lib/api/model/model.g.dart | 8 +++--- test/api/model/model_checks.dart | 10 +++++-- test/model/message_list_test.dart | 2 +- test/model/message_test.dart | 6 ++-- test/model/unreads_test.dart | 18 ++++++------ test/widgets/message_list_test.dart | 2 +- 7 files changed, 61 insertions(+), 29 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index c389780f51..157307c3d4 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -591,7 +591,7 @@ extension type const TopicName(String _value) { String toJson() => apiName; } -/// As in [StreamMessage.conversation] and [DmMessage.conversation]. +/// As in [MessageBase.conversation]. /// /// Different from [MessageDestination], this information comes from /// [getMessages] or [getEvents], identifying the conversation that contains a @@ -637,10 +637,35 @@ class DmConversation extends Conversation { : assert(isSortedWithoutDuplicates(allRecipientIds.toList())); } +/// A message or message-like object, for showing in a message list. +/// +/// Other than [Message], we use this for "outbox messages", +/// representing outstanding [sendMessage] requests. +abstract class MessageBase { + /// The Zulip message ID. + /// + /// If null, the message doesn't have an ID acknowledged by the server + /// (e.g.: a locally-echoed message). + int? get id; + + final int senderId; + final int timestamp; + + /// The conversation that contains this message. + /// + /// When implementing this, the return type should be either + /// [StreamConversation] or [DmConversation]; it should never be + /// [Conversation], because we expect a concrete subclass of [MessageBase] + /// to represent either a channel message or a DM message, not both. + T get conversation; + + const MessageBase({required this.senderId, required this.timestamp}); +} + /// As in the get-messages response. /// /// https://zulip.com/api/get-messages#response -sealed class Message { +sealed class Message extends MessageBase { // final String? avatarUrl; // Use [User.avatarUrl] instead; will live-update final String client; String content; @@ -650,6 +675,7 @@ sealed class Message { @JsonKey(readValue: MessageEditState._readFromMessage, fromJson: Message._messageEditStateFromJson) MessageEditState editState; + @override final int id; bool isMeMessage; int? lastEditTimestamp; @@ -660,14 +686,12 @@ sealed class Message { final int recipientId; final String senderEmail; final String senderFullName; - final int senderId; final String senderRealmStr; /// Poll data if "submessages" describe a poll, `null` otherwise. @JsonKey(name: 'submessages', readValue: _readPoll, fromJson: Poll.fromJson, toJson: Poll.toJson) Poll? poll; - final int timestamp; String get type; // final List topicLinks; // TODO handle @@ -717,14 +741,16 @@ sealed class Message { required this.recipientId, required this.senderEmail, required this.senderFullName, - required this.senderId, + required super.senderId, required this.senderRealmStr, - required this.timestamp, + required super.timestamp, required this.flags, required this.matchContent, required this.matchTopic, }); + // TODO(dart): This has to be a static method, because factories/constructors + // do not support type parameters: https://github.com/dart-lang/language/issues/647 static Message fromJson(Map json) { final type = json['type'] as String; if (type == 'stream') return StreamMessage.fromJson(json); @@ -762,7 +788,7 @@ enum MessageFlag { } @JsonSerializable(fieldRename: FieldRename.snake) -class StreamMessage extends Message { +class StreamMessage extends Message { @override @JsonKey(includeToJson: true) String get type => 'stream'; @@ -780,6 +806,7 @@ class StreamMessage extends Message { @JsonKey(includeToJson: true) String? get displayRecipient => conversation.displayRecipient; + @override @JsonKey(readValue: _readConversation, includeToJson: false) StreamConversation conversation; @@ -816,7 +843,7 @@ class StreamMessage extends Message { } @JsonSerializable(fieldRename: FieldRename.snake) -class DmMessage extends Message { +class DmMessage extends Message { @override @JsonKey(includeToJson: true) String get type => 'private'; @@ -834,6 +861,7 @@ class DmMessage extends Message { @JsonKey(name: 'display_recipient', toJson: _allRecipientIdsToJson, includeToJson: true) List get allRecipientIds => conversation.allRecipientIds; + @override @JsonKey(name: 'display_recipient', fromJson: _conversationFromJson, includeToJson: false) final DmConversation conversation; diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart index c928be4e19..cddf78beb0 100644 --- a/lib/api/model/model.g.dart +++ b/lib/api/model/model.g.dart @@ -296,6 +296,8 @@ StreamMessage _$StreamMessageFromJson(Map json) => Map _$StreamMessageToJson(StreamMessage instance) => { + 'sender_id': instance.senderId, + 'timestamp': instance.timestamp, 'client': instance.client, 'content': instance.content, 'content_type': instance.contentType, @@ -307,10 +309,8 @@ Map _$StreamMessageToJson(StreamMessage instance) => 'recipient_id': instance.recipientId, 'sender_email': instance.senderEmail, 'sender_full_name': instance.senderFullName, - 'sender_id': instance.senderId, 'sender_realm_str': instance.senderRealmStr, 'submessages': Poll.toJson(instance.poll), - 'timestamp': instance.timestamp, 'flags': instance.flags, 'match_content': instance.matchContent, 'match_subject': instance.matchTopic, @@ -352,6 +352,8 @@ DmMessage _$DmMessageFromJson(Map json) => DmMessage( )..poll = Poll.fromJson(Message._readPoll(json, 'submessages')); Map _$DmMessageToJson(DmMessage instance) => { + 'sender_id': instance.senderId, + 'timestamp': instance.timestamp, 'client': instance.client, 'content': instance.content, 'content_type': instance.contentType, @@ -363,10 +365,8 @@ Map _$DmMessageToJson(DmMessage instance) => { 'recipient_id': instance.recipientId, 'sender_email': instance.senderEmail, 'sender_full_name': instance.senderFullName, - 'sender_id': instance.senderId, 'sender_realm_str': instance.senderRealmStr, 'submessages': Poll.toJson(instance.poll), - 'timestamp': instance.timestamp, 'flags': instance.flags, 'match_content': instance.matchContent, 'match_subject': instance.matchTopic, diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 260c1ec6de..8791c1b9d9 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -33,6 +33,13 @@ extension StreamConversationChecks on Subject { Subject get displayRecipient => has((x) => x.displayRecipient, 'displayRecipient'); } +extension MessageBaseChecks on Subject> { + Subject get id => has((e) => e.id, 'id'); + Subject get senderId => has((e) => e.senderId, 'senderId'); + Subject get timestamp => has((e) => e.timestamp, 'timestamp'); + Subject get conversation => has((e) => e.conversation, 'conversation'); +} + extension MessageChecks on Subject { Subject get client => has((e) => e.client, 'client'); Subject get content => has((e) => e.content, 'content'); @@ -45,10 +52,8 @@ extension MessageChecks on Subject { Subject get recipientId => has((e) => e.recipientId, 'recipientId'); Subject get senderEmail => has((e) => e.senderEmail, 'senderEmail'); Subject get senderFullName => has((e) => e.senderFullName, 'senderFullName'); - Subject get senderId => has((e) => e.senderId, 'senderId'); Subject get senderRealmStr => has((e) => e.senderRealmStr, 'senderRealmStr'); Subject get poll => has((e) => e.poll, 'poll'); - Subject get timestamp => has((e) => e.timestamp, 'timestamp'); Subject get type => has((e) => e.type, 'type'); Subject> get flags => has((e) => e.flags, 'flags'); Subject get matchContent => has((e) => e.matchContent, 'matchContent'); @@ -59,7 +64,6 @@ extension StreamMessageChecks on Subject { Subject get streamId => has((e) => e.streamId, 'streamId'); Subject get topic => has((e) => e.topic, 'topic'); Subject get displayRecipient => has((e) => e.displayRecipient, 'displayRecipient'); - Subject get conversation => has((e) => e.conversation, 'conversation'); } extension ReactionsChecks on Subject { diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 6f00d83dc6..062ab70e49 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -526,7 +526,7 @@ void main() { test('unmute a topic -> refetch from scratch', () => awaitFakeAsync((async) async { await prepare(narrow: const CombinedFeedNarrow()); await prepareMutes(true); - final messages = [ + final messages = [ eg.dmMessage(id: 1, from: eg.otherUser, to: [eg.selfUser]), eg.streamMessage(id: 2, stream: stream, topic: topic), ]; diff --git a/test/model/message_test.dart b/test/model/message_test.dart index 5b4457f30b..1f774e32b9 100644 --- a/test/model/message_test.dart +++ b/test/model/message_test.dart @@ -82,7 +82,7 @@ void main() { final message1 = eg.streamMessage(); final message2 = eg.streamMessage(); final message3 = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); - final messages = [message1, message2, message3]; + final messages = [message1, message2, message3]; store.reconcileMessages(messages); check(messages).deepEquals( [message1, message2, message3] @@ -97,7 +97,7 @@ void main() { final message1 = eg.streamMessage(); final message2 = eg.streamMessage(); final message3 = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]); - final messages = [message1, message2, message3]; + final messages = [message1, message2, message3]; await addMessages(messages); final newMessage = eg.streamMessage(); store.reconcileMessages([newMessage]); @@ -137,7 +137,7 @@ void main() { test('from not-empty', () async { await prepare(); - final messages = [ + final messages = [ eg.streamMessage(), eg.streamMessage(), eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]), diff --git a/test/model/unreads_test.dart b/test/model/unreads_test.dart index 42e799624f..de554b220c 100644 --- a/test/model/unreads_test.dart +++ b/test/model/unreads_test.dart @@ -247,7 +247,7 @@ void main() { final unreadChannelMessage = eg.streamMessage(flags: []); final readChannelMessage = eg.streamMessage(flags: [MessageFlag.read]); - final allMessages = [ + final allMessages = [ unreadDmMessage, unreadChannelMessage, readDmMessage, readChannelMessage, ]; @@ -314,7 +314,7 @@ void main() { if (isDirectMentioned) MessageFlag.mentioned, if (isWildcardMentioned) MessageFlag.wildcardMentioned, ]; - final message = isStream + final Message message = isStream ? eg.streamMessage(flags: flags) : eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: flags); model.handleMessageEvent(eg.messageEvent(message)); @@ -401,7 +401,7 @@ void main() { for (final isKnownToModel in [true, false]) { for (final isRead in [false, true]) { final baseFlags = [if (isRead) MessageFlag.read]; - for (final (messageDesc, message) in [ + for (final (messageDesc, message) in <(String, Message)>[ ('stream', eg.streamMessage(flags: baseFlags)), ('1:1 dm', eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: baseFlags)), ]) { @@ -661,7 +661,7 @@ void main() { final message13 = eg.streamMessage(id: 13, stream: stream2, topic: 'b', flags: []); final message14 = eg.streamMessage(id: 14, stream: stream2, topic: 'b', flags: [MessageFlag.mentioned]); - final messages = [ + final messages = [ message1, message2, message3, message4, message5, message6, message7, message8, message9, message10, message11, message12, message13, message14, @@ -848,7 +848,7 @@ void main() { // That case is indistinguishable from an unread that's unknown to // the model, so we get coverage for that case too. test('add flag: ${mentionFlag.name}', () { - final messages = [ + final messages = [ eg.streamMessage(flags: []), eg.streamMessage(flags: [MessageFlag.read]), eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: []), @@ -885,7 +885,7 @@ void main() { // That case is indistinguishable from an unread that's unknown to // the model, so we get coverage for that case too. test('remove flag: ${mentionFlag.name}', () { - final messages = [ + final messages = [ eg.streamMessage(flags: [mentionFlag]), eg.streamMessage(flags: [mentionFlag, MessageFlag.read]), eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: [mentionFlag]), @@ -924,7 +924,7 @@ void main() { final message2 = eg.streamMessage(id: 2, flags: [MessageFlag.mentioned]); final message3 = eg.dmMessage(id: 3, from: eg.otherUser, to: [eg.selfUser], flags: []); final message4 = eg.dmMessage(id: 4, from: eg.otherUser, to: [eg.selfUser], flags: [MessageFlag.wildcardMentioned]); - final messages = [message1, message2, message3, message4]; + final messages = [message1, message2, message3, message4]; prepare(); fillWithMessages([message1, message2, message3, message4]); @@ -973,7 +973,7 @@ void main() { final message13 = eg.streamMessage(id: 13, stream: stream2, topic: 'b', flags: []); final message14 = eg.streamMessage(id: 14, stream: stream2, topic: 'b', flags: [MessageFlag.mentioned]); - final messages = [ + final messages = [ message1, message2, message3, message4, message5, message6, message7, message8, message9, message10, message11, message12, message13, message14, @@ -1085,7 +1085,7 @@ void main() { final message13 = eg.streamMessage(id: 13, stream: stream2, topic: 'b', flags: [MessageFlag.read]); final message14 = eg.streamMessage(id: 14, stream: stream2, topic: 'b', flags: [MessageFlag.mentioned, MessageFlag.read]); - final messages = [ + final messages = [ message1, message2, message3, message4, message5, message6, message7, message8, message9, message10, message11, message12, message13, message14, diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 53ad1334ef..18717d2c93 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -538,7 +538,7 @@ void main() { final streamMessage = eg.streamMessage(); final topicNarrow = TopicNarrow.ofMessage(streamMessage); - for (final (description, message, narrow) in [ + for (final (description, message, narrow) in <(String, Message, SendableNarrow)>[ ('typing in dm', dmMessage, dmNarrow), ('typing in topic', streamMessage, topicNarrow), ]) { From 575f1c633f1d9024c96565ea729abeaae022af64 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 27 Mar 2025 18:40:04 -0400 Subject: [PATCH 6/8] msglist [nfc]: Migrate most MessageListItem subclasses to use MessageBase except MessageListMessageItem. This keeps changes minimal, leaving most of the helpers in lib/model/message_list.dart untouched, to avoid unnecessary generalization. This hoists streamId in StreamMessageRecipientHeader.build, where we used to access streamId via message instead from the onLongPress callback. Because Message.streamId only changes on message moves, we expect the build method to be called again, so this should be fine. --- lib/model/message_list.dart | 7 +++-- lib/model/narrow.dart | 10 ++++--- lib/widgets/message_list.dart | 48 +++++++++++++++++-------------- test/model/message_list_test.dart | 4 +-- 4 files changed, 39 insertions(+), 30 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index a1df255cb2..58a0e1bb95 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -24,13 +24,13 @@ sealed class MessageListItem { } class MessageListRecipientHeaderItem extends MessageListItem { - final Message message; + final MessageBase message; MessageListRecipientHeaderItem(this.message); } class MessageListDateSeparatorItem extends MessageListItem { - final Message message; + final MessageBase message; MessageListDateSeparatorItem(this.message); } @@ -155,7 +155,8 @@ mixin _MessageSequence { } case MessageListRecipientHeaderItem(:var message): case MessageListDateSeparatorItem(:var message): - return (message.id <= messageId) ? -1 : 1; + if (message.id == null) return 1; // TODO(#1441): test + return message.id! <= messageId ? -1 : 1; case MessageListMessageItem(:var message): return message.id.compareTo(messageId); } } diff --git a/lib/model/narrow.dart b/lib/model/narrow.dart index c5bbbde599..2ef2a0092f 100644 --- a/lib/model/narrow.dart +++ b/lib/model/narrow.dart @@ -94,8 +94,8 @@ class ChannelNarrow extends Narrow { class TopicNarrow extends Narrow implements SendableNarrow { const TopicNarrow(this.streamId, this.topic, {this.with_}); - factory TopicNarrow.ofMessage(StreamMessage message) { - return TopicNarrow(message.streamId, message.topic); + factory TopicNarrow.ofMessage(MessageBase message) { + return TopicNarrow(message.conversation.streamId, message.conversation.topic); } final int streamId; @@ -194,9 +194,11 @@ class DmNarrow extends Narrow implements SendableNarrow { ); } - factory DmNarrow.ofMessage(DmMessage message, {required int selfUserId}) { + factory DmNarrow.ofMessage(MessageBase message, { + required int selfUserId, + }) { return DmNarrow( - allRecipientIds: List.unmodifiable(message.allRecipientIds), + allRecipientIds: List.unmodifiable(message.conversation.allRecipientIds), selfUserId: selfUserId, ); } diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 1e95a1be49..f00f873b1a 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -901,15 +901,19 @@ class _MarkAsReadAnimationState extends State { class RecipientHeader extends StatelessWidget { const RecipientHeader({super.key, required this.message, required this.narrow}); - final Message message; + final MessageBase message; final Narrow narrow; @override Widget build(BuildContext context) { final message = this.message; return switch (message) { - StreamMessage() => StreamMessageRecipientHeader(message: message, narrow: narrow), - DmMessage() => DmRecipientHeader(message: message, narrow: narrow), + MessageBase() => + StreamMessageRecipientHeader(message: message, narrow: narrow), + MessageBase() => + DmRecipientHeader(message: message, narrow: narrow), + MessageBase() => + throw StateError('Bad concrete subclass of MessageBase'), }; } } @@ -917,7 +921,7 @@ class RecipientHeader extends StatelessWidget { class DateSeparator extends StatelessWidget { const DateSeparator({super.key, required this.message}); - final Message message; + final MessageBase message; @override Widget build(BuildContext context) { @@ -1027,7 +1031,7 @@ class StreamMessageRecipientHeader extends StatelessWidget { required this.narrow, }); - final StreamMessage message; + final MessageBase message; final Narrow narrow; static bool _containsDifferentChannels(Narrow narrow) { @@ -1053,11 +1057,12 @@ class StreamMessageRecipientHeader extends StatelessWidget { final designVariables = DesignVariables.of(context); final zulipLocalizations = ZulipLocalizations.of(context); - final topic = message.topic; + final streamId = message.conversation.streamId; + final topic = message.conversation.topic; final messageListTheme = MessageListTheme.of(context); - final subscription = store.subscriptions[message.streamId]; + final subscription = store.subscriptions[streamId]; final Color backgroundColor; final Color iconColor; if (subscription != null) { @@ -1073,16 +1078,16 @@ class StreamMessageRecipientHeader extends StatelessWidget { if (!_containsDifferentChannels(narrow)) { streamWidget = const SizedBox(width: 16); } else { - final stream = store.streams[message.streamId]; + final stream = store.streams[streamId]; final streamName = stream?.name - ?? message.displayRecipient + ?? message.conversation.displayRecipient ?? zulipLocalizations.unknownChannelName; // TODO(log) streamWidget = GestureDetector( onTap: () => Navigator.push(context, MessageListPage.buildRoute(context: context, - narrow: ChannelNarrow(message.streamId))), - onLongPress: () => showChannelActionSheet(context, channelId: message.streamId), + narrow: ChannelNarrow(streamId))), + onLongPress: () => showChannelActionSheet(context, channelId: streamId), child: Row( crossAxisAlignment: CrossAxisAlignment.center, children: [ @@ -1130,7 +1135,7 @@ class StreamMessageRecipientHeader extends StatelessWidget { Icon(size: 14, color: designVariables.title.withFadedAlpha(0.5), // A null [Icon.icon] makes a blank space. iconDataForTopicVisibilityPolicy( - store.topicVisibilityPolicy(message.streamId, topic))), + store.topicVisibilityPolicy(streamId, topic))), ])); return GestureDetector( @@ -1143,7 +1148,7 @@ class StreamMessageRecipientHeader extends StatelessWidget { MessageListPage.buildRoute(context: context, narrow: TopicNarrow.ofMessage(message))), onLongPress: () => showTopicActionSheet(context, - channelId: message.streamId, + channelId: streamId, topic: topic, someMessageIdInTopic: message.id), child: ColoredBox( @@ -1168,7 +1173,7 @@ class DmRecipientHeader extends StatelessWidget { required this.narrow, }); - final DmMessage message; + final MessageBase message; final Narrow narrow; @override @@ -1176,12 +1181,13 @@ class DmRecipientHeader extends StatelessWidget { final zulipLocalizations = ZulipLocalizations.of(context); final store = PerAccountStoreWidget.of(context); final String title; - if (message.allRecipientIds.length > 1) { - title = zulipLocalizations.messageListGroupYouAndOthers(message.allRecipientIds - .where((id) => id != store.selfUserId) - .map(store.userDisplayName) - .sorted() - .join(", ")); + if (message.conversation.allRecipientIds.length > 1) { + title = zulipLocalizations.messageListGroupYouAndOthers( + message.conversation.allRecipientIds + .where((id) => id != store.selfUserId) + .map(store.userDisplayName) + .sorted() + .join(", ")); } else { title = zulipLocalizations.messageListGroupYouWithYourself; } @@ -1233,7 +1239,7 @@ TextStyle recipientHeaderTextStyle(BuildContext context, {FontStyle? fontStyle}) class RecipientHeaderDate extends StatelessWidget { const RecipientHeaderDate({super.key, required this.message}); - final Message message; + final MessageBase message; @override Widget build(BuildContext context) { diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 062ab70e49..2c5f0711dc 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -1994,11 +1994,11 @@ void checkInvariants(MessageListView model) { } extension MessageListRecipientHeaderItemChecks on Subject { - Subject get message => has((x) => x.message, 'message'); + Subject get message => has((x) => x.message, 'message'); } extension MessageListDateSeparatorItemChecks on Subject { - Subject get message => has((x) => x.message, 'message'); + Subject get message => has((x) => x.message, 'message'); } extension MessageListMessageItemChecks on Subject { From 2c4085e96626567c0a06bf1e2753618733581c2a Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 15 Apr 2025 00:41:02 -0400 Subject: [PATCH 7/8] narrow test: Add containsMessage tests for ChannelNarrow and TopicNarrow --- test/model/narrow_test.dart | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/model/narrow_test.dart b/test/model/narrow_test.dart index 9d676a531b..56df3aedc8 100644 --- a/test/model/narrow_test.dart +++ b/test/model/narrow_test.dart @@ -22,6 +22,20 @@ void main() { }); }); + group('ChannelNarrow', () { + test('containsMessage', () { + final stream = eg.stream(); + final otherStream = eg.stream(); + final narrow = ChannelNarrow(stream.streamId); + check(narrow.containsMessage( + eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]))).isFalse(); + check(narrow.containsMessage( + eg.streamMessage(stream: otherStream, topic: 'topic'))).isFalse(); + check(narrow.containsMessage( + eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); + }); + }); + group('TopicNarrow', () { test('ofMessage', () { final stream = eg.stream(); @@ -29,6 +43,20 @@ void main() { final actual = TopicNarrow.ofMessage(message); check(actual).equals(TopicNarrow(stream.streamId, message.topic)); }); + + test('containsMessage', () { + final stream = eg.stream(); + final otherStream = eg.stream(); + final narrow = eg.topicNarrow(stream.streamId, 'topic'); + check(narrow.containsMessage( + eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]))).isFalse(); + check(narrow.containsMessage( + eg.streamMessage(stream: otherStream, topic: 'topic'))).isFalse(); + check(narrow.containsMessage( + eg.streamMessage(stream: stream, topic: 'topic2'))).isFalse(); + check(narrow.containsMessage( + eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); + }); }); group('DmNarrow', () { From 5ec097c2f87ce4dd5d2385e695e4392406c0eb9d Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 1 Apr 2025 14:38:31 -0400 Subject: [PATCH 8/8] narrow [nfc]: Make containsMessage support MessageBase This is NFC because we don't have non-testing subclasses of MessageBase. --- lib/model/narrow.dart | 31 ++++++++++-------- test/model/narrow_test.dart | 65 +++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 13 deletions(-) diff --git a/lib/model/narrow.dart b/lib/model/narrow.dart index 2ef2a0092f..104334a956 100644 --- a/lib/model/narrow.dart +++ b/lib/model/narrow.dart @@ -19,7 +19,7 @@ sealed class Narrow { /// This does not necessarily mean the message list would show this message /// when navigated to this narrow; in particular it does not address the /// question of whether the stream or topic, or the sending user, is muted. - bool containsMessage(Message message); + bool containsMessage(MessageBase message); /// This narrow, expressed as an [ApiNarrow]. ApiNarrow apiEncode(); @@ -47,7 +47,7 @@ class CombinedFeedNarrow extends Narrow { const CombinedFeedNarrow(); @override - bool containsMessage(Message message) { + bool containsMessage(MessageBase message) { return true; } @@ -71,8 +71,9 @@ class ChannelNarrow extends Narrow { final int streamId; @override - bool containsMessage(Message message) { - return message is StreamMessage && message.streamId == streamId; + bool containsMessage(MessageBase message) { + final conversation = message.conversation; + return conversation is StreamConversation && conversation.streamId == streamId; } @override @@ -105,9 +106,10 @@ class TopicNarrow extends Narrow implements SendableNarrow { TopicNarrow sansWith() => TopicNarrow(streamId, topic); @override - bool containsMessage(Message message) { - return (message is StreamMessage - && message.streamId == streamId && message.topic == topic); + bool containsMessage(MessageBase message) { + final conversation = message.conversation; + return conversation is StreamConversation + && conversation.streamId == streamId && conversation.topic == topic; } @override @@ -263,11 +265,12 @@ class DmNarrow extends Narrow implements SendableNarrow { late final String _key = otherRecipientIds.join(','); @override - bool containsMessage(Message message) { - if (message is! DmMessage) return false; - if (message.allRecipientIds.length != allRecipientIds.length) return false; + bool containsMessage(MessageBase message) { + final conversation = message.conversation; + if (conversation is! DmConversation) return false; + if (conversation.allRecipientIds.length != allRecipientIds.length) return false; int i = 0; - for (final userId in message.allRecipientIds) { + for (final userId in conversation.allRecipientIds) { if (userId != allRecipientIds[i]) return false; i++; } @@ -307,7 +310,8 @@ class MentionsNarrow extends Narrow { const MentionsNarrow(); @override - bool containsMessage(Message message) { + bool containsMessage(MessageBase message) { + if (message is! Message) return false; return message.flags.any((flag) { switch (flag) { case MessageFlag.mentioned: @@ -346,7 +350,8 @@ class StarredMessagesNarrow extends Narrow { ApiNarrow apiEncode() => [ApiNarrowIs(IsOperand.starred)]; @override - bool containsMessage(Message message) { + bool containsMessage(MessageBase message) { + if (message is! Message) return false; return message.flags.contains(MessageFlag.starred); } diff --git a/test/model/narrow_test.dart b/test/model/narrow_test.dart index 56df3aedc8..06c82ed117 100644 --- a/test/model/narrow_test.dart +++ b/test/model/narrow_test.dart @@ -7,6 +7,32 @@ import 'package:zulip/model/narrow.dart'; import '../example_data.dart' as eg; import 'narrow_checks.dart'; +/// A [MessageBase] subclass for testing. +// TODO(#1441): switch to outbox-messages instead +sealed class _TestMessage extends MessageBase { + @override + final int? id = null; + + _TestMessage() : super(senderId: eg.selfUser.userId, timestamp: 123456789); +} + +class _TestStreamMessage extends _TestMessage { + @override + final StreamConversation conversation; + + _TestStreamMessage({required ZulipStream stream, required String topic}) + : conversation = StreamConversation( + stream.streamId, TopicName(topic), displayRecipient: null); +} + +class _TestDmMessage extends _TestMessage { + @override + final DmConversation conversation; + + _TestDmMessage({required List allRecipientIds}) + : conversation = DmConversation(allRecipientIds: allRecipientIds); +} + void main() { group('SendableNarrow', () { test('ofMessage: stream message', () { @@ -33,6 +59,13 @@ void main() { eg.streamMessage(stream: otherStream, topic: 'topic'))).isFalse(); check(narrow.containsMessage( eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); + + check(narrow.containsMessage( + _TestDmMessage(allRecipientIds: [1]))).isFalse(); + check(narrow.containsMessage( + _TestStreamMessage(stream: otherStream, topic: 'topic'))).isFalse(); + check(narrow.containsMessage( + _TestStreamMessage(stream: stream, topic: 'topic'))).isTrue(); }); }); @@ -56,6 +89,15 @@ void main() { eg.streamMessage(stream: stream, topic: 'topic2'))).isFalse(); check(narrow.containsMessage( eg.streamMessage(stream: stream, topic: 'topic'))).isTrue(); + + check(narrow.containsMessage( + _TestDmMessage(allRecipientIds: [1]))).isFalse(); + check(narrow.containsMessage( + _TestStreamMessage(stream: otherStream, topic: 'topic'))).isFalse(); + check(narrow.containsMessage( + _TestStreamMessage(stream: stream, topic: 'topic2'))).isFalse(); + check(narrow.containsMessage( + _TestStreamMessage(stream: stream, topic: 'topic'))).isTrue(); }); }); @@ -176,6 +218,19 @@ void main() { check(narrow123.containsMessage(dm(user2, [user1, user3]))).isTrue(); check(narrow123.containsMessage(dm(user3, [user1, user2]))).isTrue(); }); + + test('containsMessage with non-Message', () { + final narrow = DmNarrow(allRecipientIds: [1, 2], selfUserId: 2); + + check(narrow.containsMessage( + _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + check(narrow.containsMessage( + _TestDmMessage(allRecipientIds: [2]))).isFalse(); + check(narrow.containsMessage( + _TestDmMessage(allRecipientIds: [2, 3]))).isFalse(); + check(narrow.containsMessage( + _TestDmMessage(allRecipientIds: [1, 2]))).isTrue(); + }); }); group('MentionsNarrow', () { @@ -188,6 +243,11 @@ void main() { eg.streamMessage(flags:[MessageFlag.mentioned]))).isTrue(); check(narrow.containsMessage( eg.streamMessage(flags: [MessageFlag.wildcardMentioned]))).isTrue(); + + check(narrow.containsMessage( + _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + check(narrow.containsMessage( + _TestDmMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); }); }); @@ -199,6 +259,11 @@ void main() { eg.streamMessage(flags: []))).isFalse(); check(narrow.containsMessage( eg.streamMessage(flags:[MessageFlag.starred]))).isTrue(); + + check(narrow.containsMessage( + _TestStreamMessage(stream: eg.stream(), topic: 'topic'))).isFalse(); + check(narrow.containsMessage( + _TestDmMessage(allRecipientIds: [eg.selfUser.userId]))).isFalse(); }); }); }