From 8f797fc379655970cca6dfefeb53150e23ab449f Mon Sep 17 00:00:00 2001 From: "auto-submit[bot]" <98614782+auto-submit[bot]@users.noreply.github.com> Date: Fri, 12 Jan 2024 21:19:18 +0000 Subject: [PATCH] Reverts "Remove hack from PageView." (#141479) Reverts flutter/flutter#141138 Initiated by: itsjustkevin This change reverts the following previous change: Original Description: Fixes https://github.com/flutter/flutter/issues/141119 The change is breaking, because now controller is nullable. Migration path: https://github.com/flutter/website/pull/10033 Packages to fix: --- .../widgets/page_view/page_view.0_test.dart | 2 +- .../flutter/lib/src/widgets/page_view.dart | 58 +++++---------- packages/flutter/test/material/tabs_test.dart | 42 +++++------ .../flutter/test/widgets/page_view_test.dart | 73 ------------------- 4 files changed, 41 insertions(+), 134 deletions(-) diff --git a/examples/api/test/widgets/page_view/page_view.0_test.dart b/examples/api/test/widgets/page_view/page_view.0_test.dart index 86876c2b3f4..a994f22d0aa 100644 --- a/examples/api/test/widgets/page_view/page_view.0_test.dart +++ b/examples/api/test/widgets/page_view/page_view.0_test.dart @@ -49,7 +49,7 @@ void main() { // Verify that page view index is also updated with same index to page indicator. final PageView pageView = tester.widget(find.byType(PageView)); - expect(pageView.controller!.page, 1); + expect(pageView.controller.page, 1); // Tap backward button on page indicator area. await tester.tap(find.byIcon(Icons.arrow_left_rounded)); diff --git a/packages/flutter/lib/src/widgets/page_view.dart b/packages/flutter/lib/src/widgets/page_view.dart index 5da1f1c3b8e..33a139e87e4 100644 --- a/packages/flutter/lib/src/widgets/page_view.dart +++ b/packages/flutter/lib/src/widgets/page_view.dart @@ -576,6 +576,11 @@ class PageScrollPhysics extends ScrollPhysics { bool get allowImplicitScrolling => false; } +// Having this global (mutable) page controller is a bit of a hack. We need it +// to plumb in the factory for _PagePosition, but it will end up accumulating +// a large list of scroll positions. As long as you don't try to actually +// control the scroll positions, everything should be fine. +final PageController _defaultPageController = PageController(); const PageScrollPhysics _kPagePhysics = PageScrollPhysics(); /// A scrollable list that works page by page. @@ -640,7 +645,7 @@ class PageView extends StatefulWidget { super.key, this.scrollDirection = Axis.horizontal, this.reverse = false, - this.controller, + PageController? controller, this.physics, this.pageSnapping = true, this.onPageChanged, @@ -651,7 +656,8 @@ class PageView extends StatefulWidget { this.clipBehavior = Clip.hardEdge, this.scrollBehavior, this.padEnds = true, - }) : childrenDelegate = SliverChildListDelegate(children); + }) : controller = controller ?? _defaultPageController, + childrenDelegate = SliverChildListDelegate(children); /// Creates a scrollable list that works page by page using widgets that are /// created on demand. @@ -682,7 +688,7 @@ class PageView extends StatefulWidget { super.key, this.scrollDirection = Axis.horizontal, this.reverse = false, - this.controller, + PageController? controller, this.physics, this.pageSnapping = true, this.onPageChanged, @@ -695,7 +701,8 @@ class PageView extends StatefulWidget { this.clipBehavior = Clip.hardEdge, this.scrollBehavior, this.padEnds = true, - }) : childrenDelegate = SliverChildBuilderDelegate( + }) : controller = controller ?? _defaultPageController, + childrenDelegate = SliverChildBuilderDelegate( itemBuilder, findChildIndexCallback: findChildIndexCallback, childCount: itemCount, @@ -712,11 +719,11 @@ class PageView extends StatefulWidget { /// {@end-tool} /// /// {@macro flutter.widgets.PageView.allowImplicitScrolling} - const PageView.custom({ + PageView.custom({ super.key, this.scrollDirection = Axis.horizontal, this.reverse = false, - this.controller, + PageController? controller, this.physics, this.pageSnapping = true, this.onPageChanged, @@ -727,7 +734,7 @@ class PageView extends StatefulWidget { this.clipBehavior = Clip.hardEdge, this.scrollBehavior, this.padEnds = true, - }); + }) : controller = controller ?? _defaultPageController; /// Controls whether the widget's pages will respond to /// [RenderObject.showOnScreen], which will allow for implicit accessibility @@ -769,7 +776,7 @@ class PageView extends StatefulWidget { /// An object that can be used to control the position to which this page /// view is scrolled. - final PageController? controller; + final PageController controller; /// How the page view should respond to user input. /// @@ -841,37 +848,10 @@ class PageView extends StatefulWidget { class _PageViewState extends State { int _lastReportedPage = 0; - late PageController _controller; - @override void initState() { super.initState(); - _initController(); - _lastReportedPage = _controller.initialPage; - } - - @override - void dispose() { - if (widget.controller == null) { - _controller.dispose(); - } - super.dispose(); - } - - - void _initController() { - _controller = widget.controller ?? PageController(); - } - - @override - void didUpdateWidget(PageView oldWidget) { - if (oldWidget.controller != widget.controller) { - if (oldWidget.controller == null) { - _controller.dispose(); - } - _initController(); - } - super.didUpdateWidget(oldWidget); + _lastReportedPage = widget.controller.initialPage; } AxisDirection _getDirection(BuildContext context) { @@ -912,7 +892,7 @@ class _PageViewState extends State { child: Scrollable( dragStartBehavior: widget.dragStartBehavior, axisDirection: axisDirection, - controller: _controller, + controller: widget.controller, physics: physics, restorationId: widget.restorationId, scrollBehavior: widget.scrollBehavior ?? ScrollConfiguration.of(context).copyWith(scrollbars: false), @@ -928,7 +908,7 @@ class _PageViewState extends State { clipBehavior: widget.clipBehavior, slivers: [ SliverFillViewport( - viewportFraction: _controller.viewportFraction, + viewportFraction: widget.controller.viewportFraction, delegate: widget.childrenDelegate, padEnds: widget.padEnds, ), @@ -944,7 +924,7 @@ class _PageViewState extends State { super.debugFillProperties(description); description.add(EnumProperty('scrollDirection', widget.scrollDirection)); description.add(FlagProperty('reverse', value: widget.reverse, ifTrue: 'reversed')); - description.add(DiagnosticsProperty('controller', _controller, showName: false)); + description.add(DiagnosticsProperty('controller', widget.controller, showName: false)); description.add(DiagnosticsProperty('physics', widget.physics, showName: false)); description.add(FlagProperty('pageSnapping', value: widget.pageSnapping, ifFalse: 'snapping disabled')); description.add(FlagProperty('allowImplicitScrolling', value: widget.allowImplicitScrolling, ifTrue: 'allow implicit scrolling')); diff --git a/packages/flutter/test/material/tabs_test.dart b/packages/flutter/test/material/tabs_test.dart index 07d92445413..536bbf04f1a 100644 --- a/packages/flutter/test/material/tabs_test.dart +++ b/packages/flutter/test/material/tabs_test.dart @@ -1482,7 +1482,7 @@ void main() { expect(tabController.index, 1); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; final ScrollPosition position = pageController.position; // The TabBarView's page width is 400, so page 0 is at scroll offset 0.0, @@ -1531,7 +1531,7 @@ void main() { expect(tabController.index, 0); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; final ScrollPosition position = pageController.position; expect(position.pixels, 0.0); @@ -1592,7 +1592,7 @@ void main() { expect(tabController.index, 1); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; // The TabView was initialized with viewportFraction as 0.8 // So it's expected the PageView inside would obtain the same viewportFraction @@ -1635,7 +1635,7 @@ void main() { expect(tabController.index, 1); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; // The TabView was initialized with default viewportFraction // So it's expected the PageView inside would obtain the value 1 @@ -1680,13 +1680,13 @@ void main() { await tester.pumpWidget(buildFrame(0.8)); PageView pageView = tester.widget(find.byType(PageView)); - PageController pageController = pageView.controller!; + PageController pageController = pageView.controller; expect(pageController.viewportFraction, 0.8); // Rebuild with a different viewport fraction. await tester.pumpWidget(buildFrame(0.5)); pageView = tester.widget(find.byType(PageView)); - pageController = pageView.controller!; + pageController = pageView.controller; expect(pageController.viewportFraction, 0.5); }); @@ -1861,7 +1861,7 @@ void main() { expect(tabController.index, 1); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; final ScrollPosition position = pageController.position; // The TabBarView's page width is 400, so page 0 is at scroll offset 0.0, @@ -1905,7 +1905,7 @@ void main() { expect(tabController.index, 0); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; final ScrollPosition position = pageController.position; // The TabBarView's page width is 400, so page 0 is at scroll offset 0.0, @@ -1950,7 +1950,7 @@ void main() { expect(tabController.index, 0); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; final ScrollPosition position = pageController.position; // The TabBarView's page width is 400, so page 0 is at scroll offset 0.0, @@ -1999,7 +1999,7 @@ void main() { expect(tabController.index, 0); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; final ScrollPosition position = pageController.position; // The TabBarView's page width is 400, so page 0 is at scroll offset 0.0, @@ -2050,7 +2050,7 @@ void main() { expect(tabController.index, 0); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; final ScrollPosition position = pageController.position; // The TabBarView's page width is 400, so page 0 is at scroll offset 0.0, @@ -2246,7 +2246,7 @@ void main() { expect(tabController.index, 1); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; final ScrollPosition position = pageController.position; // The TabBarView's page width is 400, so page 0 is at scroll offset 0.0, @@ -2432,7 +2432,7 @@ void main() { expect(tabController.index, 1); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; final ScrollPosition position = pageController.position; // The TabBarView's page width is 400, so page 0 is at scroll offset 0.0, @@ -4208,7 +4208,7 @@ void main() { await tester.pumpWidget(buildFrame(15)); PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController1 = pageView.controller!; + final PageController pageController1 = pageView.controller; TabController tabController = DefaultTabController.of(tester.element(find.text('Page 14'))); expect(tabController.index, 14); expect(pageController1.page, 14); @@ -4216,7 +4216,7 @@ void main() { // Rebuild with a new default tab controller with more tabs. await tester.pumpWidget(buildFrame(10)); pageView = tester.widget(find.byType(PageView)); - final PageController pageController2 = pageView.controller!; + final PageController pageController2 = pageView.controller; tabController = DefaultTabController.of(tester.element(find.text('Page 9'))); expect(tabController.index, 9); expect(pageController2.page, 9); @@ -5065,7 +5065,7 @@ void main() { double expectedIndicatorLeft = canvas.indicatorRect.left; final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; void pageControllerListener() { // Whenever TabBarView scrolls due to changing TabController's index, // check if indicator stays idle in its expectedIndicatorLeft @@ -5225,7 +5225,7 @@ void main() { )); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; final ScrollPosition position = pageController.position; expect(tabController.index, 0); @@ -5715,7 +5715,7 @@ void main() { await tester.pumpWidget(buildFrame(controller1, showLast: true)); final PageView pageView = tester.widget(find.byType(PageView)); - final PageController pageController = pageView.controller!; + final PageController pageController = pageView.controller; await tester.tap(find.text('three')); await tester.pumpAndSettle(); expect(controller1.index, 2); @@ -5782,7 +5782,7 @@ void main() { await tester.pumpWidget(buildFrame(controller1, showLast: true)); PageView pageView = tester.widget(find.byType(PageView)); - PageController pageController = pageView.controller!; + PageController pageController = pageView.controller; await tester.tap(find.text('three')); await tester.pumpAndSettle(); expect(controller1.index, 2); @@ -5792,7 +5792,7 @@ void main() { await tester.pumpWidget(buildFrame(controller2, showLast: false)); await tester.pumpAndSettle(); pageView = tester.widget(find.byType(PageView)); - pageController = pageView.controller!; + pageController = pageView.controller; expect(controller2.index, 0); expect(pageController.page, 0); @@ -5800,7 +5800,7 @@ void main() { await tester.pumpWidget(buildFrame(controller1, showLast: true)); await tester.pumpAndSettle(); pageView = tester.widget(find.byType(PageView)); - pageController = pageView.controller!; + pageController = pageView.controller; expect(controller1.index, 2); expect(pageController.page, 2); }); diff --git a/packages/flutter/test/widgets/page_view_test.dart b/packages/flutter/test/widgets/page_view_test.dart index 3fbe1480502..6d3717a0121 100644 --- a/packages/flutter/test/widgets/page_view_test.dart +++ b/packages/flutter/test/widgets/page_view_test.dart @@ -1303,77 +1303,4 @@ void main() { expect(attach, 1); expect(detach, 1); }); - - group('$PageView handles change of controller', () { - final GlobalKey key = GlobalKey(); - - Widget createPageView(PageController? controller) { - return MaterialApp( - home: Scaffold( - body: PageView( - key: key, - controller: controller, - children: const [ - Center(child: Text('0')), - Center(child: Text('1')), - Center(child: Text('2')), - ], - ), - ), - ); - } - - Future testPageViewWithController(PageController controller, WidgetTester tester, bool controls) async { - int curentVisiblePage() { - return int.parse(tester.widgetList(find.byType(Text)).whereType().first.data!); - } - - final int initialPageInView = curentVisiblePage(); - - for (int i = 0; i < 3; i++) { - if (controls) { - controller.jumpToPage(i); - await tester.pumpAndSettle(); - expect(curentVisiblePage(), i); - } else { - expect(()=> controller.jumpToPage(i), throwsAssertionError); - expect(curentVisiblePage(), initialPageInView); - } - } - } - - testWidgets('null to value', (WidgetTester tester) async { - final PageController controller = PageController(); - addTearDown(controller.dispose); - await tester.pumpWidget(createPageView(null)); - await tester.pumpWidget(createPageView(controller)); - await testPageViewWithController(controller, tester, true); - }); - - testWidgets('value to value', (WidgetTester tester) async { - final PageController controller1 = PageController(); - addTearDown(controller1.dispose); - final PageController controller2 = PageController(); - addTearDown(controller2.dispose); - await tester.pumpWidget(createPageView(controller1)); - await testPageViewWithController(controller1, tester, true); - await tester.pumpWidget(createPageView(controller2)); - await testPageViewWithController(controller1, tester, false); - await testPageViewWithController(controller2, tester, true); - }); - - testWidgets('value to null', (WidgetTester tester) async { - final PageController controller = PageController(); - addTearDown(controller.dispose); - await tester.pumpWidget(createPageView(controller)); - await testPageViewWithController(controller, tester, true); - await tester.pumpWidget(createPageView(null)); - await testPageViewWithController(controller, tester, false); - }); - - testWidgets('null to null', (WidgetTester tester) async { - await tester.pumpWidget(createPageView(null)); - await tester.pumpWidget(createPageView(null)); - }); - }); }