Skip to content

Commit 888ec02

Browse files
committed
msglist: Handle UserTopicEvent, hiding/showing messages as needed
In particular this will allow us to start offering UI for muting and unmuting topics, and have the message list the user is looking at update appropriately when they do so. Fixes: #421
1 parent 96438fa commit 888ec02

File tree

4 files changed

+256
-0
lines changed

4 files changed

+256
-0
lines changed

lib/model/message.dart

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,12 @@ class MessageStoreImpl with MessageStore {
8282
}
8383
}
8484

85+
void handleUserTopicEvent(UserTopicEvent event) {
86+
for (final view in _messageListViews) {
87+
view.handleUserTopicEvent(event);
88+
}
89+
}
90+
8591
void handleMessageEvent(MessageEvent event) {
8692
// If the message is one we already know about (from a fetch),
8793
// clobber it with the one from the event system.

lib/model/message_list.dart

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import 'algorithms.dart';
88
import 'content.dart';
99
import 'narrow.dart';
1010
import 'store.dart';
11+
import 'stream.dart';
1112

1213
/// The number of messages to fetch in each request.
1314
const kMessageListFetchBatchSize = 100; // TODO tune
@@ -158,6 +159,38 @@ mixin _MessageSequence {
158159
_processMessage(messages.length - 1);
159160
}
160161

162+
/// Removes all messages from the list that satisfy [test].
163+
///
164+
/// Returns true if any messages were removed, false otherwise.
165+
bool _removeMessagesWhere(bool Function(Message) test) {
166+
// Before we find a message to remove, there's no need to copy elements.
167+
// This is like the loop below, but simplified for `target == candidate`.
168+
int candidate = 0;
169+
while (true) {
170+
if (candidate == messages.length) return false;
171+
if (test(messages[candidate])) break;
172+
candidate++;
173+
}
174+
175+
int target = candidate;
176+
candidate++;
177+
assert(contents.length == messages.length);
178+
while (candidate < messages.length) {
179+
if (test(messages[candidate])) {
180+
candidate++;
181+
continue;
182+
}
183+
messages[target] = messages[candidate];
184+
contents[target] = contents[candidate];
185+
target++; candidate++;
186+
}
187+
messages.length = target;
188+
contents.length = target;
189+
assert(contents.length == messages.length);
190+
_reprocessAll();
191+
return true;
192+
}
193+
161194
/// Removes the given messages, if present.
162195
///
163196
/// Returns true if at least one message was present, false otherwise.
@@ -388,6 +421,23 @@ class MessageListView with ChangeNotifier, _MessageSequence {
388421
}
389422
}
390423

424+
/// Whether this event could affect the result that [_messageVisible]
425+
/// would ever have returned for any possible message in this message list.
426+
VisibilityEffect _canAffectVisibility(UserTopicEvent event) {
427+
switch (narrow) {
428+
case CombinedFeedNarrow():
429+
return store.willChangeIfTopicVisible(event);
430+
431+
case StreamNarrow(:final streamId):
432+
if (event.streamId != streamId) return VisibilityEffect.none;
433+
return store.willChangeIfTopicVisibleInStream(event);
434+
435+
case TopicNarrow():
436+
case DmNarrow():
437+
return VisibilityEffect.none;
438+
}
439+
}
440+
391441
/// Whether [_messageVisible] is true for all possible messages.
392442
///
393443
/// This is useful for an optimization.
@@ -473,6 +523,31 @@ class MessageListView with ChangeNotifier, _MessageSequence {
473523
}
474524
}
475525

526+
void handleUserTopicEvent(UserTopicEvent event) {
527+
switch (_canAffectVisibility(event)) {
528+
case VisibilityEffect.none:
529+
return;
530+
531+
case VisibilityEffect.muted:
532+
if (_removeMessagesWhere((message) =>
533+
(message is StreamMessage
534+
&& message.streamId == event.streamId
535+
&& message.topic == event.topicName))) {
536+
notifyListeners();
537+
}
538+
539+
case VisibilityEffect.unmuted:
540+
// TODO get the newly-unmuted messages from the message store
541+
// For now, we simplify the task by just refetching this message list
542+
// from scratch.
543+
if (fetched) {
544+
_reset();
545+
notifyListeners();
546+
fetchInitial();
547+
}
548+
}
549+
}
550+
476551
void handleDeleteMessageEvent(DeleteMessageEvent event) {
477552
if (_removeMessagesById(event.messageIds)) {
478553
notifyListeners();

lib/model/store.dart

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -473,6 +473,8 @@ class PerAccountStore extends ChangeNotifier with StreamStore, MessageStore {
473473
notifyListeners();
474474
} else if (event is UserTopicEvent) {
475475
assert(debugLog("server event: user_topic"));
476+
_messages.handleUserTopicEvent(event);
477+
// Update _streams last, so other handlers can compare to the old value.
476478
_streams.handleUserTopicEvent(event);
477479
notifyListeners();
478480
} else if (event is MessageEvent) {

test/model/message_list_test.dart

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,179 @@ void main() {
270270
check(model).fetched.isFalse();
271271
});
272272

273+
group('UserTopicEvent', () {
274+
// The StreamStore.willChangeIfTopicVisible/InStream methods have their own
275+
// thorough unit tests. So these tests focus on the rest of the logic.
276+
277+
final stream = eg.stream();
278+
const String topic = 'foo';
279+
280+
Future<void> setVisibility(UserTopicVisibilityPolicy policy) async {
281+
await store.handleEvent(eg.userTopicEvent(stream.streamId, topic, policy));
282+
}
283+
284+
/// (Should run after `prepare`.)
285+
Future<void> prepareMutes([
286+
bool streamMuted = false,
287+
UserTopicVisibilityPolicy policy = UserTopicVisibilityPolicy.none,
288+
]) async {
289+
await store.addStream(stream);
290+
await store.addSubscription(eg.subscription(stream, isMuted: streamMuted));
291+
await setVisibility(policy);
292+
}
293+
294+
void checkHasMessageIds(Iterable<int> messageIds) {
295+
check(model.messages.map((m) => m.id)).deepEquals(messageIds);
296+
}
297+
298+
test('mute a visible topic', () async {
299+
await prepare(narrow: const CombinedFeedNarrow());
300+
await prepareMutes();
301+
final otherStream = eg.stream();
302+
await store.addStream(otherStream);
303+
await store.addSubscription(eg.subscription(otherStream));
304+
await prepareMessages(foundOldest: true, messages: [
305+
eg.streamMessage(id: 1, stream: stream, topic: 'bar'),
306+
eg.streamMessage(id: 2, stream: stream, topic: 'foo'),
307+
eg.streamMessage(id: 3, stream: otherStream, topic: 'elsewhere'),
308+
eg.dmMessage( id: 4, from: eg.otherUser, to: [eg.selfUser]),
309+
]);
310+
checkHasMessageIds([1, 2, 3, 4]);
311+
312+
await setVisibility(UserTopicVisibilityPolicy.muted);
313+
checkNotifiedOnce();
314+
checkHasMessageIds([1, 3, 4]);
315+
});
316+
317+
test('in CombinedFeedNarrow, use combined-feed visibility', () async {
318+
// Compare the parallel StreamNarrow test below.
319+
await prepare(narrow: const CombinedFeedNarrow());
320+
// Mute the stream, so that combined-feed vs. stream visibility differ.
321+
await prepareMutes(true, UserTopicVisibilityPolicy.followed);
322+
await prepareMessages(foundOldest: true, messages: [
323+
eg.streamMessage(id: 1, stream: stream, topic: 'foo'),
324+
]);
325+
checkHasMessageIds([1]);
326+
327+
// Dropping from followed to none hides the message
328+
// (whereas it'd have no effect in a stream narrow).
329+
await setVisibility(UserTopicVisibilityPolicy.none);
330+
checkNotifiedOnce();
331+
checkHasMessageIds([]);
332+
333+
// Dropping from none to muted has no further effect
334+
// (whereas it'd hide the message in a stream narrow).
335+
await setVisibility(UserTopicVisibilityPolicy.muted);
336+
checkNotNotified();
337+
checkHasMessageIds([]);
338+
});
339+
340+
test('in StreamNarrow, use stream visibility', () async {
341+
// Compare the parallel CombinedFeedNarrow test above.
342+
await prepare(narrow: StreamNarrow(stream.streamId));
343+
// Mute the stream, so that combined-feed vs. stream visibility differ.
344+
await prepareMutes(true, UserTopicVisibilityPolicy.followed);
345+
await prepareMessages(foundOldest: true, messages: [
346+
eg.streamMessage(id: 1, stream: stream, topic: 'foo'),
347+
]);
348+
checkHasMessageIds([1]);
349+
350+
// Dropping from followed to none has no effect
351+
// (whereas it'd hide the message in the combined feed).
352+
await setVisibility(UserTopicVisibilityPolicy.none);
353+
checkNotNotified();
354+
checkHasMessageIds([1]);
355+
356+
// Dropping from none to muted hides the message
357+
// (whereas it'd have no effect in a stream narrow).
358+
await setVisibility(UserTopicVisibilityPolicy.muted);
359+
checkNotifiedOnce();
360+
checkHasMessageIds([]);
361+
});
362+
363+
test('in TopicNarrow, stay visible', () async {
364+
await prepare(narrow: TopicNarrow(stream.streamId, 'foo'));
365+
await prepareMutes();
366+
await prepareMessages(foundOldest: true, messages: [
367+
eg.streamMessage(id: 1, stream: stream, topic: 'foo'),
368+
]);
369+
checkHasMessageIds([1]);
370+
371+
await setVisibility(UserTopicVisibilityPolicy.muted);
372+
checkNotNotified();
373+
checkHasMessageIds([1]);
374+
});
375+
376+
test('in DmNarrow, do nothing (smoke test)', () async {
377+
await prepare(narrow:
378+
DmNarrow.withUser(eg.otherUser.userId, selfUserId: eg.selfUser.userId));
379+
await prepareMutes();
380+
await prepareMessages(foundOldest: true, messages: [
381+
eg.dmMessage(id: 1, from: eg.otherUser, to: [eg.selfUser]),
382+
]);
383+
checkHasMessageIds([1]);
384+
385+
await setVisibility(UserTopicVisibilityPolicy.muted);
386+
checkNotNotified();
387+
checkHasMessageIds([1]);
388+
});
389+
390+
test('no affected messages -> no notification', () async {
391+
await prepare(narrow: const CombinedFeedNarrow());
392+
await prepareMutes();
393+
await prepareMessages(foundOldest: true, messages: [
394+
eg.streamMessage(id: 1, stream: stream, topic: 'bar'),
395+
]);
396+
checkHasMessageIds([1]);
397+
398+
await setVisibility(UserTopicVisibilityPolicy.muted);
399+
checkNotNotified();
400+
checkHasMessageIds([1]);
401+
});
402+
403+
test('unmute a topic -> refetch from scratch', () => awaitFakeAsync((async) async {
404+
await prepare(narrow: const CombinedFeedNarrow());
405+
await prepareMutes(true);
406+
final messages = [
407+
eg.dmMessage(id: 1, from: eg.otherUser, to: [eg.selfUser]),
408+
eg.streamMessage(id: 2, stream: stream, topic: 'foo'),
409+
];
410+
await prepareMessages(foundOldest: true, messages: messages);
411+
checkHasMessageIds([1]);
412+
413+
connection.prepare(
414+
json: newestResult(foundOldest: true, messages: messages).toJson());
415+
await setVisibility(UserTopicVisibilityPolicy.unmuted);
416+
checkNotifiedOnce();
417+
check(model).fetched.isFalse();
418+
checkHasMessageIds([]);
419+
420+
async.elapse(Duration.zero);
421+
checkNotifiedOnce();
422+
checkHasMessageIds([1, 2]);
423+
}));
424+
425+
test('unmute a topic before initial fetch completes -> do nothing', () => awaitFakeAsync((async) async {
426+
await prepare(narrow: const CombinedFeedNarrow());
427+
await prepareMutes(true);
428+
final messages = [
429+
eg.streamMessage(id: 1, stream: stream, topic: 'foo'),
430+
];
431+
432+
connection.prepare(
433+
json: newestResult(foundOldest: true, messages: messages).toJson());
434+
final fetchFuture = model.fetchInitial();
435+
436+
await setVisibility(UserTopicVisibilityPolicy.unmuted);
437+
checkNotNotified();
438+
439+
// The new policy does get applied when the fetch eventually completes.
440+
await fetchFuture;
441+
checkNotifiedOnce();
442+
checkHasMessageIds([1]);
443+
}));
444+
});
445+
273446
group('DeleteMessageEvent', () {
274447
final stream = eg.stream();
275448
final messages = List.generate(30, (i) => eg.streamMessage(stream: stream));

0 commit comments

Comments
 (0)