msglist: Only show stream name in AllMessagesNarrow

sirpengi · sirpengi · commit 9067f7a05f99 · 2023-11-29T14:16:55.000Z
First in a series of changes to update stream/topic
recipient headers. Stream name is now only displayed
in `AllMessagesNarrow` and clicking this name navigates
to the stream. Renamed `StreamTopicRecipientHeader`
to `StreamMessageRecipientHeader` to reflect this
conceptual change. Also adjusted existing test for
unknown streams to explicitly use `AllMessagesNarrow`.

Added doc links to design specs and upcoming
commits will incrementally bring these headers closer to
the desired design.
diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart
@@ -341,12 +341,14 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
                 padding: EdgeInsets.symmetric(vertical: 16.0),
                 child: CircularProgressIndicator())); // TODO perhaps a different indicator
           case MessageListRecipientHeaderItem():
-            final header = RecipientHeader(message: data.message);
+            final header = RecipientHeader(message: data.message, narrow: widget.narrow);
             return StickyHeaderItem(allowOverflow: true,
               header: header, child: header);
           case MessageListMessageItem():
+            final header = RecipientHeader(message: data.message, narrow: widget.narrow);
             return MessageItem(
               key: ValueKey(data.message.id),
+              header: header,
               trailingWhitespace: i == 1 ? 8 : 11,
               item: data);
         }
@@ -445,16 +447,17 @@ class MarkAsReadWidget extends StatelessWidget {
 }
 
 class RecipientHeader extends StatelessWidget {
-  const RecipientHeader({super.key, required this.message});
+  const RecipientHeader({super.key, required this.message, required this.narrow});
 
   final Message message;
+  final Narrow narrow;
 
   @override
   Widget build(BuildContext context) {
-    // TODO recipient headings depend on narrow
     final message = this.message;
     return switch (message) {
-      StreamMessage() => StreamTopicRecipientHeader(message: message),
+      StreamMessage() => StreamMessageRecipientHeader(message: message,
+        showStream: narrow is AllMessagesNarrow),
       DmMessage() => DmRecipientHeader(message: message),
     };
   }
@@ -464,18 +467,20 @@ class MessageItem extends StatelessWidget {
   const MessageItem({
     super.key,
     required this.item,
+    required this.header,
     this.trailingWhitespace,
   });
 
   final MessageListMessageItem item;
+  final Widget header;
   final double? trailingWhitespace;
 
   @override
   Widget build(BuildContext context) {
     final message = item.message;
     return StickyHeaderItem(
       allowOverflow: !item.isLastInBlock,
-      header: RecipientHeader(message: message),
+      header: header,
       child: _UnreadMarker(
         isRead: message.flags.contains(MessageFlag.read),
         child: ColoredBox(
@@ -528,17 +533,23 @@ class _UnreadMarker extends StatelessWidget {
   }
 }
 
-class StreamTopicRecipientHeader extends StatelessWidget {
-  const StreamTopicRecipientHeader({super.key, required this.message});
+class StreamMessageRecipientHeader extends StatelessWidget {
+  const StreamMessageRecipientHeader({
+    super.key,
+    required this.message,
+    required this.showStream,
+  });
 
   final StreamMessage message;
+  final bool showStream;
 
   @override
   Widget build(BuildContext context) {
+    // For design specs, see:
+    //   https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538%3A20849&mode=dev
+    //   https://github.com/zulip/zulip-mobile/issues/5511
     final store = PerAccountStoreWidget.of(context);
 
-    final stream = store.streams[message.streamId];
-    final streamName = stream?.name ?? message.displayRecipient; // TODO(log) if missing
     final topic = message.subject;
 
     final subscription = store.subscriptions[message.streamId];
@@ -548,6 +559,24 @@ class StreamTopicRecipientHeader extends StatelessWidget {
         ? Colors.white
         : Colors.black;
 
+    final Widget streamWidget;
+    if (!showStream) {
+      streamWidget = const SizedBox.shrink();
+    } else {
+      final stream = store.streams[message.streamId];
+      final streamName = stream?.name ?? message.displayRecipient; // TODO(log) if missing
+
+      streamWidget = GestureDetector(
+        onTap: () => Navigator.push(context,
+          MessageListPage.buildRoute(context: context,
+            narrow: StreamNarrow(message.streamId))),
+        child: RecipientHeaderChevronContainer(
+          color: streamColor,
+          // TODO globe/lock icons for web-public and private streams
+          child: Text(streamName,
+            style: TextStyle(color: contrastingColor))));
+    }
+
     return GestureDetector(
       onTap: () => Navigator.push(context,
         MessageListPage.buildRoute(context: context,
@@ -556,14 +585,7 @@ class StreamTopicRecipientHeader extends StatelessWidget {
         color: _kStreamMessageBorderColor,
         child: Row(mainAxisAlignment: MainAxisAlignment.start, children: [
           // TODO(#282): Long stream name will break layout; find a fix.
-          GestureDetector(
-            onTap: () => Navigator.push(context,
-              MessageListPage.buildRoute(context: context,
-                narrow: StreamNarrow(message.streamId))),
-            child: RecipientHeaderChevronContainer(
-              color: streamColor,
-              // TODO globe/lock icons for web-public and private streams
-              child: Text(streamName, style: TextStyle(color: contrastingColor)))),
+          streamWidget,
           Expanded(
             child: Padding(
               // Web has padding 9, 3, 3, 2 here; but 5px is the chevron.
diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart
@@ -40,10 +40,12 @@ void main() {
     bool foundOldest = true,
     int? messageCount,
     List<Message>? messages,
+    List<ZulipStream>? streams,
     UnreadMessagesSnapshot? unreadMsgs,
   }) async {
     addTearDown(testBinding.reset);
-    await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot(unreadMsgs: unreadMsgs));
+    await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot(
+      streams: streams, unreadMsgs: unreadMsgs));
     store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
     connection = store.connection as FakeApiConnection;
 
@@ -194,22 +196,65 @@ void main() {
   });
 
   group('recipient headers', () {
+    group('StreamMessageRecipientHeader', () {
+      final stream = eg.stream(name: 'stream name');
+      final message = eg.streamMessage(stream: stream, topic: 'topic name');
+
+      FinderResult findInMessageList(String text) {
+        // Stream name shows up in [AppBar] so need to avoid matching that
+        return find.descendant(
+          of: find.byType(MessageList),
+          matching: find.text(text)).evaluate();
+      }
+
+      testWidgets('show stream name in AllMessagesNarrow', (tester) async {
+        await setupMessageListPage(tester,
+          narrow: const AllMessagesNarrow(),
+          messages: [message], streams: [stream]);
+        await tester.pump();
+        check(findInMessageList('stream name')).length.equals(1);
+        check(findInMessageList('topic name')).length.equals(1);
+      });
+
+      testWidgets('do not show stream name in StreamNarrow', (tester) async {
+        await setupMessageListPage(tester,
+          narrow: StreamNarrow(stream.streamId),
+          messages: [message], streams: [stream]);
+        await tester.pump();
+        check(findInMessageList('stream name')).length.equals(0);
+        check(findInMessageList('topic name')).length.equals(1);
+      });
+
+      testWidgets('do not show stream name in TopicNarrow', (tester) async {
+        await setupMessageListPage(tester,
+          narrow: TopicNarrow.ofMessage(message),
+          messages: [message], streams: [stream]);
+        await tester.pump();
+        check(findInMessageList('stream name')).length.equals(0);
+        check(findInMessageList('topic name')).length.equals(1);
+      });
+    });
+
     testWidgets('show stream name from message when stream unknown', (tester) async {
       // This can perfectly well happen, because message fetches can race
       // with events.
       final stream = eg.stream(name: 'stream name');
-      await setupMessageListPage(tester, messages: [
-        eg.streamMessage(stream: stream),
-      ]);
+      await setupMessageListPage(tester,
+        narrow: const AllMessagesNarrow(),
+        messages: [
+          eg.streamMessage(stream: stream),
+        ]);
       await tester.pump();
       tester.widget(find.text('stream name'));
     });
 
     testWidgets('show stream name from stream data when known', (tester) async {
       final stream = eg.stream(name: 'old stream name');
-      await setupMessageListPage(tester, messages: [
-        eg.streamMessage(stream: stream),
-      ]);
+      await setupMessageListPage(tester,
+        narrow: const AllMessagesNarrow(),
+        messages: [
+          eg.streamMessage(stream: stream),
+        ]);
       // TODO(#182) this test would be more realistic using a StreamUpdateEvent
       store.handleEvent(StreamCreateEvent(id: stream.streamId, streams: [
         ZulipStream.fromJson({
