Skip to content

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Jun 6, 2022

Description

This PR fixes an exception (Build scheduled during frame) thrown when TabController.index is updated and TabController.animationDuration is set to Duration.zero.
See this comment #102600 (comment) for a reproducible code sample.

Related Issue

Fixes #102600

Tests

Adds 1 test.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 6, 2022
@HansMuller HansMuller requested a review from Piinks June 10, 2022 21:45
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix! Can you help me understand a bit better? Thanks!

@bleroux bleroux force-pushed the fix_tab_controller_build_scheduled_during_frame branch from 9c2374b to b2898c2 Compare June 30, 2022 10:54
@bleroux
Copy link
Contributor Author

bleroux commented Jun 30, 2022

Thank you for the fix! Can you help me understand a bit better? Thanks!

@Piinks
TabBar and TabBarView interactions are quite complex (a lot of controllers and listeners involved). 😅

While trying to write a detailed explanation for this PR, I found an easier fix. I put the draft explanation, I started to write, at the end of this comment for future reference.

The issue happened only when switching from one tab to a non adjacent one (using PageController.jumpToPage on a non-adjacent page triggers scroll notifications and scroll activities that produces a listeners hell 😄 ). When TabController.animationDuration is not set to Duration.zero, existing code already dealt with this by treating separately adjacent and non adjacent moves. When TabController.animationDuration is set to Duration.zero, existing code jump to selected tab without taking care if it is an adjacent or non-adjacent tab.

So instead of fixing the consequences of jumping straight away to a non adjacent page (my previous fix), this new fix relies on the same solution used by existing code (jumping in two steps).


Draft explanation for future reference.

The TabBarView manages two controllers, a PageController and a TabController.
TabBars have a reference to the same TabController and also create a ScrollController (when TabBar.isScrollable is true).

TabController also owns an AnimationController. This AnimationController has several listeners :

  • To animate the TabBar label, TabBar inserts a _TabStyle Widget which is an AnimatedContainer tied to the AnimationController owned by the TabController.
  • _TabBarViewState listens to the AnimationController and updates the PageController accordingly.

When the user selects a new tab, _TabBarState_handleTap is called, the TabController selected index is updated (in _TabController_changeIndex) and the AnimationController owned by TabController is updated too. The PageController owned by TabBarView is also updated because it listens to the AnimationController.
Updating the PageController (jumpTo) emits a ScrollNotification.
_TabBarViewState listens to the PageController scroll notifications and calls TabController.index setter which calls again in _TabController_changeIndex and a new ScrollNotification is emitted. The issue was related to the second scroll notification triggering a new ScrollActivity that started a Ticker. When this Ticker ticks, a scroll notification is again emitted and TabController AnimationController is again updated.

@bleroux bleroux requested a review from Piinks June 30, 2022 11:45
@Piinks
Copy link
Contributor

Piinks commented Jul 8, 2022

Thank you for the detailed break down! Very thorough!

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bleroux bleroux force-pushed the fix_tab_controller_build_scheduled_during_frame branch from b2898c2 to cf359f5 Compare July 11, 2022 06:20
@bleroux bleroux added autosubmit Merge PR when tree becomes green via auto submit App waiting for tree to go green labels Jul 11, 2022
@fluttergithubbot fluttergithubbot merged commit 082ad11 into flutter:master Jul 11, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 11, 2022
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
@bleroux bleroux deleted the fix_tab_controller_build_scheduled_during_frame branch September 22, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TabController]The following assertion was thrown while notifying listeners for AnimationController: Build scheduled during frame

3 participants