From c629006748f4f32e456fa013bfc379493277d877 Mon Sep 17 00:00:00 2001 From: Adam Barth Date: Fri, 26 Feb 2016 11:04:10 -0800 Subject: [PATCH] Scrollable's callbacks should follow a state machine Now onScroll callbacks are always bracketed by onScrollStart and onScrollEnd callbacks. Fixes #1822 --- .../flutter/lib/src/widgets/scrollable.dart | 45 ++++++--- .../test/widget/scroll_events_test.dart | 98 +++++++++++++++++++ .../newton/lib/src/scroll_simulation.dart | 4 + packages/newton/lib/src/tolerance.dart | 2 + 4 files changed, 138 insertions(+), 11 deletions(-) create mode 100644 packages/flutter/test/widget/scroll_events_test.dart diff --git a/packages/flutter/lib/src/widgets/scrollable.dart b/packages/flutter/lib/src/widgets/scrollable.dart index b81f4836637..5e6c30edc1e 100644 --- a/packages/flutter/lib/src/widgets/scrollable.dart +++ b/packages/flutter/lib/src/widgets/scrollable.dart @@ -290,7 +290,8 @@ abstract class ScrollableState extends State { Future _animateTo(double newScrollOffset, Duration duration, Curve curve) { _controller.stop(); _controller.value = scrollOffset; - return _controller.animateTo(newScrollOffset, duration: duration, curve: curve); + _dispatchOnScrollStartIfNeeded(); + return _controller.animateTo(newScrollOffset, duration: duration, curve: curve).then(_dispatchOnScrollEndIfNeeded); } bool _scrollOffsetIsInBounds(double scrollOffset) { @@ -303,7 +304,7 @@ abstract class ScrollableState extends State { Simulation _createFlingSimulation(double scrollVelocity) { final Simulation simulation = scrollBehavior.createFlingScrollSimulation(scrollOffset, scrollVelocity); if (simulation != null) { - final double endVelocity = pixelOffsetToScrollOffset(kPixelScrollTolerance.velocity) * scrollVelocity.sign; + final double endVelocity = pixelOffsetToScrollOffset(kPixelScrollTolerance.velocity).abs() * (scrollVelocity < 0.0 ? -1.0 : 1.0); final double endDistance = pixelOffsetToScrollOffset(kPixelScrollTolerance.distance).abs(); simulation.tolerance = new Tolerance(velocity: endVelocity, distance: endDistance); } @@ -336,7 +337,7 @@ abstract class ScrollableState extends State { return null; final double snapVelocity = scrollVelocity.abs() * (snappedScrollOffset - scrollOffset).sign; - final double endVelocity = pixelOffsetToScrollOffset(kPixelScrollTolerance.velocity).abs() * scrollVelocity.sign; + final double endVelocity = pixelOffsetToScrollOffset(kPixelScrollTolerance.velocity).abs() * (scrollVelocity < 0.0 ? -1.0 : 1.0); Simulation toSnapSimulation = scrollBehavior.createSnapScrollSimulation( scrollOffset, snappedScrollOffset, snapVelocity, endVelocity ); @@ -353,7 +354,8 @@ abstract class ScrollableState extends State { Simulation simulation = _createSnapSimulation(scrollVelocity) ?? _createFlingSimulation(scrollVelocity); if (simulation == null) return new Future.value(); - return _controller.animateWith(simulation); + _dispatchOnScrollStartIfNeeded(); + return _controller.animateWith(simulation).then(_dispatchOnScrollEndIfNeeded); } void dispose() { @@ -373,7 +375,15 @@ abstract class ScrollableState extends State { }); PageStorage.of(context)?.writeState(context, _scrollOffset); new ScrollNotification(this, _scrollOffset).dispatch(context); + final needsScrollStart = !_isBetweenOnScrollStartAndOnScrollEnd; + if (needsScrollStart) { + dispatchOnScrollStart(); + assert(_isBetweenOnScrollStartAndOnScrollEnd); + } dispatchOnScroll(); + assert(_isBetweenOnScrollStartAndOnScrollEnd); + if (needsScrollStart) + dispatchOnScrollEnd(); } /// Scroll this widget to the given scroll offset. @@ -413,10 +423,8 @@ abstract class ScrollableState extends State { /// offset with the given value as the initial velocity. The physics /// simulation used is determined by the scroll behavior. Future fling(double scrollVelocity) { - if (scrollVelocity != 0.0) + if (scrollVelocity != 0.0 || !_controller.isAnimating) return _startToEndAnimation(scrollVelocity); - if (!_controller.isAnimating) - return settleScrollOffset(); return new Future.value(); } @@ -429,10 +437,24 @@ abstract class ScrollableState extends State { return _startToEndAnimation(0.0); } + bool _isBetweenOnScrollStartAndOnScrollEnd = false; + + void _dispatchOnScrollStartIfNeeded() { + if (!_isBetweenOnScrollStartAndOnScrollEnd) + dispatchOnScrollStart(); + } + + void _dispatchOnScrollEndIfNeeded(_) { + if (_isBetweenOnScrollStartAndOnScrollEnd) + dispatchOnScrollEnd(); + } + /// Calls the onScrollStart callback. /// /// Subclasses can override this function to hook the scroll start callback. void dispatchOnScrollStart() { + assert(!_isBetweenOnScrollStartAndOnScrollEnd); + _isBetweenOnScrollStartAndOnScrollEnd = true; if (config.onScrollStart != null) config.onScrollStart(_scrollOffset); } @@ -441,6 +463,7 @@ abstract class ScrollableState extends State { /// /// Subclasses can override this function to hook the scroll callback. void dispatchOnScroll() { + assert(_isBetweenOnScrollStartAndOnScrollEnd); if (config.onScroll != null) config.onScroll(_scrollOffset); } @@ -449,6 +472,8 @@ abstract class ScrollableState extends State { /// /// Subclasses can override this function to hook the scroll end callback. void dispatchOnScrollEnd() { + assert(_isBetweenOnScrollStartAndOnScrollEnd); + _isBetweenOnScrollStartAndOnScrollEnd = false; if (config.onScrollEnd != null) config.onScrollEnd(_scrollOffset); } @@ -458,7 +483,7 @@ abstract class ScrollableState extends State { } void _handleDragStart(_) { - dispatchOnScrollStart(); + _dispatchOnScrollStartIfNeeded(); } void _handleDragUpdate(double delta) { @@ -468,9 +493,7 @@ abstract class ScrollableState extends State { Future _handleDragEnd(Velocity velocity) { double scrollVelocity = pixelDeltaToScrollOffset(velocity.pixelsPerSecond) / Duration.MILLISECONDS_PER_SECOND; // The gesture velocity properties are pixels/second, config min,max limits are pixels/ms - return fling(scrollVelocity.clamp(-kMaxFlingVelocity, kMaxFlingVelocity)).then((_) { - dispatchOnScrollEnd(); - }); + return fling(scrollVelocity.clamp(-kMaxFlingVelocity, kMaxFlingVelocity)).then(_dispatchOnScrollEndIfNeeded); } } diff --git a/packages/flutter/test/widget/scroll_events_test.dart b/packages/flutter/test/widget/scroll_events_test.dart new file mode 100644 index 00000000000..0bc09ae96e0 --- /dev/null +++ b/packages/flutter/test/widget/scroll_events_test.dart @@ -0,0 +1,98 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:flutter_test/flutter_test.dart'; +import 'package:flutter/widgets.dart'; +import 'package:test/test.dart'; + +Widget _buildScroller({Key key, List log}) { + return new ScrollableViewport( + key: key, + onScrollStart: (double scrollOffset) { + log.add('scrollstart'); + }, + onScroll: (double scrollOffset) { + log.add('scroll'); + }, + onScrollEnd: (double scrollOffset) { + log.add('scrollend'); + }, + child: new Container(width: 1000.0, height: 1000.0) + ); +} + +void main() { + test('Scroll event drag', () { + testWidgets((WidgetTester tester) { + List log = []; + tester.pumpWidget(_buildScroller(log: log)); + + expect(log, equals([])); + TestGesture gesture = tester.startGesture(new Point(100.0, 100.0)); + expect(log, equals(['scrollstart'])); + tester.pump(const Duration(seconds: 1)); + expect(log, equals(['scrollstart'])); + gesture.moveBy(new Offset(-10.0, -10.0)); + expect(log, equals(['scrollstart', 'scroll'])); + tester.pump(const Duration(seconds: 1)); + expect(log, equals(['scrollstart', 'scroll'])); + gesture.up(); + expect(log, equals(['scrollstart', 'scroll'])); + tester.pump(const Duration(seconds: 1)); + expect(log, equals(['scrollstart', 'scroll', 'scrollend'])); + }); + }); + + test('Scroll scrollTo animation', () { + testWidgets((WidgetTester tester) { + GlobalKey scrollKey = new GlobalKey(); + List log = []; + tester.pumpWidget(_buildScroller(key: scrollKey, log: log)); + + expect(log, equals([])); + scrollKey.currentState.scrollTo(100.0, duration: const Duration(seconds: 1)); + expect(log, equals(['scrollstart'])); + tester.pump(const Duration(milliseconds: 100)); + expect(log, equals(['scrollstart'])); + tester.pump(const Duration(milliseconds: 100)); + expect(log, equals(['scrollstart', 'scroll'])); + tester.pump(const Duration(milliseconds: 1500)); + expect(log, equals(['scrollstart', 'scroll', 'scroll', 'scrollend'])); + }); + }); + + test('Scroll scrollTo no animation', () { + testWidgets((WidgetTester tester) { + GlobalKey scrollKey = new GlobalKey(); + List log = []; + tester.pumpWidget(_buildScroller(key: scrollKey, log: log)); + + expect(log, equals([])); + scrollKey.currentState.scrollTo(100.0); + expect(log, equals(['scrollstart', 'scroll', 'scrollend'])); + }); + }); + + test('Scroll during animation', () { + testWidgets((WidgetTester tester) { + GlobalKey scrollKey = new GlobalKey(); + List log = []; + tester.pumpWidget(_buildScroller(key: scrollKey, log: log)); + + expect(log, equals([])); + scrollKey.currentState.scrollTo(100.0, duration: const Duration(seconds: 1)); + expect(log, equals(['scrollstart'])); + tester.pump(const Duration(milliseconds: 100)); + expect(log, equals(['scrollstart'])); + tester.pump(const Duration(milliseconds: 100)); + expect(log, equals(['scrollstart', 'scroll'])); + scrollKey.currentState.scrollTo(100.0); + expect(log, equals(['scrollstart', 'scroll', 'scroll'])); + tester.pump(const Duration(milliseconds: 100)); + expect(log, equals(['scrollstart', 'scroll', 'scroll', 'scrollend'])); + tester.pump(const Duration(milliseconds: 1500)); + expect(log, equals(['scrollstart', 'scroll', 'scroll', 'scrollend'])); + }); + }); +} diff --git a/packages/newton/lib/src/scroll_simulation.dart b/packages/newton/lib/src/scroll_simulation.dart index 64e0a45ba11..39126502f29 100644 --- a/packages/newton/lib/src/scroll_simulation.dart +++ b/packages/newton/lib/src/scroll_simulation.dart @@ -64,4 +64,8 @@ class ScrollSimulation extends SimulationGroup { return false; } + + String toString() { + return 'ScrollSimulation(leadingExtent: $_leadingExtent, trailingExtent: $_trailingExtent)'; + } } diff --git a/packages/newton/lib/src/tolerance.dart b/packages/newton/lib/src/tolerance.dart index b52d075e9c2..ae417f8d08e 100644 --- a/packages/newton/lib/src/tolerance.dart +++ b/packages/newton/lib/src/tolerance.dart @@ -11,6 +11,8 @@ class Tolerance { const Tolerance({this.distance: epsilonDefault, this.time: epsilonDefault, this.velocity: epsilonDefault}); + + String toString() => 'Tolerance(distance: $distance, time=$time, velocity: $velocity)'; } const double epsilonDefault = 1e-3;