_TabBarViewState should not recreate page controller #135500

bleroux · 2023-09-26T13:00:26Z

Description

This PR replaces the unconditional instantiation of PageController in _TabBarViewState.didChangeDependencies as suggested in #134091 (comment).

Related Issue

Fixes #134253.

Tests

Adds 1 test.

chunhtai · 2023-09-26T20:51:53Z

packages/flutter/lib/src/material/tabs.dart

-    // TODO(chunhtai): https://github.com/flutter/flutter/issues/134253
-    _pageController?.dispose();
-    _pageController = PageController(
+    _pageController ??= PageController(


you will need to update current page index. probably through a jumpTo

I think you also need to recreate PageController if viewportFraction changes, but I think it should be done in didUpdateWidget

you will need to update current page index. probably through a jumpTo

This is something I considered. I checked that, even without this PR, when the DefaultTabController is updated with a one with a different initialIndex this has a no impact on the current index.

Maybe I made a bad assumption, from my understanding didChangeDependencies will only be called when the TabController is provided via a DefaultTabController (because of the call to DefaultTabController.maybeOf(context);in _TabBarViewState._updateTabController. If this true, the only way to trigger didChangeDependencies would be similar to the one in the test I added, and there is no possibility to set a different index in this case (the current index is stored in the TabController stored in _DefaultTabControllerState).

I will do more investigation, I'm very interested to have your view on the above assumption.

I think you also need to recreate PageController if viewportFraction changes, but I think it should be done in didUpdateWidget.

Yes, I noticed this and I planned to file another issue to give more context and a PR to fix it.

I added a call to jumpToPage when the page controller is reused.

chunhtai

LGTM, but the test should also check for index

chunhtai · 2023-09-29T20:44:21Z

packages/flutter/test/material/tabs_test.dart

+    pageView = tester.widget(find.byType(PageView));
+    final PageController pageController2 = pageView.controller;
+
+    expect(pageController1, equals(pageController2));


also check whether the index is correct?

@chunhtai I consider it but I found it confusing to add this check in the test because, with or without the PR, the index will not change (DefaultTabCotnroller.initialIndex is read once when initializing the PageView).
I don't know if this is the correct behavior?

I thought the jumpTo will make it jump to whatever less rebuild index which is 14. This just to test the jumpTo is there

I thought the jumpTo will make it jump to whatever less rebuild index which is 14

It won't because changing the DefaultTabController.initialPage has no impact on the page view current page index. Technically, it is because _DefaultTabControllerState.didUpdateWidget does not check if initialPage was changed. I would say it makes sense because changing the initialIndex does not mean changing the current index, but I don't have a strong opinion there.

For the moment, I have not find a case were the jumpTo added in this PR would be necessary (but it would if we decided that _DefaultTabControllerState.didUpdateWidget should update the selected page when DefaultTabController.initialPage is changed.

ah yes. in this case, can you shorten the length to force it to update the index in controller?

for example, if current index is 10, you rebuild DefaultTabController with length 5, it should force the index to be 4

That's exactly what I tried this morning 😄 .
In this case the index changes so I thought it will be ok to update the test accordingly... until I checked what happened if I remove the call to jumpTo: the index still change in this case because it is updated earlier (because of the logic in _DefaultTabControllerState.didUpdateWidget and because the viewport dimension change lead to a call to _PagePosition.forcePixels which correctly updates the scroll position on which the page computation is based).

You meant viewportFraction change? I can't figure out how the method is called in the update.

Either way, I think I will still put the check in this test. Even though it pass without the jumpTo, as long as it is checking correct behavior, it is worth adding there to prevent future regressions.

I totaly agree that it is worth adding this to prevent future regressions. I have updated the PR accordingly.
Many thanks for sharing your thoughts.

You meant viewportFraction change?

No, I meant when the length of the TabController is shortened, it has an impact on the page view content dimensions and the logic in _PagePosition.applyContentDimensions will take care of updating the page controller position (I was wrong mentionning forcePixels it is correctPixels which is called).

For posterity 😄 , the way to get deeper on this is to run the test in this PR. Then comment the call to jumpTo added in this PR and see that the test still pass. To understand why put a break point in ScrollPosition.correctPixels and re-run the test. The call stack will be the following one:

ah this totally makes sense now, thanks for explanation.

auto-submit · 2023-10-04T22:44:50Z

auto label is removed for flutter/flutter/135500, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

chunhtai · 2023-10-05T17:00:44Z

I can't really tell what may be wrong with the internal test. I need to investigate further to figure out.

chunhtai · 2023-12-19T23:54:06Z

@bleroux can you update the branch to retrigger the ci. It looks like the google test was erased

bleroux · 2023-12-20T11:11:09Z

@bleroux can you update the branch to retrigger the ci. It looks like the google test was erased

@chunhtai Great! After rebasing everything is green 🎉
I will add the auto-submit label tomorrow (waiting a little in case you want to double check).

…/flutter#135500)

flutter/flutter@da0cd69...11def8e 2023-12-21 mit@google.com Update README.md (flutter/flutter#140382) 2023-12-21 flar@google.com Revert "Integrate testWidgets with leak tracking. (#138057)" (flutter/flutter#140502) 2023-12-21 polinach@google.com Integrate testWidgets with leak tracking. (flutter/flutter#138057) 2023-12-21 36861262+QuncCccccc@users.noreply.github.com Fix import pattern (flutter/flutter#140425) 2023-12-20 godofredoc@google.com Update job permissions (flutter/flutter#140476) 2023-12-20 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#140472) 2023-12-20 goderbauer@google.com Remove outdated ignores from tool (flutter/flutter#140467) 2023-12-20 goderbauer@google.com Remove outdated ignores from framework (flutter/flutter#140465) 2023-12-20 31859944+LongCatIsLooong@users.noreply.github.com Reland `find.textRange.ofSubstring` changes (flutter/flutter#140469) 2023-12-20 reidbaker@google.com Part 1/n migration steps for kotlin migration (flutter/flutter#140452) 2023-12-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make `TextSpan` hit testing precise." (flutter/flutter#140468) 2023-12-20 1961493+harryterkelsen@users.noreply.github.com [web] Re-enable test now that source of flakiness is fixed (flutter/flutter#140462) 2023-12-20 chris@bracken.jp Eliminate Cirrus build status badge (flutter/flutter#140461) 2023-12-20 zanderso@users.noreply.github.com Move tests shifted to Pixel 7 from staging to prod (flutter/flutter#140438) 2023-12-20 engine-flutter-autoroll@skia.org Roll Packages from be52ac8 to dc5b267 (5 revisions) (flutter/flutter#140450) 2023-12-20 leroux_bruno@yahoo.fr _TabBarViewState should not recreate page controller (flutter/flutter#135500) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

## Description This PR replaces the unconditional instantiation of `PageController` in `_TabBarViewState.didChangeDependencies` as suggested in flutter#134091 (comment). ## Related Issue Fixes flutter#134253. ## Tests Adds 1 test.

…/flutter#135500)

flutter/flutter@da0cd69...11def8e 2023-12-21 mit@google.com Update README.md (flutter/flutter#140382) 2023-12-21 flar@google.com Revert "Integrate testWidgets with leak tracking. (#138057)" (flutter/flutter#140502) 2023-12-21 polinach@google.com Integrate testWidgets with leak tracking. (flutter/flutter#138057) 2023-12-21 36861262+QuncCccccc@users.noreply.github.com Fix import pattern (flutter/flutter#140425) 2023-12-20 godofredoc@google.com Update job permissions (flutter/flutter#140476) 2023-12-20 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#140472) 2023-12-20 goderbauer@google.com Remove outdated ignores from tool (flutter/flutter#140467) 2023-12-20 goderbauer@google.com Remove outdated ignores from framework (flutter/flutter#140465) 2023-12-20 31859944+LongCatIsLooong@users.noreply.github.com Reland `find.textRange.ofSubstring` changes (flutter/flutter#140469) 2023-12-20 reidbaker@google.com Part 1/n migration steps for kotlin migration (flutter/flutter#140452) 2023-12-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make `TextSpan` hit testing precise." (flutter/flutter#140468) 2023-12-20 1961493+harryterkelsen@users.noreply.github.com [web] Re-enable test now that source of flakiness is fixed (flutter/flutter#140462) 2023-12-20 chris@bracken.jp Eliminate Cirrus build status badge (flutter/flutter#140461) 2023-12-20 zanderso@users.noreply.github.com Move tests shifted to Pixel 7 from staging to prod (flutter/flutter#140438) 2023-12-20 engine-flutter-autoroll@skia.org Roll Packages from be52ac8 to dc5b267 (5 revisions) (flutter/flutter#140450) 2023-12-20 leroux_bruno@yahoo.fr _TabBarViewState should not recreate page controller (flutter/flutter#135500) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Sep 26, 2023

bleroux force-pushed the TabBarViewState_should_not_recreate_page_controller branch from 75f3b9d to 9cb5c6d Compare September 26, 2023 13:00

bleroux requested a review from chunhtai September 26, 2023 20:37

chunhtai reviewed Sep 26, 2023

View reviewed changes

bleroux force-pushed the TabBarViewState_should_not_recreate_page_controller branch 2 times, most recently from e32af61 to d6f2e64 Compare September 27, 2023 11:53

bleroux requested a review from chunhtai September 27, 2023 12:56

chunhtai approved these changes Sep 29, 2023

View reviewed changes

bleroux force-pushed the TabBarViewState_should_not_recreate_page_controller branch from d6f2e64 to f744379 Compare October 4, 2023 18:04

bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 4, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 4, 2023

bleroux force-pushed the TabBarViewState_should_not_recreate_page_controller branch 4 times, most recently from 9ca473d to 31fb55e Compare October 5, 2023 11:10

bleroux force-pushed the TabBarViewState_should_not_recreate_page_controller branch from 31fb55e to 13bc7ab Compare October 9, 2023 06:48

bleroux added 3 commits December 20, 2023 09:56

_TabBarViewState should not recreate page controller

becb1dc

Add a call to jumpToPage

dfccd0d

Update test to check tab and page controllers states

bbfbf4b

bleroux force-pushed the TabBarViewState_should_not_recreate_page_controller branch from 13bc7ab to bbfbf4b Compare December 20, 2023 08:56

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023

auto-submit bot merged commit 0d90014 into flutter:master Dec 20, 2023
67 checks passed

bleroux deleted the TabBarViewState_should_not_recreate_page_controller branch December 21, 2023 06:44

engine-flutter-autoroll mentioned this pull request Dec 21, 2023

Roll Flutter from da0cd69659c5 to 11def8eff455 (16 revisions) flutter/packages#5732

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 21, 2023

0d90014 _TabBarViewState should not recreate page controller (flutter…

82df379

…/flutter#135500)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024

0d90014 _TabBarViewState should not recreate page controller (flutter…

8a948af

…/flutter#135500)

SuicaLondon mentioned this pull request Aug 5, 2024

Add guard statement for TabBarView for the error that caused by initialRoute #152861

Closed

9 tasks

Piinks mentioned this pull request Aug 6, 2024

[WIP] Fix tab crash when index changes before layout #152947

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

_TabBarViewState should not recreate page controller #135500

_TabBarViewState should not recreate page controller #135500

bleroux commented Sep 26, 2023

chunhtai Sep 26, 2023

bleroux Sep 26, 2023

bleroux Sep 27, 2023

chunhtai left a comment

chunhtai Sep 29, 2023

bleroux Oct 3, 2023

chunhtai Oct 3, 2023

bleroux Oct 4, 2023

chunhtai Oct 4, 2023

bleroux Oct 4, 2023 •

edited

Loading

chunhtai Oct 4, 2023

bleroux Oct 4, 2023 •

edited

Loading

chunhtai Oct 4, 2023

auto-submit bot commented Oct 4, 2023

chunhtai commented Oct 5, 2023

chunhtai commented Dec 19, 2023

bleroux commented Dec 20, 2023

_TabBarViewState should not recreate page controller #135500

_TabBarViewState should not recreate page controller #135500

Conversation

bleroux commented Sep 26, 2023

Description

Related Issue

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunhtai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleroux Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleroux Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auto-submit bot commented Oct 4, 2023

chunhtai commented Oct 5, 2023

chunhtai commented Dec 19, 2023

bleroux commented Dec 20, 2023

bleroux Oct 4, 2023 •

edited

Loading

bleroux Oct 4, 2023 •

edited

Loading