flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
165.02k stars 27.2k forks source link

PageView controller's jumpToPage doesn't work properly on Android when jumping to a page after a new child was added, since 1.19.0-4.1.pre #62209

Closed dannyvalentesonos closed 4 years ago

dannyvalentesonos commented 4 years ago

When using a PageView, if the developer wants to keep the user on the "same" page when the children change, so that the user's experience doesn't get affected, it will not work on Android as of 1.19.0-4.1.pre, if the user is on the last page of the PageView.

So imagine there are 4 pages, and the user is on Page 4. If a new page is added somewhere before Page 4, jumping to Page 5 to keep the user's experience unaffected no longer works on Android only as of 1.19.0-4.1.pre (from the beta channel). This does still work on iOS (using 1.19.0-4.1.pre and even the current 1.20 builds), and it also worked properly on Android on 1.19.0-4.0.pre (from the dev channel).

The trick I use is to call jumpToPage on the pageController right from the didUpdateWidget method, so that the next frame that is built correctly shows the same page as if nothing changed.

The following code reproduces this issue on Android, when building with Flutter 1.19.0-4.1.pre or later, and still works well on iOS. If you run this with flutter 1.19.0-4.0.pre on Android, it works as well.

Simply tap "Add" and you'll see the same page remains, but you can see that if you scroll, new pages can be found. If you go all the way to the last page (100), and press Add, you'll see that the PageView scrolls.

Another issue also in build 1.19.0-4.1.pre and later (only on Android as well) is that if you stay on the last page and Tap remove until all pages have been removed except for page 100, then trying to scroll does nothing (which is expected). Now add a new page, and notice you can no longer scroll, until you add yet another page again. This also does not happen on iOS.

Expand to see code ``` import 'package:flutter/material.dart'; import 'package:flutter/rendering.dart'; import 'dart:math' as Math; void main() { runApp(MyApp()); } class MyApp extends StatelessWidget { // This widget is the root of your application. @override Widget build(BuildContext context) { return MaterialApp( title: 'Flutter Demo', theme: ThemeData( primarySwatch: Colors.blue, ), home: PageViewHomePage(), ); } } class PageViewHomePage extends StatefulWidget { const PageViewHomePage(); @override _PageViewHomePageState createState() => _PageViewHomePageState(); } class _PageViewHomePageState extends State { int _nextPageNum = 1; Math.Random _rand = Math.Random(); List _pages = List(); @override void initState() { super.initState(); for (int i=0; i<5; i++) { _pages.add(CarouselPage( key: ValueKey("$_nextPageNum"), color: Color.fromARGB( 255, (255 * _rand.nextDouble()).toInt(), (255 * _rand.nextDouble()).toInt(), (255 * _rand.nextDouble()).toInt() ), number: _nextPageNum++ ) ); } _pages.add(CarouselPage(color: Colors.white, number: 100)); } @override void dispose() { super.dispose(); } @override Widget build(BuildContext context) { return Scaffold( body: Column( children: [ Expanded(child: Carousel( pages: _pages )), Row( children: [ Expanded(child: Container()), FlatButton( child: Text("Add"), onPressed: () { int index = _pages.length == 1 ? 0 : _rand.nextInt(_pages.length - 1); _pages.insert(index, CarouselPage( key: ValueKey("$_nextPageNum"), color: Color.fromARGB( 255, (255 * _rand.nextDouble()).toInt(), (255 * _rand.nextDouble()).toInt(), (255 * _rand.nextDouble()).toInt() ), number: _nextPageNum++ )); setState(() { }); }, ), Text(" | "), FlatButton( child: Text("Remove"), onPressed: () { if (_pages.length > 1) { _pages.removeAt(0); setState(() { }); } }, ), Expanded(child: Container()) ], ) ], ), ); } } class CarouselPage extends StatelessWidget { const CarouselPage({ this.color, this.number, Key key }) : super(key: key); final Color color; final int number; @override Widget build(BuildContext context) { return Container( color: color, child: Center( child: Text("$number") ) ); } } class Carousel extends StatefulWidget { const Carousel({ Key key, this.pages }) : super(key: key); final List pages; @override _CarouselState createState() => _CarouselState(); } class _CarouselState extends State { // page variables PageController _pageController; int _currentPage = 0; // controls updates outside of user interaction List _pages; List _queuedPages; bool _scrolling = false; bool _pagesChanged = false; bool _pageChangeQueued = false; bool _handlingPagesChange = false; bool _jumpingToPage = false; @override void initState() { super.initState(); _pages = List.from(widget.pages); _pageController = PageController( initialPage: 0, keepPage: false ); } @override void didUpdateWidget(Carousel oldWidget) { super.didUpdateWidget(oldWidget); // we are still updating the pages from a previous request, // so we can just queue this request and it will be // processed when the previous request is complete if (_pagesChanged) { _pageChangeQueued = true; return; } _pagesChanged = true; // handle the changes _handlePagesChanged(); } @override void dispose() { _pageController.dispose(); super.dispose(); } static const double scrollBoundaryEpsilon = 0.001; bool _isOnPageBoundary(int page) { if (!_pageController.hasClients) { // the page view hasn't been built yet, so we should just return true return true; } double currentPage = _pageController.page; return currentPage >= page.toDouble() - scrollBoundaryEpsilon && currentPage <= page.toDouble() + scrollBoundaryEpsilon; } bool _onScrollNotification(ScrollNotification notification) { if (notification is ScrollStartNotification) { _scrolling = true; // only setState if _handlingPagesChange is false // otherwise, we are already potentially calling setState() // This setState() is needed because the _scrolling variable is changing // which controls our IgnorePointer widget if (!_handlingPagesChange) { setState(() {}); } } else if (notification is ScrollUpdateNotification) { // calculate the current page int page = _pageController.page.round(); // check if the page has changed if (!_jumpingToPage) { if (_currentPage != page) { print("Carousel page changed: $page"); _onPageChanged(page); } } } else if (notification is ScrollEndNotification) { // we always only evaluate when the page // value is on a true page boundary if (_isOnPageBoundary(_pageController.page.round())) { _scrolling = false; // only setState if _handlingPagesChange is false // otherwise, we are already potentially calling setState() // This setState() is needed because the _scrolling variable is changing // which controls our IgnorePointer widget if (!_handlingPagesChange) { setState(() {}); } if (_pagesChanged) { _handlePagesChanged(); } } } // this event doesn't need to bubble up the tree return true; } void _onPageChanged(int newPage) { _currentPage = newPage; } void _handlePagesChanged() { // this should technically never happen, but just in case // a scroll ends and we come back in here, let's ignore. if (_handlingPagesChange) { return; } // let's determine if the current selected page is still present // if so, let's determine which page it is in the new list // while we're at it, let's also determine if the pages are identical // if they are, we'll allow an update even if we're currently scrolling int oldPage = -1; int newPage = -1; bool pagesAreIdentical = widget.pages.length == _pages.length; if (_pages[_currentPage].number >= 0) { oldPage = _currentPage; for (int i = 0; i < widget.pages.length; i++) { if (widget.pages[i].number == _pages[oldPage].number) { newPage = i; } if (pagesAreIdentical) { if (_pages[i].number != widget.pages[i].number) { pagesAreIdentical = false; } } } } if (!pagesAreIdentical && _scrolling) { // let's handle this only once the scroll completes return; } // if the new page is the same as the old, // just update the pages immediately if (newPage == oldPage) { // no setState() needed here because all paths that call this method (_handlePagesChanged) // already have caused a build phase to be scheduled, and seeing nothing extra is needed, // it will build properly _pages = List.from(widget.pages); _pagesChanged = false; _onPageChanged(_currentPage); } else { // otherwise, let's wait until the end of the frame and then update the pages // at the same time, jump to the new page _handlingPagesChange = true; _jumpingToPage = true; // save the widget.pages list now, as it could change before the next frame completes _queuedPages = List.from(widget.pages); WidgetsBinding.instance.addPostFrameCallback((_) { if (mounted) { setState(() { _pages = _queuedPages; _queuedPages = null; // at this point, the page controller will accept the new page, but if it's // an index beyond the range of the current list, the widget will think the // current page is the last page index in the list because the scroll dimensions // haven't been updated yet. // this will happen during the jumpToPage call below // so let's set the actual current page properly ourselves // We set the current page to 0 if newPage is -1, because the widget // will automatically scroll to the first page for us. print("carousel jumping to page: $newPage"); _onPageChanged(newPage >= 0 ? newPage : 0); // jump to the new page seamlessly _pageController.jumpToPage(newPage); WidgetsBinding.instance.addPostFrameCallback((_) { if (mounted) { setState(() { _jumpingToPage = false; _onPagesChangedComplete(); }); } }); }); } }); } } void _onPagesChangedComplete() { _handlingPagesChange = false; if (_pageChangeQueued) { _pageChangeQueued = false; _handlePagesChanged(); } else { _pagesChanged = false; } } @override Widget build(BuildContext context) { Widget carousel = ClipRRect( borderRadius: BorderRadius.vertical(top: Radius.circular(16)), child: NotificationListener( onNotification: _onScrollNotification, child: PageView.builder( controller: _pageController, itemCount: _pages.length, itemBuilder: (context, index) { // when scrolling, let's prevent any interactions with the page contents return IgnorePointer( ignoring: _scrolling, child: _pages[index] ); } ), ) ); // when the pages are changing, prevent any further scrolling until done return AbsorbPointer( absorbing: _pagesChanged && !_scrolling, child: carousel ); } } ```
dannyvalentesonos commented 4 years ago

Flutter doctor on 1.19.0-4.1.pre

985aeb8ceee6:all danny.valente$ flutter doctor -v [✓] Flutter (Channel beta, 1.19.0-4.1.pre, on Mac OS X 10.15.5 19F101, locale en-US) • Flutter version 1.19.0-4.1.pre at /Users/danny.valente/Library/flutter/1.19.0-4.1.pre-beta • Framework revision f994b76974 (6 weeks ago), 2020-06-09 15:53:13 -0700 • Engine revision 9a28c3bcf4 • Dart version 2.9.0 (build 2.9.0-14.1.beta)

[✓] Android toolchain - develop for Android devices (Android SDK version 29.0.2) • Android SDK at /Users/danny.valente/Library/Android/sdk • Platform android-29, build-tools 29.0.2 • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593) • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 11.4.1) • Xcode at /Applications/Xcode.app/Contents/Developer • Xcode 11.4.1, Build version 11E503a • CocoaPods version 1.9.3

[✓] Android Studio (version 4.0) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin version 47.1.2 • Dart plugin version 193.7361 • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593)

TahaTesser commented 4 years ago

Hi @dannyvalentesonos I see there's an open issue addressing the case you described. https://github.com/flutter/flutter/issues/58959 If you have any details, please add them there Please follow up on that issue, I'm closing the current one as a duplicate. If you disagree, please write in the comments and I will reopen it. Thank you

dannyvalentesonos commented 4 years ago

@TahaTesser I'd prefer we reopen this issue. I have code (that is attached in the description) that worked perfectly to work around the Feature Request which #58959 is about. That code still works perfectly on iOS, however something changed between Flutter 1.19.0-4.0.pre and 1.19.0-4.1.pre that broke this specifically on Android, and this is a rather very big problem for us, and we're about to ship our app.

If you run that code, you'll see that it works on iOS and doesn't work on Android. However, is you use 1.19.0-4.0.pre, you'll see it works.

Thanks.

TahaTesser commented 4 years ago

Hi @dannyvalentesonos Just reopened the issue, it's not clear based on description on how to reproduce the issue Can you please exact provide steps and actual results vs expected results? Thank you

dannyvalentesonos commented 4 years ago

1st issue:

  1. Launch the app.
  2. Tap "Add" and you'll see the same page remains.
  3. Scroll to the last page (100).
  4. Tap "Add" and you'll see that the PageView scrolls on Android, but remains on page 100 on iOS. Also, it remains on 100 on Android with Flutter 1.19.0-4.0.pre.

The expected result is to remain on page 100 after tapping Add.

Second issue (which might just be a consequence of the same problem):

  1. Launch the app.
  2. Scroll to the last page (100).
  3. Tap Remove until there is only Page 100 left in the PageView (you won't be able to remove page 100). You can see this from the logs, once it says that we're on page 0, or by simply swiping to the left to verify no other pages exist.
  4. Tap "Add" and on Android, it will scroll to the new page, but you can no longer scroll the PageView. If you tap "Add" again, scrolling will work again.

The expected result is to still be able to scroll once you've removed all pages except for page 100, and add a new page. Again, this works on iOS, and on Android when running with Flutter 1.19.0-4.0.pre.

jason-simmons commented 4 years ago

This is another side effect of the correctForNewDimensions change introduced in https://github.com/flutter/flutter/commit/98bc1766273fcdcf02f22f0bb193266b8c50145d

See https://github.com/flutter/flutter/issues/60288 and https://github.com/flutter/flutter/issues/61156

chunhtai commented 4 years ago

Seems like @Hixie is assigned on the other issue. I will reassign this to @Hixie as well. Feel free to reassign back if you don't have time.

TahaTesser commented 4 years ago

i can reproduce the issue based on https://github.com/flutter/flutter/issues/62209#issuecomment-664390879 steps on beta and master, works on stable

Code Sample ``` import 'package:flutter/material.dart'; import 'package:flutter/rendering.dart'; import 'dart:math' as Math; void main() { runApp(MyApp()); } class MyApp extends StatelessWidget { // This widget is the root of your application. @override Widget build(BuildContext context) { return MaterialApp( title: 'Flutter Demo', theme: ThemeData( primarySwatch: Colors.blue, ), home: PageViewHomePage(), ); } } class PageViewHomePage extends StatefulWidget { const PageViewHomePage(); @override _PageViewHomePageState createState() => _PageViewHomePageState(); } class _PageViewHomePageState extends State { int _nextPageNum = 1; Math.Random _rand = Math.Random(); List _pages = List(); @override void initState() { super.initState(); for (int i = 0; i < 5; i++) { _pages.add(CarouselPage( key: ValueKey("$_nextPageNum"), color: Color.fromARGB( 255, (255 * _rand.nextDouble()).toInt(), (255 * _rand.nextDouble()).toInt(), (255 * _rand.nextDouble()).toInt()), number: _nextPageNum++)); } _pages.add(CarouselPage(color: Colors.white, number: 100)); } @override void dispose() { super.dispose(); } @override Widget build(BuildContext context) { return Scaffold( body: Column( children: [ Expanded(child: Carousel(pages: _pages)), Row( children: [ Expanded(child: Container()), FlatButton( child: Text("Add"), onPressed: () { int index = _pages.length == 1 ? 0 : _rand.nextInt(_pages.length - 1); _pages.insert( index, CarouselPage( key: ValueKey("$_nextPageNum"), color: Color.fromARGB( 255, (255 * _rand.nextDouble()).toInt(), (255 * _rand.nextDouble()).toInt(), (255 * _rand.nextDouble()).toInt()), number: _nextPageNum++)); setState(() {}); }, ), Text(" | "), FlatButton( child: Text("Remove"), onPressed: () { if (_pages.length > 1) { _pages.removeAt(0); setState(() {}); } }, ), Expanded(child: Container()) ], ) ], ), ); } } class CarouselPage extends StatelessWidget { const CarouselPage({this.color, this.number, Key key}) : super(key: key); final Color color; final int number; @override Widget build(BuildContext context) { return Container(color: color, child: Center(child: Text("$number"))); } } class Carousel extends StatefulWidget { const Carousel({Key key, this.pages}) : super(key: key); final List pages; @override _CarouselState createState() => _CarouselState(); } class _CarouselState extends State { // page variables PageController _pageController; int _currentPage = 0; // controls updates outside of user interaction List _pages; List _queuedPages; bool _scrolling = false; bool _pagesChanged = false; bool _pageChangeQueued = false; bool _handlingPagesChange = false; bool _jumpingToPage = false; @override void initState() { super.initState(); _pages = List.from(widget.pages); _pageController = PageController(initialPage: 0, keepPage: false); } @override void didUpdateWidget(Carousel oldWidget) { super.didUpdateWidget(oldWidget); // we are still updating the pages from a previous request, // so we can just queue this request and it will be // processed when the previous request is complete if (_pagesChanged) { _pageChangeQueued = true; return; } _pagesChanged = true; // handle the changes _handlePagesChanged(); } @override void dispose() { _pageController.dispose(); super.dispose(); } static const double scrollBoundaryEpsilon = 0.001; bool _isOnPageBoundary(int page) { if (!_pageController.hasClients) { // the page view hasn't been built yet, so we should just return true return true; } double currentPage = _pageController.page; return currentPage >= page.toDouble() - scrollBoundaryEpsilon && currentPage <= page.toDouble() + scrollBoundaryEpsilon; } bool _onScrollNotification(ScrollNotification notification) { if (notification is ScrollStartNotification) { _scrolling = true; // only setState if _handlingPagesChange is false // otherwise, we are already potentially calling setState() // This setState() is needed because the _scrolling variable is changing // which controls our IgnorePointer widget if (!_handlingPagesChange) { setState(() {}); } } else if (notification is ScrollUpdateNotification) { // calculate the current page int page = _pageController.page.round(); // check if the page has changed if (!_jumpingToPage) { if (_currentPage != page) { print("Carousel page changed: $page"); _onPageChanged(page); } } } else if (notification is ScrollEndNotification) { // we always only evaluate when the page // value is on a true page boundary if (_isOnPageBoundary(_pageController.page.round())) { _scrolling = false; // only setState if _handlingPagesChange is false // otherwise, we are already potentially calling setState() // This setState() is needed because the _scrolling variable is changing // which controls our IgnorePointer widget if (!_handlingPagesChange) { setState(() {}); } if (_pagesChanged) { _handlePagesChanged(); } } } // this event doesn't need to bubble up the tree return true; } void _onPageChanged(int newPage) { _currentPage = newPage; } void _handlePagesChanged() { // this should technically never happen, but just in case // a scroll ends and we come back in here, let's ignore. if (_handlingPagesChange) { return; } // let's determine if the current selected page is still present // if so, let's determine which page it is in the new list // while we're at it, let's also determine if the pages are identical // if they are, we'll allow an update even if we're currently scrolling int oldPage = -1; int newPage = -1; bool pagesAreIdentical = widget.pages.length == _pages.length; if (_pages[_currentPage].number >= 0) { oldPage = _currentPage; for (int i = 0; i < widget.pages.length; i++) { if (widget.pages[i].number == _pages[oldPage].number) { newPage = i; } if (pagesAreIdentical) { if (_pages[i].number != widget.pages[i].number) { pagesAreIdentical = false; } } } } if (!pagesAreIdentical && _scrolling) { // let's handle this only once the scroll completes return; } // if the new page is the same as the old, // just update the pages immediately if (newPage == oldPage) { // no setState() needed here because all paths that call this method (_handlePagesChanged) // already have caused a build phase to be scheduled, and seeing nothing extra is needed, // it will build properly _pages = List.from(widget.pages); _pagesChanged = false; _onPageChanged(_currentPage); } else { // otherwise, let's wait until the end of the frame and then update the pages // at the same time, jump to the new page _handlingPagesChange = true; _jumpingToPage = true; // save the widget.pages list now, as it could change before the next frame completes _queuedPages = List.from(widget.pages); WidgetsBinding.instance.addPostFrameCallback((_) { if (mounted) { setState(() { _pages = _queuedPages; _queuedPages = null; // at this point, the page controller will accept the new page, but if it's // an index beyond the range of the current list, the widget will think the // current page is the last page index in the list because the scroll dimensions // haven't been updated yet. // this will happen during the jumpToPage call below // so let's set the actual current page properly ourselves // We set the current page to 0 if newPage is -1, because the widget // will automatically scroll to the first page for us. print("carousel jumping to page: $newPage"); _onPageChanged(newPage >= 0 ? newPage : 0); // jump to the new page seamlessly _pageController.jumpToPage(newPage); WidgetsBinding.instance.addPostFrameCallback((_) { if (mounted) { setState(() { _jumpingToPage = false; _onPagesChangedComplete(); }); } }); }); } }); } } void _onPagesChangedComplete() { _handlingPagesChange = false; if (_pageChangeQueued) { _pageChangeQueued = false; _handlePagesChanged(); } else { _pagesChanged = false; } } @override Widget build(BuildContext context) { Widget carousel = ClipRRect( borderRadius: BorderRadius.vertical(top: Radius.circular(16)), child: NotificationListener( onNotification: _onScrollNotification, child: PageView.builder( controller: _pageController, itemCount: _pages.length, itemBuilder: (context, index) { // when scrolling, let's prevent any interactions with the page contents return IgnorePointer( ignoring: _scrolling, child: _pages[index]); }), )); // when the pages are changing, prevent any further scrolling until done return AbsorbPointer( absorbing: _pagesChanged && !_scrolling, child: carousel); } } ```
flutter doctor -v ``` [✓] Flutter (Channel beta, 1.20.0-7.2.pre, on Mac OS X 10.15.6 19G73, locale en-GB) • Flutter version 1.20.0-7.2.pre at /Users/tahatesser/Code/flutter_beta • Framework revision a2bde82fbd (9 days ago), 2020-07-18 15:16:35 -0700 • Engine revision 60b269d898 • Dart version 2.9.0 (build 2.9.0-21.2.beta) [✓] Android toolchain - develop for Android devices (Android SDK version 30.0.1) • Android SDK at /Users/tahatesser/Code/sdk • Platform android-30, build-tools 30.0.1 • ANDROID_HOME = /Users/tahatesser/Code/sdk • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593) • All Android licenses accepted. [✓] Xcode - develop for iOS and macOS (Xcode 11.6) • Xcode at /Applications/Xcode.app/Contents/Developer • Xcode 11.6, Build version 11E708 • CocoaPods version 1.9.3 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] Android Studio (version 4.0) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin version 47.1.2 • Dart plugin version 193.7361 • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593) [✓] VS Code (version 1.47.3) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.12.2 [✓] Connected device (3 available) • SM M305F (mobile) • 32003c30dc19668f • android-arm64 • Android 10 (API 29) • Web Server (web) • web-server • web-javascript • Flutter Tools • Chrome (web) • chrome • web-javascript • Google Chrome 84.0.4147.89 • No issues found! ```
flutter doctor -v ``` [✓] Flutter (Channel master, 1.21.0-6.0.pre.41, on Mac OS X 10.15.6 19G73, locale en-GB) • Flutter version 1.21.0-6.0.pre.41 at /Users/tahatesser/Code/flutter_master • Framework revision 39fa00201d (3 hours ago), 2020-07-27 21:11:43 -0700 • Engine revision f27729e97b • Dart version 2.10.0 (build 2.10.0-0.0.dev 24c7666def) [✓] Android toolchain - develop for Android devices (Android SDK version 30.0.1) • Android SDK at /Users/tahatesser/Code/sdk • Platform android-30, build-tools 30.0.1 • ANDROID_HOME = /Users/tahatesser/Code/sdk • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593) • All Android licenses accepted. [✓] Xcode - develop for iOS and macOS (Xcode 11.6) • Xcode at /Applications/Xcode.app/Contents/Developer • Xcode 11.6, Build version 11E708 • CocoaPods version 1.9.3 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] Android Studio (version 4.0) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin version 47.1.2 • Dart plugin version 193.7361 • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593) [✓] VS Code (version 1.47.3) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.12.2 [✓] Connected device (4 available) • SM M305F (mobile) • 32003c30dc19668f • android-arm64 • Android 10 (API 29) • macOS (desktop) • macos • darwin-x64 • Mac OS X 10.15.6 19G73 • Web Server (web) • web-server • web-javascript • Flutter Tools • Chrome (web) • chrome • web-javascript • Google Chrome 84.0.4147.89 • No issues found! ```
Hixie commented 4 years ago

I'll try to take a look this week.

Hixie commented 4 years ago

I don't really understand why this ever worked, or why it would still be working today on any platform. Fundamentally, the code is requesting that we jump to a position that isn't yet in range (it does the jump just before the frame where we add the new page to the PageView). The scroll physics scroll to the new position and immediately try to go ballistic, and that means pulling back from the new position back to the nearest in-range position, which is what happens.

Hixie commented 4 years ago

Oh, interesting. On stable, when the internal dimensions change, we go ballistic again, which essentially terminates the original ballistic scroll...

dannyvalentesonos commented 4 years ago

I don't really understand why this ever worked, or why it would still be working today on any platform. Fundamentally, the code is requesting that we jump to a position that isn't yet in range (it does the jump just before the frame where we add the new page to the PageView). The scroll physics scroll to the new position and immediately try to go ballistic, and that means pulling back from the new position back to the nearest in-range position, which is what happens.

I'm a little concerned about this comment, as this is a really important feature for us, and it seems like comments in #58959 suggest that developers should make this possible in their code, which I have. It really should be a natural use case to support to be honest. If a page is added before the current page, we should be able to "stay" on that page without disrupting the user's experience.

Oh, interesting. On stable, when the internal dimensions change, we go ballistic again, which essentially terminates the original ballistic scroll...

Does this comment mean there's hope we can get a fix for this?

Thanks very much!

Hixie commented 4 years ago

Ah, I see. This is hitting the new logic for position correction. The test code sends the page position out of range (since the new page doesn't exist yet). Then we build with the new page. When we discover that the range has changed, we try to maintain the previous relationship between the position and the range, so we move ourselves out of range again, but then when we get the new set of dimensions, they aren't different that the previous call, so we never call applyNewDimensions and never go ballistic.

Hixie commented 4 years ago

(Or rather, we only go ballistic once.)

But if we do go ballistic the second time, it'll be from the newly out-of-range position...

Hixie commented 4 years ago

It really should be a natural use case to support to be honest.

This is absolutely something we should support.

Does this comment mean there's hope we can get a fix for this?

Working on it. :-)

Hixie commented 4 years ago

So the ideal solution here would be that when one adds something to the list of contents in a list that is before the first rendered widget, either something that gets built or something that affects the known offset of the first rendered widget, the scroll position would be adjusted accordingly. Not sure how to do that...

Hixie commented 4 years ago

Another option would be to provide a separate way to scroll that is immune to the logic mentioned above, something like "scroll to this position but hold it until after the next build then go ballistic" or something. should be possible with a new ScrollActivity...

Hixie commented 4 years ago

I had to make a couple of framework changes but I created a ScrollActivity that the code can use to make this happen. Need to figure out if I think it should be in the base framework or if it's ok for someone to have to create their own.

Hixie commented 4 years ago

https://github.com/flutter/flutter/pull/62757 makes this possible in a more reliable way. It's not built into the framework. Not sure if it's good enough or if we should do more...

dannyvalentesonos commented 4 years ago

I will apply your patch and give this a try. I think it's ok to create the scroll activity, but would be nice to have this built in, but if this works, this will be fine for me.

2 quick questions:

  1. Why does this work for iOS but not Android, just out of curiosity.
  2. In your test code, why are you doing + math.max(0, _pageController.position.viewportDimension * (_pageController.viewportFraction - 1) / 2)? What exactly does this do? Seeing this is being used in the call to jumToPage, shouldn't the pixels fall on a perfect page boundary?

Thanks!

dannyvalentesonos commented 4 years ago

@Hixie So far, this is working and looking good. Thanks again for the quick fix!

Hixie commented 4 years ago

re 1, I wasn't able to reproduce that this worked for iOS. It failed for both for me. re 2, I just inlined all the code that I couldn't get to because those members are private. I haven't examined the maths to see if it's correct or not. :-)

Hixie commented 4 years ago

An argument to not add the activity itself to the framework is that it's a really narrow use case and what we should really do is find a way to make this whole thing moot (by having a way to add content to a list without moving the perceived scroll position). An argument for adding it is that I have no idea how to do that...

goderbauer commented 4 years ago

If the goal is to keep the user on the "same" page when the children change, one could also use the old center work-around with a CustomScrollView:

import 'package:flutter/material.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: WrapHomePage(),
    );
  }
}

class WrapHomePage extends StatefulWidget {
  const WrapHomePage();

  @override
  _WrapHomePageState createState() => _WrapHomePageState();
}

class _WrapHomePageState extends State<WrapHomePage> {
  List<Widget> _pages;
  List<Widget> _newPages = <Widget>[];

  @override
  void initState() {
    super.initState();
    _pages = List.generate(2, (index) {
      final Key key = ValueKey(index);
      return Container(
        key: key,
        height: 50,
        color: index % 2 == 0 ? Colors.green : Colors.yellow,
        child: Text('Tile $index'),
      );
    });
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: SizedBox(
          height: 100,
          child: CustomScrollView(
            center: ValueKey(0),  /// <-- Here...
            physics: PageScrollPhysics(),
            slivers: [
              SliverFillViewport(
                delegate: SliverChildBuilderDelegate(
                  (BuildContext context, int index) {
                    return _newPages[index];
                  },
                  childCount: _newPages.length,
                ),
              ),
              SliverFillViewport(
                key: ValueKey(0), /// <-- and here...
                delegate: SliverChildBuilderDelegate(
                  (BuildContext context, int index) {
                    return _pages[index];
                  },
                  childCount: _pages.length,
                ),
              ),
            ],
          ),
        ),
      ),
      persistentFooterButtons: [
        FloatingActionButton(
          child: Text('B'),
          onPressed: () {
            setState(() {
              _newPages.add(
                Container(
                  height: 50,
                  color: _newPages.length % 2 == 0 ? Colors.green : Colors.yellow,
                  child: Text('New Tile ${_newPages.length}'),
                ),
              );
            });
          },
        ),
        FloatingActionButton(
          child: Text('E'),
          onPressed: () {
            setState(() {
              _pages.add(
                Container(
                  height: 50,
                  color: _pages.length % 2 == 0 ? Colors.green : Colors.yellow,
                  child: Text('New Tile ${_pages.length}'),
                ),
              );
            });
          },
        ),
      ],
    );
  }
}
dannyvalentesonos commented 4 years ago

Thanks @goderbauer . This seems like a very big change for us to change our use of PageView to instead use a CustomScrollView and have to use value keys to keep track of what is showing. I prefer @Hixie 's change which allows our changes to be very minimal, and I've already confirmed that they work very well.

goderbauer commented 4 years ago

and have to use value keys to keep track of what is showing.

You don't have to keep track of what's showing. The key is statically assigned to one of the SliverFillViewport that act as one of two page views.

Hixie commented 4 years ago

I wonder if the right fix here is really just to drop the RangeMaintainingScrollPhysics from PageViews...

dannyvalentesonos commented 4 years ago

I wonder if the right fix here is really just to drop the RangeMaintainingScrollPhysics from PageViews...

@Hixie I'm not sure what that would entail, but one thing I really like is it takes care of animating should the page the user is on gets removed. At that point, it automatically scrolls and takes care of animating for us, so that the user is aware that something has changed.

Would your suggestion remove that? Basically, I call jumpToPage(-1) and it automatically scrolls to the last page nicely.

Hixie commented 4 years ago

that should still work. RangeMaintainingScrollPhysics is essentially just doing the same thing you're doing, you don't need it in your case. (It does it in a way that has a different effect than what you want, hence this bug.)

I'm looking at https://github.com/flutter/flutter/issues/60288 which the PR above didn't fix, but which is fundamentally the same problem, some code tries to fix the position of a PageView before the PageView metrics change (in their case, the viewport metrics rather than the position, bu the effect is the same), and the RangeMaintainingScrollPhysics logic keeps the scroll position in "the same position" except that it doesn't know the position was already "fixed".

Hixie commented 4 years ago

After some discussion on Discord I think we have a fix now that will let your old code work unmodified.

Hixie commented 4 years ago

Proposed fix: https://github.com/flutter/flutter/pull/63146

chakrihacker commented 4 years ago

Can we open this? as the PR is reverted

xster commented 3 years ago

For closure, the original fix was re-applied in https://github.com/flutter/flutter/pull/65135 and is in 1.22.0 and newer.

github-actions[bot] commented 3 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.