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

Handle UpdateMessageEvent in MessageListView (#118) #238

Merged
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
16 changes: 12 additions & 4 deletions lib/api/model/events.dart
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class UnexpectedEvent extends Event {
Map<String, dynamic> toJson() => json;
}

/// A Zulip event of type `alert_words`.
/// A Zulip event of type `alert_words`: https://zulip.com/api/get-events#alert_words
@JsonSerializable(fieldRename: FieldRename.snake)
class AlertWordsEvent extends Event {
@override
Expand All @@ -74,6 +74,9 @@ class AlertWordsEvent extends Event {
}

/// A Zulip event of type `realm_user`.
///
/// The corresponding API docs are in several places for
/// different values of `op`; see subclasses.
abstract class RealmUserEvent extends Event {
@override
@JsonKey(includeToJson: true)
Expand Down Expand Up @@ -122,6 +125,7 @@ class RealmUserRemoveEvent extends RealmUserEvent {
}
}

/// As in [RealmUserUpdateEvent.customProfileField].
@JsonSerializable(fieldRename: FieldRename.snake)
class RealmUserUpdateCustomProfileField {
final int id;
Expand Down Expand Up @@ -184,6 +188,9 @@ class RealmUserUpdateEvent extends RealmUserEvent {
}

/// A Zulip event of type `stream`.
///
/// The corresponding API docs are in several places for
/// different values of `op`; see subclasses.
abstract class StreamEvent extends Event {
@override
@JsonKey(includeToJson: true)
Expand Down Expand Up @@ -231,7 +238,7 @@ class StreamDeleteEvent extends StreamEvent {
// TODO(#182) StreamUpdateEvent, for a [StreamEvent] with op `update`:
// https://zulip.com/api/get-events#stream-update

/// A Zulip event of type `message`.
/// A Zulip event of type `message`: https://zulip.com/api/get-events#message
// TODO use [JsonSerializable] here too, using its customization features,
// in order to skip the boilerplate in [fromJson] and [toJson].
class MessageEvent extends Event {
Expand Down Expand Up @@ -267,7 +274,7 @@ class MessageEvent extends Event {
}
}

/// A Zulip event of type `update_message`.
/// A Zulip event of type `update_message`: https://zulip.com/api/get-events#update_message
@JsonSerializable(fieldRename: FieldRename.snake)
class UpdateMessageEvent extends Event {
@override
Expand Down Expand Up @@ -329,7 +336,7 @@ enum PropagateMode {
changeAll;
}

/// A Zulip event of type `delete_message`.
/// A Zulip event of type `delete_message`: https://zulip.com/api/get-events#delete_message
@JsonSerializable(fieldRename: FieldRename.snake)
class DeleteMessageEvent extends Event {
@override
Expand Down Expand Up @@ -363,6 +370,7 @@ enum MessageType {
private;
}

/// A Zulip event of type `heartbeat`: https://zulip.com/api/get-events#heartbeat
@JsonSerializable(fieldRename: FieldRename.snake)
class HeartbeatEvent extends Event {
@override
Expand Down
8 changes: 4 additions & 4 deletions lib/api/model/model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -250,13 +250,13 @@ class Subscription {
sealed class Message {
final String? avatarUrl;
final String client;
final String content;
String content;
final String contentType;

// final List<MessageEditHistory> editHistory; // TODO handle
final int id;
final bool isMeMessage;
final int? lastEditTimestamp;
bool isMeMessage;
int? lastEditTimestamp;

// final List<Reaction> reactions; // TODO handle
final int recipientId;
Expand All @@ -271,7 +271,7 @@ sealed class Message {

// final List<TopicLink> topicLinks; // TODO handle
// final string type; // handled by runtime type of object
final List<String> flags; // TODO enum
List<String> flags; // TODO enum
final String? matchContent;
final String? matchSubject;

Expand Down
65 changes: 65 additions & 0 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import 'package:flutter/foundation.dart';

import '../api/model/events.dart';
import '../api/model/model.dart';
import '../api/route/messages.dart';
import 'content.dart';
Expand Down Expand Up @@ -86,6 +87,70 @@ class MessageListView extends ChangeNotifier {
notifyListeners();
}

_applyChangesToMessage(UpdateMessageEvent event, Message message) {
// TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
final isRenderingOnly = event.renderingOnly ?? (event.userId == null);
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.
message.lastEditTimestamp = event.editTimestamp;
}

message.flags = event.flags;

if (event.renderedContent != null) {
oxling marked this conversation as resolved.
Show resolved Hide resolved
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!;
}
}

// This is almost directly copied from package:collection/src/algorithms.dart
// The way that package was set up doesn't allow us to search
// for a message ID among a bunch of message objects - this is a quick
// modification of that method to work here for us.
@visibleForTesting
int findMessageWithId(int messageId) {
oxling marked this conversation as resolved.
Show resolved Hide resolved
var min = 0;
var max = messages.length;
while (min < max) {
var mid = min + ((max - min) >> 1);
final message = messages[mid];
var comp = message.id.compareTo(messageId);
if (comp == 0) return mid;
if (comp < 0) {
min = mid + 1;
} else {
max = mid;
}
}
return -1;
}

/// Update the message the given event applies to, if present in this view.
///
/// This method only handles the case where the message's contents
/// were changed, and ignores any changes to its stream or topic.
///
/// TODO(#150): Handle message moves.
void maybeUpdateMessage(UpdateMessageEvent event) {
final idx = findMessageWithId(event.messageId);
if (idx == -1) {
return;
}

final message = messages[idx];
_applyChangesToMessage(event, message);

contents[idx] = parseContent(message.content);
notifyListeners();
}

/// Called when the app is reassembled during debugging, e.g. for hot reload.
///
/// This will redo from scratch any computations we can, such as parsing
Expand Down
4 changes: 3 additions & 1 deletion lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,9 @@ class PerAccountStore extends ChangeNotifier {
}
} else if (event is UpdateMessageEvent) {
assert(debugLog("server event: update_message ${event.messageId}"));
// TODO handle
for (final view in _messageListViews) {
view.maybeUpdateMessage(event);
}
} else if (event is DeleteMessageEvent) {
assert(debugLog("server event: delete_message ${event.messageIds}"));
// TODO handle
Expand Down
3 changes: 3 additions & 0 deletions test/api/model/model_checks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ extension MessageChecks on Subject<Message> {
toJson.deepEquals(expected.toJson());
}

Subject<String> get content => has((e) => e.content, 'content');
Subject<bool> get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage');
Subject<int?> get lastEditTimestamp => has((e) => e.lastEditTimestamp, 'lastEditTimestamp');
Subject<List<String>> get flags => has((e) => e.flags, 'flags');

// TODO accessors for other fields
Expand Down
6 changes: 4 additions & 2 deletions test/example_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ StreamMessage streamMessage({
String? topic,
String? content,
String? contentMarkdown,
List<String>? flags,
oxling marked this conversation as resolved.
Show resolved Hide resolved
}) {
final effectiveStream = stream ?? _stream();
// The use of JSON here is convenient in order to delegate parts of the data
Expand All @@ -140,7 +141,7 @@ StreamMessage streamMessage({
..._messagePropertiesFromContent(content, contentMarkdown),
'display_recipient': effectiveStream.name,
'stream_id': effectiveStream.streamId,
'flags': [],
'flags': flags ?? [],
'id': id ?? 1234567, // TODO generate example IDs
'subject': topic ?? 'example topic',
'timestamp': 1678139636,
Expand All @@ -158,6 +159,7 @@ DmMessage dmMessage({
required List<User> to,
String? content,
String? contentMarkdown,
List<String>? flags,
}) {
assert(!to.any((user) => user.userId == from.userId));
return DmMessage.fromJson({
Expand All @@ -168,7 +170,7 @@ DmMessage dmMessage({
.map((u) => {'id': u.userId, 'email': u.email, 'full_name': u.fullName})
.toList(growable: false),

'flags': [],
'flags': flags ?? [],
'id': id ?? 1234567, // TODO generate example IDs
'subject': '',
'timestamp': 1678139636,
Expand Down
Loading