Skip to content

Commit dd2dbe0

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. --- OutboxMessage is designed to keep the timers internal to itself, and expose only the callback functions, to keep message store code simple. To make sure that OutboxMessage.dispose is always called when the outbox message is removed from the store, we expose an unmodifiable view of the actual map. For testing, similar to TypingNotifier.debugEnabled, we add MessageStoreImpl.debugOutboxEnabled in case a test does not intend to cover outbox messages.
1 parent 9b2013b commit dd2dbe0

File tree

11 files changed

+734
-15
lines changed

11 files changed

+734
-15
lines changed

lib/api/model/model.dart

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -559,6 +559,13 @@ class DmRecipient extends Recipient {
559559
DmRecipient({required this.allRecipientIds})
560560
: assert(isSortedWithoutDuplicates(allRecipientIds.toList()));
561561

562+
factory DmRecipient.fromDmDestination(DmDestination destination, {
563+
required int selfUserId,
564+
}) {
565+
assert(destination.userIds.contains(selfUserId));
566+
return DmRecipient(allRecipientIds: destination.userIds);
567+
}
568+
562569
/// The user IDs of all users in the thread, sorted numerically.
563570
///
564571
/// This lists the sender as well as all (other) recipients, and it

lib/model/message.dart

Lines changed: 313 additions & 7 deletions
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,175 @@ 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 Recipient> implements MessageBase<T> {
39+
OutboxMessage({
40+
required this.localMessageId,
41+
required int selfUserId,
42+
required this.content,
43+
required this.onDebounceTimeout,
44+
required this.onSendTimeLimitTimeout,
45+
}) : senderId = selfUserId,
46+
timestamp = (DateTime.timestamp().millisecondsSinceEpoch / 1000).toInt(),
47+
_state = OutboxMessageLifecycle.sending,
48+
_debounceTimer = Timer(kLocalEchoDebounceDuration, onDebounceTimeout),
49+
_sendTimeLimitTimer = Timer(kSendMessageTimeLimit, onSendTimeLimitTimeout);
50+
51+
static OutboxMessage fromDestination(MessageDestination destination, {
52+
required int localMessageId,
53+
required int selfUserId,
54+
required String content,
55+
required VoidCallback onDebounceTimeout,
56+
required VoidCallback onSendTimeLimitTimeout,
57+
}) {
58+
return switch (destination) {
59+
StreamDestination() => StreamOutboxMessage(
60+
localMessageId: localMessageId,
61+
selfUserId: selfUserId,
62+
recipient: StreamRecipient(destination.streamId, destination.topic),
63+
content: content,
64+
onDebounceTimeout: onDebounceTimeout,
65+
onSendTimeLimitTimeout: onSendTimeLimitTimeout,
66+
),
67+
DmDestination() => DmOutboxMessage(
68+
localMessageId: localMessageId,
69+
selfUserId: selfUserId,
70+
recipient: DmRecipient.fromDmDestination(destination, selfUserId: selfUserId),
71+
content: content,
72+
onDebounceTimeout: onDebounceTimeout,
73+
onSendTimeLimitTimeout: onSendTimeLimitTimeout,
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+
/// Called after [kLocalEchoDebounceDuration] if the outbox message is still
92+
/// [hidden].
93+
///
94+
/// If [hidden] gets set to false before the timeout, this will not get called.
95+
final VoidCallback onDebounceTimeout;
96+
final Timer _debounceTimer;
97+
98+
/// Called after the time limit ([kSendMessageTimeLimit]) for the message
99+
/// event to arrive.
100+
///
101+
/// If the [state] is [OutboxMessageLifecycle.failed] prior to the time limit
102+
/// this will not get called.
103+
final VoidCallback onSendTimeLimitTimeout;
104+
final Timer _sendTimeLimitTimer;
105+
106+
OutboxMessageLifecycle get state => _state;
107+
OutboxMessageLifecycle _state;
108+
set state(OutboxMessageLifecycle value) {
109+
assert(!_disposed);
110+
// See [OutboxMessageLifecycle] for valid state transitions.
111+
assert(_state != value);
112+
switch (value) {
113+
case OutboxMessageLifecycle.sending:
114+
assert(false);
115+
case OutboxMessageLifecycle.sent:
116+
assert(_state == OutboxMessageLifecycle.sending);
117+
case OutboxMessageLifecycle.failed:
118+
assert(_state == OutboxMessageLifecycle.sending || _state == OutboxMessageLifecycle.sent);
119+
_sendTimeLimitTimer.cancel();
120+
}
121+
_state = value;
122+
}
123+
124+
/// Whether the [OutboxMessage] will be hidden to [MessageListView] or not.
125+
///
126+
/// When set to false with [unhide], this cannot be toggled back to true again.
127+
bool get hidden => _hidden;
128+
bool _hidden = true;
129+
void unhide() {
130+
assert(!_disposed);
131+
assert(_hidden);
132+
_debounceTimer.cancel();
133+
_hidden = false;
134+
}
135+
136+
bool _disposed = false;
137+
/// Clean up all pending timers to prepare for abandoning this instance.
138+
///
139+
/// This must be called whenever the outbox message is removed from the store.
140+
void dispose() {
141+
assert(!_disposed);
142+
_disposed = true;
143+
_debounceTimer.cancel();
144+
_sendTimeLimitTimer.cancel();
145+
}
146+
}
147+
148+
class StreamOutboxMessage extends OutboxMessage<StreamRecipient> {
149+
StreamOutboxMessage({
150+
required super.localMessageId,
151+
required super.selfUserId,
152+
required this.recipient,
153+
required super.content,
154+
required super.onDebounceTimeout,
155+
required super.onSendTimeLimitTimeout,
156+
});
157+
158+
@override
159+
final StreamRecipient recipient;
160+
}
161+
162+
class DmOutboxMessage extends OutboxMessage<DmRecipient> {
163+
DmOutboxMessage({
164+
required super.localMessageId,
165+
required super.selfUserId,
166+
required this.recipient,
167+
required super.content,
168+
required super.onDebounceTimeout,
169+
required super.onSendTimeLimitTimeout,
170+
});
171+
172+
@override
173+
final DmRecipient recipient;
174+
}
11175

12176
/// The portion of [PerAccountStore] for messages and message lists.
13177
mixin MessageStore {
14178
/// All known messages, indexed by [Message.id].
15179
Map<int, Message> get messages;
16180

181+
/// Messages sent by the user, indexed by [OutboxMessage.localMessageId].
182+
Map<int, OutboxMessage> get outboxMessages;
183+
17184
Set<MessageListView> get debugMessageListViews;
18185

19186
void registerMessageList(MessageListView view);
@@ -24,6 +191,11 @@ mixin MessageStore {
24191
required String content,
25192
});
26193

194+
/// Remove from [outboxMessages] given the [localMessageId].
195+
///
196+
/// The message to remove must already exist.
197+
void removeOutboxMessage(int localMessageId);
198+
27199
/// Reconcile a batch of just-fetched messages with the store,
28200
/// mutating the list.
29201
///
@@ -41,11 +213,21 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore {
41213
MessageStoreImpl({required super.core})
42214
// There are no messages in InitialSnapshot, so we don't have
43215
// a use case for initializing MessageStore with nonempty [messages].
44-
: messages = {};
216+
: messages = {},
217+
_outboxMessages = {};
218+
219+
/// A fresh ID to use for [OutboxMessage.localMessageId],
220+
/// unique within the [PerAccountStore] instance.
221+
int _nextLocalMessageId = 0;
45222

46223
@override
47224
final Map<int, Message> messages;
48225

226+
@override
227+
late final UnmodifiableMapView<int, OutboxMessage> outboxMessages =
228+
UnmodifiableMapView(_outboxMessages);
229+
final Map<int, OutboxMessage> _outboxMessages;
230+
49231
final Set<MessageListView> _messageListViews = {};
50232

51233
@override
@@ -84,17 +266,111 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore {
84266
// [InheritedNotifier] to rebuild in the next frame) before the owner's
85267
// `dispose` or `onNewStore` is called. Discussion:
86268
// https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2086893
269+
270+
for (final outboxMessage in outboxMessages.values) {
271+
outboxMessage.dispose();
272+
}
273+
_outboxMessages.clear();
87274
}
88275

89276
@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,
277+
Future<void> sendMessage({required MessageDestination destination, required String content}) async {
278+
if (!debugOutboxEnabled) {
279+
await _apiSendMessage(connection,
280+
destination: destination,
281+
content: content,
282+
readBySender: true);
283+
return;
284+
}
285+
286+
final localMessageId = _nextLocalMessageId++;
287+
assert(!outboxMessages.containsKey(localMessageId));
288+
_outboxMessages[localMessageId] = OutboxMessage.fromDestination(destination,
289+
localMessageId: localMessageId,
290+
selfUserId: selfUserId,
95291
content: content,
96-
readBySender: true,
292+
onDebounceTimeout: () {
293+
assert(outboxMessages.containsKey(localMessageId));
294+
_unhideOutboxMessage(localMessageId);
295+
},
296+
onSendTimeLimitTimeout: () {
297+
assert(outboxMessages.containsKey(localMessageId));
298+
// This should be called before `_unhideOutboxMessage(localMessageId)`
299+
// to avoid unnecessarily notifying the listeners twice.
300+
_updateOutboxMessage(localMessageId, newState: OutboxMessageLifecycle.failed);
301+
_unhideOutboxMessage(localMessageId);
302+
},
97303
);
304+
305+
try {
306+
await _apiSendMessage(connection,
307+
destination: destination,
308+
content: content,
309+
readBySender: true,
310+
queueId: queueId,
311+
localId: localMessageId.toString());
312+
if (_outboxMessages[localMessageId]?.state == OutboxMessageLifecycle.failed) {
313+
// Reached time limit while request was pending.
314+
// No state update is needed.
315+
return;
316+
}
317+
_updateOutboxMessage(localMessageId, newState: OutboxMessageLifecycle.sent);
318+
} catch (e) {
319+
// This should be called before `_unhideOutboxMessage(localMessageId)`
320+
// to avoid unnecessarily notifying the listeners twice.
321+
_updateOutboxMessage(localMessageId, newState: OutboxMessageLifecycle.failed);
322+
_unhideOutboxMessage(localMessageId);
323+
rethrow;
324+
}
325+
}
326+
327+
/// Unhide the [OutboxMessage] with the given [localMessageId],
328+
/// and notify listeners if necessary.
329+
///
330+
/// This is a no-op if the outbox message does not exist or is not hidden.
331+
void _unhideOutboxMessage(int localMessageId) {
332+
final outboxMessage = outboxMessages[localMessageId];
333+
if (outboxMessage == null || !outboxMessage.hidden) {
334+
return;
335+
}
336+
outboxMessage.unhide();
337+
for (final view in _messageListViews) {
338+
view.handleOutboxMessage(outboxMessage);
339+
}
340+
}
341+
342+
/// Update the state of the [OutboxMessage] with the given [localMessageId],
343+
/// and notify listeners if necessary.
344+
///
345+
/// This is a no-op if the outbox message does not exists, or that
346+
/// [OutboxMessage.state] already equals [newState].
347+
void _updateOutboxMessage(int localMessageId, {
348+
required OutboxMessageLifecycle newState,
349+
}) {
350+
final outboxMessage = outboxMessages[localMessageId];
351+
if (outboxMessage == null || outboxMessage.state == newState) {
352+
return;
353+
}
354+
outboxMessage.state = newState;
355+
if (outboxMessage.hidden) {
356+
return;
357+
}
358+
for (final view in _messageListViews) {
359+
view.notifyListenersIfOutboxMessagePresent(localMessageId);
360+
}
361+
}
362+
363+
364+
@override
365+
void removeOutboxMessage(int localMessageId) {
366+
final removed = _outboxMessages.remove(localMessageId)?..dispose();
367+
if (removed == null) {
368+
assert(false, 'Removing unknown outbox message with localMessageId: $localMessageId');
369+
return;
370+
}
371+
for (final view in _messageListViews) {
372+
view.removeOutboxMessageIfExists(removed);
373+
}
98374
}
99375

100376
@override
@@ -132,6 +408,11 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore {
132408
// See [fetchedMessages] for reasoning.
133409
messages[event.message.id] = event.message;
134410

411+
if (event.localMessageId != null) {
412+
final localMessageId = int.parse(event.localMessageId!, radix: 10);
413+
_outboxMessages.remove(localMessageId)?.dispose();
414+
}
415+
135416
for (final view in _messageListViews) {
136417
view.handleMessageEvent(event);
137418
}
@@ -325,4 +606,29 @@ class MessageStoreImpl extends PerAccountStoreBase with MessageStore {
325606
// [Poll] is responsible for notifying the affected listeners.
326607
poll.handleSubmessageEvent(event);
327608
}
609+
610+
/// In debug mode, controls whether outbox messages should be created when
611+
/// [sendMessage] is called.
612+
///
613+
/// Outside of debug mode, this is always true and the setter has no effect.
614+
static bool get debugOutboxEnabled {
615+
bool result = true;
616+
assert(() {
617+
result = _debugOutboxEnabled;
618+
return true;
619+
}());
620+
return result;
621+
}
622+
static bool _debugOutboxEnabled = true;
623+
static set debugOutboxEnabled(bool value) {
624+
assert(() {
625+
_debugOutboxEnabled = value;
626+
return true;
627+
}());
628+
}
629+
630+
@visibleForTesting
631+
static void debugReset() {
632+
_debugOutboxEnabled = true;
633+
}
328634
}

0 commit comments

Comments
 (0)