Skip to content

Commit 13c5388

Browse files
committed
wip msglist: Fix speed calculations in scroll to bottom; TODO issue
This logic for deciding how long the scroll-to-bottom animation should take, introduced in 5abeb88 (zulip#223), didn't have any tests. As a result, when its unstated assumption about the message list -- that scrolling up from the end was represented by positive scroll positions -- was broken a few months later (in bdac26f), nothing alerted us to that. I did notice a few times over the past year or so that the effect of the scroll-to-bottom button seemed jerky, as if it were trying to move much farther in each frame than it should. Now I know why. (Discovered this in the course of revisiting this code in order to adapt it to the more radical change to the message list's scroll positions which is coming up: "zero" won't be the end, but somewhere in the middle.)
1 parent 37366ad commit 13c5388

File tree

2 files changed

+37
-2
lines changed

2 files changed

+37
-2
lines changed

lib/widgets/message_list.dart

+3-2
Original file line numberDiff line numberDiff line change
@@ -697,11 +697,12 @@ class ScrollToBottomButton extends StatelessWidget {
697697
final ScrollController scrollController;
698698

699699
Future<void> _scrollToBottom() {
700-
final distance = scrollController.position.pixels;
700+
final target = 0.0;
701+
final distance = target - scrollController.position.pixels;
701702
final durationMsAtSpeedLimit = (1000 * distance / 8000).ceil();
702703
final durationMs = max(300, durationMsAtSpeedLimit);
703704
return scrollController.animateTo(
704-
0,
705+
target,
705706
duration: Duration(milliseconds: durationMs),
706707
curve: Curves.ease);
707708
}

test/widgets/message_list_test.dart

+34
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,40 @@ void main() {
510510
// … and for good measure confirm the button disappeared.
511511
check(isButtonVisible(tester)).equals(false);
512512
});
513+
514+
testWidgets('scrolls at reasonable speed', (tester) async {
515+
const referenceSpeed = 8000.0;
516+
const distance = 40000.0;
517+
await setupMessageListPage(tester, messageCount: 1000);
518+
final controller = findMessageListScrollController(tester)!;
519+
520+
// Scroll a long distance up, many screenfuls.
521+
controller.jumpTo(-distance);
522+
await tester.pump();
523+
check(controller.position.pixels).equals(-distance);
524+
525+
// Tap button.
526+
await tester.tap(find.byType(ScrollToBottomButton));
527+
await tester.pump();
528+
529+
// Measure speed.
530+
final log = <double>[];
531+
double pos = controller.position.pixels;
532+
while (pos < 0) {
533+
check(log.length).isLessThan(30);
534+
await tester.pump(const Duration(seconds: 1));
535+
final lastPos = pos;
536+
pos = controller.position.pixels;
537+
log.add(pos - lastPos);
538+
}
539+
// Check the main question: the speed stayed in range throughout.
540+
const maxSpeed = 2 * referenceSpeed;
541+
check(log).every((it) => it..isGreaterThan(0)..isLessThan(maxSpeed));
542+
// Also check the test's assumptions: the scroll reached the end…
543+
check(pos).equals(0);
544+
// … and scrolled far enough to effectively test the max speed.
545+
check(log.sum).isGreaterThan(2 * maxSpeed);
546+
});
513547
});
514548

515549
group('TypingStatusWidget', () {

0 commit comments

Comments
 (0)