Fetch older messages on scrolling up in message list #264
Conversation
Also make the comment a bit more specific and more concise.
…ynamic This doesn't use any data beyond what it's passed, so marking as `static` makes that explicit for the reader. That will also be helpful when we potentially go to move this logic elsewhere in the future.
This change is NFC if you include a well-behaved server in the system. That is, this request should produce the exact same response as before.
These tests were relying, for the consistency of their data, on the test messages lying within the test stream they created. That's currently true but might not remain so. Instead, use the all-messages narrow, so that there's no such constraint. As a bonus the test code gets slightly simpler.
…ests In particular this will allow individual test cases to call `connection.prepare` for setting up further API responses.
We'll soon want to add some tests where this detail varies.
| int _findItemWithMessageId(int messageId) { | ||
| return binarySearchByKey(items, messageId, _compareItemToMessageId); | ||
| } |
There was a problem hiding this comment.
Just to check my understanding: when items starts having multiple items with the same message ID (a date separator and recipient header, in addition to MessageListMessageItem), this method will need to be replaced, right, because it assumes there is just one item per message ID.
There was a problem hiding this comment.
Good question. It'll need some adjustments and I suppose some renamings for clarification, but I think the core logic will actually continue to work fine: the key is that
- it's really looking for the message item for the message ID;
- there'll still be exactly one of those per message;
- and the other items with a given message ID will all be in a predictable direction relative to the message item (specifically, date separator and recipient header will be before the message, and if we later dream up some item that goes after its associated message then that'll fit into this scheme too).
So when the comparison function sees some item that has the right message ID but isn't a message item, it can just say those compare before the key.
lib/model/message_list.dart
Outdated
| switch ((items.firstOrNull, haveOldest)) { | ||
| case (MessageListHistoryStartItem(), true): break; | ||
| case (MessageListHistoryStartItem(), _ ): items.removeFirst(); | ||
|
|
||
| case (_, true): items.addFirst(const MessageListHistoryStartItem()); | ||
| case (_, _): break; | ||
| } |
There was a problem hiding this comment.
Cool! This is nice and compact!
| TestZulipBinding.ensureInitialized(); | ||
| await TestZulipBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot()); | ||
| store = await TestZulipBinding.instance.globalStore.perAccount(eg.selfAccount.id); | ||
| connection = store.connection as FakeApiConnection; |
There was a problem hiding this comment.
msglist test [nfc]: Use shared store/connection variables in widget tests
In particular this will allow individual test cases to call
`connection.prepare` for setting up further API responses.
Hmm, couldn't this make it easier for problematic state leakage between tests to happen?
There was a problem hiding this comment.
In principle this sort of thing can. The key is that all of the shared variables get reset at the start of each test, by doing so in this one central helper function that each test calls at the start.
This is similar to what was done in #260. Like there, the alternative for getting similar functionality but with full encapsulation between tests would be to put things on a class and have each test make its own instance of the class.
| // TODO parse/process messages in smaller batches, to not drop frames. | ||
| // On a Pixel 5, a batch of 100 messages takes ~15-20ms in _insertAllMessages. | ||
| // (Before that, ~2-5ms in jsonDecode and 0ms in fromJson, | ||
| // so skip worrying about those steps.) |
There was a problem hiding this comment.
Could this strategy become counterproductive with extremely long messages/contents? Those are just Lists, so I expect inserting at index 0 (as in fetchOlder) to be linear time in the length of those lists.
Ah, though I guess this doesn't say inserting in smaller batches, just parsing/processing. So maybe this means still doing one large insert for each list, but of unparsed/unprocessed placeholders, and then processing those placeholders in batches.
There was a problem hiding this comment.
Yeah, or perhaps better yet: don't insert until we have fully-processed items to insert, but do the parsing/processing in smaller batches and then insert them all when we're done.
If processing the whole overall batch takes a frame or two, the extra wait of a frame or two isn't too bad — the network request will often/typically have more latency than that, and we compensate for the latency by just starting the request early before the user scrolls all the way to the edge. So probably best not to add the complexity of having a concept of placeholder items.
Or really probably the right answer at that point is to just turn messages and contents into QueueLists too :-)
|
Thanks for the replies above! Please merge when ready. |
|
Thanks for the review! Merged. |
Fixes: #78
Most of the changes here are in MessageListView, making use of the tests added in #260. We also make some tweaks to the message-list widget tests, before the main commit.