Skip to content

Commit e21f601

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 667f20e commit e21f601

File tree

3 files changed

+306
-23
lines changed

3 files changed

+306
-23
lines changed

‎lib/model/message_list.dart

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

108108
/// Whether [messages] and [items] represent the results of a fetch.
109109
///
@@ -252,6 +252,7 @@ mixin _MessageSequence {
252252
candidate++;
253253
assert(contents.length == messages.length);
254254
while (candidate < messages.length) {
255+
if (candidate == middleMessage) middleMessage = target;
255256
if (test(messages[candidate])) {
256257
candidate++;
257258
continue;
@@ -260,6 +261,7 @@ mixin _MessageSequence {
260261
contents[target] = contents[candidate];
261262
target++; candidate++;
262263
}
264+
if (candidate == middleMessage) middleMessage = target;
263265
messages.length = target;
264266
contents.length = target;
265267
assert(contents.length == messages.length);
@@ -282,6 +284,13 @@ mixin _MessageSequence {
282284
}
283285
if (messagesToRemoveById.isEmpty) return false;
284286

287+
if (middleMessage == messages.length) {
288+
middleMessage -= messagesToRemoveById.length;
289+
} else {
290+
final middleMessageId = messages[middleMessage].id;
291+
middleMessage -= messagesToRemoveById
292+
.where((id) => id < middleMessageId).length;
293+
}
285294
assert(contents.length == messages.length);
286295
messages.removeWhere((message) => messagesToRemoveById.contains(message.id));
287296
contents.removeWhere((content) => contentToRemove.contains(content));
@@ -296,18 +305,23 @@ mixin _MessageSequence {
296305
// On a Pixel 5, a batch of 100 messages takes ~15-20ms in _insertAllMessages.
297306
// (Before that, ~2-5ms in jsonDecode and 0ms in fromJson,
298307
// so skip worrying about those steps.)
308+
final oldLength = messages.length;
299309
assert(contents.length == messages.length);
300310
messages.insertAll(index, toInsert);
301311
contents.insertAll(index, toInsert.map(
302312
(message) => _parseMessageContent(message)));
303313
assert(contents.length == messages.length);
314+
if (index <= middleMessage) {
315+
middleMessage += messages.length - oldLength;
316+
}
304317
_reprocessAll();
305318
}
306319

307320
/// Reset all [_MessageSequence] data, and cancel any active fetches.
308321
void _reset() {
309322
generation += 1;
310323
messages.clear();
324+
middleMessage = 0;
311325
_fetched = false;
312326
_haveOldest = false;
313327
_fetchingOlder = false;
@@ -530,13 +544,18 @@ class MessageListView with ChangeNotifier, _MessageSequence {
530544
numAfter: 0,
531545
);
532546
if (this.generation > generation) return;
547+
533548
_adjustNarrowForTopicPermalink(result.messages.firstOrNull);
549+
534550
store.reconcileMessages(result.messages);
535551
store.recentSenders.handleMessages(result.messages); // TODO(#824)
552+
553+
// We'll make the bottom slice start at the last visible message, if any.
536554
for (final message in result.messages) {
537-
if (_messageVisible(message)) {
538-
_addMessage(message);
539-
}
555+
if (!_messageVisible(message)) continue;
556+
middleMessage = messages.length;
557+
_addMessage(message);
558+
// Now [middleMessage] is the last message (the one just added).
540559
}
541560
_fetched = true;
542561
_haveOldest = result.foundOldest;

‎test/model/message_list_test.dart

Lines changed: 234 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';
@@ -1645,6 +1646,209 @@ void main() {
16451646
});
16461647
});
16471648

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

2126+
MessageListView? _lastModel;
2127+
List<Message>? _lastMessages;
2128+
int? _lastMiddleMessage;
2129+
19222130
void checkInvariants(MessageListView model) {
19232131
if (!model.fetched) {
19242132
check(model)
@@ -1961,6 +2169,21 @@ void checkInvariants(MessageListView model) {
19612169
..isGreaterOrEqual(0)
19622170
..isLessOrEqual(model.messages.length);
19632171

2172+
if (identical(model, _lastModel)
2173+
&& model.generation == _lastModel!.generation) {
2174+
// All messages that were present, and still are, should be on the same side
2175+
// of `middleMessage` (still top or bottom slice respectively) as they were.
2176+
_checkNoIntersection(ListSlice(model.messages, 0, model.middleMessage),
2177+
ListSlice(_lastMessages!, _lastMiddleMessage!, _lastMessages!.length),
2178+
because: 'messages moved from bottom slice to top slice');
2179+
_checkNoIntersection(ListSlice(_lastMessages!, 0, _lastMiddleMessage!),
2180+
ListSlice(model.messages, model.middleMessage, model.messages.length),
2181+
because: 'messages moved from top slice to bottom slice');
2182+
}
2183+
_lastModel = model;
2184+
_lastMessages = model.messages.toList();
2185+
_lastMiddleMessage = model.middleMessage;
2186+
19642187
check(model).contents.length.equals(model.messages.length);
19652188
for (int i = 0; i < model.contents.length; i++) {
19662189
final poll = model.messages[i].poll;
@@ -2018,6 +2241,17 @@ void checkInvariants(MessageListView model) {
20182241
}
20192242
}
20202243

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+
20212255
extension MessageListRecipientHeaderItemChecks on Subject<MessageListRecipientHeaderItem> {
20222256
Subject<MessageBase> get message => has((x) => x.message, 'message');
20232257
}

0 commit comments

Comments
 (0)