Skip to content

Commit eeb6ef2

Browse files
committed
message: Create an outbox message on send; manage its states
While we do create outbox messages, there are in no way user-visible changes since the outbox messages don't end up in message list views. We create skeletons for helpers needed from message list view, but don't implement them yet, to make the diff smaller. For testing, similar to TypingNotifier.debugEnable, we add MessageStoreImpl.debugOutboxEnable for tests that do not intend to cover outbox messages. Some of the delays to fake responses added in tests are not necessary because the future of sendMessage is not completed immediately, but we still add them to keep the tests realistic.
1 parent 03aae66 commit eeb6ef2

11 files changed

+757
-19
lines changed

lib/model/message.dart

+311-9
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
1+
import 'dart:async';
2+
import 'dart:collection';
13
import 'dart:convert';
24

5+
import 'package:flutter/foundation.dart';
6+
37
import '../api/model/events.dart';
48
import '../api/model/model.dart';
59
import '../api/route/messages.dart';
@@ -8,12 +12,141 @@ import 'message_list.dart';
812
import 'store.dart';
913

1014
const _apiSendMessage = sendMessage; // Bit ugly; for alternatives, see: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20PerAccountStore.20methods/near/1545809
15+
const kLocalEchoDebounceDuration = Duration(milliseconds: 300);
16+
const kSendMessageTimeLimit = Duration(seconds: 10);
17+
18+
/// States outlining where an [OutboxMessage] is, in its lifecycle.
19+
///
20+
/// ```
21+
//// ┌─────────────────────────────────────┐
22+
/// │ Event received, │
23+
/// Send │ or we abandoned │
24+
/// immediately. │ 200. the queue. ▼
25+
/// (create) ──────────────► sending ──────► sent ────────────────► (delete)
26+
/// │ ▲
27+
/// │ 4xx or User │
28+
/// │ other error. cancels. │
29+
/// └────────► failed ────────────────────┘
30+
/// ```
31+
enum OutboxMessageLifecycle {
32+
sending,
33+
sent,
34+
failed,
35+
}
36+
37+
/// A message sent by the self-user.
38+
sealed class OutboxMessage<T extends Conversation> implements MessageBase<T> {
39+
OutboxMessage({
40+
required this.localMessageId,
41+
required int selfUserId,
42+
required this.content,
43+
}) : senderId = selfUserId,
44+
timestamp = (DateTime.timestamp().millisecondsSinceEpoch / 1000).toInt(),
45+
_state = OutboxMessageLifecycle.sending;
46+
47+
static OutboxMessage fromDestination(MessageDestination destination, {
48+
required int localMessageId,
49+
required int selfUserId,
50+
required String content,
51+
required int zulipFeatureLevel,
52+
required String? realmEmptyTopicDisplayName,
53+
}) {
54+
if (destination case DmDestination(:final userIds)) {
55+
assert(userIds.contains(selfUserId));
56+
}
57+
return switch (destination) {
58+
StreamDestination() => StreamOutboxMessage(
59+
localMessageId: localMessageId,
60+
selfUserId: selfUserId,
61+
conversation: StreamConversation(
62+
destination.streamId,
63+
destination.topic.interpretAsServer(
64+
zulipFeatureLevel: zulipFeatureLevel,
65+
realmEmptyTopicDisplayName: realmEmptyTopicDisplayName),
66+
displayRecipient: null),
67+
content: content,
68+
),
69+
DmDestination() => DmOutboxMessage(
70+
localMessageId: localMessageId,
71+
selfUserId: selfUserId,
72+
conversation: DmConversation(allRecipientIds: destination.userIds),
73+
content: content,
74+
),
75+
};
76+
}
77+
78+
/// ID corresponding to [MessageEvent.localMessageId], which uniquely
79+
/// identifies a locally echoed message in events from the same event queue.
80+
///
81+
/// See also [sendMessage].
82+
final int localMessageId;
83+
@override
84+
int? get id => null;
85+
@override
86+
final int senderId;
87+
@override
88+
final int timestamp;
89+
final String content;
90+
91+
OutboxMessageLifecycle get state => _state;
92+
OutboxMessageLifecycle _state;
93+
set state(OutboxMessageLifecycle value) {
94+
// See [OutboxMessageLifecycle] for valid state transitions.
95+
assert(_state != value);
96+
switch (value) {
97+
case OutboxMessageLifecycle.sending:
98+
assert(false);
99+
case OutboxMessageLifecycle.sent:
100+
assert(_state == OutboxMessageLifecycle.sending);
101+
case OutboxMessageLifecycle.failed:
102+
assert(_state == OutboxMessageLifecycle.sending || _state == OutboxMessageLifecycle.sent);
103+
}
104+
_state = value;
105+
}
106+
107+
/// Whether the [OutboxMessage] will be hidden to [MessageListView] or not.
108+
///
109+
/// When set to false with [unhide], this cannot be toggled back to true again.
110+
bool get hidden => _hidden;
111+
bool _hidden = true;
112+
void unhide() {
113+
assert(_hidden);
114+
_hidden = false;
115+
}
116+
}
117+
118+
class StreamOutboxMessage extends OutboxMessage<StreamConversation> {
119+
StreamOutboxMessage({
120+
required super.localMessageId,
121+
required super.selfUserId,
122+
required this.conversation,
123+
required super.content,
124+
});
125+
126+
@override
127+
final StreamConversation conversation;
128+
}
129+
130+
class DmOutboxMessage extends OutboxMessage<DmConversation> {
131+
DmOutboxMessage({
132+
required super.localMessageId,
133+
required super.selfUserId,
134+
required this.conversation,
135+
required super.content,
136+
});
137+
138+
@override
139+
final DmConversation conversation;
140+
}
11141

12142
/// The portion of [PerAccountStore] for messages and message lists.
13143
mixin MessageStore {
14144
/// All known messages, indexed by [Message.id].
15145
Map<int, Message> get messages;
16146

147+
/// Messages sent by the user, indexed by [OutboxMessage.localMessageId].
148+
Map<int, OutboxMessage> get outboxMessages;
149+
17150
Set<MessageListView> get debugMessageListViews;
18151

19152
void registerMessageList(MessageListView view);
@@ -24,6 +157,11 @@ mixin MessageStore {
24157
required String content,
25158
});
26159

160+
/// Remove from [outboxMessages] given the [localMessageId].
161+
///
162+
/// The message to remove must already exist.
163+
void removeOutboxMessage(int localMessageId);
164+
27165
/// Reconcile a batch of just-fetched messages with the store,
28166
/// mutating the list.
29167
///
@@ -38,14 +176,43 @@ mixin MessageStore {
38176
}
39177

40178
class MessageStoreImpl extends PerAccountStoreBase with MessageStore {
41-
MessageStoreImpl({required super.core})
179+
MessageStoreImpl({required super.core, required this.realmEmptyTopicDisplayName})
42180
// There are no messages in InitialSnapshot, so we don't have
43181
// a use case for initializing MessageStore with nonempty [messages].
44-
: messages = {};
182+
: messages = {},
183+
_outboxMessages = {},
184+
_outboxMessageDebounceTimers = {},
185+
_outboxMessageSendTimeLimitTimers = {};
186+
187+
/// A fresh ID to use for [OutboxMessage.localMessageId],
188+
/// unique within the [PerAccountStore] instance.
189+
int _nextLocalMessageId = 0;
190+
191+
final String? realmEmptyTopicDisplayName;
45192

46193
@override
47194
final Map<int, Message> messages;
48195

196+
@override
197+
late final UnmodifiableMapView<int, OutboxMessage> outboxMessages =
198+
UnmodifiableMapView(_outboxMessages);
199+
final Map<int, OutboxMessage> _outboxMessages;
200+
201+
/// A map of timers to unhide outbox messages after a delay,
202+
/// indexed by [OutboxMessage.localMessageId].
203+
///
204+
/// If the outbox message was unhidden prior to the timeout,
205+
/// its timer gets removed and cancelled.
206+
final Map<int, Timer> _outboxMessageDebounceTimers;
207+
208+
/// A map of timers to update outbox messages state to
209+
/// [OutboxMessageLifecycle.failed] after a delay,
210+
/// indexed by [OutboxMessage.localMessageId].
211+
///
212+
/// If the outbox message's state is set to [OutboxMessageLifecycle.failed]
213+
/// within the time limit, its timer gets removed and cancelled.
214+
final Map<int, Timer> _outboxMessageSendTimeLimitTimers;
215+
49216
final Set<MessageListView> _messageListViews = {};
50217

51218
@override
@@ -84,17 +251,120 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore {
84251
// [InheritedNotifier] to rebuild in the next frame) before the owner's
85252
// `dispose` or `onNewStore` is called. Discussion:
86253
// https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2086893
254+
255+
for (final localMessageId in outboxMessages.keys) {
256+
_outboxMessageDebounceTimers.remove(localMessageId)?.cancel();
257+
_outboxMessageSendTimeLimitTimers.remove(localMessageId)?.cancel();
258+
}
259+
_outboxMessages.clear();
260+
assert(_outboxMessageDebounceTimers.isEmpty);
261+
assert(_outboxMessageSendTimeLimitTimers.isEmpty);
87262
}
88263

89264
@override
90-
Future<void> sendMessage({required MessageDestination destination, required String content}) {
91-
// TODO implement outbox; see design at
92-
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3881.20Sending.20outbox.20messages.20is.20fraught.20with.20issues/near/1405739
93-
return _apiSendMessage(connection,
94-
destination: destination,
265+
Future<void> sendMessage({required MessageDestination destination, required String content}) async {
266+
if (!debugOutboxEnable) {
267+
await _apiSendMessage(connection,
268+
destination: destination,
269+
content: content,
270+
readBySender: true);
271+
return;
272+
}
273+
274+
final localMessageId = _nextLocalMessageId++;
275+
assert(!outboxMessages.containsKey(localMessageId));
276+
_outboxMessages[localMessageId] = OutboxMessage.fromDestination(destination,
277+
localMessageId: localMessageId,
278+
selfUserId: selfUserId,
95279
content: content,
96-
readBySender: true,
97-
);
280+
zulipFeatureLevel: zulipFeatureLevel,
281+
realmEmptyTopicDisplayName: realmEmptyTopicDisplayName);
282+
_outboxMessageDebounceTimers[localMessageId] = Timer(kLocalEchoDebounceDuration, () {
283+
assert(outboxMessages.containsKey(localMessageId));
284+
_unhideOutboxMessage(localMessageId);
285+
});
286+
_outboxMessageSendTimeLimitTimers[localMessageId] = Timer(kSendMessageTimeLimit, () {
287+
assert(outboxMessages.containsKey(localMessageId));
288+
// This should be called before `_unhideOutboxMessage(localMessageId)`
289+
// to avoid unnecessarily notifying the listeners twice.
290+
_updateOutboxMessage(localMessageId, newState: OutboxMessageLifecycle.failed);
291+
_unhideOutboxMessage(localMessageId);
292+
});
293+
294+
try {
295+
await _apiSendMessage(connection,
296+
destination: destination,
297+
content: content,
298+
readBySender: true,
299+
queueId: queueId,
300+
localId: localMessageId.toString());
301+
if (_outboxMessages[localMessageId]?.state == OutboxMessageLifecycle.failed) {
302+
// Reached time limit while request was pending.
303+
// No state update is needed.
304+
return;
305+
}
306+
_updateOutboxMessage(localMessageId, newState: OutboxMessageLifecycle.sent);
307+
} catch (e) {
308+
// This should be called before `_unhideOutboxMessage(localMessageId)`
309+
// to avoid unnecessarily notifying the listeners twice.
310+
_updateOutboxMessage(localMessageId, newState: OutboxMessageLifecycle.failed);
311+
_unhideOutboxMessage(localMessageId);
312+
rethrow;
313+
}
314+
}
315+
316+
/// Unhide the [OutboxMessage] with the given [localMessageId],
317+
/// and notify listeners if necessary.
318+
///
319+
/// This is a no-op if the outbox message does not exist or is not hidden.
320+
void _unhideOutboxMessage(int localMessageId) {
321+
final outboxMessage = outboxMessages[localMessageId];
322+
if (outboxMessage == null || !outboxMessage.hidden) {
323+
return;
324+
}
325+
_outboxMessageDebounceTimers.remove(localMessageId)?.cancel();
326+
outboxMessage.unhide();
327+
for (final view in _messageListViews) {
328+
view.handleOutboxMessage(outboxMessage);
329+
}
330+
}
331+
332+
/// Update the state of the [OutboxMessage] with the given [localMessageId],
333+
/// and notify listeners if necessary.
334+
///
335+
/// This is a no-op if the outbox message does not exists, or that
336+
/// [OutboxMessage.state] already equals [newState].
337+
void _updateOutboxMessage(int localMessageId, {
338+
required OutboxMessageLifecycle newState,
339+
}) {
340+
final outboxMessage = outboxMessages[localMessageId];
341+
if (outboxMessage == null || outboxMessage.state == newState) {
342+
return;
343+
}
344+
if (newState == OutboxMessageLifecycle.failed) {
345+
_outboxMessageSendTimeLimitTimers.remove(localMessageId)?.cancel();
346+
}
347+
outboxMessage.state = newState;
348+
if (outboxMessage.hidden) {
349+
return;
350+
}
351+
for (final view in _messageListViews) {
352+
view.notifyListenersIfOutboxMessagePresent(localMessageId);
353+
}
354+
}
355+
356+
@override
357+
void removeOutboxMessage(int localMessageId) {
358+
final removed = _outboxMessages.remove(localMessageId);
359+
_outboxMessageDebounceTimers.remove(localMessageId)?.cancel();
360+
_outboxMessageSendTimeLimitTimers.remove(localMessageId)?.cancel();
361+
if (removed == null) {
362+
assert(false, 'Removing unknown outbox message with localMessageId: $localMessageId');
363+
return;
364+
}
365+
for (final view in _messageListViews) {
366+
view.removeOutboxMessageIfExists(removed);
367+
}
98368
}
99369

100370
@override
@@ -132,6 +402,13 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore {
132402
// See [fetchedMessages] for reasoning.
133403
messages[event.message.id] = event.message;
134404

405+
if (event.localMessageId != null) {
406+
final localMessageId = int.parse(event.localMessageId!, radix: 10);
407+
_outboxMessages.remove(localMessageId);
408+
_outboxMessageDebounceTimers.remove(localMessageId)?.cancel();
409+
_outboxMessageSendTimeLimitTimers.remove(localMessageId)?.cancel();
410+
}
411+
135412
for (final view in _messageListViews) {
136413
view.handleMessageEvent(event);
137414
}
@@ -325,4 +602,29 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore {
325602
// [Poll] is responsible for notifying the affected listeners.
326603
poll.handleSubmessageEvent(event);
327604
}
605+
606+
/// In debug mode, controls whether outbox messages should be created when
607+
/// [sendMessage] is called.
608+
///
609+
/// Outside of debug mode, this is always true and the setter has no effect.
610+
static bool get debugOutboxEnable {
611+
bool result = true;
612+
assert(() {
613+
result = _debugOutboxEnable;
614+
return true;
615+
}());
616+
return result;
617+
}
618+
static bool _debugOutboxEnable = true;
619+
static set debugOutboxEnable(bool value) {
620+
assert(() {
621+
_debugOutboxEnable = value;
622+
return true;
623+
}());
624+
}
625+
626+
@visibleForTesting
627+
static void debugReset() {
628+
_debugOutboxEnable = true;
629+
}
328630
}

0 commit comments

Comments
 (0)