From 862fc0513973932b423988d1ed33fcb814eae0ed Mon Sep 17 00:00:00 2001 From: Adam Barth Date: Tue, 21 Feb 2017 12:12:34 -0800 Subject: [PATCH] Call onPageChanged at the halfway mark (#8302) Previously we called onPageChanged when the scroll ended, but that is too late. Now we call onPageChanged when we cross the halfway mark, which, for example, makes the tab indicator update earlier. Fixes #8265 --- packages/flutter/lib/src/material/tabs.dart | 31 +---- .../flutter/lib/src/widgets/page_view.dart | 129 +++++++++++------- packages/flutter/test/material/tabs_test.dart | 42 ++++++ .../flutter/test/widgets/page_view_test.dart | 47 +++++++ 4 files changed, 174 insertions(+), 75 deletions(-) diff --git a/packages/flutter/lib/src/material/tabs.dart b/packages/flutter/lib/src/material/tabs.dart index ceab87ea36..594065eb08 100644 --- a/packages/flutter/lib/src/material/tabs.dart +++ b/packages/flutter/lib/src/material/tabs.dart @@ -639,8 +639,6 @@ class _TabBarViewState extends State { TabController _controller; PageController _pageController; List _children; - double _offsetAnchor; - double _offsetBias = 0.0; int _currentIndex; int _warpUnderwayCount = 0; @@ -742,30 +740,11 @@ class _TabBarViewState extends State { if (notification.depth != 0) return false; - if (notification is ScrollStartNotification) { - _offsetAnchor = null; - } else if (notification is ScrollUpdateNotification) { - if (!_controller.indexIsChanging) { - _offsetAnchor ??= _pageController.page; - _controller.offset = (_offsetBias + _pageController.page - _offsetAnchor).clamp(-1.0, 1.0); - } - } else if (notification is ScrollEndNotification) { - // Either the the animation that follows a fling has completed and we've landed - // on a new tab view, or a new pointer gesture has interrupted the fling - // animation before it has completed. - final double integralScrollOffset = _pageController.page.floorToDouble(); - if (integralScrollOffset == _pageController.page) { - _offsetBias = 0.0; - // The animation duration is short since the tab indicator and this - // page view have already moved. - _controller.animateTo( - integralScrollOffset.floor(), - duration: const Duration(milliseconds: 30) - ); - } else { - // The fling scroll animation was interrupted. - _offsetBias = _controller.offset; - } + if (notification is ScrollUpdateNotification && !_controller.indexIsChanging) { + _currentIndex = _pageController.page.round(); + if (_currentIndex != _controller.index) + _controller.index = _currentIndex; + _controller.offset = (_pageController.page - _controller.index).clamp(-1.0, 1.0); } return false; diff --git a/packages/flutter/lib/src/widgets/page_view.dart b/packages/flutter/lib/src/widgets/page_view.dart index de1c8818db..6d8feb3b50 100644 --- a/packages/flutter/lib/src/widgets/page_view.dart +++ b/packages/flutter/lib/src/widgets/page_view.dart @@ -4,6 +4,7 @@ import 'dart:async'; +import 'package:flutter/rendering.dart'; import 'package:meta/meta.dart'; import 'basic.dart'; @@ -14,7 +15,9 @@ import 'scroll_notification.dart'; import 'scroll_physics.dart'; import 'scroll_position.dart'; import 'scroll_view.dart'; +import 'scrollable.dart'; import 'sliver.dart'; +import 'viewport.dart'; class PageController extends ScrollController { PageController({ @@ -110,91 +113,119 @@ final PageController _defaultPageController = new PageController(); /// * [SingleChildScrollView], when you need to make a single child scrollable. /// * [ListView], for a scrollable list of boxes. /// * [GridView], for a scrollable grid of boxes. -class PageView extends BoxScrollView { +class PageView extends StatefulWidget { PageView({ Key key, - Axis scrollDirection: Axis.horizontal, - bool reverse: false, + this.scrollDirection: Axis.horizontal, + this.reverse: false, PageController controller, - ScrollPhysics physics: const PageScrollPhysics(), - bool shrinkWrap: false, - EdgeInsets padding, + this.physics: const PageScrollPhysics(), this.onPageChanged, List children: const [], - }) : childrenDelegate = new SliverChildListDelegate(children), super( - key: key, - scrollDirection: scrollDirection, - reverse: reverse, - controller: controller ?? _defaultPageController, - physics: physics, - shrinkWrap: shrinkWrap, - padding: padding, - ); + }) : controller = controller ?? _defaultPageController, + childrenDelegate = new SliverChildListDelegate(children), + super(key: key); PageView.builder({ Key key, - Axis scrollDirection: Axis.horizontal, - bool reverse: false, + this.scrollDirection: Axis.horizontal, + this.reverse: false, PageController controller, - ScrollPhysics physics: const PageScrollPhysics(), - bool shrinkWrap: false, - EdgeInsets padding, + this.physics: const PageScrollPhysics(), this.onPageChanged, IndexedWidgetBuilder itemBuilder, int itemCount, - }) : childrenDelegate = new SliverChildBuilderDelegate(itemBuilder, childCount: itemCount), super( - key: key, - scrollDirection: scrollDirection, - reverse: reverse, - controller: controller ?? _defaultPageController, - physics: physics, - shrinkWrap: shrinkWrap, - padding: padding, - ); + }) : controller = controller ?? _defaultPageController, + childrenDelegate = new SliverChildBuilderDelegate(itemBuilder, childCount: itemCount), + super(key: key); PageView.custom({ Key key, - Axis scrollDirection: Axis.horizontal, - bool reverse: false, + this.scrollDirection: Axis.horizontal, + this.reverse: false, PageController controller, - ScrollPhysics physics: const PageScrollPhysics(), - bool shrinkWrap: false, - EdgeInsets padding, + this.physics: const PageScrollPhysics(), this.onPageChanged, @required this.childrenDelegate, - }) : super( - key: key, - scrollDirection: scrollDirection, - reverse: reverse, - controller: controller ?? _defaultPageController, - physics: physics, - shrinkWrap: shrinkWrap, - padding: padding, - ) { + }) : controller = controller ?? _defaultPageController, super(key: key) { assert(childrenDelegate != null); } + final Axis scrollDirection; + + final bool reverse; + + final PageController controller; + + final ScrollPhysics physics; + final ValueChanged onPageChanged; final SliverChildDelegate childrenDelegate; @override - Widget buildChildLayout(BuildContext context) { - return new SliverFill(delegate: childrenDelegate); + _PageViewState createState() => new _PageViewState(); +} + +class _PageViewState extends State { + int _lastReportedPage = 0; + + @override + void initState() { + super.initState(); + _lastReportedPage = config.controller.initialPage; + } + + AxisDirection _getDirection(BuildContext context) { + // TODO(abarth): Consider reading direction. + switch (config.scrollDirection) { + case Axis.horizontal: + return config.reverse ? AxisDirection.left : AxisDirection.right; + case Axis.vertical: + return config.reverse ? AxisDirection.up : AxisDirection.down; + } + return null; } @override Widget build(BuildContext context) { - final Widget scrollable = super.build(context); + AxisDirection axisDirection = _getDirection(context); return new NotificationListener( onNotification: (ScrollNotification notification) { - if (notification.depth == 0 && onPageChanged != null && notification is ScrollEndNotification) { + if (notification.depth == 0 && config.onPageChanged != null && notification is ScrollUpdateNotification) { final ScrollMetrics metrics = notification.metrics; - onPageChanged(metrics.extentBefore ~/ metrics.viewportDimension); + final int currentPage = (metrics.extentBefore / metrics.viewportDimension).round(); + if (currentPage != _lastReportedPage) { + _lastReportedPage = currentPage; + config.onPageChanged(currentPage); + } } return false; }, - child: scrollable, + child: new Scrollable( + axisDirection: axisDirection, + controller: config.controller, + physics: config.physics, + viewportBuilder: (BuildContext context, ViewportOffset offset) { + return new Viewport( + axisDirection: axisDirection, + offset: offset, + slivers: [ + new SliverFill(delegate: config.childrenDelegate), + ], + ); + }, + ), ); } + + @override + void debugFillDescription(List description) { + super.debugFillDescription(description); + description.add('${config.scrollDirection}'); + if (config.reverse) + description.add('reversed'); + description.add('${config.controller}'); + description.add('${config.physics}'); + } } diff --git a/packages/flutter/test/material/tabs_test.dart b/packages/flutter/test/material/tabs_test.dart index 10b2156ddd..5560f5e7c9 100644 --- a/packages/flutter/test/material/tabs_test.dart +++ b/packages/flutter/test/material/tabs_test.dart @@ -618,4 +618,46 @@ void main() { expect(secondColor, equals(Colors.blue[500])); }); + testWidgets('TabBar unselectedLabelColor control test', (WidgetTester tester) async { + TabController controller = new TabController( + vsync: const TestVSync(), + length: 2, + ); + + await tester.pumpWidget( + new Material( + child: new TabBarView( + controller: controller, + children: [ new Text('First'), new Text('Second') ], + ), + ), + ); + + expect(controller.index, equals(0)); + + TestGesture gesture = await tester.startGesture(const Point(100.0, 100.0)); + + expect(controller.index, equals(0)); + + await gesture.moveBy(const Offset(-380.0, 0.0)); + + expect(controller.index, equals(0)); + + await gesture.moveBy(const Offset(-40.0, 0.0)); + + expect(controller.index, equals(1)); + + await gesture.moveBy(const Offset(-40.0, 0.0)); + await tester.pump(); + + expect(controller.index, equals(1)); + + await gesture.up(); + await tester.pumpUntilNoTransientCallbacks(); + expect(controller.index, equals(1)); + + expect(find.text('First'), findsNothing); + expect(find.text('Second'), findsOneWidget); + }); + } diff --git a/packages/flutter/test/widgets/page_view_test.dart b/packages/flutter/test/widgets/page_view_test.dart index 747b5f3b88..f3e8e7b72f 100644 --- a/packages/flutter/test/widgets/page_view_test.dart +++ b/packages/flutter/test/widgets/page_view_test.dart @@ -217,4 +217,51 @@ void main() { expect(find.text('Alabama'), findsOneWidget); }); + + testWidgets('Page changes at halfway point', (WidgetTester tester) async { + final List log = []; + await tester.pumpWidget(new PageView( + onPageChanged: (int page) { log.add(page); }, + children: kStates.map((String state) => new Text(state)).toList(), + )); + + expect(log, isEmpty); + + TestGesture gesture = await tester.startGesture(const Point(100.0, 100.0)); + // The page view is 800.0 wide, so this move is just short of halfway. + await gesture.moveBy(const Offset(-380.0, 0.0)); + + expect(log, isEmpty); + + // We've crossed the halfway mark. + await gesture.moveBy(const Offset(-40.0, 0.0)); + + expect(log, equals(const [1])); + log.clear(); + + // Moving a bit more should not generate redundant notifications. + await gesture.moveBy(const Offset(-40.0, 0.0)); + + expect(log, isEmpty); + + await gesture.moveBy(const Offset(-40.0, 0.0)); + await tester.pump(); + + await gesture.moveBy(const Offset(-40.0, 0.0)); + await tester.pump(); + + await gesture.moveBy(const Offset(-40.0, 0.0)); + await tester.pump(); + + expect(log, isEmpty); + + await gesture.up(); + await tester.pumpUntilNoTransientCallbacks(); + + expect(log, isEmpty); + + expect(find.text('Alabama'), findsNothing); + expect(find.text('Alaska'), findsOneWidget); + }); + }