Unverified Commit 09acfe63 authored by Kostia Sokolovskyi's avatar Kostia Sokolovskyi Committed by GitHub

Fix memory leak in ListWheelScrollView (#134732)

parent 1e4a1be6
......@@ -7,7 +7,6 @@ import 'dart:math' as math;
import 'package:flutter/physics.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter/scheduler.dart';
import 'basic.dart';
import 'framework.dart';
......@@ -709,12 +708,14 @@ class ListWheelScrollView extends StatefulWidget {
class _ListWheelScrollViewState extends State<ListWheelScrollView> {
int _lastReportedItemIndex = 0;
ScrollController? scrollController;
ScrollController? _backupController;
ScrollController get _effectiveController =>
widget.controller ?? (_backupController ??= FixedExtentScrollController());
@override
void initState() {
super.initState();
scrollController = widget.controller ?? FixedExtentScrollController();
if (widget.controller is FixedExtentScrollController) {
final FixedExtentScrollController controller = widget.controller! as FixedExtentScrollController;
_lastReportedItemIndex = controller.initialItem;
......@@ -722,15 +723,9 @@ class _ListWheelScrollViewState extends State<ListWheelScrollView> {
}
@override
void didUpdateWidget(ListWheelScrollView oldWidget) {
super.didUpdateWidget(oldWidget);
if (widget.controller != null && widget.controller != scrollController) {
final ScrollController? oldScrollController = scrollController;
SchedulerBinding.instance.addPostFrameCallback((_) {
oldScrollController!.dispose();
});
scrollController = widget.controller;
}
void dispose() {
_backupController?.dispose();
super.dispose();
}
bool _handleScrollNotification(ScrollNotification notification) {
......@@ -754,7 +749,7 @@ class _ListWheelScrollViewState extends State<ListWheelScrollView> {
return NotificationListener<ScrollNotification>(
onNotification: _handleScrollNotification,
child: _FixedExtentScrollable(
controller: scrollController,
controller: _effectiveController,
physics: widget.physics,
itemExtent: widget.itemExtent,
restorationId: widget.restorationId,
......
......@@ -11,6 +11,7 @@ import 'package:flutter/foundation.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter/widgets.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';
import '../rendering/rendering_tester.dart' show TestCallbackPainter, TestClipPaintingContext;
......@@ -1285,7 +1286,7 @@ void main() {
expect(controller.selectedItem, 10);
});
testWidgets('controller hot swappable', (WidgetTester tester) async {
testWidgetsWithLeakTracking('controller hot swappable', (WidgetTester tester) async {
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
......@@ -1302,14 +1303,16 @@ void main() {
await tester.drag(find.byType(ListWheelScrollView), const Offset(0.0, -500.0));
await tester.pump();
final FixedExtentScrollController newController =
final FixedExtentScrollController controller1 =
FixedExtentScrollController(initialItem: 30);
addTearDown(controller1.dispose);
// Attaching first controller.
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: ListWheelScrollView(
controller: newController,
controller: controller1,
itemExtent: 100.0,
children: List<Widget>.generate(100, (int index) {
return const Placeholder();
......@@ -1320,17 +1323,94 @@ void main() {
// initialItem doesn't do anything since the scroll position was already
// created.
expect(newController.selectedItem, 5);
expect(controller1.selectedItem, 5);
newController.jumpToItem(50);
expect(newController.selectedItem, 50);
expect(newController.position.pixels, 5000.0);
controller1.jumpToItem(50);
expect(controller1.selectedItem, 50);
expect(controller1.position.pixels, 5000.0);
final FixedExtentScrollController controller2 =
FixedExtentScrollController(initialItem: 33);
addTearDown(controller2.dispose);
// Attaching the second controller.
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: ListWheelScrollView(
controller: controller2,
itemExtent: 100.0,
children: List<Widget>.generate(100, (int index) {
return const Placeholder();
}),
),
),
);
// First controller is now detached.
expect(controller1.hasClients, isFalse);
// initialItem doesn't do anything since the scroll position was already
// created.
expect(controller2.selectedItem, 50);
controller2.jumpToItem(40);
expect(controller2.selectedItem, 40);
expect(controller2.position.pixels, 4000.0);
// Now, use the internal controller.
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: ListWheelScrollView(
itemExtent: 100.0,
children: List<Widget>.generate(100, (int index) {
return const Placeholder();
}),
),
),
);
// Both controllers are now detached.
expect(controller1.hasClients, isFalse);
expect(controller2.hasClients, isFalse);
});
testWidgetsWithLeakTracking('controller can be reused', (WidgetTester tester) async {
final FixedExtentScrollController controller =
FixedExtentScrollController(initialItem: 3);
addTearDown(controller.dispose);
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: ListWheelScrollView(
controller: controller,
itemExtent: 100.0,
children: List<Widget>.generate(100, (int index) {
return const Placeholder();
}),
),
),
);
// selectedItem is equal to the initialItem.
expect(controller.selectedItem, 3);
expect(controller.position.pixels, 300.0);
controller.jumpToItem(10);
expect(controller.selectedItem, 10);
expect(controller.position.pixels, 1000.0);
await tester.pumpWidget(const Center());
// Controller is now detached.
expect(controller.hasClients, isFalse);
// Now remove the controller
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: ListWheelScrollView(
controller: controller,
itemExtent: 100.0,
children: List<Widget>.generate(100, (int index) {
return const Placeholder();
......@@ -1339,9 +1419,10 @@ void main() {
),
);
// Internally, that same controller is still attached and still at the
// same place.
expect(newController.selectedItem, 50);
// Controller is now attached again.
expect(controller.hasClients, isTrue);
expect(controller.selectedItem, 3);
expect(controller.position.pixels, 300.0);
});
});
......
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