From 32ee00849c689503da3e28e909356132837886c6 Mon Sep 17 00:00:00 2001 From: renyou Date: Thu, 13 Aug 2020 17:46:04 -0400 Subject: [PATCH] Revert "Fix RangeMaintainingScrollPhysics" (#63611) --- .../lib/src/widgets/scroll_physics.dart | 99 +------ .../lib/src/widgets/scroll_position.dart | 19 +- .../flutter/lib/src/widgets/scrollable.dart | 7 +- ...range_maintaining_scroll_physics_test.dart | 41 --- .../test/widgets/scroll_activity_test.dart | 264 ------------------ 5 files changed, 16 insertions(+), 414 deletions(-) delete mode 100644 packages/flutter/test/widgets/scroll_activity_test.dart diff --git a/packages/flutter/lib/src/widgets/scroll_physics.dart b/packages/flutter/lib/src/widgets/scroll_physics.dart index d67c23649c0..1e9b9901234 100644 --- a/packages/flutter/lib/src/widgets/scroll_physics.dart +++ b/packages/flutter/lib/src/widgets/scroll_physics.dart @@ -412,45 +412,6 @@ class ScrollPhysics { /// These physics should be combined with other scroll physics, e.g. /// [BouncingScrollPhysics] or [ClampingScrollPhysics], to obtain a complete /// description of typical scroll physics. See [applyTo]. -/// -/// ## Implementation details -/// -/// Specifically, these physics perform two adjustments. -/// -/// The first is to maintain overscroll when the position is out of range. -/// -/// The second is to enforce the boundary when the position is in range. -/// -/// If the current velocity is zero, neither adjustment is made. The -/// assumption is that there is an ongoing animation and therefore -/// further changing the scroll position would disrupt the experience. -/// -/// If the extents haven't changed, then the overscroll adjustment is -/// not made. The assumption is that if the position is overscrolled, -/// it is intentional, otherwise the position could not have reached -/// that position. (Consider [ClampingScrollPhysics] vs -/// [BouncingScrollPhysics] for example.) -/// -/// If the position itself changed since the last animation frame, -/// then the overscroll is not maintained. The assumption is similar -/// to the previous case: the position would not have been placed out -/// of range unless it was intentional. -/// -/// In addition, if the position changed and the boundaries were and -/// still are finite, then the boundary isn't enforced either, for -/// the same reason. However, if any of the boundaries were or are -/// now infinite, the boundary _is_ enforced, on the assumption that -/// infinite boundaries indicate a lazy-loading scroll view, which -/// cannot enforce boundaries while the full list has not loaded. -/// -/// If the range was out of range, then the boundary is not enforced -/// even if the range is not maintained. If the range is maintained, -/// then the distance between the old position and the old boundary is -/// applied to the new boundary to obtain the new position. -/// -/// If the range was in range, and the boundary is to be enforced, -/// then the new position is obtained by deferring to the other physics, -/// if any, and then clamped to the new range. class RangeMaintainingScrollPhysics extends ScrollPhysics { /// Creates scroll physics that maintain the scroll position in range. const RangeMaintainingScrollPhysics({ ScrollPhysics parent }) : super(parent: parent); @@ -467,60 +428,18 @@ class RangeMaintainingScrollPhysics extends ScrollPhysics { @required bool isScrolling, @required double velocity, }) { - bool maintainOverscroll = true; - bool enforceBoundary = true; - if (velocity != 0.0) { - // Don't try to adjust an animating position, the jumping around - // would be distracting. - maintainOverscroll = false; - enforceBoundary = false; + if (velocity != 0.0 || ((oldPosition.minScrollExtent == newPosition.minScrollExtent) && (oldPosition.maxScrollExtent == newPosition.maxScrollExtent))) { + return super.adjustPositionForNewDimensions(oldPosition: oldPosition, newPosition: newPosition, isScrolling: isScrolling, velocity: velocity); } - if ((oldPosition.minScrollExtent == newPosition.minScrollExtent) && - (oldPosition.maxScrollExtent == newPosition.maxScrollExtent)) { - // If the extents haven't changed then ignore overscroll. - maintainOverscroll = false; + if (oldPosition.pixels < oldPosition.minScrollExtent) { + final double oldDelta = oldPosition.minScrollExtent - oldPosition.pixels; + return newPosition.minScrollExtent - oldDelta; } - if (oldPosition.pixels != newPosition.pixels) { - // If the position has been changed already, then it might have - // been adjusted to expect new overscroll, so don't try to - // maintain the relative overscroll. - maintainOverscroll = false; - if (oldPosition.minScrollExtent.isFinite && oldPosition.maxScrollExtent.isFinite && - newPosition.minScrollExtent.isFinite && newPosition.maxScrollExtent.isFinite) { - // In addition, if the position changed then we only enforce - // the new boundary if the previous boundary was not entirely - // finite. A common case where the position changes while one - // of the extents is infinite is a lazily-loaded list. (If the - // boundaries were finite, and the position changed, then we - // assume it was intentional.) - enforceBoundary = false; - } + if (oldPosition.pixels > oldPosition.maxScrollExtent) { + final double oldDelta = oldPosition.pixels - oldPosition.maxScrollExtent; + return newPosition.maxScrollExtent + oldDelta; } - if ((oldPosition.pixels < oldPosition.minScrollExtent) && - (oldPosition.pixels > oldPosition.maxScrollExtent)) { - // If the old position was out of range, then we should - // not try to keep the new position in range. - enforceBoundary = false; - } - if (maintainOverscroll) { - // Force the new position to be no more out of range - // than it was before, if it was overscrolled. - if (oldPosition.pixels < oldPosition.minScrollExtent) { - final double oldDelta = oldPosition.minScrollExtent - oldPosition.pixels; - return newPosition.minScrollExtent - oldDelta; - } - if (oldPosition.pixels > oldPosition.maxScrollExtent) { - final double oldDelta = oldPosition.pixels - oldPosition.maxScrollExtent; - return newPosition.maxScrollExtent + oldDelta; - } - } - // If we're not forcing the overscroll, defer to other physics. - double result = super.adjustPositionForNewDimensions(oldPosition: oldPosition, newPosition: newPosition, isScrolling: isScrolling, velocity: velocity); - if (enforceBoundary) { - // ...but if they put us out of range then reinforce the boundary. - result = result.clamp(newPosition.minScrollExtent, newPosition.maxScrollExtent) as double; - } - return result; + return newPosition.pixels.clamp(newPosition.minScrollExtent, newPosition.maxScrollExtent) as double; } } diff --git a/packages/flutter/lib/src/widgets/scroll_position.dart b/packages/flutter/lib/src/widgets/scroll_position.dart index cb8088fed95..f58d83ab2d4 100644 --- a/packages/flutter/lib/src/widgets/scroll_position.dart +++ b/packages/flutter/lib/src/widgets/scroll_position.dart @@ -226,7 +226,7 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { /// If there is any overscroll, it is reported using [didOverscrollBy]. double setPixels(double newPixels) { assert(_pixels != null); - assert(SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks, 'A scrollable\'s position should not change during the build, layout, and paint phases, otherwise the rendering will be confused.'); + assert(SchedulerBinding.instance.schedulerPhase.index <= SchedulerPhase.transientCallbacks.index); if (newPixels != pixels) { final double overscroll = applyBoundaryConditions(newPixels); assert(() { @@ -471,37 +471,27 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { return true; } - bool _pendingDimensions = false; - ScrollMetrics _lastMetrics; - @override bool applyContentDimensions(double minScrollExtent, double maxScrollExtent) { assert(minScrollExtent != null); assert(maxScrollExtent != null); - assert(haveDimensions == (_lastMetrics != null)); if (!nearEqual(_minScrollExtent, minScrollExtent, Tolerance.defaultTolerance.distance) || !nearEqual(_maxScrollExtent, maxScrollExtent, Tolerance.defaultTolerance.distance) || _didChangeViewportDimensionOrReceiveCorrection) { assert(minScrollExtent != null); assert(maxScrollExtent != null); assert(minScrollExtent <= maxScrollExtent); + final ScrollMetrics oldPosition = haveDimensions ? copyWith() : null; _minScrollExtent = minScrollExtent; _maxScrollExtent = maxScrollExtent; - final ScrollMetrics currentMetrics = haveDimensions ? copyWith() : null; + final ScrollMetrics newPosition = haveDimensions ? copyWith() : null; _didChangeViewportDimensionOrReceiveCorrection = false; - _pendingDimensions = true; - if (haveDimensions && !correctForNewDimensions(_lastMetrics, currentMetrics)) { + if (haveDimensions && !correctForNewDimensions(oldPosition, newPosition)) return false; - } _haveDimensions = true; - } - assert(haveDimensions); - if (_pendingDimensions) { applyNewDimensions(); - _pendingDimensions = false; } assert(!_didChangeViewportDimensionOrReceiveCorrection, 'Use correctForNewDimensions() (and return true) to change the scroll offset during applyContentDimensions().'); - _lastMetrics = copyWith(); return true; } @@ -556,7 +546,6 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { @mustCallSuper void applyNewDimensions() { assert(pixels != null); - assert(_pendingDimensions); activity.applyNewDimensions(); _updateSemanticActions(); // will potentially request a semantics update. } diff --git a/packages/flutter/lib/src/widgets/scrollable.dart b/packages/flutter/lib/src/widgets/scrollable.dart index 50ac9fef421..9f175481fea 100644 --- a/packages/flutter/lib/src/widgets/scrollable.dart +++ b/packages/flutter/lib/src/widgets/scrollable.dart @@ -694,7 +694,7 @@ class ScrollableState extends State with TickerProviderStateMixin, R key: _scrollSemanticsKey, child: result, position: position, - allowImplicitScrolling: _physics.allowImplicitScrolling, + allowImplicitScrolling: widget?.physics?.allowImplicitScrolling ?? _physics.allowImplicitScrolling, semanticChildCount: widget.semanticChildCount, ); } @@ -706,7 +706,6 @@ class ScrollableState extends State with TickerProviderStateMixin, R void debugFillProperties(DiagnosticPropertiesBuilder properties) { super.debugFillProperties(properties); properties.add(DiagnosticsProperty('position', position)); - properties.add(DiagnosticsProperty('effective physics', _physics)); } @override @@ -974,7 +973,7 @@ class ScrollAction extends Action { assert(state.position.viewportDimension != null); assert(state.position.maxScrollExtent != null); assert(state.position.minScrollExtent != null); - assert(state._physics == null || state._physics.shouldAcceptUserOffset(state.position)); + assert(state.widget.physics == null || state.widget.physics.shouldAcceptUserOffset(state.position)); if (state.widget.incrementCalculator != null) { return state.widget.incrementCalculator( ScrollIncrementDetails( @@ -1063,7 +1062,7 @@ class ScrollAction extends Action { assert(state.position.minScrollExtent != null); // Don't do anything if the user isn't allowed to scroll. - if (state._physics != null && !state._physics.shouldAcceptUserOffset(state.position)) { + if (state.widget.physics != null && !state.widget.physics.shouldAcceptUserOffset(state.position)) { return; } final double increment = _getIncrement(state, intent); diff --git a/packages/flutter/test/widgets/range_maintaining_scroll_physics_test.dart b/packages/flutter/test/widgets/range_maintaining_scroll_physics_test.dart index 256e87a0e46..8beabd87f24 100644 --- a/packages/flutter/test/widgets/range_maintaining_scroll_physics_test.dart +++ b/packages/flutter/test/widgets/range_maintaining_scroll_physics_test.dart @@ -219,45 +219,4 @@ void main() { expect(position.maxScrollExtent, 100.0); expect(position.pixels, 0.0); }); - - testWidgets('expanding page views', (WidgetTester tester) async { - await tester.pumpWidget(Padding(padding: const EdgeInsets.only(right: 200.0), child: TabBarDemo())); - await tester.tap(find.text('bike')); - await tester.pump(); - await tester.pump(const Duration(seconds: 1)); - final Rect bike1 = tester.getRect(find.byIcon(Icons.directions_bike)); - await tester.pumpWidget(Padding(padding: EdgeInsets.zero, child: TabBarDemo())); - final Rect bike2 = tester.getRect(find.byIcon(Icons.directions_bike)); - expect(bike2.center, bike1.shift(const Offset(100.0, 0.0)).center); - }); } - -class TabBarDemo extends StatelessWidget { - @override - Widget build(BuildContext context) { - return MaterialApp( - home: DefaultTabController( - length: 3, - child: Scaffold( - appBar: AppBar( - bottom: const TabBar( - tabs: [ - Tab(text: 'car'), - Tab(text: 'transit'), - Tab(text: 'bike'), - ], - ), - title: const Text('Tabs Demo'), - ), - body: const TabBarView( - children: [ - Icon(Icons.directions_car), - Icon(Icons.directions_transit), - Icon(Icons.directions_bike), - ], - ), - ), - ), - ); - } -} \ No newline at end of file diff --git a/packages/flutter/test/widgets/scroll_activity_test.dart b/packages/flutter/test/widgets/scroll_activity_test.dart deleted file mode 100644 index 9d05801e9c9..00000000000 --- a/packages/flutter/test/widgets/scroll_activity_test.dart +++ /dev/null @@ -1,264 +0,0 @@ -// Copyright 2014 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -// @dart = 2.8 - -import 'package:flutter/material.dart'; -import 'package:flutter/scheduler.dart'; -import 'package:flutter_test/flutter_test.dart'; - -List children(int n) { - return List.generate(n, (int i) { - return Container(height: 100.0, child: Text('$i')); - }); -} - -void main() { - testWidgets('Scrolling with list view changes', (WidgetTester tester) async { - final ScrollController controller = ScrollController(); - await tester.pumpWidget(MaterialApp(home: ListView(children: children(30), controller: controller))); - final double thirty = controller.position.maxScrollExtent; - controller.jumpTo(thirty); - await tester.pump(); - controller.jumpTo(thirty + 100.0); // past the end - await tester.pump(); - await tester.pumpWidget(MaterialApp(home: ListView(children: children(31), controller: controller))); - expect(controller.position.pixels, thirty + 200.0); // same distance past the end - expect(await tester.pumpAndSettle(), 7); // now it goes ballistic... - expect(controller.position.pixels, thirty + 100.0); // and ends up at the end - }); - - testWidgets('Ability to keep a PageView at the end manually (issue 62209)', (WidgetTester tester) async { - await tester.pumpWidget(const MaterialApp(home: PageView62209())); - expect(find.text('Page 1'), findsOneWidget); - expect(find.text('Page 100'), findsNothing); - await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 1'), findsNothing); - expect(find.text('Page 2'), findsOneWidget); - expect(find.text('Page 100'), findsNothing); - await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 1'), findsNothing); - expect(find.text('Page 3'), findsOneWidget); - expect(find.text('Page 100'), findsNothing); - await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 1'), findsNothing); - expect(find.text('Page 4'), findsOneWidget); - expect(find.text('Page 100'), findsNothing); - await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 1'), findsNothing); - expect(find.text('Page 5'), findsOneWidget); - expect(find.text('Page 100'), findsNothing); - await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 1'), findsNothing); - expect(find.text('Page 5'), findsNothing); - expect(find.text('Page 100'), findsOneWidget); - await tester.tap(find.byType(FlatButton)); // 6 - await tester.pump(); - expect(find.text('Page 1'), findsNothing); - expect(find.text('Page 6'), findsNothing); - expect(find.text('Page 5'), findsNothing); - expect(find.text('Page 100'), findsOneWidget); - await tester.tap(find.byType(FlatButton)); // 7 - await tester.pump(); - expect(find.text('Page 1'), findsNothing); - expect(find.text('Page 6'), findsNothing); - expect(find.text('Page 7'), findsNothing); - expect(find.text('Page 5'), findsNothing); - expect(find.text('Page 100'), findsOneWidget); - await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 1'), findsNothing); - expect(find.text('Page 5'), findsOneWidget); - expect(find.text('Page 100'), findsNothing); - await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 1'), findsNothing); - expect(find.text('Page 4'), findsOneWidget); - expect(find.text('Page 5'), findsNothing); - expect(find.text('Page 100'), findsNothing); - await tester.tap(find.byType(FlatButton)); // 8 - await tester.pump(); - expect(find.text('Page 1'), findsNothing); - expect(find.text('Page 8'), findsNothing); - expect(find.text('Page 4'), findsOneWidget); - expect(find.text('Page 5'), findsNothing); - expect(find.text('Page 100'), findsNothing); - await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 3'), findsOneWidget); - await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 2'), findsOneWidget); - await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 6'), findsOneWidget); - await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 7'), findsOneWidget); - await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 8'), findsOneWidget); - await tester.drag(find.byType(PageView62209), const Offset(800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 1'), findsOneWidget); - await tester.tap(find.byType(FlatButton)); // 9 - await tester.pump(); - expect(find.text('Page 1'), findsOneWidget); - expect(find.text('Page 9'), findsNothing); - await tester.drag(find.byType(PageView62209), const Offset(-800.0, 0.0)); - await tester.pump(); - expect(find.text('Page 9'), findsOneWidget); - }); -} - -class PageView62209 extends StatefulWidget { - const PageView62209(); - - @override - _PageView62209State createState() => _PageView62209State(); -} - -class _PageView62209State extends State { - int _nextPageNum = 1; - final List _pages = []; - - @override - void initState() { - super.initState(); - for (int i = 0; i < 5; i++) { - _pages.add(Carousel62209Page( - key: Key('$_nextPageNum'), - number: _nextPageNum++, - )); - } - _pages.add(const Carousel62209Page(number: 100)); - } - - @override - Widget build(BuildContext context) { - return Scaffold( - body: Column( - children: [ - Expanded(child: Carousel62209(pages: _pages)), - FlatButton( - child: const Text('ADD PAGE'), - onPressed: () { - setState(() { - _pages.insert( - 1, - Carousel62209Page( - key: Key('$_nextPageNum'), - number: _nextPageNum++, - ), - ); - }); - }, - ) - ], - ), - ); - } -} - -class Carousel62209Page extends StatelessWidget { - const Carousel62209Page({this.number, Key key}) : super(key: key); - - final int number; - - @override - Widget build(BuildContext context) { - return Center(child: Text('Page $number')); - } -} - -class Carousel62209 extends StatefulWidget { - const Carousel62209({Key key, this.pages}) : super(key: key); - - final List pages; - - @override - _Carousel62209State createState() => _Carousel62209State(); -} - -class _Carousel62209State extends State { - // page variables - PageController _pageController; - int _currentPage = 0; - - // controls updates outside of user interaction - List _pages; - bool _jumpingToPage = false; - - @override - void initState() { - super.initState(); - _pages = widget.pages.toList(); - _pageController = PageController(initialPage: 0, keepPage: false); - } - - @override - void didUpdateWidget(Carousel62209 oldWidget) { - super.didUpdateWidget(oldWidget); - if (!_jumpingToPage) { - int newPage = -1; - for (int i = 0; i < widget.pages.length; i++) { - if (widget.pages[i].number == _pages[_currentPage].number) { - newPage = i; - } - } - if (newPage == _currentPage) { - _pages = widget.pages.toList(); - } else { - _jumpingToPage = true; - SchedulerBinding.instance.addPostFrameCallback((_) { - if (mounted) { - setState(() { - _pages = widget.pages.toList(); - _currentPage = newPage; - _pageController.jumpToPage(_currentPage); - SchedulerBinding.instance.addPostFrameCallback((_) { - _jumpingToPage = false; - }); - }); - } - }); - } - } - } - - @override - void dispose() { - _pageController.dispose(); - super.dispose(); - } - - bool _handleScrollNotification(ScrollNotification notification) { - if (notification is ScrollUpdateNotification) { - final int page = _pageController.page.round(); - if (!_jumpingToPage && _currentPage != page) { - _currentPage = page; - } - } - return true; - } - - @override - Widget build(BuildContext context) { - return NotificationListener( - onNotification: _handleScrollNotification, - child: PageView.builder( - controller: _pageController, - itemCount: _pages.length, - itemBuilder: (BuildContext context, int index) { - return _pages[index]; - }, - ), - ); - } -}