Skip to content

Commit 0811136

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 09758b2 commit 0811136

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;
@@ -531,13 +545,18 @@ class MessageListView with ChangeNotifier, _MessageSequence {
531545
allowEmptyTopicName: true,
532546
);
533547
if (this.generation > generation) return;
548+
534549
_adjustNarrowForTopicPermalink(result.messages.firstOrNull);
550+
535551
store.reconcileMessages(result.messages);
536552
store.recentSenders.handleMessages(result.messages); // TODO(#824)
553+
554+
// We'll make the bottom slice start at the last visible message, if any.
537555
for (final message in result.messages) {
538-
if (_messageVisible(message)) {
539-
_addMessage(message);
540-
}
556+
if (!_messageVisible(message)) continue;
557+
middleMessage = messages.length;
558+
_addMessage(message);
559+
// Now [middleMessage] is the last message (the one just added).
541560
}
542561
_fetched = true;
543562
_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';
@@ -1649,6 +1650,209 @@ 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, isMuted: true));
1683+
await store.addSubscription(eg.subscription(stream2));
1684+
await prepareMessages(foundOldest: true, 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+
check(model)
1692+
..messages.length.equals(5)
1693+
..middleMessage.equals(4);
1694+
});
1695+
1696+
/// Like [prepareMessages], but arrange for the given top and bottom slices.
1697+
Future<void> prepareMessageSplit(List<Message> top, List<Message> bottom, {
1698+
bool foundOldest = true,
1699+
}) async {
1700+
assert(bottom.isNotEmpty); // could handle this too if necessary
1701+
await prepareMessages(foundOldest: foundOldest, messages: [
1702+
...top,
1703+
bottom.first,
1704+
]);
1705+
if (bottom.length > 1) {
1706+
await store.addMessages(bottom.skip(1));
1707+
checkNotifiedOnce();
1708+
}
1709+
check(model)
1710+
..messages.length.equals(top.length + bottom.length)
1711+
..middleMessage.equals(top.length);
1712+
}
1713+
1714+
test('on fetchOlder', () async {
1715+
await prepare(narrow: const CombinedFeedNarrow());
1716+
final stream = eg.stream();
1717+
await store.addStream(stream);
1718+
await store.addSubscription(eg.subscription(stream));
1719+
await prepareMessageSplit(foundOldest: false,
1720+
[eg.streamMessage(id: 100, stream: stream)],
1721+
[eg.streamMessage(id: 101, stream: stream)]);
1722+
1723+
connection.prepare(json: olderResult(anchor: 100, foundOldest: true,
1724+
messages: List.generate(5, (i) =>
1725+
eg.streamMessage(id: 95 + i, stream: stream))).toJson());
1726+
await model.fetchOlder();
1727+
checkNotified(count: 2);
1728+
});
1729+
1730+
test('on fetchOlder, from top empty', () async {
1731+
await prepare(narrow: const CombinedFeedNarrow());
1732+
final stream = eg.stream();
1733+
await store.addStream(stream);
1734+
await store.addSubscription(eg.subscription(stream));
1735+
await prepareMessageSplit(foundOldest: false,
1736+
[], [eg.streamMessage(id: 100, stream: stream)]);
1737+
1738+
connection.prepare(json: olderResult(anchor: 100, foundOldest: true,
1739+
messages: List.generate(5, (i) =>
1740+
eg.streamMessage(id: 95 + i, stream: stream))).toJson());
1741+
await model.fetchOlder();
1742+
checkNotified(count: 2);
1743+
// The messages from fetchOlder should go in the top sliver, always.
1744+
check(model).middleMessage.equals(5);
1745+
});
1746+
1747+
test('on MessageEvent', () async {
1748+
await prepare(narrow: const CombinedFeedNarrow());
1749+
final stream = eg.stream();
1750+
await store.addStream(stream);
1751+
await store.addSubscription(eg.subscription(stream));
1752+
await prepareMessageSplit(foundOldest: false,
1753+
[eg.streamMessage(stream: stream)],
1754+
[eg.streamMessage(stream: stream)]);
1755+
1756+
await store.addMessage(eg.streamMessage(stream: stream));
1757+
checkNotifiedOnce();
1758+
});
1759+
1760+
test('on messages muted, including anchor', () async {
1761+
await prepare(narrow: const CombinedFeedNarrow());
1762+
final stream = eg.stream();
1763+
await store.addStream(stream);
1764+
await store.addSubscription(eg.subscription(stream));
1765+
await prepareMessageSplit([
1766+
eg.streamMessage(stream: stream, topic: 'foo'),
1767+
eg.streamMessage(stream: stream, topic: 'bar'),
1768+
], [
1769+
eg.streamMessage(stream: stream, topic: 'bar'),
1770+
eg.streamMessage(stream: stream, topic: 'foo'),
1771+
]);
1772+
1773+
await store.handleEvent(eg.userTopicEvent(
1774+
stream.streamId, 'bar', UserTopicVisibilityPolicy.muted));
1775+
checkNotifiedOnce();
1776+
});
1777+
1778+
test('on messages muted, not including anchor', () async {
1779+
await prepare(narrow: const CombinedFeedNarrow());
1780+
final stream = eg.stream();
1781+
await store.addStream(stream);
1782+
await store.addSubscription(eg.subscription(stream));
1783+
await prepareMessageSplit([
1784+
eg.streamMessage(stream: stream, topic: 'foo'),
1785+
eg.streamMessage(stream: stream, topic: 'bar'),
1786+
], [
1787+
eg.streamMessage(stream: stream, topic: 'foo'),
1788+
]);
1789+
1790+
await store.handleEvent(eg.userTopicEvent(
1791+
stream.streamId, 'bar', UserTopicVisibilityPolicy.muted));
1792+
checkNotifiedOnce();
1793+
});
1794+
1795+
test('on messages muted, bottom empty', () async {
1796+
await prepare(narrow: const CombinedFeedNarrow());
1797+
final stream = eg.stream();
1798+
await store.addStream(stream);
1799+
await store.addSubscription(eg.subscription(stream));
1800+
await prepareMessageSplit([
1801+
eg.streamMessage(stream: stream, topic: 'foo'),
1802+
eg.streamMessage(stream: stream, topic: 'bar'),
1803+
], [
1804+
eg.streamMessage(stream: stream, topic: 'third'),
1805+
]);
1806+
1807+
await store.handleEvent(eg.deleteMessageEvent([
1808+
model.messages.last as StreamMessage]));
1809+
checkNotifiedOnce();
1810+
check(model).middleMessage.equals(model.messages.length);
1811+
1812+
await store.handleEvent(eg.userTopicEvent(
1813+
stream.streamId, 'bar', UserTopicVisibilityPolicy.muted));
1814+
checkNotifiedOnce();
1815+
});
1816+
1817+
test('on messages deleted', () async {
1818+
await prepare(narrow: const CombinedFeedNarrow());
1819+
final stream = eg.stream();
1820+
await store.addStream(stream);
1821+
await store.addSubscription(eg.subscription(stream));
1822+
final messages = [
1823+
eg.streamMessage(id: 1, stream: stream),
1824+
eg.streamMessage(id: 2, stream: stream),
1825+
eg.streamMessage(id: 3, stream: stream),
1826+
eg.streamMessage(id: 4, stream: stream),
1827+
];
1828+
await prepareMessageSplit(messages.sublist(0, 2), messages.sublist(2));
1829+
1830+
await store.handleEvent(eg.deleteMessageEvent(messages.sublist(1, 3)));
1831+
checkNotifiedOnce();
1832+
});
1833+
1834+
test('on messages deleted, bottom empty', () async {
1835+
await prepare(narrow: const CombinedFeedNarrow());
1836+
final stream = eg.stream();
1837+
await store.addStream(stream);
1838+
await store.addSubscription(eg.subscription(stream));
1839+
final messages = [
1840+
eg.streamMessage(id: 1, stream: stream),
1841+
eg.streamMessage(id: 2, stream: stream),
1842+
eg.streamMessage(id: 3, stream: stream),
1843+
eg.streamMessage(id: 4, stream: stream),
1844+
];
1845+
await prepareMessageSplit(messages.sublist(0, 3), messages.sublist(3));
1846+
1847+
await store.handleEvent(eg.deleteMessageEvent(messages.sublist(3)));
1848+
checkNotifiedOnce();
1849+
check(model).middleMessage.equals(model.messages.length);
1850+
1851+
await store.handleEvent(eg.deleteMessageEvent(messages.sublist(1, 2)));
1852+
checkNotifiedOnce();
1853+
});
1854+
});
1855+
16521856
group('handle content parsing into subclasses of ZulipMessageContent', () {
16531857
test('ZulipContent', () async {
16541858
final stream = eg.stream();
@@ -1923,6 +2127,10 @@ void main() {
19232127
});
19242128
}
19252129

2130+
MessageListView? _lastModel;
2131+
List<Message>? _lastMessages;
2132+
int? _lastMiddleMessage;
2133+
19262134
void checkInvariants(MessageListView model) {
19272135
if (!model.fetched) {
19282136
check(model)
@@ -1965,6 +2173,21 @@ void checkInvariants(MessageListView model) {
19652173
..isGreaterOrEqual(0)
19662174
..isLessOrEqual(model.messages.length);
19672175

2176+
if (identical(model, _lastModel)
2177+
&& model.generation == _lastModel!.generation) {
2178+
// All messages that were present, and still are, should be on the same side
2179+
// of `middleMessage` (still top or bottom slice respectively) as they were.
2180+
_checkNoIntersection(ListSlice(model.messages, 0, model.middleMessage),
2181+
ListSlice(_lastMessages!, _lastMiddleMessage!, _lastMessages!.length),
2182+
because: 'messages moved from bottom slice to top slice');
2183+
_checkNoIntersection(ListSlice(_lastMessages!, 0, _lastMiddleMessage!),
2184+
ListSlice(model.messages, model.middleMessage, model.messages.length),
2185+
because: 'messages moved from top slice to bottom slice');
2186+
}
2187+
_lastModel = model;
2188+
_lastMessages = model.messages.toList();
2189+
_lastMiddleMessage = model.middleMessage;
2190+
19682191
check(model).contents.length.equals(model.messages.length);
19692192
for (int i = 0; i < model.contents.length; i++) {
19702193
final poll = model.messages[i].poll;
@@ -2022,6 +2245,17 @@ void checkInvariants(MessageListView model) {
20222245
}
20232246
}
20242247

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

0 commit comments

Comments
 (0)