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

Fixes: #1499

Co-authored-by: Sayed Mahmood Sayedi <[email protected]>

Author: PIG208

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';
@@ -125,16 +126,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<GetChannelTopicsEntry>? lastFetchedTopics;
 
   @override
   void onNewStore() {
     unreadsModel?.removeListener(_modelChanged);
     final store = PerAccountStoreWidget.of(context);
     unreadsModel = store.unreads..addListener(_modelChanged);
-    _fetchTopics();
+    _fetchTopics(store);
   }
 
   @override
@@ -149,31 +147,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 getChannelTopics(store.connection,
-      channelId: widget.streamId,
-      allowEmptyTopicName: true);
+    await store.fetchChannelTopics(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
+    // 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 +180,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.
@@ -235,6 +224,14 @@ class _TopicItem extends StatelessWidget {
 
     final store = PerAccountStoreWidget.of(context);
     final designVariables = DesignVariables.of(context);
+    // `maxId` might be incorrect (see [ChannelStore.getChannelTopics]).
+    // Check if it refers to a message that's currently in the topic;
+    // if not, we just won't have `someMessageIdInTopic` for the action sheet.
+    final maxIdMessage = store.messages[maxId];
+    final someMessageIdInTopic =
+      (maxIdMessage != null && TopicNarrow(streamId, topic).containsMessage(maxIdMessage))
+        ? maxIdMessage.id
+        : null;
 
     final visibilityPolicy = store.topicVisibilityPolicy(streamId, topic);
     final double opacity;
@@ -263,7 +260,7 @@ class _TopicItem extends StatelessWidget {
         onLongPress: () => showTopicActionSheet(context,
           channelId: streamId,
           topic: topic,
-          someMessageIdInTopic: maxId),
+          someMessageIdInTopic: someMessageIdInTopic),
         splashFactory: NoSplash.splashFactory,
         child: ConstrainedBox(
           constraints: BoxConstraints(minHeight: 40),

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

@@ -14,8 +14,10 @@ import 'package:zulip/widgets/message_list.dart';
 import 'package:zulip/widgets/topic_list.dart';
 
 import '../api/fake_api.dart';
+import '../api/route/route_checks.dart';
 import '../example_data.dart' as eg;
 import '../model/binding.dart';
+import '../model/store_checks.dart';
 import '../model/test_store.dart';
 import '../stdlib_checks.dart';
 import 'test_app.dart';
@@ -124,7 +126,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 +147,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(
@@ -169,6 +176,18 @@ void main() {
     of: topicItemFinder.at(index),
     matching: finder);
 
+  testWidgets('sort topics by maxId', (tester) async {
+    await prepare(tester, topics: [
+      eg.getChannelTopicsEntry(name: 'A', maxId: 3),
+      eg.getChannelTopicsEntry(name: 'B', maxId: 2),
+      eg.getChannelTopicsEntry(name: 'C', maxId: 4),
+    ]);
+
+    check(findInTopicItemAt(0, find.text('C'))).findsOne();
+    check(findInTopicItemAt(1, find.text('A'))).findsOne();
+    check(findInTopicItemAt(2, find.text('B'))).findsOne();
+  });
+
   testWidgets('show topic action sheet', (tester) async {
     final channel = eg.stream();
     await prepare(tester, channel: channel,
@@ -190,16 +209,70 @@ void main() {
       });
   });
 
-  testWidgets('sort topics by maxId', (tester) async {
-    await prepare(tester, topics: [
-      eg.getChannelTopicsEntry(name: 'A', maxId: 3),
-      eg.getChannelTopicsEntry(name: 'B', maxId: 2),
-      eg.getChannelTopicsEntry(name: 'C', maxId: 4),
-    ]);
+  testWidgets('show topic action sheet before and after moves', (tester) async {
+    final channel = eg.stream();
+    final message = eg.streamMessage(id: 100, stream: channel, topic: 'foo');
+    await prepare(tester, channel: channel,
+      topics: [eg.getChannelTopicsEntry(name: 'foo', maxId: 100)],
+      messages: [message]);
+    check(store).getChannelTopics(channel.streamId).isNotNull().single
+      .maxId.equals(100);
+    check(topicItemFinder).findsOne();
+
+    // Before the move, "foo"'s maxId is known to be accurate. This makes
+    // topic actions that require `someMessageIdInTopic` available.
+    await tester.longPress(find.text('foo'));
+    await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation
+    check(find.text('Mark as resolved')).findsOne();
+    await tester.tap(find.text('Cancel'));
 
-    check(findInTopicItemAt(0, find.text('C'))).findsOne();
-    check(findInTopicItemAt(1, find.text('A'))).findsOne();
-    check(findInTopicItemAt(2, find.text('B'))).findsOne();
+    await store.handleEvent(eg.updateMessageEventMoveFrom(
+      origMessages: [message],
+      newTopicStr: 'bar'));
+    await tester.pump();
+    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.
+    await tester.longPress(find.text('foo'));
+    await tester.pump(Duration(milliseconds: 150)); // bottom-sheet animation
+    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: 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 {