diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index d792b2f105..dabea853f3 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -1027,6 +1027,15 @@ class _MessageListState extends State with PerAccountStoreAwareStat ?? GlobalStoreWidget.settingsOf(context).markReadOnScrollForNarrow(widget.narrow); } + void _fetchMoreIfNeeded(ScrollMetrics scrollMetrics) { + if (scrollMetrics.extentBefore < kFetchMessagesBufferPixels) { + model.fetchOlder(); + } + if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) { + model.fetchNewer(); + } + } + void _handleScrollMetrics(ScrollMetrics scrollMetrics) { if (_effectiveMarkReadOnScroll()) { _markReadFromScroll(); @@ -1038,17 +1047,17 @@ class _MessageListState extends State with PerAccountStoreAwareStat _scrollToBottomVisible.value = true; } - if (scrollMetrics.extentBefore < kFetchMessagesBufferPixels) { - // TODO: This ends up firing a second time shortly after we fetch a batch. - // The result is that each time we decide to fetch a batch, we end up - // fetching two batches in quick succession. This is basically harmless - // but makes things a bit more complicated to reason about. - // The cause seems to be that this gets called again with maxScrollExtent - // still not yet updated to account for the newly-added messages. - model.fetchOlder(); - } - if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) { - model.fetchNewer(); + if (SchedulerBinding.instance.schedulerPhase == .transientCallbacks) { + SchedulerBinding.instance.addPostFrameCallback((_) { + if (scrollController.hasClients) { + // From the `transientCallbacks` phase to `postFrameCallbacks` phase, + // `scrollMetrics` can become stale; so we use the fresh value + // from `scrollController`. + _fetchMoreIfNeeded(scrollController.position); + } + }); + } else { + _fetchMoreIfNeeded(scrollMetrics); } } diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 92c3d2c3e7..38b969471c 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -841,35 +841,34 @@ void main() { check(itemCount(tester)).equals(401); }); - testWidgets('observe double-fetch glitch', (tester) async { + testWidgets('no double-fetch glitch', (tester) async { + // Regression test for: https://github.com/zulip/zulip-flutter/issues/2104 await setupMessageListPage(tester, foundOldest: false, - messages: List.generate(100, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser))); - check(itemCount(tester)).equals(101); + messages: List.generate(300, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser))); + connection.takeRequests(); + check(itemCount(tester)).equals(301); // Fling-scroll upward... await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000); await tester.pump(); // ... and we fetch more messages as we go. - connection.prepare(json: eg.olderGetMessagesResult(anchor: 950, foundOldest: false, - messages: List.generate(100, (i) => eg.streamMessage(id: 850 + i, sender: eg.selfUser))).toJson()); + connection.prepare(delay: Duration(milliseconds: 1), + json: eg.olderGetMessagesResult(anchor: 950, foundOldest: false, + messages: List.generate(100, (i) => eg.streamMessage(id: 850 + i, sender: eg.selfUser))).toJson()); for (int i = 0; i < 30; i++) { // Find the point in the fling where the fetch starts. await tester.pump(const Duration(milliseconds: 100)); - if (itemCount(tester)! > 101) break; // The loading indicator appeared. + if (connection.takeRequests().isNotEmpty) break; } - await tester.pump(Duration.zero); // Allow a frame for the response to arrive. - check(itemCount(tester)).equals(201); - // On the next frame, we promptly fetch *another* batch. - // This is a glitch and it'd be nicer if we didn't. - connection.prepare(json: eg.olderGetMessagesResult(anchor: 850, foundOldest: false, - messages: List.generate(100, (i) => eg.streamMessage(id: 750 + i, sender: eg.selfUser))).toJson()); - await tester.pump(const Duration(milliseconds: 1)); - await tester.pump(Duration.zero); - check(itemCount(tester)).equals(301); - }, skip: true); // TODO this still reproduces manually, still needs debugging, - // but has become harder to reproduce in a test. + // Allow a delayed frame for the response to arrive. + await tester.pump(Duration(milliseconds: 1)); + check(itemCount(tester)).equals(401); + await tester.pumpAndSettle(); + // Check there is no additional request made to cause double-fetch glitch. + check(connection.takeRequests()).isEmpty(); + }); testWidgets("avoid getting distracted by nested viewports' metrics", (tester) async { // Regression test for: https://github.com/zulip/zulip-flutter/issues/507