Skip to content

Add skeleton of handling a move UpdateMessageEvent #753

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

Merged
merged 3 commits into from
Jun 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions lib/api/model/events.dart
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,7 @@ class MessageEvent extends Event {
@JsonSerializable(fieldRename: FieldRename.snake)
class UpdateMessageEvent extends Event {
@override
@JsonKey(includeToJson: true)
String get type => 'update_message';

final int? userId; // TODO(server-5)
Expand Down Expand Up @@ -683,6 +684,7 @@ enum PropagateMode {
@JsonSerializable(fieldRename: FieldRename.snake)
class DeleteMessageEvent extends Event {
@override
@JsonKey(includeToJson: true)
String get type => 'delete_message';

final List<int> messageIds;
Expand Down
2 changes: 2 additions & 0 deletions lib/api/model/events.g.dart

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

64 changes: 55 additions & 9 deletions lib/model/message.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import '../api/model/events.dart';
import '../api/model/model.dart';
import '../log.dart';
import 'message_list.dart';

/// The portion of [PerAccountStore] for messages and message lists.
Expand Down Expand Up @@ -93,32 +94,42 @@ class MessageStoreImpl with MessageStore {
}

void handleUpdateMessageEvent(UpdateMessageEvent event) {
assert(event.messageIds.contains(event.messageId), "See https://github.com/zulip/zulip-flutter/pull/753#discussion_r1649463633");
_handleUpdateMessageEventTimestamp(event);
_handleUpdateMessageEventContent(event);
// TODO(#150): Handle message moves. The views' recipient headers
// may need updating, and consequently showSender too.
_handleUpdateMessageEventMove(event);
for (final view in _messageListViews) {
view.notifyListenersIfAnyMessagePresent(event.messageIds);
}
}

void _handleUpdateMessageEventContent(UpdateMessageEvent event) {
final message = messages[event.messageId];
if (message == null) return;

void _handleUpdateMessageEventTimestamp(UpdateMessageEvent event) {
// TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
final isRenderingOnly = event.renderingOnly ?? (event.userId == null);
if (event.editTimestamp != null && !isRenderingOnly) {
if (event.editTimestamp == null || isRenderingOnly) {
// A rendering-only update gets omitted from the message edit history,
// and [Message.lastEditTimestamp] is the last timestamp of that history.
// So on a rendering-only update, the timestamp doesn't get updated.
return;
}

for (final messageId in event.messageIds) {
final message = messages[messageId];
if (message == null) continue;
message.lastEditTimestamp = event.editTimestamp;
}
}

message.flags = event.flags;
void _handleUpdateMessageEventContent(UpdateMessageEvent event) {
final message = messages[event.messageId];
if (message == null) return;

message.flags = event.flags;
if (event.renderedContent != null) {
assert(message.contentType == 'text/html',
"Message contentType was ${message.contentType}; expected text/html.");
message.content = event.renderedContent!;
}

if (event.isMeMessage != null) {
message.isMeMessage = event.isMeMessage!;
}
Expand All @@ -128,6 +139,41 @@ class MessageStoreImpl with MessageStore {
}
}

void _handleUpdateMessageEventMove(UpdateMessageEvent event) {
// The interaction between the fields of these events are a bit tricky.
// For reference, see: https://zulip.com/api/get-events#update_message

if (event.origTopic == null) {
// There was no move.
assert(() {
if (event.newStreamId != null && event.origStreamId != null
&& event.newStreamId != event.origStreamId) {
// This should be impossible; `orig_subject` (aka origTopic) is
// documented to be present when either the stream or topic changed.
debugLog('Malformed UpdateMessageEvent: stream move but no origTopic'); // TODO(log)
}
return true;
}());
return;
}

if (event.newTopic == null) {
Copy link
Member

Choose a reason for hiding this comment

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

I found this check a bit confusing (as well as the following one on origStreamId) before looking at the API doc. It could be helpful to spell out what we are expecting these checks (something that I can add in my PRs stacked atop this).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm yeah, these are a bit terse.

Would this version explain it?

@@ -139,6 +139,9 @@ class MessageStoreImpl with MessageStore {
   }
 
   void _handleUpdateMessageEventMove(UpdateMessageEvent event) {
+    // The interaction between the fields of these events are a bit tricky.
+    // For reference, see: https://zulip.com/api/get-events#update_message
+
     if (event.origTopic == null) {
       // There was no move.
       assert(() {
@@ -154,10 +157,12 @@ class MessageStoreImpl with MessageStore {
     }
 
     if (event.newTopic == null) {
-      assert(debugLog('Malformed UpdateMessageEvent: move but no topic')); // TODO(log)
+      // The `subject` field (aka newTopic) is documented to be present on moves.
+      assert(debugLog('Malformed UpdateMessageEvent: move but no newTopic')); // TODO(log)
       return;
     }
     if (event.origStreamId == null) {
+      // The `stream_id` field (aka origStreamId) is documented to be present on moves.
       assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log)
       return;
     }

(That also fixes "no topic" to "no newTopic" — that's from a draft where I'd renamed that field only to "topic" rather than "newTopic".)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this works.

// The `subject` field (aka newTopic) is documented to be present on moves.
assert(debugLog('Malformed UpdateMessageEvent: move but no newTopic')); // TODO(log)
return;
}
if (event.origStreamId == null) {
// The `stream_id` field (aka origStreamId) is documented to be present on moves.
assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log)
return;
}

// final newStreamId = event.newStreamId; // null if topic-only move
// final newTopic = event.newTopic!;
// TODO(#150): Handle message moves. The views' recipient headers
// may need updating, and consequently showSender too.
}

void handleDeleteMessageEvent(DeleteMessageEvent event) {
// TODO handle DeleteMessageEvent, particularly in MessageListView
}
Expand Down
5 changes: 4 additions & 1 deletion lib/model/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -467,11 +467,14 @@ class MessageListView with ChangeNotifier, _MessageSequence {
}
}

/// Update data derived from the content of the given message.
///
/// This does not notify listeners.
/// The caller should ensure that happens later.
void messageContentChanged(int messageId) {
final index = _findMessageWithId(messageId);
if (index != -1) {
_reparseContent(index);
notifyListeners();
}
}

Expand Down
28 changes: 28 additions & 0 deletions test/model/message_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,34 @@ void main() {
});

group('handleUpdateMessageEvent', () {
test('update timestamps on all messages', () async {
const t1 = 1718748879;
const t2 = t1 + 60;
final message1 = eg.streamMessage(lastEditTimestamp: null);
final message2 = eg.streamMessage(lastEditTimestamp: t1);
// This event is a bit artificial, but convenient.
// TODO use a realistic move-messages event here
final updateEvent = Event.fromJson({
...eg.updateMessageEditEvent(message1).toJson(),
Comment on lines +170 to +173
Copy link
Member Author

Choose a reason for hiding this comment

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

@PIG208 as part of #171 you'll be writing a helper for conveniently making UpdateMessageEvent values representing a move (an eg.updateMessageMoveEvent similar to the existing eg.updateMessageEditEvent). So then you can follow that with a small commit fixing this TODO.

'message_ids': [message1.id, message2.id],
'edit_timestamp': t2,
}) as UpdateMessageEvent;
await prepare();
await prepareMessages([message1, message2]);

check(store).messages.values.unorderedMatches(<Condition<Message>>[
(it) => it.lastEditTimestamp.isNull,
(it) => it.lastEditTimestamp.equals(t1),
]);

await store.handleEvent(updateEvent);
checkNotifiedOnce();
check(store).messages.values.unorderedMatches(<Condition<Message>>[
(it) => it.lastEditTimestamp.equals(t2),
(it) => it.lastEditTimestamp.equals(t2),
]);
});

test('update a message', () async {
final originalMessage = eg.streamMessage(
content: "<p>Hello, world</p>");
Expand Down
Loading