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.

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

Mark as resolved/unresolved prep 1 #1242

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
8 changes: 0 additions & 8 deletions lib/api/model/events.dart
Original file line number Diff line number Diff line change
Expand Up @@ -766,14 +766,6 @@ class UpdateMessageEvent extends Event {
Map<String, dynamic> toJson() => _$UpdateMessageEventToJson(this);
}

/// As in [UpdateMessageEvent.propagateMode].
@JsonEnum(fieldRename: FieldRename.snake)
enum PropagateMode {
Comment on lines -769 to -771
Copy link
Member

Choose a reason for hiding this comment

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

And this can move from lib/api/model/events.dart to lib/api/model/model.dart, since it's no longer specific to an event.

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 {
Expand Down
2 changes: 1 addition & 1 deletion lib/api/model/events.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

60 changes: 51 additions & 9 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -682,6 +695,22 @@ 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.
///
/// If [resolvedTopicPrefixRegexp] is absent, [this].
TopicName unresolve() {
final match = resolvedTopicPrefixRegexp.firstMatch(_value);
if (match == null) {
return this;
} else {
return TopicName(_value.substring(match.end));
}
}

/// 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;
Expand Down Expand Up @@ -844,25 +873,28 @@ 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.
///
/// The Zulip "resolved topics" feature is implemented by renaming the topic;
/// 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;
}

Expand Down Expand Up @@ -915,3 +947,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]!;
}
6 changes: 6 additions & 0 deletions lib/api/model/model.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

33 changes: 33 additions & 0 deletions lib/api/route/messages.dart
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,39 @@ class SendMessageResult {
Map<String, dynamic> toJson() => _$SendMessageResultToJson(this);
}

/// https://zulip.com/api/update-message
Future<UpdateMessageResult> 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<DetachedUpload> detachedUploads; // TODO handle

UpdateMessageResult();

factory UpdateMessageResult.fromJson(Map<String, dynamic> json) =>
_$UpdateMessageResultFromJson(json);

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

/// https://zulip.com/api/upload-file
Future<UploadFileResult> uploadFile(
ApiConnection connection, {
Expand Down
7 changes: 7 additions & 0 deletions lib/api/route/messages.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions lib/widgets/action_sheet.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +69 to +73
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yeah.

Looking for PerAccountStoreWidget.of calls in this file, I think there's one place we don't currently follow this: the hasSelfVote in ReactionButtons.

That one shouldn't cause this problem in practice, though, because it changes by the user's own action. (Or by the message getting deleted, but then it doesn't matter whether the user tries to add a reaction or remove one.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In a quick manual test, I don't even see the hasSelfVote live-updating in the message action sheet as I add or remove one of the popular emojis from the web app.

hasSelfVote uses PerAccountStoreWidget.of to get the self-user's ID. My user ID doesn't change when I add/remove a reaction. And although we're still signing up to refresh whenever the PerAccountStore notifies listeners, it doesn't notify listeners on ReactionEvents. (Any MessageListViews do notify their listeners.)

Copy link
Member

Choose a reason for hiding this comment

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

In a quick manual test, I don't even see the hasSelfVote live-updating in the message action sheet as I add or remove one of the popular emojis from the web app.

Right, but then it's still your own action — you know that you just made that change (even though via a different client). So there isn't a risk of confusion.

/// 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.
Comment on lines +73 to +76
Copy link
Member

Choose a reason for hiding this comment

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

A key point here is that these races are always possible anyway — the change may have happened on the server but not reached us yet.

///
/// (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});

Expand Down
50 changes: 29 additions & 21 deletions lib/widgets/inbox.dart
Original file line number Diff line number Diff line change
Expand Up @@ -132,23 +132,28 @@ class _InboxPageState extends State<InboxPageBody> 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) {
if (!store.isTopicVisible(streamId, topic)) continue;
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));
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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]!;

Expand Down
19 changes: 16 additions & 3 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -214,9 +220,12 @@ class _MessageListPageState extends State<MessageListPage> implements MessageLis

@override
ComposeBoxController? get composeBoxController => _composeBoxKey.currentState?.controller;

final GlobalKey<ComposeBoxState> _composeBoxKey = GlobalKey();

@override
MessageListView? get model => _messageListKey.currentState?.model;
final GlobalKey<_MessageListState> _messageListKey = GlobalKey();

@override
void initState() {
super.initState();
Expand Down Expand Up @@ -302,7 +311,11 @@ class _MessageListPageState extends State<MessageListPage> 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)
]))));
Expand Down
Loading