diff --git a/lib/api/model/events.dart b/lib/api/model/events.dart index f543144ce1..9faa3d367e 100644 --- a/lib/api/model/events.dart +++ b/lib/api/model/events.dart @@ -766,14 +766,6 @@ class UpdateMessageEvent extends Event { Map toJson() => _$UpdateMessageEventToJson(this); } -/// As in [UpdateMessageEvent.propagateMode]. -@JsonEnum(fieldRename: FieldRename.snake) -enum PropagateMode { - changeOne, - changeLater, - changeAll; -} - /// A Zulip event of type `delete_message`: https://zulip.com/api/get-events#delete_message @JsonSerializable(fieldRename: FieldRename.snake) class DeleteMessageEvent extends Event { diff --git a/lib/api/model/events.g.dart b/lib/api/model/events.g.dart index 9229bf9757..5d47444ecd 100644 --- a/lib/api/model/events.g.dart +++ b/lib/api/model/events.g.dart @@ -455,7 +455,7 @@ Map _$UpdateMessageEventToJson(UpdateMessageEvent instance) => 'edit_timestamp': instance.editTimestamp, 'stream_id': instance.origStreamId, 'new_stream_id': instance.newStreamId, - 'propagate_mode': _$PropagateModeEnumMap[instance.propagateMode], + 'propagate_mode': instance.propagateMode, 'orig_subject': instance.origTopic, 'subject': instance.newTopic, 'orig_content': instance.origContent, diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index e55dc1d1f0..03af104baf 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -664,6 +664,19 @@ enum MessageFlag { // Using as a Map key is almost certainly a bug because it won't case-fold; // see for example #739, #980, #1205. extension type const TopicName(String _value) { + /// The canonical form of the resolved-topic prefix. + // This is RESOLVED_TOPIC_PREFIX in web: + // https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts + static const resolvedTopicPrefix = '✔ '; + + /// Pattern for an arbitrary resolved-topic prefix. + /// + /// These always begin with [resolvedTopicPrefix] + /// but can be weird and go on longer, like "✔ ✔✔ ". + // This is RESOLVED_TOPIC_PREFIX_RE in web: + // https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts#L4-L12 + static final resolvedTopicPrefixRegexp = RegExp(r'^✔ [ ✔]*'); + /// The string this topic is identified by in the Zulip API. /// /// This should be used in constructing HTTP requests to the server, @@ -682,6 +695,14 @@ extension type const TopicName(String _value) { /// The key to use for "same topic as" comparisons. String canonicalize() => apiName.toLowerCase(); + /// A [TopicName] with [resolvedTopicPrefixRegexp] stripped if present. + TopicName unresolve() => + TopicName(_value.replaceFirst(resolvedTopicPrefixRegexp, '')); + + /// Whether [this] and [other] have the same canonical form, + /// using [canonicalize]. + bool isSameAs(TopicName other) => canonicalize() == other.canonicalize(); + TopicName.fromJson(this._value); String toJson() => apiName; @@ -844,11 +865,6 @@ enum MessageEditState { edited, moved; - // Adapted from the shared code: - // https://github.com/zulip/zulip/blob/1fac99733/web/shared/src/resolved_topic.ts - // The canonical resolved-topic prefix. - static const String _resolvedTopicPrefix = '✔ '; - /// Whether the given topic move reflected either a "resolve topic" /// or "unresolve topic" operation. /// @@ -856,13 +872,21 @@ enum MessageEditState { /// but for purposes of [Message.editState], we want to ignore such renames. /// This method identifies topic moves that should be ignored in that context. static bool topicMoveWasResolveOrUnresolve(TopicName topic, TopicName prevTopic) { - if (topic.apiName.startsWith(_resolvedTopicPrefix) - && topic.apiName.substring(_resolvedTopicPrefix.length) == prevTopic.apiName) { + // Implemented to match web; see analyze_edit_history in zulip/zulip's + // web/src/message_list_view.ts. + // + // Also, this is a hot codepath (decoding messages, a high-volume type of + // data we get from the server), so we avoid calling [canonicalize] and + // using [TopicName.resolvedTopicPrefixRegexp], to be performance-sensitive. + // Discussion: + // https://github.com/zulip/zulip-flutter/pull/1242#discussion_r1917592157 + if (topic.apiName.startsWith(TopicName.resolvedTopicPrefix) + && topic.apiName.substring(TopicName.resolvedTopicPrefix.length) == prevTopic.apiName) { return true; } - if (prevTopic.apiName.startsWith(_resolvedTopicPrefix) - && prevTopic.apiName.substring(_resolvedTopicPrefix.length) == topic.apiName) { + if (prevTopic.apiName.startsWith(TopicName.resolvedTopicPrefix) + && prevTopic.apiName.substring(TopicName.resolvedTopicPrefix.length) == topic.apiName) { return true; } @@ -915,3 +939,13 @@ enum MessageEditState { return MessageEditState.none; } } + +/// As in [updateMessage] or [UpdateMessageEvent.propagateMode]. +@JsonEnum(fieldRename: FieldRename.snake, alwaysCreate: true) +enum PropagateMode { + changeOne, + changeLater, + changeAll; + + String toJson() => _$PropagateModeEnumMap[this]!; +} diff --git a/lib/api/model/model.g.dart b/lib/api/model/model.g.dart index c8434a0fa8..cfa38aec5f 100644 --- a/lib/api/model/model.g.dart +++ b/lib/api/model/model.g.dart @@ -403,3 +403,9 @@ const _$MessageFlagEnumMap = { MessageFlag.historical: 'historical', MessageFlag.unknown: 'unknown', }; + +const _$PropagateModeEnumMap = { + PropagateMode.changeOne: 'change_one', + PropagateMode.changeLater: 'change_later', + PropagateMode.changeAll: 'change_all', +}; diff --git a/lib/api/route/messages.dart b/lib/api/route/messages.dart index ea7421733e..3374741c51 100644 --- a/lib/api/route/messages.dart +++ b/lib/api/route/messages.dart @@ -258,6 +258,39 @@ class SendMessageResult { Map toJson() => _$SendMessageResultToJson(this); } +/// https://zulip.com/api/update-message +Future updateMessage( + ApiConnection connection, { + required int messageId, + TopicName? topic, + PropagateMode? propagateMode, + bool? sendNotificationToOldThread, + bool? sendNotificationToNewThread, + String? content, + int? streamId, +}) { + return connection.patch('updateMessage', UpdateMessageResult.fromJson, 'messages/$messageId', { + if (topic != null) 'topic': RawParameter(topic.apiName), + if (propagateMode != null) 'propagate_mode': RawParameter(propagateMode.toJson()), + if (sendNotificationToOldThread != null) 'send_notification_to_old_thread': sendNotificationToOldThread, + if (sendNotificationToNewThread != null) 'send_notification_to_new_thread': sendNotificationToNewThread, + if (content != null) 'content': RawParameter(content), + if (streamId != null) 'stream_id': streamId, + }); +} + +@JsonSerializable(fieldRename: FieldRename.snake) +class UpdateMessageResult { + // final List detachedUploads; // TODO handle + + UpdateMessageResult(); + + factory UpdateMessageResult.fromJson(Map json) => + _$UpdateMessageResultFromJson(json); + + Map toJson() => _$UpdateMessageResultToJson(this); +} + /// https://zulip.com/api/upload-file Future uploadFile( ApiConnection connection, { diff --git a/lib/api/route/messages.g.dart b/lib/api/route/messages.g.dart index 161ea7588e..3a449d73ac 100644 --- a/lib/api/route/messages.g.dart +++ b/lib/api/route/messages.g.dart @@ -50,6 +50,13 @@ Map _$SendMessageResultToJson(SendMessageResult instance) => 'id': instance.id, }; +UpdateMessageResult _$UpdateMessageResultFromJson(Map json) => + UpdateMessageResult(); + +Map _$UpdateMessageResultToJson( + UpdateMessageResult instance) => + {}; + UploadFileResult _$UploadFileResultFromJson(Map json) => UploadFileResult( uri: json['uri'] as String, diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index b89e9fbdb3..6a90b61e64 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -64,6 +64,20 @@ void _showActionSheet( }); } +/// A button in an action sheet. +/// +/// When built from server data, the action sheet ignores changes in that data; +/// we intentionally don't live-update the buttons on events. +/// If a button's label, action, or position changes suddenly, +/// it can be confusing and make the on-tap behavior unexpected. +/// Better to let the user decide to tap +/// based on information that's comfortably in their working memory, +/// even if we sometimes have to explain (where we handle the tap) +/// that that information has changed and they need to decide again. +/// +/// (Even if we did live-update the buttons, it's possible anyway that a user's +/// action can race with a change that's already been applied on the server, +/// because it takes some time for the server to report changes to us.) abstract class ActionSheetMenuItemButton extends StatelessWidget { const ActionSheetMenuItemButton({super.key, required this.pageContext}); diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index 04d5246195..598b99beac 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -132,7 +132,7 @@ class _InboxPageState extends State with PerAccountStoreAwareStat }); for (final MapEntry(key: streamId, value: topics) in sortedUnreadStreams) { - final topicItems = <(TopicName, int, bool, int)>[]; + final topicItems = <_StreamSectionTopicData>[]; int countInStream = 0; bool streamHasMention = false; for (final MapEntry(key: topic, value: messageIds) in topics.entries) { @@ -140,15 +140,20 @@ class _InboxPageState extends State with PerAccountStoreAwareStat final countInTopic = messageIds.length; final hasMention = messageIds.any((messageId) => unreadsModel!.mentions.contains(messageId)); if (hasMention) streamHasMention = true; - topicItems.add((topic, countInTopic, hasMention, messageIds.last)); + topicItems.add(_StreamSectionTopicData( + topic: topic, + count: countInTopic, + hasMention: hasMention, + lastUnreadId: messageIds.last, + )); countInStream += countInTopic; } if (countInStream == 0) { continue; } topicItems.sort((a, b) { - final (_, _, _, aLastUnreadId) = a; - final (_, _, _, bLastUnreadId) = b; + final aLastUnreadId = a.lastUnreadId; + final bLastUnreadId = b.lastUnreadId; return bLastUnreadId.compareTo(aLastUnreadId); }); sections.add(_StreamSectionData(streamId, countInStream, streamHasMention, topicItems)); @@ -192,11 +197,25 @@ class _StreamSectionData extends _InboxSectionData { final int streamId; final int count; final bool hasMention; - final List<(TopicName, int, bool, int)> items; + final List<_StreamSectionTopicData> items; const _StreamSectionData(this.streamId, this.count, this.hasMention, this.items); } +class _StreamSectionTopicData { + final TopicName topic; + final int count; + final bool hasMention; + final int lastUnreadId; + + const _StreamSectionTopicData({ + required this.topic, + required this.count, + required this.hasMention, + required this.lastUnreadId, + }); +} + abstract class _HeaderItem extends StatelessWidget { final bool collapsed; final _InboxPageState pageState; @@ -466,33 +485,22 @@ class _StreamSection extends StatelessWidget { child: Column(children: [ header, if (!collapsed) ...data.items.map((item) { - final (topic, count, hasMention, _) = item; - return _TopicItem( - streamId: data.streamId, - topic: topic, - count: count, - hasMention: hasMention, - ); + return _TopicItem(streamId: data.streamId, data: item); }), ])); } } class _TopicItem extends StatelessWidget { - const _TopicItem({ - required this.streamId, - required this.topic, - required this.count, - required this.hasMention, - }); + const _TopicItem({required this.streamId, required this.data}); final int streamId; - final TopicName topic; - final int count; - final bool hasMention; + final _StreamSectionTopicData data; @override Widget build(BuildContext context) { + final _StreamSectionTopicData(:topic, :count, :hasMention) = data; + final store = PerAccountStoreWidget.of(context); final subscription = store.subscriptions[streamId]!; diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index f5416e3ccf..c428524712 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -177,8 +177,14 @@ abstract class MessageListPageState { Narrow get narrow; /// The controller for this [MessageListPage]'s compose box, - /// if this [MessageListPage] offers a compose box. + /// if this [MessageListPage] offers a compose box and it has mounted, + /// else null. ComposeBoxController? get composeBoxController; + + /// The active [MessageListView]. + /// + /// This is null if [MessageList] has not mounted yet. + MessageListView? get model; } class MessageListPage extends StatefulWidget { @@ -214,9 +220,12 @@ class _MessageListPageState extends State implements MessageLis @override ComposeBoxController? get composeBoxController => _composeBoxKey.currentState?.controller; - final GlobalKey _composeBoxKey = GlobalKey(); + @override + MessageListView? get model => _messageListKey.currentState?.model; + final GlobalKey<_MessageListState> _messageListKey = GlobalKey(); + @override void initState() { super.initState(); @@ -302,7 +311,11 @@ class _MessageListPageState extends State implements MessageLis removeBottom: ComposeBox.hasComposeBox(narrow), child: Expanded( - child: MessageList(narrow: narrow, onNarrowChanged: _narrowChanged))), + child: MessageList( + key: _messageListKey, + narrow: narrow, + onNarrowChanged: _narrowChanged, + ))), if (ComposeBox.hasComposeBox(narrow)) ComposeBox(key: _composeBoxKey, narrow: narrow) ])))); diff --git a/test/api/model/model_test.dart b/test/api/model/model_test.dart index 5013b9c6c1..95737c173f 100644 --- a/test/api/model/model_test.dart +++ b/test/api/model/model_test.dart @@ -126,6 +126,43 @@ void main() { // MessageEditState group. }); + group('TopicName', () { + test('unresolve', () { + void doCheck(TopicName input, TopicName expected) { + final output = input.unresolve(); + check(output).apiName.equals(expected.apiName); + } + + doCheck(eg.t('some topic'), eg.t('some topic')); + doCheck(eg.t('Some Topic'), eg.t('Some Topic')); + doCheck(eg.t('✔ some topic'), eg.t('some topic')); + doCheck(eg.t('✔ Some Topic'), eg.t('Some Topic')); + + doCheck(eg.t('Some ✔ Topic'), eg.t('Some ✔ Topic')); + doCheck(eg.t('✔ Some ✔ Topic'), eg.t('Some ✔ Topic')); + + doCheck(eg.t('✔ ✔✔✔ some topic'), eg.t('some topic')); + doCheck(eg.t('✔ ✔ ✔✔some topic'), eg.t('some topic')); + }); + + test('isSameAs', () { + void doCheck(TopicName topicA, TopicName topicB, bool expected) { + check(topicA.isSameAs(topicB)).equals(expected); + } + + doCheck(eg.t('some topic'), eg.t('some topic'), true); + doCheck(eg.t('SOME TOPIC'), eg.t('SOME TOPIC'), true); + doCheck(eg.t('Some Topic'), eg.t('sOME tOPIC'), true); + doCheck(eg.t('✔ a'), eg.t('✔ a'), true); + + doCheck(eg.t('✔ some topic'), eg.t('some topic'), false); + doCheck(eg.t('SOME TOPIC'), eg.t('✔ SOME TOPIC'), false); + doCheck(eg.t('✔ Some Topic'), eg.t('sOME tOPIC'), false); + + doCheck(eg.t('✔ a'), eg.t('✔ b'), false); + }); + }); + group('DmMessage', () { final Map baseJson = Map.unmodifiable(deepToJson( eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]), @@ -288,10 +325,35 @@ void main() { ]); }); + // Technically the topic *was* unresolved, so MessageEditState.none + // would be valid and preferable -- if it didn't need more intense + // computation than we're comfortable with in a hot codepath, i.e., + // a regex test instead of a simple `startsWith` / `substring` check. + // See comment on the implementation, and discussion: + // https://github.com/zulip/zulip-flutter/pull/1242#discussion_r1917592157 test('Unresolving topic with a weird prefix -> moved', () { checkEditState(MessageEditState.moved, [{'prev_topic': '✔ ✔old_topic', 'topic': 'old_topic'}]); }); + + // Similar reasoning as in the previous test. + // Also, Zulip doesn't produce topics with a weird resolved-topic prefix, + // so this case can only be produced by unusual input in an + // edit/move-topic UI. A "moved" marker seems like a fine response + // in that circumstance. + test('Resolving topic with a weird prefix -> moved', () { + checkEditState(MessageEditState.moved, + [{'prev_topic': 'old_topic', 'topic': '✔ ✔old_topic'}]); + }); + + // Similar reasoning as the previous test, including that this case had to + // involve unusual input in an edit/move-topic UI. + // Here the computation burden would have come from calling + // [TopicName.canonicalize]. + test('Topic was resolved but with changed case -> moved', () { + checkEditState(MessageEditState.moved, + [{'prev_topic': 'old ToPiC', 'topic': '✔ OLD tOpIc'}]); + }); }); }); } diff --git a/test/api/route/messages_test.dart b/test/api/route/messages_test.dart index eb8fbdac29..4da4334bae 100644 --- a/test/api/route/messages_test.dart +++ b/test/api/route/messages_test.dart @@ -420,6 +420,67 @@ void main() { }); }); + group('updateMessage', () { + Future checkUpdateMessage( + FakeApiConnection connection, { + required int messageId, + TopicName? topic, + PropagateMode? propagateMode, + bool? sendNotificationToOldThread, + bool? sendNotificationToNewThread, + String? content, + int? streamId, + required Map expected, + }) async { + final result = await updateMessage(connection, + messageId: messageId, + topic: topic, + propagateMode: propagateMode, + sendNotificationToOldThread: sendNotificationToOldThread, + sendNotificationToNewThread: sendNotificationToNewThread, + content: content, + streamId: streamId, + ); + check(connection.lastRequest).isA() + ..method.equals('PATCH') + ..url.path.equals('/api/v1/messages/$messageId') + ..bodyFields.deepEquals(expected); + return result; + } + + test('topic/content change', () { + // A separate test exercises `streamId`; + // the API doesn't allow changing channel and content at the same time. + return FakeApiConnection.with_((connection) async { + connection.prepare(json: UpdateMessageResult().toJson()); + await checkUpdateMessage(connection, + messageId: eg.streamMessage().id, + topic: eg.t('new topic'), + propagateMode: PropagateMode.changeAll, + sendNotificationToOldThread: true, + sendNotificationToNewThread: true, + content: 'asdf', + expected: { + 'topic': 'new topic', + 'propagate_mode': 'change_all', + 'send_notification_to_old_thread': 'true', + 'send_notification_to_new_thread': 'true', + 'content': 'asdf', + }); + }); + }); + + test('channel change', () { + return FakeApiConnection.with_((connection) async { + connection.prepare(json: UpdateMessageResult().toJson()); + await checkUpdateMessage(connection, + messageId: eg.streamMessage().id, + streamId: 1, + expected: {'stream_id': '1'}); + }); + }); + }); + group('uploadFile', () { Future checkUploadFile(FakeApiConnection connection, { required List> content,