Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support topic muting/unmuting/following #1041

Merged
merged 11 commits into from
Dec 11, 2024
Merged

Support topic muting/unmuting/following #1041

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3e7683d
msglist [nfc]: Remove extra parens
PIG208 Dec 11, 2024
1e04f7b
api: Add ApiConnection.patch
PIG208 Nov 22, 2024
868f8b7
api: Add route updateUserTopic and its compat helper
PIG208 Oct 24, 2024
df10d69
icons: Take mute/unmute/following icons from the web app
PIG208 Oct 24, 2024
b1767b2
msglist: Show channel name and topic name on two rows
PIG208 Nov 25, 2024
8540c29
msglist: Show topic visibility on app bar
PIG208 Dec 4, 2024
7b74c06
msglist [nfc]: Extract topicWidget
PIG208 Dec 11, 2024
585ef8b
msglist: Show visibility policy on recipient headers
PIG208 Dec 4, 2024
4404537
inbox [nfc]: Generalize _AtMentionMarker to take different icons
PIG208 Dec 4, 2024
9f18c53
inbox: Show topic visibility marker for topic items
PIG208 Dec 4, 2024
c5f1cd4
action_sheet: Support muting/unmuting/following topics
PIG208 Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified assets/icons/ZulipIcons.ttf
View file
Open in desktop
Binary file not shown.
1 change: 1 addition & 0 deletions assets/icons/follow.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 4 additions & 0 deletions assets/icons/inherit.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 1 addition & 5 deletions assets/icons/mute.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 4 additions & 4 deletions assets/icons/unmute.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
32 changes: 32 additions & 0 deletions assets/l10n/app_en.arb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,22 @@
"@permissionsDeniedReadExternalStorage": {
"description": "Message for dialog asking the user to grant permissions for external storage read access."
},
"actionSheetOptionMuteTopic": "Mute topic",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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.

"@actionSheetOptionMuteTopic": {
"description": "Label for muting a topic on action sheet."
},
"actionSheetOptionUnmuteTopic": "Unmute topic",
"@actionSheetOptionUnmuteTopic": {
"description": "Label for unmuting a topic on action sheet."
},
"actionSheetOptionFollowTopic": "Follow topic",
"@actionSheetOptionFollowTopic": {
"description": "Label for following a topic on action sheet."
},
"actionSheetOptionUnfollowTopic": "Unfollow topic",
"@actionSheetOptionUnfollowTopic": {
"description": "Label for unfollowing a topic on action sheet."
},
"actionSheetOptionCopyMessageText": "Copy message text",
"@actionSheetOptionCopyMessageText": {
"description": "Label for copy message text button on action sheet."
Expand Down Expand Up @@ -201,6 +217,22 @@
"event": {"type": "String", "example": "UpdateMessageEvent(id: 123, messageIds: [2345, 3456], newTopic: 'dinner')"}
}
},
"errorMuteTopicFailed": "Failed to mute topic",
"@errorMuteTopicFailed": {
"description": "Error message when muting a topic failed."
},
"errorUnmuteTopicFailed": "Failed to unmute topic",
"@errorUnmuteTopicFailed": {
"description": "Error message when unmuting a topic failed."
},
"errorFollowTopicFailed": "Failed to follow topic",
"@errorFollowTopicFailed": {
"description": "Error message when following a topic failed."
},
"errorUnfollowTopicFailed": "Failed to unfollow topic",
"@errorUnfollowTopicFailed": {
"description": "Error message when unfollowing a topic failed."
},
"errorSharingFailed": "Sharing failed",
"@errorSharingFailed": {
"description": "Error message when sharing a message failed."
Expand Down
10 changes: 10 additions & 0 deletions lib/api/core.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,16 @@ class ApiConnection {
return send(routeName, fromJson, request);
}

Future<T> patch<T>(String routeName, T Function(Map<String, dynamic>) fromJson,
String path, Map<String, dynamic>? params) async {
final url = realmUrl.replace(path: "/api/v1/$path");
final request = http.Request('PATCH', url);
if (params != null) {
request.bodyFields = encodeParameters(params)!;
}
return send(routeName, fromJson, request);
}

Future<T> delete<T>(String routeName, T Function(Map<String, dynamic>) fromJson,
String path, Map<String, dynamic>? params) async {
final url = realmUrl.replace(path: "/api/v1/$path");
Expand Down
2 changes: 1 addition & 1 deletion lib/api/model/model.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ enum UserTopicVisibilityPolicy {
muted(apiValue: 1),
unmuted(apiValue: 2), // TODO(server-7) newly added
followed(apiValue: 3), // TODO(server-8) newly added
unknown(apiValue: null);
unknown(apiValue: null); // TODO(#1074) remove this

const UserTopicVisibilityPolicy({required this.apiValue});

Expand Down
49 changes: 49 additions & 0 deletions lib/api/route/channels.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import 'package:json_annotation/json_annotation.dart';

import '../core.dart';
import '../model/model.dart';
part 'channels.g.dart';

/// https://zulip.com/api/get-stream-topics
Expand Down Expand Up @@ -38,3 +39,51 @@ class GetStreamTopicsEntry {

Map<String, dynamic> toJson() => _$GetStreamTopicsEntryToJson(this);
}

/// Update a topic's visibility policy.
///
/// This encapsulates a server-feature check.
// TODO(server-7): remove this and just use updateUserTopic
Future<void> updateUserTopicCompat(ApiConnection connection, {
required int streamId,
required String topic,
required UserTopicVisibilityPolicy visibilityPolicy,
}) {
final useLegacyApi = connection.zulipFeatureLevel! < 170;
if (useLegacyApi) {
final op = switch (visibilityPolicy) {
UserTopicVisibilityPolicy.none => 'remove',
UserTopicVisibilityPolicy.muted => 'add',
_ => throw UnsupportedError('$visibilityPolicy on old server'),
};
// https://zulip.com/api/mute-topic
return connection.patch('muteTopic', (_) {}, 'users/me/subscriptions/muted_topics', {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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 <zixuan@zulip.com>

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'stream_id': streamId,
'topic': RawParameter(topic),
'op': RawParameter(op),
});
} else {
return updateUserTopic(connection,
streamId: streamId,
topic: topic,
visibilityPolicy: visibilityPolicy);
}
}

/// https://zulip.com/api/update-user-topic
///
/// This binding only supports feature levels 170+.
// TODO(server-7) remove FL 170+ mention in doc, and the related `assert`
Future<void> updateUserTopic(ApiConnection connection, {
required int streamId,
required String topic,
required UserTopicVisibilityPolicy visibilityPolicy,
}) {
assert(visibilityPolicy != UserTopicVisibilityPolicy.unknown);
assert(connection.zulipFeatureLevel! >= 170);
return connection.post('updateUserTopic', (_) {}, 'user_topics', {
'stream_id': streamId,
'topic': RawParameter(topic),
'visibility_policy': visibilityPolicy,
});
}
48 changes: 48 additions & 0 deletions lib/generated/l10n/zulip_localizations.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,30 @@ abstract class ZulipLocalizations {
/// **'To upload files, please grant Zulip additional permissions in Settings.'**
String get permissionsDeniedReadExternalStorage;

/// Label for muting a topic on action sheet.
///
/// In en, this message translates to:
/// **'Mute topic'**
String get actionSheetOptionMuteTopic;

/// Label for unmuting a topic on action sheet.
///
/// In en, this message translates to:
/// **'Unmute topic'**
String get actionSheetOptionUnmuteTopic;

/// Label for following a topic on action sheet.
///
/// In en, this message translates to:
/// **'Follow topic'**
String get actionSheetOptionFollowTopic;

/// Label for unfollowing a topic on action sheet.
///
/// In en, this message translates to:
/// **'Unfollow topic'**
String get actionSheetOptionUnfollowTopic;

/// Label for copy message text button on action sheet.
///
/// In en, this message translates to:
Expand Down Expand Up @@ -361,6 +385,30 @@ abstract class ZulipLocalizations {
/// **'Error handling a Zulip event from {serverUrl}; will retry.\n\nError: {error}\n\nEvent: {event}'**
String errorHandlingEventDetails(String serverUrl, String error, String event);

/// Error message when muting a topic failed.
///
/// In en, this message translates to:
/// **'Failed to mute topic'**
String get errorMuteTopicFailed;

/// Error message when unmuting a topic failed.
///
/// In en, this message translates to:
/// **'Failed to unmute topic'**
String get errorUnmuteTopicFailed;

/// Error message when following a topic failed.
///
/// In en, this message translates to:
/// **'Failed to follow topic'**
String get errorFollowTopicFailed;

/// Error message when unfollowing a topic failed.
///
/// In en, this message translates to:
/// **'Failed to unfollow topic'**
String get errorUnfollowTopicFailed;

/// Error message when sharing a message failed.
///
/// In en, this message translates to:
Expand Down
24 changes: 24 additions & 0 deletions lib/generated/l10n/zulip_localizations_ar.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ class ZulipLocalizationsAr extends ZulipLocalizations {
@override
String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.';

@override
String get actionSheetOptionMuteTopic => 'Mute topic';

@override
String get actionSheetOptionUnmuteTopic => 'Unmute topic';

@override
String get actionSheetOptionFollowTopic => 'Follow topic';

@override
String get actionSheetOptionUnfollowTopic => 'Unfollow topic';

@override
String get actionSheetOptionCopyMessageText => 'Copy message text';

Expand Down Expand Up @@ -165,6 +177,18 @@ class ZulipLocalizationsAr extends ZulipLocalizations {
return 'Error handling a Zulip event from $serverUrl; will retry.\n\nError: $error\n\nEvent: $event';
}

@override
String get errorMuteTopicFailed => 'Failed to mute topic';

@override
String get errorUnmuteTopicFailed => 'Failed to unmute topic';

@override
String get errorFollowTopicFailed => 'Failed to follow topic';

@override
String get errorUnfollowTopicFailed => 'Failed to unfollow topic';

@override
String get errorSharingFailed => 'Sharing failed';

Expand Down
24 changes: 24 additions & 0 deletions lib/generated/l10n/zulip_localizations_en.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ class ZulipLocalizationsEn extends ZulipLocalizations {
@override
String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.';

@override
String get actionSheetOptionMuteTopic => 'Mute topic';

@override
String get actionSheetOptionUnmuteTopic => 'Unmute topic';

@override
String get actionSheetOptionFollowTopic => 'Follow topic';

@override
String get actionSheetOptionUnfollowTopic => 'Unfollow topic';

@override
String get actionSheetOptionCopyMessageText => 'Copy message text';

Expand Down Expand Up @@ -165,6 +177,18 @@ class ZulipLocalizationsEn extends ZulipLocalizations {
return 'Error handling a Zulip event from $serverUrl; will retry.\n\nError: $error\n\nEvent: $event';
}

@override
String get errorMuteTopicFailed => 'Failed to mute topic';

@override
String get errorUnmuteTopicFailed => 'Failed to unmute topic';

@override
String get errorFollowTopicFailed => 'Failed to follow topic';

@override
String get errorUnfollowTopicFailed => 'Failed to unfollow topic';

@override
String get errorSharingFailed => 'Sharing failed';

Expand Down
24 changes: 24 additions & 0 deletions lib/generated/l10n/zulip_localizations_fr.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ class ZulipLocalizationsFr extends ZulipLocalizations {
@override
String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.';

@override
String get actionSheetOptionMuteTopic => 'Mute topic';

@override
String get actionSheetOptionUnmuteTopic => 'Unmute topic';

@override
String get actionSheetOptionFollowTopic => 'Follow topic';

@override
String get actionSheetOptionUnfollowTopic => 'Unfollow topic';

@override
String get actionSheetOptionCopyMessageText => 'Copy message text';

Expand Down Expand Up @@ -165,6 +177,18 @@ class ZulipLocalizationsFr extends ZulipLocalizations {
return 'Error handling a Zulip event from $serverUrl; will retry.\n\nError: $error\n\nEvent: $event';
}

@override
String get errorMuteTopicFailed => 'Failed to mute topic';

@override
String get errorUnmuteTopicFailed => 'Failed to unmute topic';

@override
String get errorFollowTopicFailed => 'Failed to follow topic';

@override
String get errorUnfollowTopicFailed => 'Failed to unfollow topic';

@override
String get errorSharingFailed => 'Sharing failed';

Expand Down
24 changes: 24 additions & 0 deletions lib/generated/l10n/zulip_localizations_ja.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ class ZulipLocalizationsJa extends ZulipLocalizations {
@override
String get permissionsDeniedReadExternalStorage => 'To upload files, please grant Zulip additional permissions in Settings.';

@override
String get actionSheetOptionMuteTopic => 'Mute topic';

@override
String get actionSheetOptionUnmuteTopic => 'Unmute topic';

@override
String get actionSheetOptionFollowTopic => 'Follow topic';

@override
String get actionSheetOptionUnfollowTopic => 'Unfollow topic';

@override
String get actionSheetOptionCopyMessageText => 'Copy message text';

Expand Down Expand Up @@ -165,6 +177,18 @@ class ZulipLocalizationsJa extends ZulipLocalizations {
return 'Error handling a Zulip event from $serverUrl; will retry.\n\nError: $error\n\nEvent: $event';
}

@override
String get errorMuteTopicFailed => 'Failed to mute topic';

@override
String get errorUnmuteTopicFailed => 'Failed to unmute topic';

@override
String get errorFollowTopicFailed => 'Failed to follow topic';

@override
String get errorUnfollowTopicFailed => 'Failed to unfollow topic';

@override
String get errorSharingFailed => 'Sharing failed';

Expand Down
Loading