Better management of archived channel names

[chimnayajith]

Pull Request

Description

This PR improves archived stream support in the mobile app to match the web app’s behavior. Key updates include:

• Appending " (archived)" to stream names when viewing messages.
• Updating UI elements (e.g., stream headers) to clearly indicate archived streams.
• Filtering out archived channels from relevant pages.
• Displaying the archived tag in a distinct font and color, consistent with the web app.
• Replacing the compose box with a "no permission" error banner instead of offering message composition.

Related Issues

• Closes Support better management of names of archived channels #800

Screenshots

[PIG208]

Thanks for working on this! One more thing that I think needs to be handled is the topic actions sheet. None of the supported actions (follow/mute/unmute/resolve/unresolve) works on a topic in an archived channel. Not sure what's the best way to deal with this, though. It would be a good question to ask on #mobile-design.

[PIG208]

nit: let's reformat this to show that these expressions are parallel:

Suggested change

	if (channel == null || !store.hasPostingPermission(inChannel: channel,
	user: selfUser, byDate: DateTime.now())) {
	user: selfUser, byDate: DateTime.now()) || channel.isArchived) {
	if (channel == null
	|| !store.hasPostingPermission(inChannel: channel,
	user: selfUser, byDate: DateTime.now())
	|| channel.isArchived) {

[PIG208]

Is the user role relevant here? If not, this can be removed.

[PIG208]

nit: we can use continue here, so that the ifs don't need to be nested

[PIG208]

I think this line of code is pretty straightforward, and the comment doesn't add much here, so let's just leave it out.

[PIG208]

Additionally, is it possible for subscriptions to contain archived subscriptions, or is it the job of SubscriptionListPageBody to ensure that no archived subscriptions are passed here?

If the latter, it might be better performance-wise to have an assertion that none of these subscriptions is archived, so that we don't need to re-filter the subscriptions.

[chimnayajith]

Ah now that I check, its already being handled. removed this part.

[PIG208]

nit: extra comma

[PIG208]

Let's also reorder the list to make it easier to read:

    await setupStreamListPage(tester, subscriptions: [
      eg.subscription(eg.stream(streamId: 1, isArchived: true), pinToTop: true),
      eg.subscription(eg.stream(streamId: 2, isArchived: true), pinToTop: false),
      eg.subscription(eg.stream(streamId: 3), pinToTop: true),
      eg.subscription(eg.stream(streamId: 4), pinToTop: false),
    ]);

[PIG208]

This looks unintentional.

[PIG208]

If there is a specific design that we are following here (and other UI changes), it would be helpful to have a link in the commit message.

[chimnayajith]

There isn't a particular design that i could reference. I'm trying to keep it consistent with web.

[PIG208]

For tests, it is fine to not use ZulipLocalizations and have plain strings unless we are specifically testing something related to l10n. This way we have a better idea of what exactly we are expecting in tests.

[PIG208]

This is a bit ambiguous because it doesn't check if the remaining streams are the ones that are not archived.

[PIG208]

nit: Make sure to match the order of these fields in the API documentation. See the dartdoc of ZulipStream to find the relevant piece.

[chimnayajith]

this seems to be the order in the API documentation as I checked. Please let me know if its not

https://zulip.com/api/register-queue and go to subscriptions return value

[PIG208]

I see. The location the dartdoc points to has a different order. Let's match that:

/// For docs, search for "if stream"
/// in <https://zulip.com/api/register-queue>.
@JsonSerializable(fieldRename: FieldRename.snake)
class ZulipStream {

[chimnayajith]

The above comments were addressed and pushed. Waiting for clarification on comment above on CZO

[PIG208]

Thanks for the update! Left some more comments.

[PIG208]

Let's make sure that _SubscriptionList checks for this with an assertion in its build method.

[PIG208]

Similar to the other place, #1285 is applicable here.

[PIG208]

Let's also mention that the design is inspired by web in the commit message.

[PIG208]

Instead of calling copyWith immediately after getting the TextStyle from the helper, let's add color to that helper so we can do it in a single call.

[PIG208]

Similar issue here.

[PIG208]

We use check in our codebase instead. This one can be written as check(richTextFinder).findsOne().

[PIG208]

This comment also applies to other similar places in the PR, so please look for them and update.

[PIG208]

Looking at the implementation of findStreamHeaderRow, this stanza seems to be repetitive. Let's just remove this check. Another benefit of removing this is that we can focus more on checking the presence of the "(archived)" label.

[PIG208]

We usually leave out checks for specific styling unless there is some extra logic to it (changing style dynamically to different inputs, etc.)

[PIG208]

Similarly, we can remove streamId here.

[PIG208]

We can remove selfUser if the tests do not rely on it.

[PIG208]

nit: Order this after "only unpinned subscriptions" because "basic subscriptions", "only pinned subscriptions" are more similar to that test and they should be next to each other.

[PIG208]

While addressing the review comments, a useful thing to do (and often necessary) is to try to find other places where the comment applies to during your self-reviews. This way we can save some back-and-forth revisions when there are some remaining known issues.

[chimnayajith]

@PIG208 Pushed a revision. PTAL. Removed unsupported options from the channel and message action sheets for archived streams.

[PIG208]

Thanks for the update! I just went through it again. Left some comments.

[PIG208]

Let's try to avoid making these checks more nested. We should keep them flat for readability.

I think the existing if/else structure makes it convenient to also check if the channel is archived; if it is, we just skip adding to visibilityOptions.

This also applies to ResolveUnresolveButton separately.

[PIG208]

Hmm, it doesn't feel quite right for isArchived to be on _HeaderItem, since _AllDmsHeaderItem subclasses this and cannot be archived (which is why the default value is needed).

_HeaderItem can gain an abstract InlineSpan? buildTrailing(BuildContext context) method. If buildTrailing returns null, then the marker is not built at all. _StreamHeaderItem will override it to build "(archieved)" following the title.

Meanwhile, we can move isArchived to _StreamHeaderItem and make it a required parameter without a default value.

You can find an example of this setup from _MenuButton.buildLeading.

[PIG208]

nit: materialFinders -> materialFinder (there is just a singular Finder)

[PIG208]

This check seems contradictory to the name of the test, which claims that quote-and-reply is "not offered in archived channels". Perhaps either the implementation or the test is not working as intended?

[PIG208]

Or perhaps it is better to simplify this to:

Suggested change

	final row = findRowByLabel(tester, someChannel.name);
	check(row).isNotNull();
	final materialFinders = find.ancestor(
	of: find.byWidget(row!),
	matching: find.byType(Material)).hitTestable();
	await tester.longPress(materialFinders.first);
	await tester.longPress(
	find.textContaining(findRichText: true, someChannel.name).first);

and then we can remove the findRowByLabel helper.

[PIG208]

We can simplify this test by extending and making use of the existing helper:

diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart
index a44b20f71..9624c93a3 100644
--- a/test/widgets/action_sheet_test.dart
+++ b/test/widgets/action_sheet_test.dart
@@ -494,15 +494,17 @@ void main() {
       /// If `isChannelMuted` is `null`, the user is not subscribed to the
       /// channel.
       Future<void> setupToTopicActionSheet(WidgetTester tester, {
+        ZulipStream? channel,
         required bool? isChannelMuted,
         required UserTopicVisibilityPolicy visibilityPolicy,
         int? zulipFeatureLevel,
       }) async {
         addTearDown(testBinding.reset);
+        channel ??= someChannel;
 
         topic = 'isChannelMuted: $isChannelMuted, policy: $visibilityPolicy';
         await prepare(
-          channel: someChannel,
+          channel: channel,
           topic: topic,
           isChannelSubscribed: isChannelMuted != null, // shorthand; see dartdoc
           isChannelMuted: isChannelMuted,
@@ -511,9 +513,10 @@ void main() {
         );
 
         final message = eg.streamMessage(
-          stream: someChannel, topic: topic, sender: eg.otherUser);
+          stream: channel, topic: topic, sender: eg.otherUser);
+        await store.handleEvent(MessageEvent(id: 1, message: message));
         await showFromAppBar(tester,
-          channel: someChannel, topic: topic, messages: [message]);
+          channel: channel, topic: topic, messages: [message]);
       }
 
       void checkButtons(List<Finder> expectedButtonFinders) {
@@ -619,43 +622,14 @@ void main() {
         }
       });
 
-      group('archived channels', () {
-        testWidgets('limited topic actions for archived channels', (tester) async {
-          final archivedChannel = eg.stream(isArchived: true);
-          final someTopic = 'my topic';
+      testWidgets('limited topic actions for archived channels', (tester) async {
+        final archivedChannel = eg.stream(isArchived: true);
 
-          final message = eg.streamMessage(
-            stream: archivedChannel,
-            topic: someTopic,
-            flags: []);
-
-          await prepare(
-            channel: archivedChannel,
-            topic: someTopic,
-            isChannelSubscribed: true,
-            isChannelMuted: false,
-            visibilityPolicy: UserTopicVisibilityPolicy.none,
-            unreadMsgs: eg.unreadMsgs(channels: [
-              eg.unreadChannelMsgs(
-                streamId: archivedChannel.streamId,
-                topic: someTopic,
-                unreadMessageIds: [message.id]),
-            ]));
-
-          await showFromAppBar(tester,
-            channel: archivedChannel,
-            topic: someTopic,
-            messages: [message]);
-          check(mute).findsNothing();
-          check(unmute).findsNothing();
-          check(follow).findsNothing();
-          check(unfollow).findsNothing();
-
-          check(findButtonForLabel('Mark as resolved')).findsNothing();
-          check(findButtonForLabel('Mark as unresolved')).findsNothing();
-
-          check(findButtonForLabel('Mark topic as read')).findsOne();
-        });
+        await setupToTopicActionSheet(tester,
+          channel: archivedChannel,
+          isChannelMuted: false,
+          visibilityPolicy: UserTopicVisibilityPolicy.none);
+        checkButtons([]);
       });

While writing tests like this, it is usually helpful to see how the adjacent tests are written, and try to reuse the test helpers. Not only does that simplify the test, it also makes them look more coherent with the surrounding tests.

[PIG208]

The proposed test only checks the buttons relevant to the test group ("UserTopicUpdateButton") this is in, as I feel that checks for the other buttons like "Mark as resolved/unresolved" belong to the other corresponding test groups like "ResolveUnresolveButton".

[PIG208]

Most of the other tests above do not have a test for the opposite case because it is covered in the earlier tests that assume the channel is not archived (which is the boring/default case). We can drop this test and then flatten the group.

[PIG208]

nit: remove this empty line as the lines before/after it are relevant.

[PIG208]

This also needs a test for the app bar.