Skip to content

local echo (2/n): Rearrange queueId and sendMessage within store #1455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 3, 2025
Merged

local echo (2/n): Rearrange queueId and sendMessage within store #1455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
942aa87
message [nfc]: Move sendMessage from PerAccountStore
PIG208 Mar 25, 2025
5634c57
store test: Add a test for throwing on missing queueId
PIG208 Mar 27, 2025
c5d0abf
store [nfc]: Move queueId to PerAccountStore
PIG208 Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion lib/model/message.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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].
Expand All @@ -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.
///
Expand All @@ -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;

Expand Down Expand Up @@ -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?
Expand Down
33 changes: 15 additions & 18 deletions lib/model/store.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -393,7 +401,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,
Expand All @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -829,16 +839,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) {
Expand All @@ -860,7 +864,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.
Expand Down Expand Up @@ -1031,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;
}

Expand Down Expand Up @@ -1110,7 +1108,6 @@ class UpdateMachine {
}

final PerAccountStore store;
final String queueId;
int lastEventId;

bool _disposed = false;
Expand Down Expand Up @@ -1260,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;
Expand Down
14 changes: 12 additions & 2 deletions test/model/store_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () {
Expand Down Expand Up @@ -772,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(),
});
}
Expand Down Expand Up @@ -937,7 +947,7 @@ void main() {

void prepareExpiredEventQueue() {
connection.prepare(apiException: eg.apiExceptionBadEventQueueId(
queueId: updateMachine.queueId));
queueId: store.queueId));
}

Future<void> prepareHandleEventError() async {
Expand Down
2 changes: 1 addition & 1 deletion test/widgets/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down