From 2da87e6108d459f39122788d7daf1c0b0c646d96 Mon Sep 17 00:00:00 2001 From: Polina Cherkasova Date: Fri, 12 Jan 2024 09:47:34 -0800 Subject: [PATCH] Remove hack from PageView. (#141138) --- .../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, 134 insertions(+), 41 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 a994f22d0aa4..86876c2b3f4e 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 33a139e87e42..5da1f1c3b8e6 100644 --- a/packages/flutter/lib/src/widgets/page_view.dart +++ b/packages/flutter/lib/src/widgets/page_view.dart @@ -576,11 +576,6 @@ 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. @@ -645,7 +640,7 @@ class PageView extends StatefulWidget { super.key, this.scrollDirection = Axis.horizontal, this.reverse = false, - PageController? controller, + this.controller, this.physics, this.pageSnapping = true, this.onPageChanged, @@ -656,8 +651,7 @@ class PageView extends StatefulWidget { this.clipBehavior = Clip.hardEdge, this.scrollBehavior, this.padEnds = true, - }) : controller = controller ?? _defaultPageController, - childrenDelegate = SliverChildListDelegate(children); + }) : childrenDelegate = SliverChildListDelegate(children); /// Creates a scrollable list that works page by page using widgets that are /// created on demand. @@ -688,7 +682,7 @@ class PageView extends StatefulWidget { super.key, this.scrollDirection = Axis.horizontal, this.reverse = false, - PageController? controller, + this.controller, this.physics, this.pageSnapping = true, this.onPageChanged, @@ -701,8 +695,7 @@ class PageView extends StatefulWidget { this.clipBehavior = Clip.hardEdge, this.scrollBehavior, this.padEnds = true, - }) : controller = controller ?? _defaultPageController, - childrenDelegate = SliverChildBuilderDelegate( + }) : childrenDelegate = SliverChildBuilderDelegate( itemBuilder, findChildIndexCallback: findChildIndexCallback, childCount: itemCount, @@ -719,11 +712,11 @@ class PageView extends StatefulWidget { /// {@end-tool} /// /// {@macro flutter.widgets.PageView.allowImplicitScrolling} - PageView.custom({ + const PageView.custom({ super.key, this.scrollDirection = Axis.horizontal, this.reverse = false, - PageController? controller, + this.controller, this.physics, this.pageSnapping = true, this.onPageChanged, @@ -734,7 +727,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 @@ -776,7 +769,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. /// @@ -848,10 +841,37 @@ class PageView extends StatefulWidget { class _PageViewState extends State { int _lastReportedPage = 0; + late PageController _controller; + @override void initState() { super.initState(); - _lastReportedPage = widget.controller.initialPage; + _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); } AxisDirection _getDirection(BuildContext context) { @@ -892,7 +912,7 @@ class _PageViewState extends State { child: Scrollable( dragStartBehavior: widget.dragStartBehavior, axisDirection: axisDirection, - controller: widget.controller, + controller: _controller, physics: physics, restorationId: widget.restorationId, scrollBehavior: widget.scrollBehavior ?? ScrollConfiguration.of(context).copyWith(scrollbars: false), @@ -908,7 +928,7 @@ class _PageViewState extends State { clipBehavior: widget.clipBehavior, slivers: [ SliverFillViewport( - viewportFraction: widget.controller.viewportFraction, + viewportFraction: _controller.viewportFraction, delegate: widget.childrenDelegate, padEnds: widget.padEnds, ), @@ -924,7 +944,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', widget.controller, showName: false)); + description.add(DiagnosticsProperty('controller', _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 536bbf04f1a9..07d924454134 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 6d3717a0121e..3fbe14805028 100644 --- a/packages/flutter/test/widgets/page_view_test.dart +++ b/packages/flutter/test/widgets/page_view_test.dart @@ -1303,4 +1303,77 @@ 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)); + }); + }); }