Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leak in TabPageSelector #147403

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
102 changes: 76 additions & 26 deletions packages/flutter/lib/src/material/tabs.dart
Expand Up @@ -2219,7 +2219,7 @@ class TabPageSelectorIndicator extends StatelessWidget {
///
/// If a [TabController] is not provided, then there must be a
/// [DefaultTabController] ancestor.
class TabPageSelector extends StatelessWidget {
class TabPageSelector extends StatefulWidget {
/// Creates a compact widget that indicates which tab has been selected.
const TabPageSelector({
super.key,
Expand Down Expand Up @@ -2256,6 +2256,73 @@ class TabPageSelector extends StatelessWidget {
/// Defaults to [BorderStyle.solid] if value is not specified.
final BorderStyle? borderStyle;

@override
State<TabPageSelector> createState() => _TabPageSelectorState();
}

class _TabPageSelectorState extends State<TabPageSelector> {
TabController? _previousTabController;
TabController get _tabController {
final TabController? tabController = widget.controller ?? DefaultTabController.maybeOf(context);
assert(() {
if (tabController == null) {
throw FlutterError(
'No TabController for $runtimeType.\n'
'When creating a $runtimeType, you must either provide an explicit TabController '
'using the "controller" property, or you must ensure that there is a '
'DefaultTabController above the $runtimeType.\n'
'In this case, there was neither an explicit controller nor a default controller.',
);
}
return true;
}());
return tabController!;
}

CurvedAnimation? _animation;

@override
void initState() {
super.initState();
_setAnimation();
}

@override
void didUpdateWidget (TabPageSelector oldWidget) {
super.didUpdateWidget(oldWidget);
if (_previousTabController?.animation != _tabController.animation) {
_setAnimation();
}
if (_previousTabController != _tabController) {
_previousTabController = _tabController;
}
}

@override
void didChangeDependencies() {
super.didChangeDependencies();
if (_previousTabController?.animation != _tabController.animation) {
_setAnimation();
}
if (_previousTabController != _tabController) {
_previousTabController = _tabController;
}
}

void _setAnimation() {
_animation?.dispose();
_animation = CurvedAnimation(
parent: _tabController.animation!,
curve: Curves.fastOutSlowIn,
);
}

@override
void dispose() {
_animation?.dispose();
super.dispose();
}

Widget _buildTabIndicator(
int tabIndex,
TabController tabController,
Expand Down Expand Up @@ -2290,44 +2357,27 @@ class TabPageSelector extends StatelessWidget {
return TabPageSelectorIndicator(
backgroundColor: background,
borderColor: selectedColorTween.end!,
size: indicatorSize,
borderStyle: borderStyle ?? BorderStyle.solid,
size: widget.indicatorSize,
borderStyle: widget.borderStyle ?? BorderStyle.solid,
);
}

@override
Widget build(BuildContext context) {
final Color fixColor = color ?? Colors.transparent;
final Color fixSelectedColor = selectedColor ?? Theme.of(context).colorScheme.secondary;
final Color fixColor = widget.color ?? Colors.transparent;
final Color fixSelectedColor = widget.selectedColor ?? Theme.of(context).colorScheme.secondary;
final ColorTween selectedColorTween = ColorTween(begin: fixColor, end: fixSelectedColor);
final ColorTween previousColorTween = ColorTween(begin: fixSelectedColor, end: fixColor);
final TabController? tabController = controller ?? DefaultTabController.maybeOf(context);
final MaterialLocalizations localizations = MaterialLocalizations.of(context);
assert(() {
if (tabController == null) {
throw FlutterError(
'No TabController for $runtimeType.\n'
'When creating a $runtimeType, you must either provide an explicit TabController '
'using the "controller" property, or you must ensure that there is a '
'DefaultTabController above the $runtimeType.\n'
'In this case, there was neither an explicit controller nor a default controller.',
);
}
return true;
}());
final Animation<double> animation = CurvedAnimation(
parent: tabController!.animation!,
curve: Curves.fastOutSlowIn,
);
return AnimatedBuilder(
animation: animation,
animation: _animation!,
builder: (BuildContext context, Widget? child) {
return Semantics(
label: localizations.tabLabel(tabIndex: tabController.index + 1, tabCount: tabController.length),
label: localizations.tabLabel(tabIndex: _tabController.index + 1, tabCount: _tabController.length),
child: Row(
mainAxisSize: MainAxisSize.min,
children: List<Widget>.generate(tabController.length, (int tabIndex) {
return _buildTabIndicator(tabIndex, tabController, selectedColorTween, previousColorTween);
children: List<Widget>.generate(_tabController.length, (int tabIndex) {
return _buildTabIndicator(tabIndex, _tabController, selectedColorTween, previousColorTween);
}).toList(),
),
);
Expand Down
6 changes: 3 additions & 3 deletions packages/flutter/test/flutter_test_config.dart
Expand Up @@ -41,7 +41,7 @@ Future<void> testExecutable(FutureOr<void> Function() testMain) {
// receive the event.
WidgetController.hitTestWarningShouldBeFatal = true;

if (_isLeakTrackingEnabled()) {
// if (_isLeakTrackingEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops ! Very sorry about that :( I reverted it in refactor: Revert bad commit

LeakTesting.enable();

LeakTracking.warnForUnsupportedPlatforms = false;
Expand All @@ -51,10 +51,10 @@ Future<void> testExecutable(FutureOr<void> Function() testMain) {
allNotGCed: true,
classes: <String>[
// TODO(polina-c): CurvedAnimation is leaking, https://github.com/flutter/flutter/issues/145600 [leaks-to-clean]
'CurvedAnimation',
// 'CurvedAnimation',
],
);
}
// }

// Enable golden file testing using Skia Gold.
return flutter_goldens.testExecutable(testMain);
Expand Down
6 changes: 5 additions & 1 deletion packages/flutter/test/material/page_selector_test.dart
Expand Up @@ -4,6 +4,7 @@

import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';

const Color kSelectedColor = Color(0xFF00FF00);
const Color kUnselectedColor = Colors.transparent;
Expand Down Expand Up @@ -86,7 +87,10 @@ void main() {
expect(indicatorColors(tester), const <Color>[kUnselectedColor, kUnselectedColor, kSelectedColor]);
});

testWidgets('PageSelector responds correctly to TabController.animateTo()', (WidgetTester tester) async {
testWidgets('PageSelector responds correctly to TabController.animateTo()',
// TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in]
experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const <String>['CurvedAnimation']),
(WidgetTester tester) async {
final TabController tabController = TabController(
vsync: const TestVSync(),
length: 3,
Expand Down