Allow TabBar to receive a TabBarScrollController #180389
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request refactors the TabBar widget to allow external control over its scrolling behavior. The internal _TabBarScrollController has been made public as TabBarScrollController, and a new scrollController property was added to the TabBar widget. The _TabBarState now uses an _effectiveScrollController getter, which prioritizes the externally provided controller or falls back to an internally managed one. All scroll-related methods and the SingleChildScrollView now utilize this _effectiveScrollController, and null safety checks have been added to accommodate the optional nature of the external controller. The TabBarScrollController also includes a new _attachToTabBar method to link it with the _TabBarState.
| /// will be used. | ||
| final TabController? controller; | ||
|
|
||
| /// The [ScrollController] for this [TabBar]. |
There was a problem hiding this comment.
Should this be explicitly TabBarScrollController ?
| _updateTabController(); | ||
| _initIndicatorPainter(); | ||
|
|
||
| final TabBarScrollController? scrollController = _effectiveScrollController; |
There was a problem hiding this comment.
I'm not sure if other things are required here.
| double _tabCenteredScrollOffset(int index) { | ||
| final ScrollPosition position = _scrollController!.position; | ||
| double? _tabCenteredScrollOffset(int index) { | ||
| final ScrollPosition? position = _effectiveScrollController?.position; |
There was a problem hiding this comment.
I was trying to clean up the ! around the internal scroll controller, so I added returns in some places. (We could also check hasClients etc, but I don't think that adds any value here?)
| ? _tabCenteredScrollOffset(_currentIndex! - 1) | ||
| : null; | ||
| final double middlePosition = _tabCenteredScrollOffset(_currentIndex!); | ||
| final double? middlePosition = _tabCenteredScrollOffset(_currentIndex!); |
There was a problem hiding this comment.
Since everything relies on the middle position, I added a return here
|
@huycozy I didn't specifically add an example in this PR, since this technically defers to how a ScrollBar widget works. (which in itself explains that it needs the same scroll controller to be considered a scrollbar for a scroll view) |
| _tabBar = tabBarState; | ||
| } | ||
|
|
||
| _TabBarState get _tabBarState { |
There was a problem hiding this comment.
Since a lint was complaining about using a setter + a missing getter once I did that, I added an assert in the getter, just in case.
huycozy
left a comment
There was a problem hiding this comment.
Thank you for submitting a separate PR! Left a comment for tests.
These approaches seem simpler than mine. However, I haven't yet thought of any other scenarios to test the risk of changing _TabBarScrollController to public. Let's see comments from others!
The only real difference between So I think that making it public, with the class modifier included, is the right thing to do. |
|
Since there are 2 proposals here, I recommend we centralize discussion here: #123965 (comment) 💙 |
This pull request makes
TabBarScrollControllerpublic, and allows aTabBarwidget to receive one.Fixes #123965
Fixes #124608
Related to #180120
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
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.