Skip to content

Commit e2b2d1d

Browse files
committed
wip; channel: Keep get-stream-topics data up-to-date
TODO widget tests Fixes: zulip#1499
1 parent f04f979 commit e2b2d1d

File tree

7 files changed

+516
-23
lines changed

7 files changed

+516
-23
lines changed

lib/model/channel.dart

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,15 @@
1+
import 'dart:async';
2+
3+
import 'package:collection/collection.dart';
14
import 'package:flutter/foundation.dart';
25

36
import '../api/model/events.dart';
47
import '../api/model/initial_snapshot.dart';
58
import '../api/model/model.dart';
9+
import '../api/route/channels.dart';
10+
import 'store.dart';
11+
12+
final _apiGetStreamTopics = getStreamTopics; // similar to _apiSendMessage in lib/model/message.dart
613

714
/// The portion of [PerAccountStore] for channels, topics, and stuff about them.
815
///
@@ -34,6 +41,26 @@ mixin ChannelStore {
3441
/// and [streamsByName].
3542
Map<int, Subscription> get subscriptions;
3643

44+
/// Fetch topics in a stream from the server.
45+
///
46+
/// The results from the last successful fetch
47+
/// can be retrieved with [getStreamTopics].
48+
Future<void> fetchTopics(int streamId);
49+
50+
/// Pairs of the known topics and its latest message ID, in the given stream.
51+
///
52+
/// Returns null if the data has never been fetched yet.
53+
/// To fetch it from the server, use [fetchTopics].
54+
///
55+
/// The result is guaranteed to be sorted by [TopicMaxId.maxId], and the
56+
/// topics are guaranteed to be distinct.
57+
///
58+
/// In some cases, the same maxId affected by message moves can be present in
59+
/// multiple [TopicMaxId] entries. For this reason, the caller should not
60+
/// rely on [getStreamTopics] to determine which topic the message is in.
61+
/// Instead, refer to [PerAccountStore.messages].
62+
List<GetStreamTopicsEntry>? getStreamTopics(int streamId);
63+
3764
/// The visibility policy that the self-user has for the given topic.
3865
///
3966
/// This does not incorporate the user's channel-level policy,
@@ -208,6 +235,30 @@ class ChannelStoreImpl extends PerAccountStoreBase with ChannelStore {
208235
@override
209236
final Map<int, Subscription> subscriptions;
210237

238+
/// Maps indexed by stream IDs, of the known latest message IDs in each topic.
239+
///
240+
/// For example: `_topicMaxIdsByStream[stream.id][topic] = maxId`.
241+
final Map<int, Map<TopicName, int>> _latestMessageIdsByStreamTopic = {};
242+
243+
@override
244+
Future<void> fetchTopics(int streamId) async {
245+
final result = await _apiGetStreamTopics(connection, streamId: streamId);
246+
_latestMessageIdsByStreamTopic[streamId] = {
247+
for (final GetStreamTopicsEntry(:name, :maxId) in result.topics)
248+
name: maxId,
249+
};
250+
}
251+
252+
@override
253+
List<GetStreamTopicsEntry>? getStreamTopics(int streamId) {
254+
final latestMessageIdsInStream = _latestMessageIdsByStreamTopic[streamId];
255+
if (latestMessageIdsInStream == null) return null;
256+
return [
257+
for (final MapEntry(:key, :value) in latestMessageIdsInStream.entries)
258+
GetStreamTopicsEntry(maxId: value, name: key),
259+
].sortedBy((value) => -value.maxId);
260+
}
261+
211262
@override
212263
Map<int, Map<TopicName, UserTopicVisibilityPolicy>> get debugTopicVisibility => topicVisibility;
213264

@@ -374,4 +425,48 @@ class ChannelStoreImpl extends PerAccountStoreBase with ChannelStore {
374425
forStream[event.topicName] = visibilityPolicy;
375426
}
376427
}
428+
429+
/// Handle a [MessageEvent], returning whether listeners should be notified.
430+
bool handleMessageEvent(MessageEvent event) {
431+
if (event.message is! StreamMessage) return false;
432+
final StreamMessage(:streamId, :topic) = event.message as StreamMessage;
433+
434+
final topicMaxIdsInStream = _latestMessageIdsByStreamTopic[streamId];
435+
if (topicMaxIdsInStream == null) return false;
436+
assert(!topicMaxIdsInStream.containsKey(topic)
437+
|| topicMaxIdsInStream[topic]! < event.message.id);
438+
topicMaxIdsInStream[topic] = event.message.id;
439+
return true;
440+
}
441+
442+
/// Handle a [UpdateMessageEvent], returning whether listeners should be
443+
/// notified.
444+
bool handleUpdateMessageEvent(UpdateMessageEvent event) {
445+
if (event.moveData == null) return false;
446+
final UpdateMessageMoveData(
447+
:origStreamId, :origTopic, :newStreamId, :newTopic, :propagateMode,
448+
) = event.moveData!;
449+
bool shouldNotify = false;
450+
451+
final origLatestMessageIdsByTopics = _latestMessageIdsByStreamTopic[origStreamId];
452+
if (propagateMode == PropagateMode.changeAll
453+
&& origLatestMessageIdsByTopics != null) {
454+
shouldNotify = origLatestMessageIdsByTopics.remove(origTopic) != null;
455+
}
456+
457+
final newLatestMessageIdsByTopics = _latestMessageIdsByStreamTopic[newStreamId];
458+
if (newLatestMessageIdsByTopics != null) {
459+
final movedMaxId = event.messageIds.max;
460+
if (!newLatestMessageIdsByTopics.containsKey(newTopic)
461+
|| newLatestMessageIdsByTopics[newTopic]! < movedMaxId) {
462+
newLatestMessageIdsByTopics[newTopic] = movedMaxId;
463+
shouldNotify = true;
464+
}
465+
}
466+
467+
return shouldNotify;
468+
}
377469
}
470+
471+
/// A topic and its known latest message ID.
472+
typedef TopicMaxId = ({TopicName topic, int maxId});

lib/model/store.dart

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import '../api/exception.dart';
1313
import '../api/model/events.dart';
1414
import '../api/model/initial_snapshot.dart';
1515
import '../api/model/model.dart';
16+
import '../api/route/channels.dart';
1617
import '../api/route/events.dart';
1718
import '../api/route/messages.dart';
1819
import '../api/backoff.dart';
@@ -693,6 +694,12 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor
693694
Map<String, ZulipStream> get streamsByName => _channels.streamsByName;
694695
@override
695696
Map<int, Subscription> get subscriptions => _channels.subscriptions;
697+
698+
@override
699+
Future<void> fetchTopics(int streamId) => _channels.fetchTopics(streamId);
700+
@override
701+
List<GetStreamTopicsEntry>? getStreamTopics(int streamId) =>
702+
_channels.getStreamTopics(streamId);
696703
@override
697704
UserTopicVisibilityPolicy topicVisibilityPolicy(int streamId, TopicName topic) =>
698705
_channels.topicVisibilityPolicy(streamId, topic);
@@ -890,13 +897,15 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor
890897
unreads.handleMessageEvent(event);
891898
recentDmConversationsView.handleMessageEvent(event);
892899
recentSenders.handleMessage(event.message); // TODO(#824)
900+
if (_channels.handleMessageEvent(event)) notifyListeners();
893901
// When adding anything here (to handle [MessageEvent]),
894902
// it probably belongs in [reconcileMessages] too.
895903

896904
case UpdateMessageEvent():
897905
assert(debugLog("server event: update_message ${event.messageId}"));
898906
_messages.handleUpdateMessageEvent(event);
899907
unreads.handleUpdateMessageEvent(event);
908+
if (_channels.handleUpdateMessageEvent(event)) notifyListeners();
900909

901910
case DeleteMessageEvent():
902911
assert(debugLog("server event: delete_message ${event.messageIds}"));

lib/widgets/topic_list.dart

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import '../api/model/model.dart';
44
import '../api/route/channels.dart';
55
import '../generated/l10n/zulip_localizations.dart';
66
import '../model/narrow.dart';
7+
import '../model/store.dart';
78
import '../model/unreads.dart';
89
import 'action_sheet.dart';
910
import 'app_bar.dart';
@@ -128,16 +129,13 @@ class _TopicList extends StatefulWidget {
128129

129130
class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMixin {
130131
Unreads? unreadsModel;
131-
// TODO(#1499): store the results on [ChannelStore], and keep them
132-
// up-to-date by handling events
133-
List<GetStreamTopicsEntry>? lastFetchedTopics;
134132

135133
@override
136134
void onNewStore() {
137135
unreadsModel?.removeListener(_modelChanged);
138136
final store = PerAccountStoreWidget.of(context);
139137
unreadsModel = store.unreads..addListener(_modelChanged);
140-
_fetchTopics();
138+
_fetchTopics(store);
141139
}
142140

143141
@override
@@ -152,30 +150,30 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi
152150
});
153151
}
154152

155-
void _fetchTopics() async {
153+
void _fetchTopics(PerAccountStore store) async {
156154
// Do nothing when the fetch fails; the topic-list will stay on
157155
// the loading screen, until the user navigates away and back.
158156
// TODO(design) show a nice error message on screen when this fails
159-
final store = PerAccountStoreWidget.of(context);
160-
final result = await getStreamTopics(store.connection,
161-
streamId: widget.streamId);
157+
await store.fetchTopics(widget.streamId);
162158
if (!mounted) return;
163159
setState(() {
164-
lastFetchedTopics = result.topics;
160+
// The actuall state lives in the PerAccountStore
165161
});
166162
}
167163

168164
@override
169165
Widget build(BuildContext context) {
170-
if (lastFetchedTopics == null) {
166+
final store = PerAccountStoreWidget.of(context);
167+
final streamTopics = store.getStreamTopics(widget.streamId);
168+
if (streamTopics == null) {
171169
return const Center(child: CircularProgressIndicator());
172170
}
173171

174172
// TODO(design) handle the rare case when `lastFetchTopics` is empty
175173

176174
// This is adapted from parts of the build method on [_InboxPageState].
177175
final topicItems = <_TopicItemData>[];
178-
for (final GetStreamTopicsEntry(:maxId, name: topic) in lastFetchedTopics!) {
176+
for (final GetStreamTopicsEntry(:maxId, name: topic) in streamTopics) {
179177
final unreadMessageIds =
180178
unreadsModel!.streams[widget.streamId]?[topic] ?? <int>[];
181179
final countInTopic = unreadMessageIds.length;
@@ -185,9 +183,6 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi
185183
topic: topic,
186184
unreadCount: countInTopic,
187185
hasMention: hasMention,
188-
// `lastFetchedTopics.maxId` can become outdated when a new message
189-
// arrives or when there are message moves, until we re-fetch.
190-
// TODO(#1499): track changes to this
191186
maxId: maxId,
192187
));
193188
}

test/api/route/route_checks.dart

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,15 @@
11
import 'package:checks/checks.dart';
2+
import 'package:zulip/api/model/model.dart';
3+
import 'package:zulip/api/route/channels.dart';
24
import 'package:zulip/api/route/messages.dart';
35

46
extension SendMessageResultChecks on Subject<SendMessageResult> {
57
Subject<int> get id => has((e) => e.id, 'id');
68
}
79

10+
extension GetStreamTopicEntryChecks on Subject<GetStreamTopicsEntry> {
11+
Subject<int> get maxId => has((e) => e.maxId, 'maxId');
12+
Subject<TopicName> get name => has((e) => e.name, 'name');
13+
}
14+
815
// TODO add similar extensions for other classes in api/route/*.dart

0 commit comments

Comments
 (0)