From 78e5d7995cdb16e0b8a3eb12bb62ff325e8c601d Mon Sep 17 00:00:00 2001 From: Paulik8 Date: Mon, 10 Feb 2025 18:39:06 +0100 Subject: [PATCH] Improved error message when PageController is not attached to PageView (#162422) Fixes https://github.com/flutter/flutter/issues/138945 - Added asserts for jumpToPage and animateToPage methods in PageController (https://github.com/flutter/flutter/issues/138945) - Covered by tests *Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.* *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- .../flutter/lib/src/widgets/page_view.dart | 14 ++ .../flutter/test/widgets/page_view_test.dart | 157 ++++++++++++++++++ 2 files changed, 171 insertions(+) diff --git a/packages/flutter/lib/src/widgets/page_view.dart b/packages/flutter/lib/src/widgets/page_view.dart index 4174748fdf..9ed9a40cfd 100644 --- a/packages/flutter/lib/src/widgets/page_view.dart +++ b/packages/flutter/lib/src/widgets/page_view.dart @@ -185,11 +185,22 @@ class PageController extends ScrollController { return position.page; } + bool _debugCheckPageControllerAttached() { + assert(positions.isNotEmpty, 'PageController is not attached to a PageView.'); + assert( + positions.length == 1, + 'Multiple PageViews are attached to ' + 'the same PageController.', + ); + return true; + } + /// Animates the controlled [PageView] from the current page to the given page. /// /// The animation lasts for the given duration and follows the given curve. /// The returned [Future] resolves when the animation completes. Future animateToPage(int page, {required Duration duration, required Curve curve}) { + assert(_debugCheckPageControllerAttached()); final _PagePosition position = this.position as _PagePosition; if (position._cachedPage != null) { position._cachedPage = page.toDouble(); @@ -213,6 +224,7 @@ class PageController extends ScrollController { /// Jumps the page position from its current value to the given value, /// without animation, and without checking if the new value is in range. void jumpToPage(int page) { + assert(_debugCheckPageControllerAttached()); final _PagePosition position = this.position as _PagePosition; if (position._cachedPage != null) { position._cachedPage = page.toDouble(); @@ -332,6 +344,7 @@ class _PagePosition extends ScrollPositionWithSingleContext implements PageMetri final int initialPage; double _pageToUseOnStartup; + // When the viewport has a zero-size, the `page` can not // be retrieved by `getPageFromPixels`, so we need to cache the page // for use when resizing the viewport to non-zero next time. @@ -362,6 +375,7 @@ class _PagePosition extends ScrollPositionWithSingleContext implements PageMetri @override double get viewportFraction => _viewportFraction; double _viewportFraction; + set viewportFraction(double value) { if (_viewportFraction == value) { return; diff --git a/packages/flutter/test/widgets/page_view_test.dart b/packages/flutter/test/widgets/page_view_test.dart index 80fdb9b5bc..9559426ef9 100644 --- a/packages/flutter/test/widgets/page_view_test.dart +++ b/packages/flutter/test/widgets/page_view_test.dart @@ -1403,6 +1403,163 @@ void main() { }); }); + group('Asserts in jumpToPage and animateToPage methods works properly', () { + Widget createPageView([PageController? controller]) { + return MaterialApp( + home: Scaffold( + body: PageView( + controller: controller, + children: [ + Container(color: Colors.red), + Container(color: Colors.green), + Container(color: Colors.blue), + ], + ), + ), + ); + } + + group('One pageController is attached to multiple PageViews', () { + Widget createMultiplePageViews(PageController controller) { + return MaterialApp( + home: Scaffold( + body: Column( + children: [ + Expanded( + child: PageView( + controller: controller, + children: [ + Container(color: Colors.red), + Container(color: Colors.green), + Container(color: Colors.blue), + ], + ), + ), + Expanded( + child: PageView( + controller: controller, + children: [ + Container(color: Colors.orange), + Container(color: Colors.purple), + Container(color: Colors.yellow), + ], + ), + ), + ], + ), + ), + ); + } + + testWidgets( + 'animateToPage assertion is working properly when pageController is attached to multiple PageViews', + (WidgetTester tester) async { + final PageController controller = PageController(); + addTearDown(controller.dispose); + await tester.pumpWidget(createMultiplePageViews(controller)); + + expect( + () => controller.animateToPage( + 2, + duration: const Duration(milliseconds: 300), + curve: Curves.ease, + ), + throwsA( + isAssertionError.having( + (AssertionError error) => error.message, + 'message', + equals( + 'Multiple PageViews are attached to ' + 'the same PageController.', + ), + ), + ), + ); + }, + ); + + testWidgets( + 'jumpToPage assertion is working properly when pageController is attached to multiple PageViews', + (WidgetTester tester) async { + final PageController controller = PageController(); + addTearDown(controller.dispose); + await tester.pumpWidget(createMultiplePageViews(controller)); + + expect( + () => controller.jumpToPage(2), + throwsA( + isAssertionError.having( + (AssertionError error) => error.message, + 'message', + equals( + 'Multiple PageViews are attached to ' + 'the same PageController.', + ), + ), + ), + ); + }, + ); + }); + + group('PageController is attached or is not attached to PageView', () { + testWidgets('Assert behavior of animateToPage works properly', (WidgetTester tester) async { + final PageController controller = PageController(); + addTearDown(controller.dispose); + + // pageController is not attached to PageView + await tester.pumpWidget(createPageView()); + expect( + () => controller.animateToPage( + 2, + duration: const Duration(milliseconds: 300), + curve: Curves.ease, + ), + throwsA( + isAssertionError.having( + (AssertionError error) => error.message, + 'message', + equals('PageController is not attached to a PageView.'), + ), + ), + ); + + // pageController is attached to PageView + await tester.pumpWidget(createPageView(controller)); + expect( + () => controller.animateToPage( + 2, + duration: const Duration(milliseconds: 300), + curve: Curves.ease, + ), + returnsNormally, + ); + }); + + testWidgets('Assert behavior of jumpToPage works properly', (WidgetTester tester) async { + final PageController controller = PageController(); + addTearDown(controller.dispose); + + // pageController is not attached to PageView + await tester.pumpWidget(createPageView()); + expect( + () => controller.jumpToPage(2), + throwsA( + isAssertionError.having( + (AssertionError error) => error.message, + 'message', + equals('PageController is not attached to a PageView.'), + ), + ), + ); + + // pageController is attached to PageView + await tester.pumpWidget(createPageView(controller)); + expect(() => controller.jumpToPage(2), returnsNormally); + }); + }); + }); + testWidgets( 'Get the page value before the content dimension is determined,do not throw an assertion and return null', (WidgetTester tester) async {