Skip to content

Commit 0922066

Browse files
committed
api [nfc]: Pull out MessageBase and Recipient
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. This requiring specifying the element type of `List`s in some tests (or in some cases, the type of a variable declared). This is a side effect of making `StreamMessage` and `DmMessage` extend `Message<T>` with different `T`'s. When both appear in the same `List`, the upper bound is `Object`, instead of the more specific `Message<Recipient>`. See "least-upper-bound" tagged issues for reference: https://github.com/dart-lang/language/issues?q=state%3Aopen%20label%3A%22least-upper-bound%22 We extracted Recipient instead of using MessageDestination because they are foundamentally different. Recipient 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.
1 parent 594a316 commit 0922066

File tree

9 files changed

+128
-40
lines changed

9 files changed

+128
-40
lines changed

lib/api/model/model.dart

+94-17
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import 'package:json_annotation/json_annotation.dart';
22

3+
import '../../model/algorithms.dart';
4+
import '../route/messages.dart';
35
import 'events.dart';
46
import 'initial_snapshot.dart';
57
import 'reaction.dart';
@@ -531,10 +533,68 @@ String? tryParseEmojiCodeToUnicode(String emojiCode) {
531533
}
532534
}
533535

536+
/// As in [MessageBase.recipient].
537+
///
538+
/// Different from [MessageDestination], this information comes from
539+
/// [getMessages] or [getEvents], identifying the conversation that contains a
540+
/// message.
541+
sealed class Recipient {}
542+
543+
/// The recipient of a stream message.
544+
@JsonSerializable(fieldRename: FieldRename.snake, createToJson: false)
545+
class StreamRecipient extends Recipient {
546+
StreamRecipient(this.streamId, this.topic);
547+
548+
int streamId;
549+
550+
@JsonKey(name: 'subject')
551+
TopicName topic;
552+
553+
factory StreamRecipient.fromJson(Map<String, dynamic> json) =>
554+
_$StreamRecipientFromJson(json);
555+
}
556+
557+
/// The recipient of a DM message.
558+
class DmRecipient extends Recipient {
559+
DmRecipient({required this.allRecipientIds})
560+
: assert(isSortedWithoutDuplicates(allRecipientIds.toList()));
561+
562+
/// The user IDs of all users in the thread, sorted numerically.
563+
///
564+
/// This lists the sender as well as all (other) recipients, and it
565+
/// lists each user just once. In particular the self-user is always
566+
/// included.
567+
///
568+
/// This is required to have an efficient `length`.
569+
final List<int> allRecipientIds;
570+
}
571+
572+
/// A message or message-like object, for showing in a message list.
573+
///
574+
/// Other than [Message], we use this for "outbox messages",
575+
/// representing outstanding [sendMessage] requests.
576+
abstract class MessageBase<T extends Recipient> {
577+
/// The Zulip message ID.
578+
///
579+
/// If null, the message doesn't have an ID acknowledged by the server
580+
/// (e.g.: a locally-echoed message).
581+
int? get id;
582+
583+
int get senderId;
584+
int get timestamp;
585+
586+
/// The recipient of this message.
587+
// When implementing this, the return type should be either [StreamRecipient]
588+
// or [DmRecipient]; it should never be [Recipient], because we
589+
// expect a concrete subclass of [MessageBase] to represent either
590+
// a channel message or a DM message, not both.
591+
T get recipient;
592+
}
593+
534594
/// As in the get-messages response.
535595
///
536596
/// https://zulip.com/api/get-messages#response
537-
sealed class Message {
597+
sealed class Message<T extends Recipient> implements MessageBase<T> {
538598
// final String? avatarUrl; // Use [User.avatarUrl] instead; will live-update
539599
final String client;
540600
String content;
@@ -544,6 +604,7 @@ sealed class Message {
544604
@JsonKey(readValue: MessageEditState._readFromMessage, fromJson: Message._messageEditStateFromJson)
545605
MessageEditState editState;
546606

607+
@override
547608
final int id;
548609
bool isMeMessage;
549610
int? lastEditTimestamp;
@@ -554,13 +615,15 @@ sealed class Message {
554615
final int recipientId;
555616
final String senderEmail;
556617
final String senderFullName;
618+
@override
557619
final int senderId;
558620
final String senderRealmStr;
559621

560622
/// Poll data if "submessages" describe a poll, `null` otherwise.
561623
@JsonKey(name: 'submessages', readValue: _readPoll, fromJson: Poll.fromJson, toJson: Poll.toJson)
562624
Poll? poll;
563625

626+
@override
564627
final int timestamp;
565628
String get type;
566629

@@ -619,6 +682,8 @@ sealed class Message {
619682
required this.matchTopic,
620683
});
621684

685+
// TODO(dart): This has to be a static method, because factories/constructors
686+
// do not support type parameters: https://github.com/dart-lang/language/issues/647
622687
static Message fromJson(Map<String, dynamic> json) {
623688
final type = json['type'] as String;
624689
if (type == 'stream') return StreamMessage.fromJson(json);
@@ -715,7 +780,7 @@ extension type const TopicName(String _value) {
715780
}
716781

717782
@JsonSerializable(fieldRename: FieldRename.snake)
718-
class StreamMessage extends Message {
783+
class StreamMessage extends Message<StreamRecipient> {
719784
@override
720785
@JsonKey(includeToJson: true)
721786
String get type => 'stream';
@@ -726,14 +791,23 @@ class StreamMessage extends Message {
726791
@JsonKey(required: true, disallowNullValue: true)
727792
String? displayRecipient;
728793

729-
int streamId;
794+
@JsonKey(includeToJson: true)
795+
int get streamId => recipient.streamId;
730796

731797
// The topic/subject is documented to be present on DMs too, just empty.
732798
// We ignore it on DMs; if a future server introduces distinct topics in DMs,
733799
// that will need new UI that we'll design then as part of that feature,
734800
// and ignoring the topics seems as good a fallback behavior as any.
735-
@JsonKey(name: 'subject')
736-
TopicName topic;
801+
@JsonKey(name: 'subject', includeToJson: true)
802+
TopicName get topic => recipient.topic;
803+
804+
@override
805+
@JsonKey(readValue: _readRecipient, includeToJson: false)
806+
StreamRecipient recipient;
807+
808+
static Map<String, dynamic> _readRecipient(Map<dynamic, dynamic> json, String key) {
809+
return {'stream_id': json['stream_id'], 'subject': json['subject']};
810+
}
737811

738812
StreamMessage({
739813
required super.client,
@@ -754,8 +828,7 @@ class StreamMessage extends Message {
754828
required super.matchContent,
755829
required super.matchTopic,
756830
required this.displayRecipient,
757-
required this.streamId,
758-
required this.topic,
831+
required this.recipient,
759832
});
760833

761834
factory StreamMessage.fromJson(Map<String, dynamic> json) =>
@@ -766,7 +839,7 @@ class StreamMessage extends Message {
766839
}
767840

768841
@JsonSerializable(fieldRename: FieldRename.snake)
769-
class DmMessage extends Message {
842+
class DmMessage extends Message<DmRecipient> {
770843
@override
771844
@JsonKey(includeToJson: true)
772845
String get type => 'private';
@@ -781,20 +854,24 @@ class DmMessage extends Message {
781854
/// included.
782855
// TODO(server): Document that it's all users. That statement is based on
783856
// reverse-engineering notes in zulip-mobile:src/api/modelTypes.js at PmMessage.
784-
@JsonKey(name: 'display_recipient', fromJson: _allRecipientIdsFromJson, toJson: _allRecipientIdsToJson)
785-
final List<int> allRecipientIds;
857+
@JsonKey(name: 'display_recipient', toJson: _allRecipientIdsToJson, includeToJson: true)
858+
List<int> get allRecipientIds => recipient.allRecipientIds;
786859

787-
static List<int> _allRecipientIdsFromJson(Object? json) {
788-
return (json as List<dynamic>).map(
789-
(element) => ((element as Map<String, dynamic>)['id'] as num).toInt()
790-
).toList(growable: false)
791-
..sort();
792-
}
860+
@override
861+
@JsonKey(name: 'display_recipient', fromJson: _recipientFromJson, includeToJson: false)
862+
final DmRecipient recipient;
793863

794864
static List<Map<String, dynamic>> _allRecipientIdsToJson(List<int> allRecipientIds) {
795865
return allRecipientIds.map((element) => {'id': element}).toList();
796866
}
797867

868+
static DmRecipient _recipientFromJson(List<dynamic> json) {
869+
return DmRecipient(allRecipientIds: json.map(
870+
(element) => ((element as Map<String, dynamic>)['id'] as num).toInt()
871+
).toList(growable: false)
872+
..sort());
873+
}
874+
798875
DmMessage({
799876
required super.client,
800877
required super.content,
@@ -813,7 +890,7 @@ class DmMessage extends Message {
813890
required super.flags,
814891
required super.matchContent,
815892
required super.matchTopic,
816-
required this.allRecipientIds,
893+
required this.recipient,
817894
});
818895

819896
factory DmMessage.fromJson(Map<String, dynamic> json) =>

lib/api/model/model.g.dart

+10-5
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

lib/model/message.dart

+2-2
Original file line numberDiff line numberDiff line change
@@ -211,14 +211,14 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore {
211211
}
212212

213213
if (newStreamId != origStreamId) {
214-
message.streamId = newStreamId;
214+
message.recipient.streamId = newStreamId;
215215
// See [StreamMessage.displayRecipient] on why the invalidation is
216216
// needed.
217217
message.displayRecipient = null;
218218
}
219219

220220
if (newTopic != origTopic) {
221-
message.topic = newTopic;
221+
message.recipient.topic = newTopic;
222222
}
223223

224224
if (!wasResolveOrUnresolve

lib/model/narrow.dart

+1
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,7 @@ class DmNarrow extends Narrow implements SendableNarrow {
235235
/// See also:
236236
/// * [otherRecipientIds], an alternate way of identifying the conversation.
237237
/// * [DmMessage.allRecipientIds], which provides this same format.
238+
/// * [DmRecipient.allRecipientIds], which also provides this same format.
238239
final List<int> allRecipientIds;
239240

240241
/// The user ID of the self-user.

test/api/model/model_checks.dart

+7-2
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,13 @@ extension UserChecks on Subject<User> {
2424
extension ZulipStreamChecks on Subject<ZulipStream> {
2525
}
2626

27+
extension MessageBaseChecks<T extends Recipient> on Subject<MessageBase<T>> {
28+
Subject<int?> get id => has((e) => e.id, 'id');
29+
Subject<int> get senderId => has((e) => e.senderId, 'senderId');
30+
Subject<int> get timestamp => has((e) => e.timestamp, 'timestamp');
31+
Subject<T> get recipient => has((e) => e.recipient, 'recipient');
32+
}
33+
2734
extension MessageChecks on Subject<Message> {
2835
Subject<String> get client => has((e) => e.client, 'client');
2936
Subject<String> get content => has((e) => e.content, 'content');
@@ -36,10 +43,8 @@ extension MessageChecks on Subject<Message> {
3643
Subject<int> get recipientId => has((e) => e.recipientId, 'recipientId');
3744
Subject<String> get senderEmail => has((e) => e.senderEmail, 'senderEmail');
3845
Subject<String> get senderFullName => has((e) => e.senderFullName, 'senderFullName');
39-
Subject<int> get senderId => has((e) => e.senderId, 'senderId');
4046
Subject<String> get senderRealmStr => has((e) => e.senderRealmStr, 'senderRealmStr');
4147
Subject<Poll?> get poll => has((e) => e.poll, 'poll');
42-
Subject<int> get timestamp => has((e) => e.timestamp, 'timestamp');
4348
Subject<String> get type => has((e) => e.type, 'type');
4449
Subject<List<MessageFlag>> get flags => has((e) => e.flags, 'flags');
4550
Subject<String?> get matchContent => has((e) => e.matchContent, 'matchContent');

test/model/message_list_test.dart

+1-1
Original file line numberDiff line numberDiff line change
@@ -526,7 +526,7 @@ void main() {
526526
test('unmute a topic -> refetch from scratch', () => awaitFakeAsync((async) async {
527527
await prepare(narrow: const CombinedFeedNarrow());
528528
await prepareMutes(true);
529-
final messages = [
529+
final messages = <Message>[
530530
eg.dmMessage(id: 1, from: eg.otherUser, to: [eg.selfUser]),
531531
eg.streamMessage(id: 2, stream: stream, topic: topic),
532532
];

test/model/message_test.dart

+3-3
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ void main() {
8282
final message1 = eg.streamMessage();
8383
final message2 = eg.streamMessage();
8484
final message3 = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]);
85-
final messages = [message1, message2, message3];
85+
final messages = <Message>[message1, message2, message3];
8686
store.reconcileMessages(messages);
8787
check(messages).deepEquals(
8888
[message1, message2, message3]
@@ -97,7 +97,7 @@ void main() {
9797
final message1 = eg.streamMessage();
9898
final message2 = eg.streamMessage();
9999
final message3 = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]);
100-
final messages = [message1, message2, message3];
100+
final messages = <Message>[message1, message2, message3];
101101
await addMessages(messages);
102102
final newMessage = eg.streamMessage();
103103
store.reconcileMessages([newMessage]);
@@ -137,7 +137,7 @@ void main() {
137137

138138
test('from not-empty', () async {
139139
await prepare();
140-
final messages = [
140+
final messages = <Message>[
141141
eg.streamMessage(),
142142
eg.streamMessage(),
143143
eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]),

0 commit comments

Comments
 (0)