Unverified Commit 72289ec8 authored by xubaolin's avatar xubaolin Committed by GitHub

Fix the showBottomSheet controller leaking (#87775)

* Fix the showBottomSheet controller leaking

* codereview feedback
parent 0e9665e4
...@@ -2690,6 +2690,7 @@ class ScaffoldState extends State<Scaffold> with TickerProviderStateMixin, Resto ...@@ -2690,6 +2690,7 @@ class ScaffoldState extends State<Scaffold> with TickerProviderStateMixin, Resto
ShapeBorder? shape, ShapeBorder? shape,
Clip? clipBehavior, Clip? clipBehavior,
BoxConstraints? constraints, BoxConstraints? constraints,
bool shouldDisposeAnimationController = true,
}) { }) {
assert(() { assert(() {
if (widget.bottomSheet != null && isPersistent && _currentBottomSheet != null) { if (widget.bottomSheet != null && isPersistent && _currentBottomSheet != null) {
...@@ -2757,6 +2758,11 @@ class ScaffoldState extends State<Scaffold> with TickerProviderStateMixin, Resto ...@@ -2757,6 +2758,11 @@ class ScaffoldState extends State<Scaffold> with TickerProviderStateMixin, Resto
}); });
} }
}, },
onDispose: () {
if (shouldDisposeAnimationController) {
animationController.dispose();
}
},
builder: builder, builder: builder,
isPersistent: isPersistent, isPersistent: isPersistent,
backgroundColor: backgroundColor, backgroundColor: backgroundColor,
...@@ -2894,6 +2900,7 @@ class ScaffoldState extends State<Scaffold> with TickerProviderStateMixin, Resto ...@@ -2894,6 +2900,7 @@ class ScaffoldState extends State<Scaffold> with TickerProviderStateMixin, Resto
shape: shape, shape: shape,
clipBehavior: clipBehavior, clipBehavior: clipBehavior,
constraints: constraints, constraints: constraints,
shouldDisposeAnimationController: transitionAnimationController == null,
); );
}); });
return _currentBottomSheet! as PersistentBottomSheetController<T>; return _currentBottomSheet! as PersistentBottomSheetController<T>;
...@@ -3069,12 +3076,6 @@ class ScaffoldState extends State<Scaffold> with TickerProviderStateMixin, Resto ...@@ -3069,12 +3076,6 @@ class ScaffoldState extends State<Scaffold> with TickerProviderStateMixin, Resto
_snackBarTimer = null; _snackBarTimer = null;
_geometryNotifier.dispose(); _geometryNotifier.dispose();
for (final _StandardBottomSheet bottomSheet in _dismissedBottomSheets) {
bottomSheet.animationController.dispose();
}
if (_currentBottomSheet != null) {
_currentBottomSheet!._widget.animationController.dispose();
}
_floatingActionButtonMoveController.dispose(); _floatingActionButtonMoveController.dispose();
_floatingActionButtonVisibilityController.dispose(); _floatingActionButtonVisibilityController.dispose();
_scaffoldMessenger?._unregister(this); _scaffoldMessenger?._unregister(this);
...@@ -3587,12 +3588,14 @@ class _StandardBottomSheet extends StatefulWidget { ...@@ -3587,12 +3588,14 @@ class _StandardBottomSheet extends StatefulWidget {
this.shape, this.shape,
this.clipBehavior, this.clipBehavior,
this.constraints, this.constraints,
this.onDispose,
}) : super(key: key); }) : super(key: key);
final AnimationController animationController; // we control it, but it must be disposed by whoever created it. final AnimationController animationController; // we control it, but it must be disposed by whoever created it.
final bool enableDrag; final bool enableDrag;
final VoidCallback? onClosing; final VoidCallback? onClosing;
final VoidCallback? onDismissed; final VoidCallback? onDismissed;
final VoidCallback? onDispose;
final WidgetBuilder builder; final WidgetBuilder builder;
final bool isPersistent; final bool isPersistent;
final Color? backgroundColor; final Color? backgroundColor;
...@@ -3619,6 +3622,12 @@ class _StandardBottomSheetState extends State<_StandardBottomSheet> { ...@@ -3619,6 +3622,12 @@ class _StandardBottomSheetState extends State<_StandardBottomSheet> {
widget.animationController.addStatusListener(_handleStatusChange); widget.animationController.addStatusListener(_handleStatusChange);
} }
@override
void dispose() {
widget.onDispose?.call();
super.dispose();
}
@override @override
void didUpdateWidget(_StandardBottomSheet oldWidget) { void didUpdateWidget(_StandardBottomSheet oldWidget) {
super.didUpdateWidget(oldWidget); super.didUpdateWidget(oldWidget);
......
...@@ -895,6 +895,106 @@ void main() { ...@@ -895,6 +895,106 @@ void main() {
expect(find.text('BottomSheet'), findsNothing); expect(find.text('BottomSheet'), findsNothing);
}); });
// Regression test for https://github.com/flutter/flutter/issues/87708
testWidgets('Each of the internal animation controllers should be disposed by the framework.', (WidgetTester tester) async {
final GlobalKey<ScaffoldState> scaffoldKey = GlobalKey();
await tester.pumpWidget(MaterialApp(
home: Scaffold(
key: scaffoldKey,
body: const Center(child: Text('body')),
),
));
scaffoldKey.currentState!.showBottomSheet<void>((_) {
return Builder(
builder: (BuildContext context) {
return Container(height: 200.0);
},
);
});
await tester.pump();
expect(find.byType(BottomSheet), findsOneWidget);
// The first sheet's animation is still running.
// Trigger the second sheet will remove the first sheet from tree.
scaffoldKey.currentState!.showBottomSheet<void>((_) {
return Builder(
builder: (BuildContext context) {
return Container(height: 200.0);
},
);
});
await tester.pump();
expect(find.byType(BottomSheet), findsOneWidget);
// Remove the Scaffold from the tree.
await tester.pumpWidget(const SizedBox.shrink());
// If the internal animation controller do not dispose will throw
// FlutterError:<ScaffoldState#1981a(tickers: tracking 1 ticker) was disposed with an active
// Ticker.
expect(tester.takeException(), isNull);
});
// Regression test for https://github.com/flutter/flutter/issues/87708
testWidgets('The framework does not dispose of the transitionAnimationController provided by user.', (WidgetTester tester) async {
const Key tapTarget = Key('tap-target');
const Key tapTargetToClose = Key('tap-target-to-close');
final AnimationController controller = AnimationController(
vsync: const TestVSync(),
duration: const Duration(seconds: 2),
reverseDuration: const Duration(seconds: 2),
);
await tester.pumpWidget(MaterialApp(
home: Scaffold(
body: Builder(
builder: (BuildContext context) {
return GestureDetector(
key: tapTarget,
onTap: () {
showBottomSheet<void>(
context: context,
transitionAnimationController: controller,
builder: (BuildContext context) {
return MaterialButton(
onPressed: () => Navigator.pop(context),
key: tapTargetToClose,
child: const Text('BottomSheet'),
);
},
);
},
behavior: HitTestBehavior.opaque,
child: const SizedBox(
height: 100.0,
width: 100.0,
),
);
},
),
),
));
expect(find.text('BottomSheet'), findsNothing);
await tester.tap(find.byKey(tapTarget)); // Open the sheet.
await tester.pumpAndSettle(); // Finish the animation.
expect(find.text('BottomSheet'), findsOneWidget);
// Tapping button on the bottom sheet to dismiss it.
await tester.tap(find.byKey(tapTargetToClose)); // Closing the sheet.
await tester.pumpAndSettle(); // Finish the animation.
expect(find.text('BottomSheet'), findsNothing);
await tester.pumpWidget(const SizedBox.shrink());
controller.dispose();
// Double dispose will throw.
expect(tester.takeException(), isNull);
});
group('constraints', () { group('constraints', () {
testWidgets('No constraints by default for bottomSheet property', (WidgetTester tester) async { testWidgets('No constraints by default for bottomSheet property', (WidgetTester tester) async {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment