Skip to content

Commit

Permalink
msglist: Handle updated events in MessageListView (zulip#118).
Browse files Browse the repository at this point in the history
Processes an UpdateMessageEvent and hands it off to the MessageListView to update, if the message is visible in the MessageListView. This completes the changes required for issue zulip#118.
  • Loading branch information
oxling committed Jul 26, 2023
1 parent 2465701 commit 5df994d
Show file tree
Hide file tree
Showing 6 changed files with 288 additions and 6 deletions.
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
66 changes: 66 additions & 0 deletions lib/model/message_list.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import 'package:collection/collection.dart';
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 +88,70 @@ class MessageListView extends ChangeNotifier {
notifyListeners();
}

_applyChangesToMessage(UpdateMessageEvent event, Message message) {
//In earlier server versions, omitting the userId indicates that this is a rendering-only update.
//That means this change was initiated by the server, not the user.
final isRenderingOnly = (event.renderingOnly != null && event.renderingOnly == true) ||
event.userId == null;

if (event.editTimestamp != null && !isRenderingOnly) {
//Only update the timestamp if this was a user-led update,
//not a server-only update
message.lastEditTimestamp = event.editTimestamp;
}

if (!event.flags.equals(message.flags)) {
message.flags = event.flags;
}

if (event.renderedContent != null) {
assert(message.contentType == 'text/html', "Expected message to have html contentType. Instead, got ${message.contentType}");
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) {
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;
}

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 @@ -9,6 +9,9 @@ extension MessageChecks on Subject<Message> {
}

Subject<List<String>> get flags => has((e) => e.flags, 'flags');
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');

// TODO accessors for other fields
}
Expand Down
3 changes: 2 additions & 1 deletion 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,
}) {
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 Down
210 changes: 210 additions & 0 deletions test/model/message_list_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
import 'package:checks/checks.dart';
import 'package:test/scaffolding.dart';
import 'package:zulip/api/model/events.dart';
import 'package:zulip/api/model/model.dart';
import 'package:zulip/api/route/messages.dart';
import 'package:zulip/model/message_list.dart';
import 'package:zulip/model/narrow.dart';
import 'package:zulip/model/store.dart';
import '../api/fake_api.dart';
import '../api/model/model_checks.dart';
import '../model/binding.dart';
import '../model/test_store.dart';
import '../example_data.dart' as eg;

const int userId = 1;
const int streamId = 2;

Future<PerAccountStore> setupStore(ZulipStream stream) async {
addTearDown(TestZulipBinding.instance.reset);

await TestZulipBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot());

final store = await TestZulipBinding.instance.globalStore.perAccount(eg.selfAccount.id);
store.addUser(eg.user(userId: userId));
store.addStream(stream);

return store;
}

Future<MessageListView> messageListViewWithMessages(List<Message> messages, PerAccountStore store, Narrow narrow) async {
final messageList = MessageListView.init(store: store, narrow: narrow);

final connection = store.connection as FakeApiConnection;

connection.prepare(json: GetMessagesResult(
anchor: messages.first.id,
foundNewest: true,
foundOldest: true,
foundAnchor: true,
historyLimited: false,
messages: messages,
).toJson());

await messageList.fetch();

check(messageList.messages.length).equals(messages.length);

return messageList;
}

void main() async {
TestZulipBinding.ensureInitialized();

const narrow = StreamNarrow(streamId);
final stream = eg.stream(streamId: streamId);

group('update message tests', () {

test('find message in message list returns index of message', () async {
final store = await setupStore(stream);

final m1 = eg.streamMessage(id: 792, stream: stream);
final m2 = eg.streamMessage(id: 793, stream: stream);
final m3 = eg.streamMessage(id: 794, stream: stream);

final messageList = await messageListViewWithMessages([m1, m2, m3], store, narrow);

var idx = messageList.findMessageWithId(793);
check(idx).equals(1);

idx = messageList.findMessageWithId(999);
check(idx).equals(-1);
});

test('update events are correctly applied to message when it is in the stream', () async {
final store = await setupStore(stream);

const oldContent = "<p>Hello, world</p>";
const newContent = "<p>Hello, edited</p>";
const newTimestamp = 99999;

List<String> oldFlags = [];
List<String> newFlags = ["starred"];

final mockMessage = eg.streamMessage(id: 243, stream: stream, content: oldContent, flags: oldFlags);
final messageList = await messageListViewWithMessages([mockMessage], store, narrow);

final updateEvent = UpdateMessageEvent(
id: 1,
messageId: mockMessage.id,
messageIds: [mockMessage.id],
flags: newFlags,
renderedContent: newContent,
editTimestamp: newTimestamp,
isMeMessage: true,
userId: userId
);

final message = messageList.messages[0];
check(message)
..content.equals(oldContent)
..flags.deepEquals(oldFlags)
..isMeMessage.equals(false);

var listenersNotified = false;

messageList.addListener(() { listenersNotified = true; });
messageList.maybeUpdateMessage(updateEvent);

final updatedMessage = messageList.messages[0];
check(updatedMessage).identicalTo(message);
check(listenersNotified).equals(true);

check(message)
..content.equals(newContent)
..lastEditTimestamp.equals(newTimestamp)
..flags.equals(newFlags)
..isMeMessage.equals(true);
});

test('update event is ignored when message is not in the message list', () async {
final store = await setupStore(stream);

const oldContent = "<p>Hello, world</p>";
const newContent = "<p>Hello, edited</p>";
const newTimestamp = 99999;

final mockMessage = eg.streamMessage(id: 243, stream: stream, content: oldContent);
final messageList = await messageListViewWithMessages([mockMessage], store, narrow);

final updateEvent = UpdateMessageEvent(
id: 1,
messageId: 972,
messageIds: [972],
flags: mockMessage.flags,
renderedContent: newContent,
editTimestamp: newTimestamp,
userId: userId,
);

final message = messageList.messages[0];
check(message).content.equals(oldContent);

var listenersNotified = false;

messageList.addListener(() { listenersNotified = true; });
messageList.maybeUpdateMessage(updateEvent);

check(listenersNotified).equals(false);
check(message).content.equals(oldContent);

});
test('rendering-only update does not change timestamp', () async {
final store = await setupStore(stream);

const oldContent = "<p>Hello, world</p>";
const newContent = "<p>Hello, world</p> <div>Some link preview</div>";
const newTimestamp = 99999;

final mockMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent);
final messageList = await messageListViewWithMessages([mockMessage], store, narrow);

final updateEvent = UpdateMessageEvent(
id: 1,
messageId: 972,
messageIds: [972],
flags: mockMessage.flags,
renderedContent: newContent,
editTimestamp: newTimestamp,
renderingOnly: true,
userId: null,
);

final message = messageList.messages[0];
messageList.maybeUpdateMessage(updateEvent);
check(message)
..content.equals(newContent)
..lastEditTimestamp.isNull();
});

test('rendering-only update does not change timestamp (for old server versions)', () async {
final store = await setupStore(stream);

const oldContent = "<p>Hello, world</p>";
const newContent = "<p>Hello, world</p> <div>Some link preview</div>";
const newTimestamp = 99999;

final mockMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent);
final messageList = await messageListViewWithMessages([mockMessage], store, narrow);

final updateEvent = UpdateMessageEvent(
id: 1,
messageId: 972,
messageIds: [972],
flags: mockMessage.flags,
renderedContent: newContent,
editTimestamp: newTimestamp,
userId: null,
);

final message = messageList.messages[0];
messageList.maybeUpdateMessage(updateEvent);
check(message)
..content.equals(newContent)
..lastEditTimestamp.isNull();
});


});
}

0 comments on commit 5df994d

Please sign in to comment.