From 51f172526107aa6ca432c06ef492deab15a53d5a Mon Sep 17 00:00:00 2001 From: Valentin Vignal <32538273+ValentinVignal@users.noreply.github.com> Date: Thu, 18 Apr 2024 02:03:48 +0800 Subject: [PATCH] Fix memory leak in paginated tables (#146755) --- .../flutter/lib/src/material/data_table.dart | 9 +- .../flutter/lib/src/material/dropdown.dart | 65 ++++++++--- .../material/paginated_data_table_test.dart | 11 +- .../test/widgets/transitions_test.dart | 104 +++++++++--------- 4 files changed, 116 insertions(+), 73 deletions(-) diff --git a/packages/flutter/lib/src/material/data_table.dart b/packages/flutter/lib/src/material/data_table.dart index c4ccb013f31c..57dd99c8956e 100644 --- a/packages/flutter/lib/src/material/data_table.dart +++ b/packages/flutter/lib/src/material/data_table.dart @@ -1271,11 +1271,11 @@ class _SortArrow extends StatefulWidget { } class _SortArrowState extends State<_SortArrow> with TickerProviderStateMixin { - late AnimationController _opacityController; - late Animation _opacityAnimation; + late final AnimationController _opacityController; + late final CurvedAnimation _opacityAnimation; - late AnimationController _orientationController; - late Animation _orientationAnimation; + late final AnimationController _orientationController; + late final Animation _orientationAnimation; double _orientationOffset = 0.0; bool? _up; @@ -1354,6 +1354,7 @@ class _SortArrowState extends State<_SortArrow> with TickerProviderStateMixin { void dispose() { _opacityController.dispose(); _orientationController.dispose(); + _opacityAnimation.dispose(); super.dispose(); } diff --git a/packages/flutter/lib/src/material/dropdown.dart b/packages/flutter/lib/src/material/dropdown.dart index ec57dd77f329..1aa48bd8a980 100644 --- a/packages/flutter/lib/src/material/dropdown.dart +++ b/packages/flutter/lib/src/material/dropdown.dart @@ -118,6 +118,40 @@ class _DropdownMenuItemButton extends StatefulWidget { } class _DropdownMenuItemButtonState extends State<_DropdownMenuItemButton> { + CurvedAnimation? _opacityAnimation; + + @override + void initState() { + super.initState(); + _setOpacityAnimation(); + } + + @override + void didUpdateWidget(_DropdownMenuItemButton oldWidget) { + super.didUpdateWidget(oldWidget); + if (oldWidget.itemIndex != widget.itemIndex || + oldWidget.route.animation != widget.route.animation || + oldWidget.route.selectedIndex != widget.route.selectedIndex || + widget.route.items.length != oldWidget.route.items.length) { + _setOpacityAnimation(); + } + } + + void _setOpacityAnimation() { + _opacityAnimation?.dispose(); + final double unit = 0.5 / (widget.route.items.length + 1.5); + if (widget.itemIndex == widget.route.selectedIndex) { + _opacityAnimation = CurvedAnimation( + parent: widget.route.animation!, curve: const Threshold(0.0)); + } else { + final double start = + clampDouble(0.5 + (widget.itemIndex + 1) * unit, 0.0, 1.0); + final double end = clampDouble(start + 1.5 * unit, 0.0, 1.0); + _opacityAnimation = CurvedAnimation( + parent: widget.route.animation!, curve: Interval(start, end)); + } + } + void _handleFocusChange(bool focused) { final bool inTraditionalMode = switch (FocusManager.instance.highlightMode) { FocusHighlightMode.touch => false, @@ -156,18 +190,16 @@ class _DropdownMenuItemButtonState extends State<_DropdownMenuItemButton> SingleActivator(LogicalKeyboardKey.arrowUp): DirectionalFocusIntent(TraversalDirection.up), }; + @override + void dispose() { + _opacityAnimation?.dispose(); + super.dispose(); + } + @override Widget build(BuildContext context) { - final DropdownMenuItem dropdownMenuItem = widget.route.items[widget.itemIndex].item!; - final CurvedAnimation opacity; - final double unit = 0.5 / (widget.route.items.length + 1.5); - if (widget.itemIndex == widget.route.selectedIndex) { - opacity = CurvedAnimation(parent: widget.route.animation!, curve: const Threshold(0.0)); - } else { - final double start = clampDouble(0.5 + (widget.itemIndex + 1) * unit, 0.0, 1.0); - final double end = clampDouble(start + 1.5 * unit, 0.0, 1.0); - opacity = CurvedAnimation(parent: widget.route.animation!, curve: Interval(start, end)); - } + final DropdownMenuItem dropdownMenuItem = + widget.route.items[widget.itemIndex].item!; Widget child = Container( padding: widget.padding, height: widget.route.itemHeight, @@ -183,7 +215,7 @@ class _DropdownMenuItemButtonState extends State<_DropdownMenuItemButton> child: child, ); } - child = FadeTransition(opacity: opacity, child: child); + child = FadeTransition(opacity: _opacityAnimation!, child: child); if (kIsWeb && dropdownMenuItem.enabled) { child = Shortcuts( shortcuts: _webShortcuts, @@ -221,8 +253,8 @@ class _DropdownMenu extends StatefulWidget { } class _DropdownMenuState extends State<_DropdownMenu> { - late CurvedAnimation _fadeOpacity; - late CurvedAnimation _resize; + late final CurvedAnimation _fadeOpacity; + late final CurvedAnimation _resize; @override void initState() { @@ -243,6 +275,13 @@ class _DropdownMenuState extends State<_DropdownMenu> { ); } + @override + void dispose() { + _fadeOpacity.dispose(); + _resize.dispose(); + super.dispose(); + } + @override Widget build(BuildContext context) { // The menu is shown in three stages (unit timing in brackets): diff --git a/packages/flutter/test/material/paginated_data_table_test.dart b/packages/flutter/test/material/paginated_data_table_test.dart index d0f67a246beb..666843048eba 100644 --- a/packages/flutter/test/material/paginated_data_table_test.dart +++ b/packages/flutter/test/material/paginated_data_table_test.dart @@ -6,6 +6,7 @@ import 'package:flutter/gestures.dart' show DragStartBehavior; import 'package:flutter/material.dart'; import 'package:flutter/rendering.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart'; import 'data_table_test_utils.dart'; @@ -70,7 +71,10 @@ void main() { setUp(() => source = TestDataSource()); tearDown(() => source.dispose()); - testWidgets('PaginatedDataTable paging', (WidgetTester tester) async { + testWidgets('PaginatedDataTable paging', + // TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in] + experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const ['CurvedAnimation']), + (WidgetTester tester) async { final List log = []; await tester.pumpWidget(MaterialApp( @@ -379,7 +383,10 @@ void main() { expect(find.byWidgetPredicate((Widget widget) => widget is SizedBox && widget.height == (rowsPerPage - (rowCount % rowsPerPage)) * 46.0), findsOneWidget); }); - testWidgets('PaginatedDataTable control test', (WidgetTester tester) async { + testWidgets('PaginatedDataTable control test', + // TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in] + experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const ['CurvedAnimation']), + (WidgetTester tester) async { TestDataSource source = TestDataSource() ..generation = 42; addTearDown(source.dispose); diff --git a/packages/flutter/test/widgets/transitions_test.dart b/packages/flutter/test/widgets/transitions_test.dart index 984638951e56..8c514b4d8e3b 100644 --- a/packages/flutter/test/widgets/transitions_test.dart +++ b/packages/flutter/test/widgets/transitions_test.dart @@ -96,60 +96,56 @@ void main() { expect(actualDecoration.boxShadow, null); }); - testWidgets( - 'animations work with curves test', - // TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in] - experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const ['CurvedAnimation']), - (WidgetTester tester) async { - final CurvedAnimation animation = CurvedAnimation( - parent: controller, - curve: Curves.easeOut, - ); - addTearDown(animation.dispose); - final Animation curvedDecorationAnimation = - decorationTween.animate(animation); - - final DecoratedBoxTransition transitionUnderTest = DecoratedBoxTransition( - decoration: curvedDecorationAnimation, - position: DecorationPosition.foreground, - child: const Text( - "Doesn't matter", - textDirection: TextDirection.ltr, - ), - ); - - await tester.pumpWidget(transitionUnderTest); - - RenderDecoratedBox actualBox = tester.renderObject(find.byType(DecoratedBox)); - BoxDecoration actualDecoration = actualBox.decoration as BoxDecoration; - - expect(actualDecoration.color, const Color(0xFFFFFFFF)); - expect(actualDecoration.boxShadow![0].blurRadius, 10.0); - expect(actualDecoration.boxShadow![0].spreadRadius, 4.0); - expect(actualDecoration.boxShadow![0].color, const Color(0x66000000)); - - controller.value = 0.5; - - await tester.pump(); - actualBox = tester.renderObject(find.byType(DecoratedBox)); - actualDecoration = actualBox.decoration as BoxDecoration; - - // Same as the test above but the values should be much closer to the - // tween's end values given the easeOut curve. - expect(actualDecoration.color, const Color(0xFF505050)); - expect(actualDecoration.border, isA()); - final Border border = actualDecoration.border! as Border; - expect(border.left.width, moreOrLessEquals(1.9, epsilon: 0.1)); - expect(border.left.style, BorderStyle.solid); - expect(border.left.color, const Color(0xFF151515)); - expect(actualDecoration.borderRadius!.resolve(TextDirection.ltr).topLeft.x, moreOrLessEquals(6.8, epsilon: 0.1)); - expect(actualDecoration.shape, BoxShape.rectangle); - expect(actualDecoration.boxShadow![0].blurRadius, moreOrLessEquals(3.1, epsilon: 0.1)); - expect(actualDecoration.boxShadow![0].spreadRadius, moreOrLessEquals(1.2, epsilon: 0.1)); - // Scaling a shadow doesn't change the color. - expect(actualDecoration.boxShadow![0].color, const Color(0x66000000)); - }, - ); + testWidgets('animations work with curves test', + // TODO(polina-c): remove when fixed https://github.com/flutter/flutter/issues/145600 [leak-tracking-opt-in] + experimentalLeakTesting: LeakTesting.settings.withTracked(classes: const ['CurvedAnimation']), + (WidgetTester tester) async { + final Animation curvedDecorationAnimation = + decorationTween.animate(CurvedAnimation( + parent: controller, + curve: Curves.easeOut, + )); + + final DecoratedBoxTransition transitionUnderTest = DecoratedBoxTransition( + decoration: curvedDecorationAnimation, + position: DecorationPosition.foreground, + child: const Text( + "Doesn't matter", + textDirection: TextDirection.ltr, + ), + ); + + await tester.pumpWidget(transitionUnderTest); + + RenderDecoratedBox actualBox = tester.renderObject(find.byType(DecoratedBox)); + BoxDecoration actualDecoration = actualBox.decoration as BoxDecoration; + + expect(actualDecoration.color, const Color(0xFFFFFFFF)); + expect(actualDecoration.boxShadow![0].blurRadius, 10.0); + expect(actualDecoration.boxShadow![0].spreadRadius, 4.0); + expect(actualDecoration.boxShadow![0].color, const Color(0x66000000)); + + controller.value = 0.5; + + await tester.pump(); + actualBox = tester.renderObject(find.byType(DecoratedBox)); + actualDecoration = actualBox.decoration as BoxDecoration; + + // Same as the test above but the values should be much closer to the + // tween's end values given the easeOut curve. + expect(actualDecoration.color, const Color(0xFF505050)); + expect(actualDecoration.border, isA()); + final Border border = actualDecoration.border! as Border; + expect(border.left.width, moreOrLessEquals(1.9, epsilon: 0.1)); + expect(border.left.style, BorderStyle.solid); + expect(border.left.color, const Color(0xFF151515)); + expect(actualDecoration.borderRadius!.resolve(TextDirection.ltr).topLeft.x, moreOrLessEquals(6.8, epsilon: 0.1)); + expect(actualDecoration.shape, BoxShape.rectangle); + expect(actualDecoration.boxShadow![0].blurRadius, moreOrLessEquals(3.1, epsilon: 0.1)); + expect(actualDecoration.boxShadow![0].spreadRadius, moreOrLessEquals(1.2, epsilon: 0.1)); + // Scaling a shadow doesn't change the color. + expect(actualDecoration.boxShadow![0].color, const Color(0x66000000)); + }); }); testWidgets('AlignTransition animates', (WidgetTester tester) async {