Skip to content

Commit 9539503

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 d877730 commit 9539503

File tree

9 files changed

+118
-35
lines changed

9 files changed

+118
-35
lines changed

lib/api/model/model.dart

+85-15
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 Iterable<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) =>
@@ -809,7 +882,7 @@ class DmDisplayRecipientListConverter extends JsonConverter<List<DmDisplayRecipi
809882
}
810883

811884
@JsonSerializable(fieldRename: FieldRename.snake)
812-
class DmMessage extends Message {
885+
class DmMessage extends Message<DmRecipient> {
813886
@override
814887
@JsonKey(includeToJson: true)
815888
String get type => 'private';
@@ -829,15 +902,12 @@ class DmMessage extends Message {
829902
@DmDisplayRecipientListConverter()
830903
final List<DmDisplayRecipient> displayRecipient;
831904

832-
/// The user IDs of all users in the thread, sorted numerically.
833-
///
834-
/// This lists the sender as well as all (other) recipients, and it
835-
/// lists each user just once. In particular the self-user is always
836-
/// included.
837-
///
838-
/// This is a result of [List.map], so it has an efficient `length`.
905+
// This is a result of [List.map], so it has an efficient `length`.
839906
Iterable<int> get allRecipientIds => displayRecipient.map((e) => e.id);
840907

908+
@override
909+
DmRecipient get recipient => DmRecipient(allRecipientIds: allRecipientIds);
910+
841911
DmMessage({
842912
required super.client,
843913
required super.content,

lib/api/model/model.g.dart

+9-2
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
@@ -213,14 +213,14 @@ class MessageStoreImpl with MessageStore {
213213
}
214214

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

222222
if (newTopic != origTopic) {
223-
message.topic = newTopic;
223+
message.recipient.topic = newTopic;
224224
}
225225

226226
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]),

test/model/unreads_test.dart

+9-9
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ void main() {
248248
final unreadChannelMessage = eg.streamMessage(flags: []);
249249
final readChannelMessage = eg.streamMessage(flags: [MessageFlag.read]);
250250

251-
final allMessages = [
251+
final allMessages = <Message>[
252252
unreadDmMessage, unreadChannelMessage,
253253
readDmMessage, readChannelMessage,
254254
];
@@ -315,7 +315,7 @@ void main() {
315315
if (isDirectMentioned) MessageFlag.mentioned,
316316
if (isWildcardMentioned) MessageFlag.wildcardMentioned,
317317
];
318-
final message = isStream
318+
final Message message = isStream
319319
? eg.streamMessage(flags: flags)
320320
: eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: flags);
321321
model.handleMessageEvent(eg.messageEvent(message));
@@ -402,7 +402,7 @@ void main() {
402402
for (final isKnownToModel in [true, false]) {
403403
for (final isRead in [false, true]) {
404404
final baseFlags = [if (isRead) MessageFlag.read];
405-
for (final (messageDesc, message) in [
405+
for (final (messageDesc, message) in <(String, Message)>[
406406
('stream', eg.streamMessage(flags: baseFlags)),
407407
('1:1 dm', eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: baseFlags)),
408408
]) {
@@ -662,7 +662,7 @@ void main() {
662662
final message13 = eg.streamMessage(id: 13, stream: stream2, topic: 'b', flags: []);
663663
final message14 = eg.streamMessage(id: 14, stream: stream2, topic: 'b', flags: [MessageFlag.mentioned]);
664664

665-
final messages = [
665+
final messages = <Message>[
666666
message1, message2, message3, message4, message5,
667667
message6, message7, message8, message9, message10,
668668
message11, message12, message13, message14,
@@ -849,7 +849,7 @@ void main() {
849849
// That case is indistinguishable from an unread that's unknown to
850850
// the model, so we get coverage for that case too.
851851
test('add flag: ${mentionFlag.name}', () {
852-
final messages = [
852+
final messages = <Message>[
853853
eg.streamMessage(flags: []),
854854
eg.streamMessage(flags: [MessageFlag.read]),
855855
eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: []),
@@ -886,7 +886,7 @@ void main() {
886886
// That case is indistinguishable from an unread that's unknown to
887887
// the model, so we get coverage for that case too.
888888
test('remove flag: ${mentionFlag.name}', () {
889-
final messages = [
889+
final messages = <Message>[
890890
eg.streamMessage(flags: [mentionFlag]),
891891
eg.streamMessage(flags: [mentionFlag, MessageFlag.read]),
892892
eg.dmMessage(from: eg.otherUser, to: [eg.selfUser], flags: [mentionFlag]),
@@ -925,7 +925,7 @@ void main() {
925925
final message2 = eg.streamMessage(id: 2, flags: [MessageFlag.mentioned]);
926926
final message3 = eg.dmMessage(id: 3, from: eg.otherUser, to: [eg.selfUser], flags: []);
927927
final message4 = eg.dmMessage(id: 4, from: eg.otherUser, to: [eg.selfUser], flags: [MessageFlag.wildcardMentioned]);
928-
final messages = [message1, message2, message3, message4];
928+
final messages = <Message>[message1, message2, message3, message4];
929929

930930
prepare();
931931
fillWithMessages([message1, message2, message3, message4]);
@@ -974,7 +974,7 @@ void main() {
974974
final message13 = eg.streamMessage(id: 13, stream: stream2, topic: 'b', flags: []);
975975
final message14 = eg.streamMessage(id: 14, stream: stream2, topic: 'b', flags: [MessageFlag.mentioned]);
976976

977-
final messages = [
977+
final messages = <Message>[
978978
message1, message2, message3, message4, message5,
979979
message6, message7, message8, message9, message10,
980980
message11, message12, message13, message14,
@@ -1086,7 +1086,7 @@ void main() {
10861086
final message13 = eg.streamMessage(id: 13, stream: stream2, topic: 'b', flags: [MessageFlag.read]);
10871087
final message14 = eg.streamMessage(id: 14, stream: stream2, topic: 'b', flags: [MessageFlag.mentioned, MessageFlag.read]);
10881088

1089-
final messages = [
1089+
final messages = <Message>[
10901090
message1, message2, message3, message4, message5,
10911091
message6, message7, message8, message9, message10,
10921092
message11, message12, message13, message14,

test/widgets/message_list_test.dart

+1-1
Original file line numberDiff line numberDiff line change
@@ -538,7 +538,7 @@ void main() {
538538
final streamMessage = eg.streamMessage();
539539
final topicNarrow = TopicNarrow.ofMessage(streamMessage);
540540

541-
for (final (description, message, narrow) in [
541+
for (final (description, message, narrow) in <(String, Message, SendableNarrow)>[
542542
('typing in dm', dmMessage, dmNarrow),
543543
('typing in topic', streamMessage, topicNarrow),
544544
]) {

0 commit comments

Comments
 (0)