From 942aa87a52dc5b911c1f04b9ed774536e1ce1518 Mon Sep 17 00:00:00 2001
From: Zixuan James Li <zixuan@zulip.com>
Date: Tue, 25 Mar 2025 17:06:23 -0400
Subject: [PATCH 1/3] message [nfc]: Move sendMessage from PerAccountStore
MessageStore stands out as a better home for sendMessage, which was on
PerAccountStore before we extracted all the separate stores. This will
make it more natural to support outbox/local echoing.
---
lib/model/message.dart | 24 +++++++++++++++++++++++-
lib/model/store.dart | 13 +++----------
2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/lib/model/message.dart b/lib/model/message.dart
index d1886b810f..398c252a96 100644
--- a/lib/model/message.dart
+++ b/lib/model/message.dart
@@ -1,10 +1,14 @@
import 'dart:convert';
+import '../api/core.dart';
import '../api/model/events.dart';
import '../api/model/model.dart';
+import '../api/route/messages.dart';
import '../log.dart';
import 'message_list.dart';
+const _apiSendMessage = sendMessage; // Bit ugly; for alternatives, see: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20PerAccountStore.20methods/near/1545809
+
/// The portion of [PerAccountStore] for messages and message lists.
mixin MessageStore {
/// All known messages, indexed by [Message.id].
@@ -15,6 +19,11 @@ mixin MessageStore {
void registerMessageList(MessageListView view);
void unregisterMessageList(MessageListView view);
+ Future<void> sendMessage({
+ required MessageDestination destination,
+ required String content,
+ });
+
/// Reconcile a batch of just-fetched messages with the store,
/// mutating the list.
///
@@ -29,11 +38,13 @@ mixin MessageStore {
}
class MessageStoreImpl with MessageStore {
- MessageStoreImpl()
+ MessageStoreImpl({required this.connection})
// There are no messages in InitialSnapshot, so we don't have
// a use case for initializing MessageStore with nonempty [messages].
: messages = {};
+ final ApiConnection connection;
+
@override
final Map<int, Message> messages;
@@ -77,6 +88,17 @@ class MessageStoreImpl with MessageStore {
// https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2086893
}
+ @override
+ Future<void> sendMessage({required MessageDestination destination, required String content}) {
+ // TODO implement outbox; see design at
+ // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3881.20Sending.20outbox.20messages.20is.20fraught.20with.20issues/near/1405739
+ return _apiSendMessage(connection,
+ destination: destination,
+ content: content,
+ readBySender: true,
+ );
+ }
+
@override
void reconcileMessages(List<Message> messages) {
// What to do when some of the just-fetched messages are already known?
diff --git a/lib/model/store.dart b/lib/model/store.dart
index 8c5d5bc44a..aee4f6fd94 100644
--- a/lib/model/store.dart
+++ b/lib/model/store.dart
@@ -393,7 +393,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
typingStartedExpiryPeriod: Duration(milliseconds: initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds),
),
channels: channels,
- messages: MessageStoreImpl(),
+ messages: MessageStoreImpl(connection: connection),
unreads: Unreads(
initial: initialSnapshot.unreadMsgs,
selfUserId: account.userId,
@@ -829,16 +829,10 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
}
}
+ @override
Future<void> sendMessage({required MessageDestination destination, required String content}) {
assert(!_disposed);
-
- // TODO implement outbox; see design at
- // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3881.20Sending.20outbox.20messages.20is.20fraught.20with.20issues/near/1405739
- return _apiSendMessage(connection,
- destination: destination,
- content: content,
- readBySender: true,
- );
+ return _messages.sendMessage(destination: destination, content: content);
}
static List<CustomProfileField> _sortCustomProfileFields(List<CustomProfileField> initialCustomProfileFields) {
@@ -860,7 +854,6 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
String toString() => '${objectRuntimeType(this, 'PerAccountStore')}#${shortHash(this)}';
}
-const _apiSendMessage = sendMessage; // Bit ugly; for alternatives, see: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20PerAccountStore.20methods/near/1545809
const _tryResolveUrl = tryResolveUrl;
/// Like [Uri.resolve], but on failure return null instead of throwing.
From 5634c57c4dea97fe9614143f8db65c53a0c3b7d7 Mon Sep 17 00:00:00 2001
From: Zixuan James Li <zixuan@zulip.com>
Date: Thu, 27 Mar 2025 16:46:51 -0400
Subject: [PATCH 2/3] store test: Add a test for throwing on missing queueId
---
test/model/store_test.dart | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/test/model/store_test.dart b/test/model/store_test.dart
index 6eba7b1299..7bd6c5607f 100644
--- a/test/model/store_test.dart
+++ b/test/model/store_test.dart
@@ -296,6 +296,16 @@ void main() {
check(connection).isOpen.isFalse();
}));
+ test('GlobalStore.perAccount throws if missing queueId', () async {
+ final globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]);
+ globalStore.prepareRegisterQueueResponse = (connection) {
+ connection.prepare(json:
+ deepToJson(eg.initialSnapshot()) as Map<String, dynamic>
+ ..['queue_id'] = null);
+ };
+ await check(globalStore.perAccount(eg.selfAccount.id)).throws();
+ });
+
// TODO test insertAccount
group('GlobalStore.updateAccount', () {
From c5d0abfe4798bd415429ee19c7e6f4d191d20092 Mon Sep 17 00:00:00 2001
From: Zixuan James Li <zixuan@zulip.com>
Date: Thu, 27 Mar 2025 16:48:21 -0400
Subject: [PATCH 3/3] store [nfc]: Move queueId to PerAccountStore
This is an NFC because UpdateMachine would have thrown
(before this change) when the null-check is not on PerAccountStore.
This queueId will later be used for local-echoing.
---
lib/model/store.dart | 20 ++++++++++++--------
test/model/store_test.dart | 4 ++--
test/widgets/message_list_test.dart | 2 +-
3 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/lib/model/store.dart b/lib/model/store.dart
index aee4f6fd94..1bef907a12 100644
--- a/lib/model/store.dart
+++ b/lib/model/store.dart
@@ -360,11 +360,19 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
connection ??= globalStore.apiConnectionFromAccount(account);
assert(connection.zulipFeatureLevel == account.zulipFeatureLevel);
+ final queueId = initialSnapshot.queueId;
+ if (queueId == null) {
+ // The queueId is optional in the type, but should only be missing in the
+ // case of unauthenticated access to a web-public realm. We authenticated.
+ throw Exception("bad initial snapshot: missing queueId");
+ }
+
final realmUrl = account.realmUrl;
final channels = ChannelStoreImpl(initialSnapshot: initialSnapshot);
return PerAccountStore._(
globalStore: globalStore,
connection: connection,
+ queueId: queueId,
realmUrl: realmUrl,
realmWildcardMentionPolicy: initialSnapshot.realmWildcardMentionPolicy,
realmMandatoryTopics: initialSnapshot.realmMandatoryTopics,
@@ -408,6 +416,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
PerAccountStore._({
required GlobalStore globalStore,
required this.connection,
+ required this.queueId,
required this.realmUrl,
required this.realmWildcardMentionPolicy,
required this.realmMandatoryTopics,
@@ -447,6 +456,7 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
final GlobalStore _globalStore;
final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events
+ final String queueId;
UpdateMachine? get updateMachine => _updateMachine;
UpdateMachine? _updateMachine;
set updateMachine(UpdateMachine? value) {
@@ -1024,12 +1034,7 @@ class UpdateMachine {
UpdateMachine.fromInitialSnapshot({
required this.store,
required InitialSnapshot initialSnapshot,
- }) : queueId = initialSnapshot.queueId ?? (() {
- // The queueId is optional in the type, but should only be missing in the
- // case of unauthenticated access to a web-public realm. We authenticated.
- throw Exception("bad initial snapshot: missing queueId");
- })(),
- lastEventId = initialSnapshot.lastEventId {
+ }) : lastEventId = initialSnapshot.lastEventId {
store.updateMachine = this;
}
@@ -1103,7 +1108,6 @@ class UpdateMachine {
}
final PerAccountStore store;
- final String queueId;
int lastEventId;
bool _disposed = false;
@@ -1253,7 +1257,7 @@ class UpdateMachine {
final GetEventsResult result;
try {
result = await getEvents(store.connection,
- queueId: queueId, lastEventId: lastEventId);
+ queueId: store.queueId, lastEventId: lastEventId);
if (_disposed) return;
} catch (e, stackTrace) {
if (_disposed) return;
diff --git a/test/model/store_test.dart b/test/model/store_test.dart
index 7bd6c5607f..1dfbb51273 100644
--- a/test/model/store_test.dart
+++ b/test/model/store_test.dart
@@ -782,7 +782,7 @@ void main() {
..method.equals('GET')
..url.path.equals('/api/v1/events')
..url.queryParameters.deepEquals({
- 'queue_id': updateMachine.queueId,
+ 'queue_id': store.queueId,
'last_event_id': lastEventId.toString(),
});
}
@@ -947,7 +947,7 @@ void main() {
void prepareExpiredEventQueue() {
connection.prepare(apiException: eg.apiExceptionBadEventQueueId(
- queueId: updateMachine.queueId));
+ queueId: store.queueId));
}
Future<void> prepareHandleEventError() async {
diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart
index 6d2fdeef27..b20aaefca9 100644
--- a/test/widgets/message_list_test.dart
+++ b/test/widgets/message_list_test.dart
@@ -161,7 +161,7 @@ void main() {
updateMachine.poll();
updateMachine.debugPrepareLoopError(
- eg.apiExceptionBadEventQueueId(queueId: updateMachine.queueId));
+ eg.apiExceptionBadEventQueueId(queueId: store.queueId));
updateMachine.debugAdvanceLoop();
await tester.pump();
// Event queue has been replaced; but the [MessageList] hasn't been