Keep track of the known topics in store

lib/model/autocomplete.dart

@@ -6,7 +6,6 @@ import 'package:unorm_dart/unorm_dart.dart' as unorm;
 
 import '../api/model/events.dart';
 import '../api/model/model.dart';
-import '../api/route/channels.dart';
 import '../generated/l10n/zulip_localizations.dart';
 import '../widgets/compose_box.dart';
 import 'algorithms.dart';
@@ -1175,10 +1174,8 @@ class TopicAutocompleteView extends AutocompleteView<TopicAutocompleteQuery, Top
   Future<void> _fetch() async {
      assert(!_isFetching);
     _isFetching = true;
-    final result = await getStreamTopics(store.connection, streamId: streamId,
-      allowEmptyTopicName: true,
-    );
-    _topics = result.topics.map((e) => e.name);
+    await store.fetchTopics(streamId);
+    _topics = store.getChannelTopics(streamId)!.map((e) => e.name);
     _isFetching = false;
     return _startSearch();
   }

lib/model/channel.dart

@@ -1,14 +1,19 @@
+import 'dart:async';
 import 'dart:collection';
 
+import 'package:collection/collection.dart';
 import 'package:flutter/foundation.dart';
 
 import '../api/model/events.dart';
 import '../api/model/initial_snapshot.dart';
 import '../api/model/model.dart';
+import '../api/route/channels.dart';
 import 'realm.dart';
 import 'store.dart';
 import 'user.dart';
 
+final _apiGetChannelTopics = getStreamTopics;  // similar to _apiSendMessage in lib/model/message.dart
+
 /// The portion of [PerAccountStore] for channels, topics, and stuff about them.
 ///
 /// This type is useful for expressing the needs of other parts of the
@@ -78,6 +83,27 @@ mixin ChannelStore on UserStore {
     };
   }
 
+  /// Fetch topics in a channel from the server, only if they're not fetched yet.
+  ///
+  /// The results from the last successful fetch
+  /// can be retrieved with [getChannelTopics].
+  Future<void> fetchTopics(int channelId);
+
+  /// Pairs of the known topics and its latest message ID, in the given channel.
+  ///
+  /// Returns null if the data has never been fetched yet.
+  /// To fetch it from the server, use [fetchTopics].
+  ///
+  /// The result is guaranteed to be sorted by [GetStreamTopicsEntry.maxId]
+  /// descending, and the topics are guaranteed to be distinct.
+  ///
+  /// In some cases, the same maxId affected by message moves can be present in
+  /// multiple [GetStreamTopicsEntry] entries. For this reason, the caller
+  /// should not rely on [getChannelTopics] to determine which topic the message
+  /// is in. Instead, refer to [PerAccountStore.messages].
+  /// See [handleUpdateMessageEvent] on how this could happen.
+  List<GetStreamTopicsEntry>? getChannelTopics(int channelId);
+
   /// The visibility policy that the self-user has for the given topic.
   ///
   /// This does not incorporate the user's channel-level policy,
@@ -288,6 +314,13 @@ mixin ProxyChannelStore on ChannelStore {
   @override
   Map<int, ChannelFolder> get channelFolders => channelStore.channelFolders;
 
+  @override
+  Future<void> fetchTopics(int channelId) => channelStore.fetchTopics(channelId);
+
+  @override
+  List<GetStreamTopicsEntry>? getChannelTopics(int channelId) =>
+    channelStore.getChannelTopics(channelId);
+
   @override
   UserTopicVisibilityPolicy topicVisibilityPolicy(int streamId, TopicName topic) =>
     channelStore.topicVisibilityPolicy(streamId, topic);
@@ -368,6 +401,37 @@ class ChannelStoreImpl extends HasUserStore with ChannelStore {
   @override
   final Map<int, ChannelFolder> channelFolders;
 
+  /// Maps indexed by channel IDs, of the known latest message IDs in each topic.
+  ///
+  /// For example: `_latestMessageIdsByChannelTopic[channel.streamId][topic] = maxId`
+  ///
+  /// In some cases, the same message IDs, when affected by message moves, can
+  /// be present for mutliple channel-topic keys.
+  /// See [handleUpdateMessageEvent] on how this could happen.
+  final Map<int, Map<TopicName, int>> _latestMessageIdsByChannelTopic = {};
+
+  @override
+  Future<void> fetchTopics(int channelId) async {
+    if (_latestMessageIdsByChannelTopic[channelId] != null) return;
+
+    final result = await _apiGetChannelTopics(connection, streamId: channelId,
+      allowEmptyTopicName: true);
+    _latestMessageIdsByChannelTopic[channelId] = {
+      for (final GetStreamTopicsEntry(:name, :maxId) in result.topics)
+        name: maxId,
+    };
+  }
+
+  @override
+  List<GetStreamTopicsEntry>? getChannelTopics(int channelId) {
+    final latestMessageIdsByTopic = _latestMessageIdsByChannelTopic[channelId];
+    if (latestMessageIdsByTopic == null) return null;
+    return [
+      for (final MapEntry(:key, :value) in latestMessageIdsByTopic.entries)
+        GetStreamTopicsEntry(maxId: value, name: key),
+    ].sortedBy((value) => -value.maxId);
+  }
+
   @override
   Map<int, TopicKeyedMap<UserTopicVisibilityPolicy>> get debugTopicVisibility => topicVisibility;
 
@@ -572,6 +636,65 @@ class ChannelStoreImpl extends HasUserStore with ChannelStore {
       forStream[event.topicName] = visibilityPolicy;
     }
   }
+
+  /// Handle a [MessageEvent], returning whether listeners should be notified.
+  bool handleMessageEvent(MessageEvent event) {
+    if (event.message is! StreamMessage) return false;
+    final StreamMessage(:streamId, :topic) = event.message as StreamMessage;
+
+    final latestMessageIdsByTopic = _latestMessageIdsByChannelTopic[streamId];
+    // If we don't already know about the list of topics of the channel this
+    // message belongs to, we don't want to proceed and put one entry about the
+    // topic of this message, otherwise [fetchTopics] and the callers of
+    // [getChannelTopics] would assume that the channel only has this one topic
+    // and would never fetch the complete list of topics for that matter.
+    if (latestMessageIdsByTopic == null) return false;
+
+    // If this message is already the latest message in the topic because it was
+    // received through fetch in fetch/event race, or it is a message sent even
+    // before the latest message of the fetch, we don't do the update.
+    final currentLatestMessageId = latestMessageIdsByTopic[topic];
+    if (currentLatestMessageId != null && currentLatestMessageId >= event.message.id) {
+      return false;
+    }
+    latestMessageIdsByTopic[topic] = event.message.id;
+    return true;
+  }
+
+  /// Handle an [UpdateMessageEvent], returning whether listeners should be
+  /// notified.
+  bool handleUpdateMessageEvent(UpdateMessageEvent event) {
+    if (event.moveData == null) return false;
+    final UpdateMessageMoveData(
+      :origStreamId, :origTopic, :newStreamId, :newTopic, :propagateMode,
+    ) = event.moveData!;
+    bool shouldNotify = false;
+
+    final origLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[origStreamId];
+    // We only handle the case where all the messages of [origTopic] are
+    // moved to [newTopic]; in that case we can remove [origTopic] safely.
+    // But if only one messsage is moved (`PropagateMode.changeOne`) or a few
+    // messages are moved (`PropagateMode.changeLater`), we cannot do anything
+    // about [origTopic] here as we cannot determine the new `maxId` for it.
+    // (This is the case where there could be multiple channel-topic keys with
+    //  the same `maxId`)
+    if (propagateMode == PropagateMode.changeAll
+        && origLatestMessageIdsByTopics != null) {
+      shouldNotify = origLatestMessageIdsByTopics.remove(origTopic) != null;
+    }
+
+    final newLatestMessageIdsByTopics = _latestMessageIdsByChannelTopic[newStreamId];
+    if (newLatestMessageIdsByTopics != null) {
+      final movedMaxId = event.messageIds.max;
+      if (!newLatestMessageIdsByTopics.containsKey(newTopic)
+          || newLatestMessageIdsByTopics[newTopic]! < movedMaxId) {
+        newLatestMessageIdsByTopics[newTopic] = movedMaxId;
+        shouldNotify = true;
+      }
+    }
+
+    return shouldNotify;
+  }
 }
 
 /// A [Map] with [TopicName] keys and [V] values.

lib/model/store.dart

@@ -862,13 +862,15 @@ class PerAccountStore extends PerAccountStoreBase with
         unreads.handleMessageEvent(event);
         recentDmConversationsView.handleMessageEvent(event);
         recentSenders.handleMessage(event.message); // TODO(#824)
+        if (_channels.handleMessageEvent(event)) notifyListeners();
         // When adding anything here (to handle [MessageEvent]),
         // it probably belongs in [reconcileMessages] too.
 
       case UpdateMessageEvent():
         assert(debugLog("server event: update_message ${event.messageId}"));
         _messages.handleUpdateMessageEvent(event);
         unreads.handleUpdateMessageEvent(event);
+        if (_channels.handleUpdateMessageEvent(event)) notifyListeners();
 
       case DeleteMessageEvent():
         assert(debugLog("server event: delete_message ${event.messageIds}"));

lib/widgets/topic_list.dart

@@ -4,6 +4,7 @@ import '../api/model/model.dart';
 import '../api/route/channels.dart';
 import '../generated/l10n/zulip_localizations.dart';
 import '../model/narrow.dart';
+import '../model/store.dart';
 import '../model/unreads.dart';
 import 'action_sheet.dart';
 import 'app_bar.dart';
@@ -124,16 +125,13 @@ class _TopicList extends StatefulWidget {
 
 class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMixin {
   Unreads? unreadsModel;
-  // TODO(#1499): store the results on [ChannelStore], and keep them
-  //   up-to-date by handling events
-  List<GetStreamTopicsEntry>? lastFetchedTopics;
 
   @override
   void onNewStore() {
     unreadsModel?.removeListener(_modelChanged);
     final store = PerAccountStoreWidget.of(context);
     unreadsModel = store.unreads..addListener(_modelChanged);
-    _fetchTopics();
+    _fetchTopics(store);
   }
 
   @override
@@ -148,31 +146,30 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi
     });
   }
 
-  void _fetchTopics() async {
+  void _fetchTopics(PerAccountStore store) async {
     // Do nothing when the fetch fails; the topic-list will stay on
     // the loading screen, until the user navigates away and back.
     // TODO(design) show a nice error message on screen when this fails
-    final store = PerAccountStoreWidget.of(context);
-    final result = await getStreamTopics(store.connection,
-      streamId: widget.streamId,
-      allowEmptyTopicName: true);
+    await store.fetchTopics(widget.streamId);
     if (!mounted) return;
     setState(() {
-      lastFetchedTopics = result.topics;
+      // The actual state lives in the PerAccountStore.
     });
   }
 
   @override
   Widget build(BuildContext context) {
-    if (lastFetchedTopics == null) {
+    final store = PerAccountStoreWidget.of(context);
+    final channelTopics = store.getChannelTopics(widget.streamId);
+    if (channelTopics == null) {
       return const Center(child: CircularProgressIndicator());
     }
 
     // TODO(design) handle the rare case when `lastFetchedTopics` is empty
 
     // This is adapted from parts of the build method on [_InboxPageState].
     final topicItems = <_TopicItemData>[];
-    for (final GetStreamTopicsEntry(:maxId, name: topic) in lastFetchedTopics!) {
+    for (final GetStreamTopicsEntry(:maxId, name: topic) in channelTopics) {
       final unreadMessageIds =
         unreadsModel!.streams[widget.streamId]?[topic] ?? <int>[];
       final countInTopic = unreadMessageIds.length;
@@ -182,17 +179,9 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi
         topic: topic,
         unreadCount: countInTopic,
         hasMention: hasMention,
-        // `lastFetchedTopics.maxId` can become outdated when a new message
-        // arrives or when there are message moves, until we re-fetch.
-        // TODO(#1499): track changes to this
         maxId: maxId,
       ));
     }
-    topicItems.sort((a, b) {
-      final aMaxId = a.maxId;
-      final bMaxId = b.maxId;
-      return bMaxId.compareTo(aMaxId);
-    });
 
     return SafeArea(
       // Don't pad the bottom here; we want the list content to do that.
@@ -234,6 +223,14 @@ class _TopicItem extends StatelessWidget {
 
     final store = PerAccountStoreWidget.of(context);
     final designVariables = DesignVariables.of(context);
+    // The message with `maxId` might not remain in `topic` since we last fetch
+    // the list of topics. Make sure we check for that before passing `maxId`
+    // to the topic action sheet.
+    // See also: [ChannelStore.getChannelTopics]
+    final message = store.messages[maxId];
+    final isMaxIdInTopic = message is StreamMessage
+      && message.streamId == streamId
+      && message.topic.isSameAs(topic);
 
     final visibilityPolicy = store.topicVisibilityPolicy(streamId, topic);
     final double opacity;
@@ -262,7 +259,7 @@ class _TopicItem extends StatelessWidget {
         onLongPress: () => showTopicActionSheet(context,
           channelId: streamId,
           topic: topic,
-          someMessageIdInTopic: maxId),
+          someMessageIdInTopic: isMaxIdInTopic ? maxId : null),
         splashFactory: NoSplash.splashFactory,
         child: Padding(padding: EdgeInsetsDirectional.fromSTEB(6, 8, 12, 8),
           child: Row(

test/api/route/route_checks.dart

@@ -1,4 +1,6 @@
 import 'package:checks/checks.dart';
+import 'package:zulip/api/model/model.dart';
+import 'package:zulip/api/route/channels.dart';
 import 'package:zulip/api/route/messages.dart';
 import 'package:zulip/api/route/realm.dart';
 import 'package:zulip/api/route/saved_snippets.dart';
@@ -15,4 +17,9 @@ extension GetServerSettingsResultChecks on Subject<GetServerSettingsResult> {
   Subject<Uri> get realmUrl => has((e) => e.realmUrl, 'realmUrl');
 }
 
+extension GetStreamTopicEntryChecks on Subject<GetStreamTopicsEntry> {
+  Subject<int> get maxId => has((e) => e.maxId, 'maxId');
+  Subject<TopicName> get name => has((e) => e.name, 'name');
+}
+
 // TODO add similar extensions for other classes in api/route/*.dart

test/model/autocomplete_test.dart

@@ -1258,7 +1258,7 @@ void main() {
       .topic.equals(third.name);
   });
 
-  test('TopicAutocompleteView updates results when streams are loaded', () async {
+  test('TopicAutocompleteView updates results when topics are loaded', () async {
     final store = eg.store();
     final connection = store.connection as FakeApiConnection;
     connection.prepare(json: GetStreamTopicsResult(
@@ -1277,19 +1277,28 @@ void main() {
     check(done).isTrue();
   });
 
-  test('TopicAutocompleteView getStreamTopics request', () async {
+  test('TopicAutocompleteView fetch topics once for a channel', () async {
     final store = eg.store();
     final connection = store.connection as FakeApiConnection;
 
     connection.prepare(json: GetStreamTopicsResult(
-      topics: [eg.getStreamTopicsEntry(name: '')],
+      topics: [eg.getStreamTopicsEntry(name: 'topic')],
     ).toJson());
-    TopicAutocompleteView.init(store: store, streamId: 1000,
-      query: TopicAutocompleteQuery('foo'));
-    check(connection.lastRequest).isA<http.Request>()
+    final view = TopicAutocompleteView.init(store: store, streamId: 1000,
+      query: TopicAutocompleteQuery(''));
+    check(connection.takeRequests()).last.isA<http.Request>()
       ..method.equals('GET')
       ..url.path.equals('/api/v1/users/me/1000/topics')
       ..url.queryParameters['allow_empty_topic_name'].equals('true');
+    await Future(() {});
+
+    view.query = TopicAutocompleteQuery('top');
+    check(connection.takeRequests()).isEmpty();
+    view.dispose();
+
+    TopicAutocompleteView.init(store: store, streamId: 1000,
+      query: TopicAutocompleteQuery('topic'));
+    check(connection.takeRequests()).isEmpty();
   });
 
   group('TopicAutocompleteQuery.testTopic', () {