topics: Keep topic-list page updated, by using data from Topics model

Fixes: #1499

Co-authored-by: Zixuan James Li <[email protected]>

Author: sm-sayedi

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/topics.dart';
 import '../model/unreads.dart';
 import 'action_sheet.dart';
 import 'app_bar.dart';
@@ -125,16 +126,16 @@ 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<GetChannelTopicsEntry>? lastFetchedTopics;
+  Topics? topicsModel;
 
   @override
   void onNewStore() {
+    final newStore = PerAccountStoreWidget.of(context);
     unreadsModel?.removeListener(_modelChanged);
-    final store = PerAccountStoreWidget.of(context);
-    unreadsModel = store.unreads..addListener(_modelChanged);
-    _fetchTopics();
+    unreadsModel = newStore.unreads..addListener(_modelChanged);
+    topicsModel?.removeListener(_modelChanged);
+    topicsModel = newStore.topics..addListener(_modelChanged);
+    _fetchTopics(topicsModel!);
   }
 
   @override
@@ -145,35 +146,33 @@ class _TopicListState extends State<_TopicList> with PerAccountStoreAwareStateMi
 
   void _modelChanged() {
     setState(() {
-      // The actual state lives in `unreadsModel`.
+      // The actual states lives in `unreadsModel` and `topicsModel`.
     });
   }
 
-  void _fetchTopics() async {
+  void _fetchTopics(Topics topicsModel) 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 getChannelTopics(store.connection,
-      channelId: widget.streamId,
-      allowEmptyTopicName: true);
+    await topicsModel.fetchChannelTopics(widget.streamId);
     if (!mounted) return;
     setState(() {
-      lastFetchedTopics = result.topics;
+      // The actual state lives in the `topicsModel`.
     });
   }
 
   @override
   Widget build(BuildContext context) {
-    if (lastFetchedTopics == null) {
+    final channelTopics = topicsModel!.getChannelTopics(widget.streamId);
+    if (channelTopics == null) {
       return const Center(child: CircularProgressIndicator());
     }
 
-    // TODO(design) handle the rare case when `lastFetchedTopics` is empty
+    // TODO(#2003) handle the rare case when `channelTopics` is empty
 
     // This is adapted from parts of the build method on [_InboxPageState].
     final topicItems = <_TopicItemData>[];
-    for (final GetChannelTopicsEntry(:maxId, name: topic) in lastFetchedTopics!) {
+    for (final GetChannelTopicsEntry(:maxId, name: topic) in channelTopics) {
       final unreadMessageIds =
         unreadsModel!.streams[widget.streamId]?[topic] ?? <int>[];
       final countInTopic = unreadMessageIds.length;
@@ -183,17 +182,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.

test/widgets/action_sheet_test.dart

@@ -222,12 +222,14 @@ void main() {
       channel ??= someChannel;
 
       connection.prepare(json: eg.newestGetMessagesResult(
-        foundOldest: true, messages: []).toJson());
-      if (narrow case ChannelNarrow()) {
-        // We auto-focus the topic input when there are no messages;
-        // this is for topic autocomplete.
-        connection.prepare(json: GetChannelTopicsResult(topics: []).toJson());
-      }
+        foundOldest: true,
+        // Include one message so that we don't auto-focus the topic input,
+        // which would trigger a topic-list fetch for topic autocomplete.
+        // That's helpful for the test that opens the topic-list page.
+        // With the topic-list fetch deferred until that page is opened,
+        // the test can prepare the fetch response as it chooses.
+        messages: [eg.streamMessage(stream: channel)],
+      ).toJson());
       await tester.pumpWidget(TestZulipApp(
         accountId: eg.selfAccount.id,
         child: MessageListPage(

test/widgets/topic_list_test.dart

@@ -124,7 +124,7 @@ void main() {
     check(find.byType(CircularProgressIndicator)).findsNothing();
   });
 
-  testWidgets('fetch again when navigating away and back', (tester) async {
+  testWidgets("don't fetch again when navigating away and back", (tester) async {
     addTearDown(testBinding.reset);
     await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
     final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
@@ -145,20 +145,25 @@ void main() {
     await tester.tap(find.byIcon(ZulipIcons.topics));
     await tester.pump();
     await tester.pump(Duration.zero);
+    check(connection.takeRequests())
+    ..length.equals(2) // one for the messages request, another for the topics
+    ..last.which((last) => last
+      .isA<http.Request>()
+      ..method.equals('GET')
+      ..url.path.equals('/api/v1/users/me/${channel.streamId}/topics'));
     check(find.text('topic A')).findsOne();
 
     // … go back to the message list page…
     await tester.pageBack();
     await tester.pump();
 
-    // … then back to the topic-list page, expecting to fetch again.
-    connection.prepare(json: GetChannelTopicsResult(
-      topics: [eg.getChannelTopicsEntry(name: 'topic B')]).toJson());
+    // … then back to the topic-list page, expecting not to fetch again but
+    // use existing data which is kept up-to-date anyway.
     await tester.tap(find.byIcon(ZulipIcons.topics));
     await tester.pump();
     await tester.pump(Duration.zero);
-    check(find.text('topic A')).findsNothing();
-    check(find.text('topic B')).findsOne();
+    check(connection.takeRequests()).isEmpty();
+    check(find.text('topic A')).findsOne();
   });
 
   Finder topicItemFinder = find.descendant(
@@ -221,9 +226,7 @@ void main() {
       origMessages: [message],
       newTopicStr: 'bar'));
     await tester.pump();
-    // There's still one topic item ("foo") even though the new message is in
-    // topic "bar", but the topic list doesn't get updated.
-    check(topicItemFinder).findsOne();
+    check(topicItemFinder).findsExactly(2);
 
     // After the move, the message with maxId moved away from "foo". The topic
     // actions that require `someMessageIdInTopic` are no longer available.
@@ -232,6 +235,40 @@ void main() {
     check(find.text('Mark as resolved')).findsNothing();
     await tester.tap(find.text('Cancel'));
     await tester.pump();
+
+    // Topic actions that require `someMessageIdInTopic` are available
+    // for "bar", the new topic that the message moved to.
+    await tester.longPress(find.text('bar'));
+    await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation
+    check(find.text('Mark as resolved')).findsOne();
+  });
+
+  // event handling is more thoroughly tested in test/model/channel_test.dart
+  testWidgets('smoke resolve topic from topic action sheet', (tester) async {
+    final channel = eg.stream();
+    final messages = List.generate(10, (i) =>
+      eg.streamMessage(id: 100 + i, stream: channel, topic: 'foo'));
+
+    await prepare(tester, channel: channel,
+      topics: [eg.getChannelTopicsEntry(maxId: 109, name: 'foo')],
+      messages: messages);
+    await tester.longPress(topicItemFinder);
+    await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation
+    check(findInTopicItemAt(0, find.byIcon(ZulipIcons.check))).findsNothing();
+    check(findInTopicItemAt(0, find.text('foo'))).findsOne();
+
+    connection.prepare(json: {});
+    await tester.tap(find.text('Mark as resolved'));
+    await tester.pump();
+    await tester.pump(Duration.zero);
+
+    await store.handleEvent(eg.updateMessageEventMoveFrom(
+      origMessages: messages,
+      newTopic: eg.t('foo').resolve(),
+      propagateMode: .changeAll));
+    await tester.pump();
+    check(findInTopicItemAt(0, find.byIcon(ZulipIcons.check))).findsOne();
+    check(findInTopicItemAt(0, find.text('foo'))).findsOne();
   });
 
   testWidgets('resolved and unresolved topics', (tester) async {