Support topic muting/unmuting/following #1041

PIG208 · 2024-11-01T22:35:30Z

Screenshots

channel	topic	screenshot
unmuted	inherit
unmuted	muted
muted	inherit
muted	followed
muted	unmuted

Reference zulip-mobile implementation: https://github.com/zulip/zulip-mobile/blob/715d60a5e87fe37032bce58bd72edb99208e15be/src/action-sheets/index.js#L656-L753

Fixes: #348

gnprice

Thanks! Quick initial comments from skimming this draft.

gnprice · 2024-11-05T00:16:42Z

lib/widgets/action_sheet.dart

+  @override void onPressed(BuildContext context) async {
+    Navigator.of(context).pop();
+    try {
+      await updateUserTopic(


This seems more specific than the name TopicActionSheetButton reflects. We'll have other options in the topic action sheet that aren't about visibility policy, like resolve/unresolve.

It makes sense to group all these options under a common base class; it should just get a more specific name.

gnprice · 2024-11-05T00:17:37Z

lib/widgets/action_sheet.dart

+
+  final int streamId;
+  final String topic;
+  final BuildContext topicParentContext;


This name doesn't feel right conceptually. I'll see about sending a quick PR refactoring the existing message-action-sheet options that might help clarify what this field is about.

Sent #1044.

chrisbobbe

Thanks! Comments below, and I agree with your comment at #348 (comment) 🙂

I think a design requirement of this feature is to also display mute/following states for each topic. At this point we can borrow that from the legacy mobile app.

chrisbobbe · 2024-11-22T01:48:21Z

lib/widgets/action_sheet.dart

-
+void _showActionSheet(
+  BuildContext context, {
+  required List<Widget> optionButtons,


Suggested change

required List<Widget> optionButtons,

required List<ActionSheetMenuItemButton> optionButtons,

chrisbobbe · 2024-11-22T01:53:34Z

lib/widgets/action_sheet.dart

-void showMessageActionSheet({required BuildContext context, required Message message}) {
+void showMessageActionSheet(BuildContext context, {required Message message}) {


action_sheet [nfc]: Make context an unamed parameter

nit: "unnamed" (spelling); also, is there room in the summary line to name the function whose params we're talking about? I guess there's just one other function in the action_sheet subsystem with a named context param, fetchRawContentWithFeedback. How about:

action_sheet [nfc]: Make showMessageActionSheet's context an unnamed param

chrisbobbe · 2024-11-22T01:58:56Z

lib/api/route/channels.dart

+    // There can be an error when muting a topic that is already muted
+    // or unmuting one that is already unmuted.  Let it throw because we can't
+    // reliably filter out the error, which doesn't have a specific "code".


It's not clear from the comment that this would be the right layer to "filter out" such errors even if we could; do we need the comment?

I agree that this isn't the right layer to handle the error. Since we are not planning to catch that anyway before we drop this fallback, calling this out doesn't really do anything helpful. I will just move this to the commit message.

chrisbobbe · 2024-11-22T02:07:03Z

lib/api/route/channels.dart

+    // There can be an error when muting a topic that is already muted
+    // or unmuting one that is already unmuted.  Let it throw because we can't
+    // reliably filter out the error, which doesn't have a specific "code".
+    return connection.post('muteTopic', (_) {}, 'users/me/subscriptions/muted_topics', {


Should be connection.patch: https://zulip.com/api/mute-topic

Oh good catch! Looks like we don't even have the connection.patch method; will add it in a prep commit.

chrisbobbe · 2024-11-22T02:23:06Z

lib/widgets/message_list.dart

@@ -305,7 +305,7 @@ class MessageListAppBarTitle extends StatelessWidget {
  }) {
    // A null [Icon.icon] makes a blank space.
    final icon = (stream != null) ? iconDataForStream(stream) : null;
-    return Row(
+    final appBar = Row(


appBar doesn't sound like the right name for something that can get returned from a function named _buildStreamRow. How about just result?

Also, the name _buildStreamRow seems like a hint that it's not really the right place for something that opens a topic action sheet. I think we want a comment in the if (narrow case TopicNarrow that the added GestureDetector will go somewhere else or be removed as part of #1039.

While updating this PR to support message list app bar with two rows, I found a better place where this GestureDetector belongs to:

case TopicNarrow(:var streamId, :var topic): final store = PerAccountStoreWidget.of(context); final stream = store.streams[streamId]; return SizedBox( width: double.infinity, child: GestureDetector( behavior: HitTestBehavior.translucent, onLongPress: () => showTopicActionSheet(context, channelId: streamId, topic: topic), child: Column( crossAxisAlignment: CrossAxisAlignment.start, children: [ _buildStreamRow(context, stream: stream), _buildTopicRow(context, stream: stream, topic: topic), ])));

chrisbobbe · 2024-11-22T02:55:22Z

test/widgets/action_sheet_test.dart

+                                            to:   UserTopicVisibilityPolicy.none);
+
+    Future<void> setupToTopicActionSheet(WidgetTester tester, {
+      required bool? isChannelMuted,


Let's be more explicit about what null means for isChannelMuted. I think it's supposed to mean that the self-user isn't subscribed to the channel (right?), but that isn't clear from test-failure output—

example

$ flutter test test/widgets/action_sheet_test.dart 00:02 +17: UserTopicUpdateButton check expected buttons null UserTopicVisibilityPolicy.none ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════ The following TestFailure was thrown running a test: Expected: a Finder that: Actual: means none were found but one was expected Which: exactly one matching candidate When the exception was thrown, this was the stack: #0 check.<anonymous closure> (package:checks/src/checks.dart:85:9) #1 _TestContext.expect (package:checks/src/checks.dart:708:12) #2 LegacyMatcher.legacyMatcher (package:legacy_checks/src/matcher_compat.dart:27:13) #3 FinderBaseChecks.findsOne (package:flutter_checks/src/flutter_checks.dart:187:5) #4 main.<anonymous closure>.checkButtons (file:///Users/chrisbobbe/dev/zulip-flutter/test/widgets/action_sheet_test.dart:225:39) #5 main.<anonymous closure>.<anonymous closure>.<anonymous closure> (file:///Users/chrisbobbe/dev/zulip-flutter/test/widgets/action_sheet_test.dart:322:11) <asynchronous suspension> #6 testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:189:15) <asynchronous suspension> #7 TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1027:5) <asynchronous suspension> <asynchronous suspension> (elided one frame from package:stack_trace) The test description was: null UserTopicVisibilityPolicy.none ════════════════════════════════════════════════════════════════════════════════════════════════════ 00:02 +17 -1: UserTopicUpdateButton check expected buttons null UserTopicVisibilityPolicy.none [E] Test failed. See exception logs above. The test description was: null UserTopicVisibilityPolicy.none

or from reading the tests, except if you dig into the implementation of setupToTopicActionSheet. A minimal fix would be to give this function a dartdoc and update the test descriptions where they currently just say $isChannelMuted.

Let's be more explicit about what null means for isChannelMuted. I think it's supposed to mean that the self-user isn't subscribed to the channel (right?), but that isn't clear from test-failure output—

Yeah. Added dartdoc, updated the test description and left the topic names as-is, because the description is what shows up when a test fails.

chrisbobbe · 2024-11-22T03:16:42Z

lib/widgets/action_sheet.dart

+        case UserTopicVisibilityPolicy.unknown:
+          assert(false);
+          return;


Can this case be reached? It seems like we should either:

Reject unrecognized policy values at the edge as malformed server data, or

Handle them gracefully in the app, e.g., not handle them by giving up on showing the topic action sheet, which might still have useful buttons like "move topic" or "resolve topic" etc. once we implement those

Yeah. By having this unknown value in the enum we're already paying most of the cost of gracefully handling the situation where the server introduces a new policy value, so we should handle it here too.

We do not expect it here because we already semi-explicitly keep it out of our data structure, so it would be a bug to get unknown here. However, I think it is not obvious that we are doing so because the enum always includes unknown.

zulip-flutter/lib/model/channel.dart

Lines 216 to 222 in 9e42f26

static bool _warnInvalidVisibilityPolicy(UserTopicVisibilityPolicy visibilityPolicy) {

if (visibilityPolicy == UserTopicVisibilityPolicy.unknown) {

// Not a value we expect. Keep it out of our data structures. // TODO(log)

return true;

}

return false;

}

zulip-flutter/lib/model/channel.dart

Lines 348 to 350 in 9e42f26

if (_warnInvalidVisibilityPolicy(visibilityPolicy)) {

visibilityPolicy = UserTopicVisibilityPolicy.none;

}

Hmm, indeed. In that case it doesn't matter what this line does — though it's still probably cleaner to have it skip this type of button rather than abort the whole function. And it should have a comment saying why it's impossible.

I've noticed in a few other places that it'd be good to have a type for "visibility policy, but only the known/valid ones". Probably that points to removing unknown from the enum, and using a nullable type where we actually want that value. Out of scope for this PR, though.

Yeah, this will allow us to remove the cases here (where this new assert(false) originally comes from). It is good to have the type confirm that we process the unknown values at the edge.

Opened #1074 and included TODO comments linked in this PR. (Also removed assert(false)'s under UserTopicVisibilityPolicy.none because it was incorrectly grouped with UserTopicVisibilityPolicy.unknown)

chrisbobbe · 2024-11-22T03:32:11Z

lib/widgets/action_sheet.dart

+    // Not subscribed to the channel; there is no user topic change to be made.
+    return;


The comment helps understand what this case is about, but I think I would remove the return.

Then, we can add chunks of code for more buttons in the future, like copy-link-to-topic, without having to go back and think about this other chunk of code that mostly looks like it's about different buttons.

chrisbobbe · 2024-11-22T03:33:32Z

lib/widgets/action_sheet.dart

+  }
+
+  if (optionButtons.isEmpty) {
+    assert(!supportsUnmutingTopics);


I think we don't need this assert; it should be redundant with tests, and it doesn't unlock any opportunities to simplify code that comes after it.

chrisbobbe · 2024-11-22T03:44:51Z

lib/widgets/action_sheet.dart

+  if (optionButtons.isEmpty) {
+    assert(!supportsUnmutingTopics);
+    return;
+  }


When we return before showing the action sheet, this long-press handler is a no-op. Screen reader software can't tell it's a no-op, so I think it still presents the element as though it offers a long-press interaction, which isn't really accurate.

So for accessibility, what we'd normally want is to pass null to the GestureDetector instead of a no-op function. Alternatively we could design an "empty" appearance for the action sheet.

Those solutions could be finicky or take some time—probably our solution is to eventually just remove the case where the action sheet has no buttons, by implementing a button that's present unconditionally. So for now let's just leave a TODO(a11y) for that. (I think "Copy link to topic" would be such a button.)

PIG208 · 2024-11-22T16:43:02Z

Thanks for the review! Updated the PR.

chrisbobbe

Thanks! Comments below, including some things I missed last time (oops).

chrisbobbe · 2024-11-22T21:55:58Z

lib/api/route/channels.dart

+           || visibilityPolicy == UserTopicVisibilityPolicy.muted);
+    final op = visibilityPolicy == UserTopicVisibilityPolicy.none ? 'remove'
+                                                                  : 'add';
+    return connection.patch('muteTopic', (_) {}, 'users/me/subscriptions/muted_topics', {


api: Add route updateUserTopic For the legacy case, there can be an error when muting a topic that is already muted or unmuting one that is already unmuted. Let it throw because we can't reliably filter out the error, which doesn't have a specific "code". Signed-off-by: Zixuan James Li <[email protected]>

Moving this from a code comment to the commit message doesn't make it more convincing 🙂—as I understand it, the reason we don't catch the error is the same reason we don't generally catch API errors in the binding layer: we want the bindings to correspond as closely as possible to the documented API, as a thin wrapper, so they don't hide or mess with things that callers might be interested in.

I see just one catch in lib/api/route—

/// Convenience function to get a single message from any server. /// /// This encapsulates a server-feature check. /// /// Gives null if the server reports that the message doesn't exist. // TODO(server-5) Simplify this away; just use getMessage. Future<Message?> getMessageCompat(ApiConnection connection, { required int messageId, bool? applyMarkdown, }) async { final useLegacyApi = connection.zulipFeatureLevel! < 120; if (useLegacyApi) { final response = await getMessages(connection, narrow: [ApiNarrowMessageId(messageId)], anchor: NumericAnchor(messageId), numBefore: 0, numAfter: 0, applyMarkdown: applyMarkdown, // Hard-code this param to `true`, as the new single-message API // effectively does: // https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60client_gravatar.60.20in.20.60messages.2F.7Bmessage_id.7D.60/near/1418337 clientGravatar: true, ); return response.messages.firstOrNull; } else { try { final response = await getMessage(connection, messageId: messageId, applyMarkdown: applyMarkdown, ); return response.message; } on ZulipApiException catch (e) { if (e.code == 'BAD_REQUEST') { // Servers use this code when the message doesn't exist, according to // the example in the doc. return null; } rethrow; } } }

In that case we have an explicitly thicker wrapper (marked "compat") that encapsulates a difference between legacy and current behavior, so callers don't have to think about the two behaviors separately. (Because of that catch, callers don't need both a null check and their own catch.)

That kind of encapsulation might actually be a fine reason to want to "filter out" these errors in this binding layer, I'm not sure. But if that's the reason, it's not clear from your paragraph about it. Are the errors useful on those legacy servers, or are they basically just an API wart? Let's write an opinion on that first, if we're going to talk about dropping things from the server, instead of just saying we don't drop things because we can't drop them.

Yeah. The story is that the caller of this is not expected to handle the error, mostly because it isn't helpful to the user. If the topic is already muted/unmuted, and that the user wants it to be muted/unmuted, respectively, there really isn't anything that requires actions from the user:

api: Add route updateUserTopic For the legacy case, there can be an error when muting a topic that - is already muted or unmuting one that is already unmuted. Let it - throw because we can't reliably filter out the error, which doesn't - have a specific "code". + is already muted or unmuting one that is already unmuted. + + The callers are not expected to handle such errors because they aren't + really actionable.

chrisbobbe · 2024-11-22T22:17:28Z

lib/widgets/action_sheet.dart

+      case UserTopicVisibilityPolicy.unknown:
+        // TODO(#1074): This should be unreachable as we keep `unknown` out of
+        //   our data structures.
+        assert(false);


Suggested change

case UserTopicVisibilityPolicy.unknown:

// TODO(#1074): This should be unreachable as we keep `unknown` out of

// our data structures.

assert(false);

case UserTopicVisibilityPolicy.unknown:

// TODO(#1074): This should be unreachable as we keep `unknown` out of

// our data structures.

assert(false);

At first I read "TODO" and "This should be unreachable" as saying that the case is currently reachable and that we plan to make it unreachable in #1074. In reality, it's currently unreachable (or we expect so, anyway), and we want the case to disappear in #1074. Is that correct?

So I think we'll want

// TODO(#1074) remove this unknown(apiValue: null);

in the UserTopicVisibilityPolicy definition, right? Then when we do that TODO, these unknown cases will fall away naturally as part of that (the analyzer will help us there), so they don't need their own separate TODOs. How about:

case UserTopicVisibilityPolicy.unknown: // This case is unreachable (or should be) because we keep `unknown` out // of our data structures. We plan to remove the `unknown` case in #1074. assert(false);

chrisbobbe · 2024-11-22T22:34:45Z

lib/widgets/action_sheet.dart

+  if (optionButtons.isEmpty) {
+    // TODO(a11y): While long press has no effect when we return early without
+    //   bringing up the action sheet, the screen readers do not have a way to
+    //   know that.  However, we may return this early return after we add a
+    //   always-present button.
+    return;
+  }


Can we be clearer about how the bug affects users? How about:

if (optionButtons.isEmpty) { // TODO(a11y): This case makes a no-op gesture handler; as a consequence, // we're presenting some UI (to people who use screen-reader software) as // though it offers a gesture interaction that it doesn't meaningfully // offer, which is confusing. The solution here is probably to remove this // is-empty case by having at least one button that's always present, // such as "copy link to topic". return; }

chrisbobbe · 2024-11-22T22:49:42Z

lib/widgets/action_sheet.dart

+      case (_, UserTopicVisibilityPolicy.none):
+        return '';


A label method returning the empty string looks like a code smell to me; that can't be a helpful label for anything.

It turned out that this would be better as a separate case and keep the assert(false). Added an explanation below:

case (_, UserTopicVisibilityPolicy.none): // This is unexpected because `UserTopicVisibilityPolicy.muted` and // `UserTopicVisibilityPolicy.followed` (handled in separate `case`'s) // are the only expected `currentVisibilityPolicy` // when `newVisibilityPolicy` is `UserTopicVisibilityPolicy.none`. assert(false); return '';

chrisbobbe · 2024-11-22T23:46:25Z

lib/widgets/action_sheet.dart

+  final mute =                 button(to:   UserTopicVisibilityPolicy.muted);
+  final unmute =               button(from: UserTopicVisibilityPolicy.muted,
+                                      to:   UserTopicVisibilityPolicy.none);
+  final unmuteInMutedChannel = button(to:   UserTopicVisibilityPolicy.unmuted);
+  final follow =               button(to:   UserTopicVisibilityPolicy.followed);
+  final unfollow =             button(from: UserTopicVisibilityPolicy.followed,
+                                      to:   UserTopicVisibilityPolicy.none);


The approach in this PR—which I understand follows zulip-mobile—doesn't support all the state transitions that web supports. Maybe we can open a followup issue for the unsupported ones?

Here's what web does:

In a muted channel, the menu for a topic lets you choose one of four options: "Mute", "Default", "Unmute", "Follow":

In an unmuted channel, the menu for a topic lets you choose one of three options: "Mute", "Default", "Follow", unless "Unmute" is currently selected; if so, that option is shown too until you select something different. (You can choose "Unmute" for a topic in a muted channel, and that choice persists when you unmute the channel.)

So that should fully describe which state transitions web supports. Among those, here are the transitions that this zulip-flutter PR doesn't yet support:

In a muted channel:

You can't go from "Mute" to "Default" (unless you go through "Follow").

You can't go from "Default" to "Mute" (unless you go through "Unmute" or "Follow").

You can't go from "Unmute" to "Default" (unless you go through "Follow").

(In a muted channel, the distinction between "Mute" and "Default" matters because it controls whether we show the topic in the whole-stream message list.)

You can't go from "Follow" to "Unmute" (unless you go through "Mute" or "Default"). ("Follow" and "Unmute" cause different behavior with notifications.)

In an unmuted channel:

You can't go from "Unmute" to "Default" (unless you go through "Mute" or "Follow"). (I'm not sure if "Unmute" and "Default" cause different behavior in an unmuted channel, I think maybe they don't.)

None of those "unless you go through" workarounds are intuitive. Even if you know about them, you might get a surprise when you try going through "Mute" and the conversation disappears from the UI and you can't find it to choose the state you wanted in the first place.

Web's radio-buttons approach seems like a fine solution to me, but again we might postpone that; we should make an issue if it's planned though. @gnprice, what do you think?

Yeah, this PR follows the state transitions in the mobile app very closely, including the UX. If we are going with a newer radio-button design, it will probably save some time to get this right with a Figma redesign.

See discussion here.

Oh I see, that discussion is helpful context! I missed it in my review because I didn't see it linked from this PR or the issue. 🙂 I agree with the outcome there:

Yeah, some version of [web's radio-buttons design] would probably be good to put in mobile's action sheet too.

Definitely post-launch, though.

Would you add the link to that discussion on #348, and file an issue for that post-launch task? It can link to my comment here to memoize the work of figuring out what state transitions we're missing. (I used a lot of post-it notes, haha.)

Sure! Opened #1078 and added the link.

chrisbobbe · 2024-11-23T00:25:21Z

assets/icons/ZulipIcons.ttf

icons: Take mute/unmute/following icons from the web app

Hmm yeah, looks like the Figma doesn't have icons for these yet. Following web seems fine, but let's note in the commit message that we checked https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=544-22131&node-type=canvas&m=dev and didn't find corresponding icons there.

chrisbobbe · 2024-11-23T00:31:43Z

assets/l10n/app_en.arb

@@ -59,6 +59,22 @@
  "@permissionsDeniedReadExternalStorage": {
    "description": "Message for dialog asking the user to grant permissions for external storage read access."
  },
+  "actionSheetOptionMuteTopic": "Mute topic",


action_sheet: Support muting/unmuting/following topics

Bump on #1041 (review) :

Thanks! Comments below, and I agree with your comment at #348 (comment) 🙂

I think a design requirement of this feature is to also display mute/following states for each topic. At this point we can borrow that from the legacy mobile app.

The Fixes: line doesn't belong on this commit until we do that 🙂 or make it not part of #348.

PIG208 · 2024-11-25T21:38:17Z

I have updated the PR to implement MessageListAppBarTitle and to address the review feedback. Thanks @chrisbobbe for the review! The UI change (app bar with two rows) is mostly separate from the topic action sheet sheet, so neither needs to block each other if we decide to implement it as a follow-up.

(sorry about the debug banner on the screenshots!)

Compare to the legacy app

	old	new
ChannelNarrow
followed
muted	/
none
dark	/

Compare to the legacy app: font sizes

	old	new
large, channel
large, topic
small, channel
small, topic

When the channel is unknown


before
after

PIG208 · 2024-12-10T05:08:24Z

Opened #1125

gnprice

Here's the start of a review from me — I've read these commits so far:
92f47a5 api: Add ApiConnection.patch
8c05e1c api: Add route updateUserTopic and its compat helper
6db70db icons: Take mute/unmute/following icons from the web app
78cf9e5 msglist: Show channel name and topic name on two rows

gnprice · 2024-12-11T02:53:29Z

lib/api/route/channels.dart

+    assert(visibilityPolicy == UserTopicVisibilityPolicy.none
+           || visibilityPolicy == UserTopicVisibilityPolicy.muted);
+    final op = visibilityPolicy == UserTopicVisibilityPolicy.none ? 'remove'
+                                                                  : 'add';


Suggested change

assert(visibilityPolicy == UserTopicVisibilityPolicy.none

|| visibilityPolicy == UserTopicVisibilityPolicy.muted);

final op = visibilityPolicy == UserTopicVisibilityPolicy.none ? 'remove'

: 'add';

final op = switch (visibilityPolicy) {

UserTopicVisibilityPolicy.none => 'remove',

UserTopicVisibilityPolicy.muted => 'add',

_ => throw UnsupportedError('$visibilityPolicy on old server'),

};

If we get to this point with unmuted or follow, best to just throw — that'll be better than going and muting the topic as if we got muted.

gnprice · 2024-12-11T02:59:39Z

test/api/route/channels_test.dart

+    test('updateUserTopic throws AssertionError when FL < 170', () {
+      return FakeApiConnection.with_(zulipFeatureLevel: 169, (connection) async {
+        check(() => updateUserTopic(connection,
+          streamId: 1, topic: 'topic',
+          visibilityPolicy: UserTopicVisibilityPolicy.followed),


Suggested change

test('updateUserTopic throws AssertionError when FL < 170', () {

return FakeApiConnection.with_(zulipFeatureLevel: 169, (connection) async {

check(() => updateUserTopic(connection,

streamId: 1, topic: 'topic',

visibilityPolicy: UserTopicVisibilityPolicy.followed),

test('updateUserTopic throws AssertionError when FL < 170', () {

return FakeApiConnection.with_(zulipFeatureLevel: 169, (connection) async {

check(() => updateUserTopic(connection,

streamId: 1, topic: 'topic',

visibilityPolicy: UserTopicVisibilityPolicy.muted),

Otherwise the test is confounded: does the function throw because update-user-topic didn't exist at that version, or because followed didn't exist? The latter didn't exist until some time later than this threshold.

gnprice · 2024-12-11T03:03:57Z

lib/widgets/message_list.dart

@@ -327,10 +328,57 @@ class MessageListAppBarTitle extends StatelessWidget {
      children: [
        Icon(size: 16, icon),
        const SizedBox(width: 4),
-        Flexible(child: Text(text)),
+        Flexible(child: Text(stream?.name ?? '(unknown stream)')),


should say "channel" (like it does in main)

gnprice · 2024-12-11T03:11:51Z

lib/widgets/message_list.dart

+  }) {
+    final store = PerAccountStoreWidget.of(context);
+    final designVariables = DesignVariables.of(context);
+    final icon = (stream == null) ? null


nit: extra parens

gnprice

OK, and here's the rest of a full review. Comments below; mostly nits, and a few substantive items in the tests.

gnprice · 2024-12-11T04:34:31Z

lib/widgets/message_list.dart

            Expanded(
              child: Padding(


This subtree gets kind of big. Let's pull it out as a local variable:

Suggested change

Expanded(

child: Padding(

Expanded(

child: topicWidget),

gnprice · 2024-12-11T04:39:09Z

test/widgets/inbox_test.dart

+      bool hasAtSign(WidgetTester tester, Widget? parent) =>
+        hasIcon(tester, parent: parent, icon: ZulipIcons.at_sign);


nit: squash these commits together:
f91cb32 inbox [nfc]: Generalize _AtMentionMarker to take different icons
0a6f72e inbox test [nfc]: Extract hasIcon

The latter is the natural test-side analogue of the former.

gnprice · 2024-12-11T04:44:34Z

test/widgets/inbox_test.dart

+        await setupPage(tester,
+          users: [eg.selfUser, eg.otherUser],
+          streams: [channel],


nit: unused setup

Suggested change

await setupPage(tester,

users: [eg.selfUser, eg.otherUser],

streams: [channel],

await setupPage(tester,

streams: [channel],

(here and below)

gnprice · 2024-12-11T04:45:16Z

test/widgets/inbox_test.dart

+      });
+
+
+      testWidgets('unmuted', (tester) async {


nit: double blank line

gnprice · 2024-12-11T05:02:57Z

lib/widgets/action_sheet.dart

+  if (channelMuted != null && !channelMuted) {
+    switch (visibilityPolicy) {


nit:

Suggested change

if (channelMuted != null && !channelMuted) {

switch (visibilityPolicy) {

if (channelMuted != null && !channelMuted) {

// Channel is subscribed and not muted.

switch (visibilityPolicy) {

And then for the next case:

// Channel is muted.

I had the thought that these conditionals were a bit opaque and required some unpacking by the reader. Then I looked at the handy linked zulip-mobile code and saw it has comments for exactly that need 🙂 — so let's carry those over.

gnprice · 2024-12-11T05:34:03Z

test/widgets/action_sheet_test.dart

+      await tester.tap(unmuteInMutedChannel);
+      await tester.pump();
+      checkUpdateUserTopicRequest(UserTopicVisibilityPolicy.unmuted);


… how about just:

Suggested change

await tester.tap(unmuteInMutedChannel);

await tester.pump();

checkUpdateUserTopicRequest(UserTopicVisibilityPolicy.unmuted);

await tester.tap(find.text('Unmute topic'));

await tester.pump();

checkUpdateUserTopicRequest(UserTopicVisibilityPolicy.unmuted);

I think that covers all the facts that matter to the app's behavior as far as the user is concerned. And it both makes the test simpler, and less brittle to changes in the implementation.

(Well, I guess there's one more fact this version doesn't check: the icon. But the icon isn't really being checked in the current revision either — just some inputs toward computing the icon.

It'd be a nice bonus to also check what icon is shown for each button, but I'll be happy merging this without that check.)

For this and #1041 (comment), maybe have it set up like this:

final mute = find.text('Mute topic'); final unmute = find.text('Unmute topic'); final follow = find.text('Follow topic'); final unfollow = find.text('Unfollow topic');

so that the later tests like this one:

final testCases = [ (false, UserTopicVisibilityPolicy.muted, [unmute, follow]), (false, UserTopicVisibilityPolicy.none, [mute, follow]), (false, UserTopicVisibilityPolicy.unmuted, [mute, follow]), (false, UserTopicVisibilityPolicy.followed, [mute, unfollow]), (true, UserTopicVisibilityPolicy.muted, [unmute, follow]), (true, UserTopicVisibilityPolicy.none, [unmute, follow]), (true, UserTopicVisibilityPolicy.unmuted, [mute, follow]), (true, UserTopicVisibilityPolicy.followed, [mute, unfollow]), (null, UserTopicVisibilityPolicy.none, <Finder>[]), ];

can still be formatted compactly.

Sure, that works.

gnprice · 2024-12-11T05:37:18Z

test/widgets/action_sheet_test.dart

+      check(find.byType(BottomSheet)).findsOne();
+    });


We can be a bit more specific in pinning down that the bottom sheet that comes up is the right bottom sheet:

Suggested change

check(find.byType(BottomSheet)).findsOne();

});

check(find.byType(BottomSheet)).findsOne();

check(find.text('Follow topic')).findsOne();

});

gnprice · 2024-12-11T05:42:24Z

test/widgets/action_sheet_test.dart

+      final testCases = {
+        (false, UserTopicVisibilityPolicy.muted,    [unmute, follow]),


nit: conceptually a list, not set

Suggested change

final testCases = {

(false, UserTopicVisibilityPolicy.muted, [unmute, follow]),

final testCases = [

(false, UserTopicVisibilityPolicy.muted, [unmute, follow]),

(here and below)

gnprice · 2024-12-11T05:44:53Z

test/widgets/action_sheet_test.dart

+          await setupToTopicActionSheet(tester,
+            isChannelMuted: isChannelMuted,
+            visibilityPolicy: visibilityPolicy,
+            featureLevel: 218);


nit: use consistent standard name for a given concept

Suggested change

featureLevel: 218);

zulipFeatureLevel: 218);

gnprice · 2024-12-11T05:51:45Z

test/widgets/action_sheet_test.dart

+        ..bodyFields.deepEquals({
+          'stream_id': '${channel.streamId}',
+          'topic': topic,
+          'visibility_policy': '${expectedPolicy.toJson()}',


Suggested change

'visibility_policy': '${expectedPolicy.toJson()}',

'visibility_policy': jsonConvert(expectedPolicy),

Just calling toString (or doing string interpolation, which calls toString) on the toJson result happens to work here, because the toJson result is an int. But it doesn't get a JSON encoding in general. Better to just use the always-correct thing, which isn't any more complicated to write.

gnprice · 2024-12-11T22:40:17Z

Thanks for the revision! Looks good; merging.

Signed-off-by: Zixuan James Li <[email protected]>

The tests, ApiConnection.{post,patch,delete}, are mostly similar to each other, because the majority of them are testing that the params are parsed into the body with the same content type. If we find the need to update these test cases with new examples, it will be ripe to refactor them with a helper. Until then, just duplicate them for simplicity. Signed-off-by: Zixuan James Li <[email protected]>

For the legacy case, there can be an error when muting a topic that is already muted or unmuting one that is already unmuted. The callers are not expected to handle such errors because they aren't really actionable. Similar to getMessageCompat, updateUserTopicCompat is expected to be dropped, eventually. Signed-off-by: Zixuan James Li <[email protected]>

Renamed "mute-new.svg", "unmute-new.svg" to "mute.svg" and "unmute.svg", respectively. These are taken from the web app because we checked https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=544-22131&node-type=canvas&m=dev and the Figma doesn't have the corresponding icons at the time this is implemented. See: https://github.com/zulip/zulip/tree/da4443f392cc8aa9e6879d905cb1ccd50b66127b/web/shared/icons Signed-off-by: Zixuan James Li <[email protected]>

This will eventually be superseded by zulip#1039, so we should keep the implementation as simple as possible for now. The two-line app bar idea comes from the legacy mobile app. This gives us a place to show the topic visibility policy on the app bar. References: https://github.com/zulip/zulip-mobile/blob/a115df1f71c9dc31e9b41060a8d57b51c017d786/src/title/TitleStream.js#L113-L141 https://github.com/zulip/zulip-mobile/blob/a115df1f71c9dc31e9b41060a8d57b51c017d786/src/styles/navStyles.js#L5-L18 Signed-off-by: Zixuan James Li <[email protected]>

The design took some inspiration from the legacy mobile app. This displays a privacy level related icon (e.g.: web public, invite only). We will have a different place to show channel mute/unmute status in zulip#347. The color for the icon is taken from the web app: https://github.com/zulip/zulip/blob/dc58c8450f8524f226115a7b449b05e01ae15d8b/web/styles/message_header.css#L296-L297 https://github.com/zulip/zulip/blob/dc58c8450f8524f226115a7b449b05e01ae15d8b/web/styles/app_variables.css#L590 https://github.com/zulip/zulip/blob/dc58c8450f8524f226115a7b449b05e01ae15d8b/web/styles/app_variables.css#L1330 In the web app, these colors are used for the topic visibility icons on message recipient headers. To maintain the different conventional title alignment on iOS and Android, we borrow some checks from AppBar in title centering. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

This design was inspired by the legacy mobile app. Reference: https://github.com/zulip/zulip-mobile/blob/a115df1f71c9dc31e9b41060a8d57b51c017d786/src/streams/TopicItem.js#L47-L51 Signed-off-by: Zixuan James Li <[email protected]>

This is helpful for adding marker of topic visibility. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

Currently, we don't have buttons, like "resolve topic", other than the ones added here. The switch statements follow the layout of the legacy app implementation. See also: https://github.com/zulip/zulip-mobile/blob/715d60a5e87fe37032bce58bd72edb99208e15be/src/action-sheets/index.js#L656-L753 Fixes: zulip#348 Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2024-12-11T22:49:31Z

Well, score one for running tools/check after I rebase and before I push! There was a non-local merge conflict (i.e. semantically a merge conflict but not detected by Git):

Analyzing flutterz...                                                   

  error • Missing concrete implementations of 'getter
         ZulipLocalizations.actionSheetOptionFollowTopic', 'getter
         ZulipLocalizations.actionSheetOptionMuteTopic', 'getter
         ZulipLocalizations.actionSheetOptionUnfollowTopic', 'getter
         ZulipLocalizations.actionSheetOptionUnmuteTopic', and 4 more •
         lib/generated/l10n/zulip_localizations_fr.dart:8:7 •
         non_abstract_class_inherits_abstract_member
  error • Missing concrete implementations of 'getter
         ZulipLocalizations.actionSheetOptionFollowTopic', 'getter
         ZulipLocalizations.actionSheetOptionMuteTopic', 'getter
         ZulipLocalizations.actionSheetOptionUnfollowTopic', 'getter
         ZulipLocalizations.actionSheetOptionUnmuteTopic', and 4 more •
         lib/generated/l10n/zulip_localizations_pl.dart:8:7 •
         non_abstract_class_inherits_abstract_member
  error • Missing concrete implementations of 'getter
         ZulipLocalizations.actionSheetOptionFollowTopic', 'getter
         ZulipLocalizations.actionSheetOptionMuteTopic', 'getter
         ZulipLocalizations.actionSheetOptionUnfollowTopic', 'getter
         ZulipLocalizations.actionSheetOptionUnmuteTopic', and 4 more •
         lib/generated/l10n/zulip_localizations_ru.dart:8:7 •
         non_abstract_class_inherits_abstract_member

3 issues found. (ran in 5.5s)

Fixed it with:

$ git rebase -x 'tools/check l10n --fix'

gnprice · 2024-12-11T22:50:32Z

(The issue was that in main we'd added those translations since this PR's current revision; so when adding strings, those generated files now needed to be updated just like the others.)

gnprice reviewed Nov 5, 2024

View reviewed changes

gnprice mentioned this pull request Nov 5, 2024

Clarify action sheet's "page context"; fix small bugs #1044

Merged

PIG208 force-pushed the pr-muting branch 8 times, most recently from c21f055 to be64a22 Compare November 7, 2024 22:53

PIG208 marked this pull request as ready for review November 7, 2024 22:53

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Nov 7, 2024

PIG208 assigned chrisbobbe Nov 7, 2024

PIG208 requested a review from chrisbobbe November 7, 2024 22:53

PIG208 force-pushed the pr-muting branch from be64a22 to b901863 Compare November 9, 2024 04:40

PIG208 force-pushed the pr-muting branch 3 times, most recently from 0b6cdac to 11209cc Compare November 21, 2024 19:06

chrisbobbe reviewed Nov 22, 2024

View reviewed changes

PIG208 force-pushed the pr-muting branch from 11209cc to 44f12dd Compare November 22, 2024 16:42

PIG208 mentioned this pull request Nov 22, 2024

Statically ensure that UserTopicVisibilityPolicy.unknown is not possible in our data models #1074

Open

PIG208 force-pushed the pr-muting branch 2 times, most recently from 21cccab to 8bd44c2 Compare November 22, 2024 18:33

chrisbobbe reviewed Nov 23, 2024

View reviewed changes

This was referenced Nov 23, 2024

UI for topic muting/unmuting/following #348

Closed

Topic muting/unmuting/following UI redesign #1078

Open

PIG208 force-pushed the pr-muting branch 2 times, most recently from 8942165 to 25f5f64 Compare November 25, 2024 21:40

PIG208 mentioned this pull request Dec 10, 2024

Series of prep commits for topic action sheet related changes #1125

Merged

PIG208 force-pushed the pr-muting branch 3 times, most recently from 0041f92 to 15e1b83 Compare December 10, 2024 23:03

PIG208 requested a review from chrisbobbe December 10, 2024 23:04

gnprice reviewed Dec 11, 2024

View reviewed changes

PIG208 force-pushed the pr-muting branch from 15e1b83 to 462d89b Compare December 11, 2024 03:22

gnprice reviewed Dec 11, 2024

View reviewed changes

PIG208 force-pushed the pr-muting branch from 462d89b to 5b6e833 Compare December 11, 2024 19:23

PIG208 requested review from gnprice and removed request for gnprice December 11, 2024 19:23

PIG208 added 11 commits December 11, 2024 14:46

msglist [nfc]: Remove extra parens

3e7683d

Signed-off-by: Zixuan James Li <[email protected]>

msglist [nfc]: Extract topicWidget

7b74c06

Signed-off-by: Zixuan James Li <[email protected]>

msglist: Show visibility policy on recipient headers

585ef8b

This design was inspired by the legacy mobile app. Reference: https://github.com/zulip/zulip-mobile/blob/a115df1f71c9dc31e9b41060a8d57b51c017d786/src/streams/TopicItem.js#L47-L51 Signed-off-by: Zixuan James Li <[email protected]>

inbox [nfc]: Generalize _AtMentionMarker to take different icons

4404537

This is helpful for adding marker of topic visibility. Signed-off-by: Zixuan James Li <[email protected]>

inbox: Show topic visibility marker for topic items

9f18c53

Signed-off-by: Zixuan James Li <[email protected]>

gnprice force-pushed the pr-muting branch from 5b6e833 to c5f1cd4 Compare December 11, 2024 22:50

gnprice merged commit c5f1cd4 into zulip:main Dec 11, 2024

PIG208 deleted the pr-muting branch December 11, 2024 23:26

	required List<Widget> optionButtons,
	required List<ActionSheetMenuItemButton> optionButtons,

		void showMessageActionSheet({required BuildContext context, required Message message}) {
		void showMessageActionSheet(BuildContext context, {required Message message}) {

	static bool _warnInvalidVisibilityPolicy(UserTopicVisibilityPolicy visibilityPolicy) {
	if (visibilityPolicy == UserTopicVisibilityPolicy.unknown) {
	// Not a value we expect. Keep it out of our data structures. // TODO(log)
	return true;
	}
	return false;
	}

	if (_warnInvalidVisibilityPolicy(visibilityPolicy)) {
	visibilityPolicy = UserTopicVisibilityPolicy.none;
	}

		// Not subscribed to the channel; there is no user topic change to be made.
		return;

		bool hasAtSign(WidgetTester tester, Widget? parent) =>
		hasIcon(tester, parent: parent, icon: ZulipIcons.at_sign);

		if (channelMuted != null && !channelMuted) {
		switch (visibilityPolicy) {

		final testCases = {
		(false, UserTopicVisibilityPolicy.muted, [unmute, follow]),

	'visibility_policy': '${expectedPolicy.toJson()}',
	'visibility_policy': jsonConvert(expectedPolicy),

Support topic muting/unmuting/following #1041

Support topic muting/unmuting/following #1041

Conversation

PIG208 commented Nov 1, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Nov 22, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

In a muted channel:

In an unmuted channel:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

PIG208 Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Nov 25, 2024 • edited Loading

PIG208 commented Dec 10, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Dec 11, 2024

PIG208 commented Nov 1, 2024 •

edited

Loading

PIG208 Nov 22, 2024 •

edited

Loading

PIG208 Nov 22, 2024 •

edited

Loading

PIG208 Nov 22, 2024 •

edited

Loading

chrisbobbe Nov 23, 2024 •

edited

Loading

PIG208 Nov 23, 2024 •

edited

Loading

PIG208 commented Nov 25, 2024 •

edited

Loading

PIG208 Dec 11, 2024 •

edited

Loading