Skip to content

Commit 35da642

Browse files
gnpricegnprice
committed
msglist: Let new messages accumulate in bottom sliver
This is NFC for the behavior at initial fetch. But thereafter, with this change, messages never move between slivers, and new messages go into the bottom sliver. I believe the main user-visible consequence of this change is that if the user is scrolled up in history and then a new message comes in, the new message will no longer cause all the messages to shift upward. This is the "90% solution" to #83. On the other hand, if the user is scrolled all the way to the end, then they still remain that way when a new message comes in -- there's specific logic to ensure that in MessageListScrollPosition, and an existing test in test/widgets/message_list_test.dart verifies it end to end. The main motivation for this change is that it brings us closer to having a `fetchNewer` method, and therefore to being able to have the message list start out in the middle of history. This change also allows us to revert a portion of fca651b, where a test had had to be weakened slightly because messages began to get moved between slivers.
1 parent dcaa3e5 commit 35da642

File tree

3 files changed

+311
-23
lines changed

3 files changed

+311
-23
lines changed

‎lib/model/message_list.dart

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ mixin _MessageSequence {
8888
/// and the indices from [middleMessage] to the end are the bottom slice.
8989
///
9090
/// The corresponding item index is [middleItem].
91-
int get middleMessage => messages.isEmpty ? 0 : messages.length - 1;
91+
int middleMessage = 0;
9292

9393
/// Whether [messages] and [items] represent the results of a fetch.
9494
///
@@ -232,6 +232,7 @@ mixin _MessageSequence {
232232
candidate++;
233233
assert(contents.length == messages.length);
234234
while (candidate < messages.length) {
235+
if (candidate == middleMessage) middleMessage = target;
235236
if (test(messages[candidate])) {
236237
candidate++;
237238
continue;
@@ -240,6 +241,7 @@ mixin _MessageSequence {
240241
contents[target] = contents[candidate];
241242
target++; candidate++;
242243
}
244+
if (candidate == middleMessage) middleMessage = target;
243245
messages.length = target;
244246
contents.length = target;
245247
assert(contents.length == messages.length);
@@ -262,6 +264,13 @@ mixin _MessageSequence {
262264
}
263265
if (messagesToRemoveById.isEmpty) return false;
264266

267+
if (middleMessage == messages.length) {
268+
middleMessage -= messagesToRemoveById.length;
269+
} else {
270+
final middleMessageId = messages[middleMessage].id;
271+
middleMessage -= messagesToRemoveById
272+
.where((id) => id < middleMessageId).length;
273+
}
265274
assert(contents.length == messages.length);
266275
messages.removeWhere((message) => messagesToRemoveById.contains(message.id));
267276
contents.removeWhere((content) => contentToRemove.contains(content));
@@ -276,18 +285,23 @@ mixin _MessageSequence {
276285
// On a Pixel 5, a batch of 100 messages takes ~15-20ms in _insertAllMessages.
277286
// (Before that, ~2-5ms in jsonDecode and 0ms in fromJson,
278287
// so skip worrying about those steps.)
288+
final oldLength = messages.length;
279289
assert(contents.length == messages.length);
280290
messages.insertAll(index, toInsert);
281291
contents.insertAll(index, toInsert.map(
282292
(message) => _parseMessageContent(message)));
283293
assert(contents.length == messages.length);
294+
if (index <= middleMessage) {
295+
middleMessage += messages.length - oldLength;
296+
}
284297
_reprocessAll();
285298
}
286299

287300
/// Reset all [_MessageSequence] data, and cancel any active fetches.
288301
void _reset() {
289302
generation += 1;
290303
messages.clear();
304+
middleMessage = 0;
291305
_fetched = false;
292306
_haveOldest = false;
293307
_fetchingOlder = false;
@@ -486,13 +500,18 @@ class MessageListView with ChangeNotifier, _MessageSequence {
486500
allowEmptyTopicName: true,
487501
);
488502
if (this.generation > generation) return;
503+
489504
_adjustNarrowForTopicPermalink(result.messages.firstOrNull);
505+
490506
store.reconcileMessages(result.messages);
491507
store.recentSenders.handleMessages(result.messages); // TODO(#824)
508+
509+
// We'll make the bottom slice start at the last visible message, if any.
492510
for (final message in result.messages) {
493-
if (_messageVisible(message)) {
494-
_addMessage(message);
495-
}
511+
if (!_messageVisible(message)) continue;
512+
middleMessage = messages.length;
513+
_addMessage(message);
514+
// Now [middleMessage] is the last message (the one just added).
496515
}
497516
_fetched = true;
498517
_haveOldest = result.foundOldest;

‎test/model/message_list_test.dart

Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import 'dart:convert';
22

33
import 'package:checks/checks.dart';
4+
import 'package:collection/collection.dart';
45
import 'package:flutter/foundation.dart';
56
import 'package:http/http.dart' as http;
67
import 'package:test/scaffolding.dart';
@@ -1649,6 +1650,214 @@ void main() {
16491650
});
16501651
});
16511652

1653+
group('middleMessage maintained', () {
1654+
// In [checkInvariants] we verify that messages don't move from the
1655+
// top to the bottom slice or vice versa.
1656+
// Most of these test cases rely on that for all the checks they need.
1657+
1658+
test('on fetchInitial empty', () async {
1659+
await prepare(narrow: const CombinedFeedNarrow());
1660+
await prepareMessages(foundOldest: true, messages: []);
1661+
check(model)..messages.isEmpty()
1662+
..middleMessage.equals(0);
1663+
});
1664+
1665+
test('on fetchInitial empty due to muting', () async {
1666+
await prepare(narrow: const CombinedFeedNarrow());
1667+
final stream = eg.stream();
1668+
await store.addStream(stream);
1669+
await store.addSubscription(eg.subscription(stream, isMuted: true));
1670+
await prepareMessages(foundOldest: true, messages: [
1671+
eg.streamMessage(stream: stream),
1672+
]);
1673+
check(model)..messages.isEmpty()
1674+
..middleMessage.equals(0);
1675+
});
1676+
1677+
test('on fetchInitial not empty', () async {
1678+
await prepare(narrow: const CombinedFeedNarrow());
1679+
final stream1 = eg.stream();
1680+
final stream2 = eg.stream();
1681+
await store.addStreams([stream1, stream2]);
1682+
await store.addSubscription(eg.subscription(stream1));
1683+
await store.addSubscription(eg.subscription(stream2, isMuted: true));
1684+
final messages = [
1685+
eg.streamMessage(stream: stream1), eg.streamMessage(stream: stream2),
1686+
eg.streamMessage(stream: stream1), eg.streamMessage(stream: stream2),
1687+
eg.streamMessage(stream: stream1), eg.streamMessage(stream: stream2),
1688+
eg.streamMessage(stream: stream1), eg.streamMessage(stream: stream2),
1689+
eg.streamMessage(stream: stream1), eg.streamMessage(stream: stream2),
1690+
];
1691+
await prepareMessages(foundOldest: true, messages: messages);
1692+
// The anchor message is the last visible message…
1693+
check(model)
1694+
..messages.length.equals(5)
1695+
..middleMessage.equals(model.messages.length - 1)
1696+
// … even though that's not the last message that was in the response.
1697+
..messages[model.middleMessage].id
1698+
.equals(messages[messages.length - 2].id);
1699+
});
1700+
1701+
/// Like [prepareMessages], but arrange for the given top and bottom slices.
1702+
Future<void> prepareMessageSplit(List<Message> top, List<Message> bottom, {
1703+
bool foundOldest = true,
1704+
}) async {
1705+
assert(bottom.isNotEmpty); // could handle this too if necessary
1706+
await prepareMessages(foundOldest: foundOldest, messages: [
1707+
...top,
1708+
bottom.first,
1709+
]);
1710+
if (bottom.length > 1) {
1711+
await store.addMessages(bottom.skip(1));
1712+
checkNotifiedOnce();
1713+
}
1714+
check(model)
1715+
..messages.length.equals(top.length + bottom.length)
1716+
..middleMessage.equals(top.length);
1717+
}
1718+
1719+
test('on fetchOlder', () async {
1720+
await prepare(narrow: const CombinedFeedNarrow());
1721+
final stream = eg.stream();
1722+
await store.addStream(stream);
1723+
await store.addSubscription(eg.subscription(stream));
1724+
await prepareMessageSplit(foundOldest: false,
1725+
[eg.streamMessage(id: 100, stream: stream)],
1726+
[eg.streamMessage(id: 101, stream: stream)]);
1727+
1728+
connection.prepare(json: olderResult(anchor: 100, foundOldest: true,
1729+
messages: List.generate(5, (i) =>
1730+
eg.streamMessage(id: 95 + i, stream: stream))).toJson());
1731+
await model.fetchOlder();
1732+
checkNotified(count: 2);
1733+
});
1734+
1735+
test('on fetchOlder, from top empty', () async {
1736+
await prepare(narrow: const CombinedFeedNarrow());
1737+
final stream = eg.stream();
1738+
await store.addStream(stream);
1739+
await store.addSubscription(eg.subscription(stream));
1740+
await prepareMessageSplit(foundOldest: false,
1741+
[], [eg.streamMessage(id: 100, stream: stream)]);
1742+
1743+
connection.prepare(json: olderResult(anchor: 100, foundOldest: true,
1744+
messages: List.generate(5, (i) =>
1745+
eg.streamMessage(id: 95 + i, stream: stream))).toJson());
1746+
await model.fetchOlder();
1747+
checkNotified(count: 2);
1748+
// The messages from fetchOlder should go in the top sliver, always.
1749+
check(model).middleMessage.equals(5);
1750+
});
1751+
1752+
test('on MessageEvent', () async {
1753+
await prepare(narrow: const CombinedFeedNarrow());
1754+
final stream = eg.stream();
1755+
await store.addStream(stream);
1756+
await store.addSubscription(eg.subscription(stream));
1757+
await prepareMessageSplit(foundOldest: false,
1758+
[eg.streamMessage(stream: stream)],
1759+
[eg.streamMessage(stream: stream)]);
1760+
1761+
await store.addMessage(eg.streamMessage(stream: stream));
1762+
checkNotifiedOnce();
1763+
});
1764+
1765+
test('on messages muted, including anchor', () async {
1766+
await prepare(narrow: const CombinedFeedNarrow());
1767+
final stream = eg.stream();
1768+
await store.addStream(stream);
1769+
await store.addSubscription(eg.subscription(stream));
1770+
await prepareMessageSplit([
1771+
eg.streamMessage(stream: stream, topic: 'foo'),
1772+
eg.streamMessage(stream: stream, topic: 'bar'),
1773+
], [
1774+
eg.streamMessage(stream: stream, topic: 'bar'),
1775+
eg.streamMessage(stream: stream, topic: 'foo'),
1776+
]);
1777+
1778+
await store.handleEvent(eg.userTopicEvent(
1779+
stream.streamId, 'bar', UserTopicVisibilityPolicy.muted));
1780+
checkNotifiedOnce();
1781+
});
1782+
1783+
test('on messages muted, not including anchor', () async {
1784+
await prepare(narrow: const CombinedFeedNarrow());
1785+
final stream = eg.stream();
1786+
await store.addStream(stream);
1787+
await store.addSubscription(eg.subscription(stream));
1788+
await prepareMessageSplit([
1789+
eg.streamMessage(stream: stream, topic: 'foo'),
1790+
eg.streamMessage(stream: stream, topic: 'bar'),
1791+
], [
1792+
eg.streamMessage(stream: stream, topic: 'foo'),
1793+
]);
1794+
1795+
await store.handleEvent(eg.userTopicEvent(
1796+
stream.streamId, 'bar', UserTopicVisibilityPolicy.muted));
1797+
checkNotifiedOnce();
1798+
});
1799+
1800+
test('on messages muted, bottom empty', () async {
1801+
await prepare(narrow: const CombinedFeedNarrow());
1802+
final stream = eg.stream();
1803+
await store.addStream(stream);
1804+
await store.addSubscription(eg.subscription(stream));
1805+
await prepareMessageSplit([
1806+
eg.streamMessage(stream: stream, topic: 'foo'),
1807+
eg.streamMessage(stream: stream, topic: 'bar'),
1808+
], [
1809+
eg.streamMessage(stream: stream, topic: 'third'),
1810+
]);
1811+
1812+
await store.handleEvent(eg.deleteMessageEvent([
1813+
model.messages.last as StreamMessage]));
1814+
checkNotifiedOnce();
1815+
check(model).middleMessage.equals(model.messages.length);
1816+
1817+
await store.handleEvent(eg.userTopicEvent(
1818+
stream.streamId, 'bar', UserTopicVisibilityPolicy.muted));
1819+
checkNotifiedOnce();
1820+
});
1821+
1822+
test('on messages deleted', () async {
1823+
await prepare(narrow: const CombinedFeedNarrow());
1824+
final stream = eg.stream();
1825+
await store.addStream(stream);
1826+
await store.addSubscription(eg.subscription(stream));
1827+
final messages = [
1828+
eg.streamMessage(id: 1, stream: stream),
1829+
eg.streamMessage(id: 2, stream: stream),
1830+
eg.streamMessage(id: 3, stream: stream),
1831+
eg.streamMessage(id: 4, stream: stream),
1832+
];
1833+
await prepareMessageSplit(messages.sublist(0, 2), messages.sublist(2));
1834+
1835+
await store.handleEvent(eg.deleteMessageEvent(messages.sublist(1, 3)));
1836+
checkNotifiedOnce();
1837+
});
1838+
1839+
test('on messages deleted, bottom empty', () async {
1840+
await prepare(narrow: const CombinedFeedNarrow());
1841+
final stream = eg.stream();
1842+
await store.addStream(stream);
1843+
await store.addSubscription(eg.subscription(stream));
1844+
final messages = [
1845+
eg.streamMessage(id: 1, stream: stream),
1846+
eg.streamMessage(id: 2, stream: stream),
1847+
eg.streamMessage(id: 3, stream: stream),
1848+
eg.streamMessage(id: 4, stream: stream),
1849+
];
1850+
await prepareMessageSplit(messages.sublist(0, 3), messages.sublist(3));
1851+
1852+
await store.handleEvent(eg.deleteMessageEvent(messages.sublist(3)));
1853+
checkNotifiedOnce();
1854+
check(model).middleMessage.equals(model.messages.length);
1855+
1856+
await store.handleEvent(eg.deleteMessageEvent(messages.sublist(1, 2)));
1857+
checkNotifiedOnce();
1858+
});
1859+
});
1860+
16521861
group('handle content parsing into subclasses of ZulipMessageContent', () {
16531862
test('ZulipContent', () async {
16541863
final stream = eg.stream();
@@ -1922,6 +2131,10 @@ void main() {
19222131
});
19232132
}
19242133

2134+
MessageListView? _lastModel;
2135+
List<Message>? _lastMessages;
2136+
int? _lastMiddleMessage;
2137+
19252138
void checkInvariants(MessageListView model) {
19262139
if (!model.fetched) {
19272140
check(model)
@@ -1964,6 +2177,21 @@ void checkInvariants(MessageListView model) {
19642177
..isGreaterOrEqual(0)
19652178
..isLessOrEqual(model.messages.length);
19662179

2180+
if (identical(model, _lastModel)
2181+
&& model.generation == _lastModel!.generation) {
2182+
// All messages that were present, and still are, should be on the same side
2183+
// of `middleMessage` (still top or bottom slice respectively) as they were.
2184+
_checkNoIntersection(ListSlice(model.messages, 0, model.middleMessage),
2185+
ListSlice(_lastMessages!, _lastMiddleMessage!, _lastMessages!.length),
2186+
because: 'messages moved from bottom slice to top slice');
2187+
_checkNoIntersection(ListSlice(_lastMessages!, 0, _lastMiddleMessage!),
2188+
ListSlice(model.messages, model.middleMessage, model.messages.length),
2189+
because: 'messages moved from top slice to bottom slice');
2190+
}
2191+
_lastModel = model;
2192+
_lastMessages = model.messages.toList();
2193+
_lastMiddleMessage = model.middleMessage;
2194+
19672195
check(model).contents.length.equals(model.messages.length);
19682196
for (int i = 0; i < model.contents.length; i++) {
19692197
final poll = model.messages[i].poll;
@@ -2013,6 +2241,17 @@ void checkInvariants(MessageListView model) {
20132241
}
20142242
}
20152243

2244+
void _checkNoIntersection(List<Message> xs, List<Message> ys, {String? because}) {
2245+
// Both lists are sorted by ID. As an optimization, bet on all or nearly all
2246+
// of the first list having smaller IDs than all or nearly all of the other.
2247+
if (xs.isEmpty || ys.isEmpty) return;
2248+
if (xs.last.id < ys.first.id) return;
2249+
final yCandidates = Set.of(ys.takeWhile((m) => m.id <= xs.last.id));
2250+
final intersection = xs.reversed.takeWhile((m) => ys.first.id <= m.id)
2251+
.where(yCandidates.contains);
2252+
check(intersection, because: because).isEmpty();
2253+
}
2254+
20162255
extension MessageListRecipientHeaderItemChecks on Subject<MessageListRecipientHeaderItem> {
20172256
Subject<MessageBase> get message => has((x) => x.message, 'message');
20182257
}

0 commit comments

Comments
 (0)