Expose the Scroll Controller of a scrollable TabBar#180120
Expose the Scroll Controller of a scrollable TabBar#180120huycozy wants to merge 2 commits intoflutter:masterfrom
Conversation
Signed-off-by: huycozy <[email protected]>
Signed-off-by: huycozy <[email protected]> Signed-off-by: huycozy <[email protected]>
ffd0833 to
b99463f
Compare
| @@ -2016,6 +2054,13 @@ class _TabBarState extends State<TabBar> { | |||
| ).add(widget.padding ?? EdgeInsets.zero) | |||
| : widget.padding; | |||
| _scrollController ??= _TabBarScrollController(this); | |||
There was a problem hiding this comment.
Why don't we make _TabBarScrollController public but final?
I understand that the private _TabBarState needs to stay private, but why don't we include a private setter for it?
final class TabBarScrollController extends ScrollController {
/// The state of the attached [TabBar], if any.
///
/// Is null when not attached to a [TabBar].
_TabBarState? _tabBarState;
void _attachToTabBar(_TabBarState tabBar) {
_tabBarState = tabBar;
}
@override
ScrollPosition createScrollPosition(
ScrollPhysics physics,
ScrollContext context,
ScrollPosition? oldPosition,
) {
return _TabBarScrollPosition(
physics: physics,
context: context,
oldPosition: oldPosition,
tabBar: _tabBarState!,
);
}
}Then we only need to call the attach method using a private helper method? (which handles creating an internal controller and attaches the this)
For example:
TabBarScrollController _setUpTabBarController() {
final tabBarController = widget.tabBarController ?? TabBarScrollController();
tabBarController._attachToTabBar(this);
return tabBarController;
}
// ...
_scrollController ??= _setUpTabBarController()There was a problem hiding this comment.
Thanks for your review, @navaronbracke!
If we proceed with this approach:
- how do we initialize a
TabBarStateto pass into theTabBarScrollControllerconstructor? And we need to make TabBarState public too, for this purpose. - Even if this were feasible, I'm quite concerned about the integrity and encapsulation of TabBarState class since it's marked as a public class.
- Since marking TabBarState as public, the cost of maintenance for it (new public API) may increase, as users may encounter unexpected issues with this change in their usage?
Also, looking back the original use case from OP at #123965 reported:"Exposing the Scroll Controller of a TabBar with isScrollable: true, will let developers apply custom behavior based on the scroll.", it means they only need offset from scrollController which is sufficient enough?
With these reasons, I think the callback approach is sufficient and safer.
There was a problem hiding this comment.
- It would never be a constructor parameter in my proposal.
For attaching a TabBarState, I had proposed using a new function, that is private to TabBarController only, so it handles this automatically.
void _attachToTabBar(_TabBarState tabBar) {
_tabBarState = tabBar;
}
which is called right after the line that currently does this:
_scrollController ??= _TabBarScrollController(this);
It would then become:
_scrollController ??= _effectiveTabBarScrollController.._attach(this);
- TabBarState stays private. We should not change this.
- The only usage change is making TabBarScrollController's constructor public. Attaching to the private TabBarState is always going to be private API. Users will be able to use the public ScrollController API though (since the tab controller inherits it), which fixes this bug.
There was a problem hiding this comment.
Thanks for your update!
It would never be a constructor parameter in my proposal.
I thought it'd be a constructor param as I saw your snippet code above, which contains widget.tabBarController:
final tabBarController = widget.tabBarController ?? TabBarScrollController();
Users will be able to use the public ScrollController API though (since the tab controller inherits it), which fixes this bug.
Could you please explain how users can access the exposed controller for their needs?
Also, would you mind submitting another pull request for your solution (which will include tests and sample codes too)? Then the team can consider which approach is better for this case. I think @Piinks is the best person to review this, just let's wait until the holidays are over.
There was a problem hiding this comment.
Oh, widget.tabBarController would be a new constructor parameter for the TabBar widget (a nullable TabBarController), not a change to the TabBarController constructor.
I will make a new pull request with my proposal. I'll add you as reviewer as well.
|
Since there are 2 proposals here, I recommend we centralize discussion here: #123965 (comment) 💙 |
|
Closing this since we decided to proceed with @navaronbracke's solution 💙 |
This pull request makes `TabBarScrollController` public, and allows a `TabBar` widget 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 - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Kate Lovett <[email protected]>
This pull request makes `TabBarScrollController` public, and allows a `TabBar` widget to receive one. Fixes flutter#123965 Fixes flutter#124608 Related to flutter#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 - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Kate Lovett <[email protected]>
This pull request makes `TabBarScrollController` public, and allows a `TabBar` widget to receive one. Fixes flutter#123965 Fixes flutter#124608 Related to flutter#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 - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Kate Lovett <[email protected]>
The Scrollbar's ScrollController has no ScrollPosition attached.#124608onScrollControllerCreatedthat exposesScrollControllerfor users to use.onScrollControllerCreated, 1 for regression test for Wrapping a Scrollbar around a TabBar throws errorThe Scrollbar's ScrollController has no ScrollPosition attached.#124608)onScrollControllerCreated, 1 for Scrollbar integration) and tests for themDemo
after.mov
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.