_TabBarViewState should not recreate page controller

[bleroux]

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]

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

[bleroux]

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.

[bleroux]

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

[chunhtai]

LGTM, but the test should also check for index

[chunhtai]

also check whether the index is correct?

[bleroux]

@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?

[chunhtai]

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

[bleroux]

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.

[chunhtai]

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

[bleroux]

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).

[chunhtai]

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.

[bleroux]

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:

[chunhtai]

ah this totally makes sense now, thanks for explanation.

[auto-submit]

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]

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

[chunhtai]

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

[bleroux]

@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).