Scrollable's callbacks should follow a state machine

Now onScroll callbacks are always bracketed by onScrollStart and onScrollEnd
callbacks.

Fixes #1822

packages/flutter/lib/src/widgets/scrollable.dart

@@ -290,7 +290,8 @@ abstract class ScrollableState<T extends Scrollable> extends State<T> {
   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<T extends Scrollable> extends State<T> {
   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<T extends Scrollable> extends State<T> {
       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<T extends Scrollable> extends State<T> {
     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<T extends Scrollable> extends State<T> {
     });
     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<T extends Scrollable> extends State<T> {
   /// 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<T extends Scrollable> extends State<T> {
     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<T extends Scrollable> extends State<T> {
   ///
   /// 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<T extends Scrollable> extends State<T> {
   ///
   /// 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<T extends Scrollable> extends State<T> {
   }
 
   void _handleDragStart(_) {
-    dispatchOnScrollStart();
+    _dispatchOnScrollStartIfNeeded();
   }
 
   void _handleDragUpdate(double delta) {
@@ -468,9 +493,7 @@ abstract class ScrollableState<T extends Scrollable> extends State<T> {
   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);
   }
 }
 

packages/flutter/test/widget/scroll_events_test.dart

+// 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<String> 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<String> log = <String>[];
+      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<ScrollableState> scrollKey = new GlobalKey<ScrollableState>();
+      List<String> log = <String>[];
+      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<ScrollableState> scrollKey = new GlobalKey<ScrollableState>();
+      List<String> log = <String>[];
+      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<ScrollableState> scrollKey = new GlobalKey<ScrollableState>();
+      List<String> log = <String>[];
+      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']));
+    });
+  });
+}

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)';
+  }
 }

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;