Skip to content

Commit cff73ee

Browse files
oxlinggnprice
oxling
authored and
gnprice
committed
msglist: Handle edited messages in view-model
Processes an UpdateMessageEvent and hands it off to the MessageListView to update, if the message is visible in the MessageListView. Fixes: #118
1 parent 7c77e93 commit cff73ee

File tree

6 files changed

+307
-7
lines changed

6 files changed

+307
-7
lines changed

‎lib/api/model/model.dart

+4-4
Original file line numberDiff line numberDiff line change
@@ -250,13 +250,13 @@ class Subscription {
250250
sealed class Message {
251251
final String? avatarUrl;
252252
final String client;
253-
final String content;
253+
String content;
254254
final String contentType;
255255

256256
// final List<MessageEditHistory> editHistory; // TODO handle
257257
final int id;
258-
final bool isMeMessage;
259-
final int? lastEditTimestamp;
258+
bool isMeMessage;
259+
int? lastEditTimestamp;
260260

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

272272
// final List<TopicLink> topicLinks; // TODO handle
273273
// final string type; // handled by runtime type of object
274-
final List<String> flags; // TODO enum
274+
List<String> flags; // TODO enum
275275
final String? matchContent;
276276
final String? matchSubject;
277277

‎lib/model/message_list.dart

+68
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import 'package:flutter/foundation.dart';
22

3+
import '../api/model/events.dart';
34
import '../api/model/model.dart';
45
import '../api/route/messages.dart';
56
import 'content.dart';
@@ -86,6 +87,73 @@ class MessageListView extends ChangeNotifier {
8687
notifyListeners();
8788
}
8889

90+
_applyChangesToMessage(UpdateMessageEvent event, Message message) {
91+
// In earlier server versions, omitting the userId indicates that this is a
92+
// rendering-only update. That means this change was initiated by the server,
93+
// not the user.
94+
//
95+
// TODO(server-5): Cut this fallback; rely on renderingOnly from FL 114
96+
final isRenderingOnly = event.renderingOnly ?? (event.userId == null);
97+
if (event.editTimestamp != null && !isRenderingOnly) {
98+
// Only update the timestamp if this was a user-led update,
99+
// not a server-only update
100+
message.lastEditTimestamp = event.editTimestamp;
101+
}
102+
103+
message.flags = event.flags;
104+
105+
if (event.renderedContent != null) {
106+
assert(message.contentType == 'text/html',
107+
"Message contentType was ${message.contentType}; expected text/html.");
108+
message.content = event.renderedContent!;
109+
}
110+
111+
if (event.isMeMessage != null) {
112+
message.isMeMessage = event.isMeMessage!;
113+
}
114+
}
115+
116+
// This is almost directly copied from package:collection/src/algorithms.dart
117+
// The way that package was set up doesn't allow us to search
118+
// for a message ID among a bunch of message objects - this is a quick
119+
// modification of that method to work here for us.
120+
@visibleForTesting
121+
int findMessageWithId(int messageId) {
122+
var min = 0;
123+
var max = messages.length;
124+
while (min < max) {
125+
var mid = min + ((max - min) >> 1);
126+
final message = messages[mid];
127+
var comp = message.id.compareTo(messageId);
128+
if (comp == 0) return mid;
129+
if (comp < 0) {
130+
min = mid + 1;
131+
} else {
132+
max = mid;
133+
}
134+
}
135+
return -1;
136+
}
137+
138+
/// Update the message the given event applies to, if present in this view.
139+
///
140+
/// This method only handles the case where the message's contents
141+
/// were changed, and ignores any changes to its stream or topic.
142+
///
143+
/// TODO(#150): Handle message moves.
144+
void maybeUpdateMessage(UpdateMessageEvent event) {
145+
final idx = findMessageWithId(event.messageId);
146+
if (idx == -1) {
147+
return;
148+
}
149+
150+
final message = messages[idx];
151+
_applyChangesToMessage(event, message);
152+
153+
contents[idx] = parseContent(message.content);
154+
notifyListeners();
155+
}
156+
89157
/// Called when the app is reassembled during debugging, e.g. for hot reload.
90158
///
91159
/// This will redo from scratch any computations we can, such as parsing

‎lib/model/store.dart

+3-1
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,9 @@ class PerAccountStore extends ChangeNotifier {
272272
}
273273
} else if (event is UpdateMessageEvent) {
274274
assert(debugLog("server event: update_message ${event.messageId}"));
275-
// TODO handle
275+
for (final view in _messageListViews) {
276+
view.maybeUpdateMessage(event);
277+
}
276278
} else if (event is DeleteMessageEvent) {
277279
assert(debugLog("server event: delete_message ${event.messageIds}"));
278280
// TODO handle

‎test/api/model/model_checks.dart

+3
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ extension MessageChecks on Subject<Message> {
88
toJson.deepEquals(expected.toJson());
99
}
1010

11+
Subject<String> get content => has((e) => e.content, 'content');
12+
Subject<bool> get isMeMessage => has((e) => e.isMeMessage, 'isMeMessage');
13+
Subject<int?> get lastEditTimestamp => has((e) => e.lastEditTimestamp, 'lastEditTimestamp');
1114
Subject<List<String>> get flags => has((e) => e.flags, 'flags');
1215

1316
// TODO accessors for other fields

‎test/example_data.dart

+4-2
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ StreamMessage streamMessage({
127127
String? topic,
128128
String? content,
129129
String? contentMarkdown,
130+
List<String>? flags,
130131
}) {
131132
final effectiveStream = stream ?? _stream();
132133
// The use of JSON here is convenient in order to delegate parts of the data
@@ -140,7 +141,7 @@ StreamMessage streamMessage({
140141
..._messagePropertiesFromContent(content, contentMarkdown),
141142
'display_recipient': effectiveStream.name,
142143
'stream_id': effectiveStream.streamId,
143-
'flags': [],
144+
'flags': flags ?? [],
144145
'id': id ?? 1234567, // TODO generate example IDs
145146
'subject': topic ?? 'example topic',
146147
'timestamp': 1678139636,
@@ -158,6 +159,7 @@ DmMessage dmMessage({
158159
required List<User> to,
159160
String? content,
160161
String? contentMarkdown,
162+
List<String>? flags,
161163
}) {
162164
assert(!to.any((user) => user.userId == from.userId));
163165
return DmMessage.fromJson({
@@ -168,7 +170,7 @@ DmMessage dmMessage({
168170
.map((u) => {'id': u.userId, 'email': u.email, 'full_name': u.fullName})
169171
.toList(growable: false),
170172

171-
'flags': [],
173+
'flags': flags ?? [],
172174
'id': id ?? 1234567, // TODO generate example IDs
173175
'subject': '',
174176
'timestamp': 1678139636,

‎test/model/message_list_test.dart

+225
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,225 @@
1+
import 'package:checks/checks.dart';
2+
import 'package:test/scaffolding.dart';
3+
import 'package:zulip/api/model/events.dart';
4+
import 'package:zulip/api/model/model.dart';
5+
import 'package:zulip/api/route/messages.dart';
6+
import 'package:zulip/model/message_list.dart';
7+
import 'package:zulip/model/narrow.dart';
8+
import 'package:zulip/model/store.dart';
9+
10+
import '../api/fake_api.dart';
11+
import '../api/model/model_checks.dart';
12+
import '../model/binding.dart';
13+
import '../model/test_store.dart';
14+
import '../example_data.dart' as eg;
15+
16+
const int userId = 1;
17+
18+
Future<PerAccountStore> setupStore(ZulipStream stream) async {
19+
addTearDown(TestZulipBinding.instance.reset);
20+
21+
await TestZulipBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot());
22+
23+
final store = await TestZulipBinding.instance.globalStore.perAccount(eg.selfAccount.id);
24+
store.addUser(eg.user(userId: userId));
25+
store.addStream(stream);
26+
27+
return store;
28+
}
29+
30+
Future<MessageListView> messageListViewWithMessages(List<Message> messages, PerAccountStore store, Narrow narrow) async {
31+
final messageList = MessageListView.init(store: store, narrow: narrow);
32+
33+
final connection = store.connection as FakeApiConnection;
34+
35+
connection.prepare(json: GetMessagesResult(
36+
anchor: messages.first.id,
37+
foundNewest: true,
38+
foundOldest: true,
39+
foundAnchor: true,
40+
historyLimited: false,
41+
messages: messages,
42+
).toJson());
43+
44+
await messageList.fetch();
45+
46+
check(messageList.messages.length).equals(messages.length);
47+
48+
return messageList;
49+
}
50+
51+
void main() async {
52+
TestZulipBinding.ensureInitialized();
53+
54+
final stream = eg.stream();
55+
final narrow = StreamNarrow(stream.streamId);
56+
57+
group('update message tests', () {
58+
59+
test('find message in message list returns index of message', () async {
60+
final store = await setupStore(stream);
61+
62+
final m1 = eg.streamMessage(id: 2, stream: stream);
63+
final m2 = eg.streamMessage(id: 4, stream: stream);
64+
final m3 = eg.streamMessage(id: 6, stream: stream);
65+
66+
final messageList = await messageListViewWithMessages([m1, m2, m3], store, narrow);
67+
// The implementation of this uses a binary search, so let's test it
68+
// a bit more exhaustively.
69+
70+
check(messageList.findMessageWithId(1)).equals(-1);
71+
check(messageList.findMessageWithId(2)).equals(0);
72+
check(messageList.findMessageWithId(3)).equals(-1);
73+
check(messageList.findMessageWithId(4)).equals(1);
74+
check(messageList.findMessageWithId(5)).equals(-1);
75+
check(messageList.findMessageWithId(6)).equals(2);
76+
check(messageList.findMessageWithId(7)).equals(-1);
77+
78+
// Invalid IDs
79+
check(messageList.findMessageWithId(-8409)).equals(-1);
80+
check(messageList.findMessageWithId(0)).equals(-1);
81+
});
82+
83+
test('update events are correctly applied to message when it is in the stream', () async {
84+
final store = await setupStore(stream);
85+
86+
const oldContent = "<p>Hello, world</p>";
87+
const newContent = "<p>Hello, edited</p>";
88+
const newTimestamp = 99999;
89+
90+
List<String> oldFlags = [];
91+
List<String> newFlags = ["starred"];
92+
93+
final originalMessage = eg.streamMessage(id: 243, stream: stream, content: oldContent, flags: oldFlags);
94+
final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
95+
96+
final updateEvent = UpdateMessageEvent(
97+
id: 1,
98+
messageId: originalMessage.id,
99+
messageIds: [originalMessage.id],
100+
flags: newFlags,
101+
renderedContent: newContent,
102+
editTimestamp: newTimestamp,
103+
isMeMessage: true,
104+
userId: userId,
105+
renderingOnly: false,
106+
);
107+
108+
final message = messageList.messages.single;
109+
check(message)
110+
..content.equals(oldContent)
111+
..flags.deepEquals(oldFlags)
112+
..isMeMessage.isFalse();
113+
114+
var listenersNotified = false;
115+
116+
messageList.addListener(() { listenersNotified = true; });
117+
messageList.maybeUpdateMessage(updateEvent);
118+
119+
check(listenersNotified).isTrue();
120+
121+
check(message)
122+
..identicalTo(messageList.messages.single)
123+
..content.equals(newContent)
124+
..lastEditTimestamp.equals(newTimestamp)
125+
..flags.equals(newFlags)
126+
..isMeMessage.isTrue();
127+
});
128+
129+
test('update event is ignored when message is not in the message list', () async {
130+
final store = await setupStore(stream);
131+
132+
const oldContent = "<p>Hello, world</p>";
133+
const newContent = "<p>Hello, edited</p>";
134+
const newTimestamp = 99999;
135+
136+
final originalMessage = eg.streamMessage(id: 243, stream: stream, content: oldContent);
137+
final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
138+
139+
final updateEvent = UpdateMessageEvent(
140+
id: 1,
141+
messageId: originalMessage.id + 1,
142+
messageIds: [originalMessage.id + 1],
143+
flags: originalMessage.flags,
144+
renderedContent: newContent,
145+
editTimestamp: newTimestamp,
146+
userId: userId,
147+
renderingOnly: false,
148+
);
149+
150+
final message = messageList.messages.single;
151+
check(message).content.equals(oldContent);
152+
153+
var listenersNotified = false;
154+
155+
messageList.addListener(() { listenersNotified = true; });
156+
messageList.maybeUpdateMessage(updateEvent);
157+
158+
check(listenersNotified).isFalse();
159+
check(message).content.equals(oldContent);
160+
});
161+
162+
test('rendering-only update does not change timestamp', () async {
163+
final store = await setupStore(stream);
164+
165+
const oldContent = "<p>Hello, world</p>";
166+
const oldTimestamp = 78492;
167+
const newContent = "<p>Hello, world</p> <div>Some link preview</div>";
168+
const newTimestamp = 99999;
169+
170+
final originalMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent);
171+
originalMessage.lastEditTimestamp = oldTimestamp;
172+
173+
final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
174+
175+
final updateEvent = UpdateMessageEvent(
176+
id: 1,
177+
messageId: originalMessage.id,
178+
messageIds: [originalMessage.id],
179+
flags: originalMessage.flags,
180+
renderedContent: newContent,
181+
editTimestamp: newTimestamp,
182+
renderingOnly: true,
183+
userId: null,
184+
);
185+
186+
final message = messageList.messages[0];
187+
messageList.maybeUpdateMessage(updateEvent);
188+
check(message)
189+
..content.equals(newContent)
190+
..lastEditTimestamp.equals(oldTimestamp);
191+
});
192+
193+
// TODO(server-5): Cut this test; rely on renderingOnly from FL 114
194+
test('rendering-only update does not change timestamp (for old server versions)', () async {
195+
final store = await setupStore(stream);
196+
197+
const oldContent = "<p>Hello, world</p>";
198+
const oldTimestamp = 78492;
199+
const newContent = "<p>Hello, world</p> <div>Some link preview</div>";
200+
const newTimestamp = 99999;
201+
202+
final originalMessage = eg.streamMessage(id: 972, stream: stream, content: oldContent);
203+
originalMessage.lastEditTimestamp = oldTimestamp;
204+
205+
final messageList = await messageListViewWithMessages([originalMessage], store, narrow);
206+
207+
final updateEvent = UpdateMessageEvent(
208+
id: 1,
209+
messageId: originalMessage.id,
210+
messageIds: [originalMessage.id],
211+
flags: originalMessage.flags,
212+
renderedContent: newContent,
213+
editTimestamp: newTimestamp,
214+
renderingOnly: null,
215+
userId: null,
216+
);
217+
218+
final message = messageList.messages.single;
219+
messageList.maybeUpdateMessage(updateEvent);
220+
check(message)
221+
..content.equals(newContent)
222+
..lastEditTimestamp.equals(oldTimestamp);
223+
});
224+
});
225+
}

0 commit comments

Comments
 (0)