Skip to content

Commit

Permalink
ScrollController creation dispatching for memory leaks tracking (#133…
Browse files Browse the repository at this point in the history 
…759)
  • Loading branch information
@ksokolovskyi
ksokolovskyi authored Aug 31, 2023
1 parent 7fe4571 commit 414bff1
Show file tree
Hide file tree
Showing 9 changed files with 217 additions and 59 deletions.
1 change: 1 addition & 0 deletions packages/flutter/lib/src/widgets/routes.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -884,6 +884,7 @@ class _ModalScopeState<T> extends State<_ModalScope<T>> {
@override
void dispose() {
focusScopeNode.dispose();
primaryScrollController.dispose();
super.dispose();
}

Expand Down
6 changes: 5 additions & 1 deletion packages/flutter/lib/src/widgets/scroll_controller.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,11 @@ class ScrollController extends ChangeNotifier {
this.debugLabel,
this.onAttach,
this.onDetach,
}) : _initialScrollOffset = initialScrollOffset;
}) : _initialScrollOffset = initialScrollOffset {
if (kFlutterMemoryAllocationsEnabled) {
maybeDispatchObjectCreation();
}
}

/// The initial value to use for [offset].
///
Expand Down
73 changes: 42 additions & 31 deletions packages/flutter/test/material/app_bar_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1609,48 +1609,53 @@ void main() {
// Generates a MaterialApp with a SliverAppBar in a CustomScrollView.
// The first cell of the scroll view contains a button at its top, and is
// initially scrolled so that it is beneath the SliverAppBar.
Widget buildWidget({
(ScrollController, Widget) buildWidget({
required bool forceMaterialTransparency,
required VoidCallback onPressed
}) {
const double appBarHeight = 120;
return MaterialApp(
home: Scaffold(
body: CustomScrollView(
controller: ScrollController(initialScrollOffset:appBarHeight),
slivers: <Widget>[
SliverAppBar(
collapsedHeight: appBarHeight,
expandedHeight: appBarHeight,
pinned: true,
elevation: 0,
backgroundColor: Colors.transparent,
forceMaterialTransparency: forceMaterialTransparency,
title: const Text('AppBar'),
),
SliverList(
delegate: SliverChildBuilderDelegate((BuildContext context, int index) {
return SizedBox(
height: appBarHeight,
child: index == 0
? Align(
alignment: Alignment.topCenter,
child: TextButton(onPressed: onPressed, child: const Text('press')))
: const SizedBox(),
);
},
childCount: 20,
final ScrollController controller = ScrollController(initialScrollOffset: appBarHeight);

return (
controller,
MaterialApp(
home: Scaffold(
body: CustomScrollView(
controller: controller,
slivers: <Widget>[
SliverAppBar(
collapsedHeight: appBarHeight,
expandedHeight: appBarHeight,
pinned: true,
elevation: 0,
backgroundColor: Colors.transparent,
forceMaterialTransparency: forceMaterialTransparency,
title: const Text('AppBar'),
),
SliverList(
delegate: SliverChildBuilderDelegate((BuildContext context, int index) {
return SizedBox(
height: appBarHeight,
child: index == 0
? Align(
alignment: Alignment.topCenter,
child: TextButton(onPressed: onPressed, child: const Text('press')))
: const SizedBox(),
);
},
childCount: 20,
),
),
),
]),
]),
),
),
);
}

testWidgetsWithLeakTracking(
'forceMaterialTransparency == true allows gestures beneath the app bar', (WidgetTester tester) async {
bool buttonWasPressed = false;
final Widget widget = buildWidget(
final (ScrollController controller, Widget widget) = buildWidget(
forceMaterialTransparency:true,
onPressed:() { buttonWasPressed = true; },
);
Expand All @@ -1663,6 +1668,8 @@ void main() {
await tester.tap(buttonFinder);
await tester.pump();
expect(buttonWasPressed, isTrue);

controller.dispose();
});

testWidgetsWithLeakTracking(
Expand All @@ -1672,7 +1679,7 @@ void main() {
WidgetController.hitTestWarningShouldBeFatal = false;

bool buttonWasPressed = false;
final Widget widget = buildWidget(
final (ScrollController controller, Widget widget) = buildWidget(
forceMaterialTransparency:false,
onPressed:() { buttonWasPressed = true; },
);
Expand All @@ -1685,6 +1692,8 @@ void main() {
await tester.tap(buttonFinder, warnIfMissed:false);
await tester.pump();
expect(buttonWasPressed, isFalse);

controller.dispose();
});
});

Expand Down Expand Up @@ -4295,6 +4304,8 @@ void main() {
);

expect(tester.takeException(), isNull);

controller.dispose();
});

testWidgetsWithLeakTracking('does not trigger on horizontal scroll', (WidgetTester tester) async {
Expand Down
48 changes: 41 additions & 7 deletions packages/flutter/test/material/page_selector_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,14 @@ void main() {
await tester.pump();
expect(tabController.index, 2);
expect(indicatorColors(tester), const <Color>[kUnselectedColor, kUnselectedColor, kSelectedColor]);
});
},
// TODO(someone): remove after fixing
// https://github.com/flutter/flutter/issues/133755
leakTrackingTestConfig: const LeakTrackingTestConfig(
notDisposedAllowList: <String, int?>{
'PageController': 1,
},
));

testWidgetsWithLeakTracking('PageSelector responds correctly to TabController.animateTo()', (WidgetTester tester) async {
final TabController tabController = TabController(
Expand Down Expand Up @@ -127,7 +134,14 @@ void main() {
await tester.pumpAndSettle();
expect(tabController.index, 2);
expect(indicatorColors(tester), const <Color>[kUnselectedColor, kUnselectedColor, kSelectedColor]);
});
},
// TODO(someone): remove after fixing
// https://github.com/flutter/flutter/issues/133755
leakTrackingTestConfig: const LeakTrackingTestConfig(
notDisposedAllowList: <String, int?>{
'PageController': 1,
},
));

testWidgetsWithLeakTracking('PageSelector responds correctly to TabBarView drags', (WidgetTester tester) async {
final TabController tabController = TabController(
Expand Down Expand Up @@ -185,8 +199,14 @@ void main() {
await tester.fling(find.byType(TabBarView), const Offset(100.0, 0.0), 1000.0);
await tester.pumpAndSettle();
expect(indicatorColors(tester), const <Color>[kUnselectedColor, kSelectedColor, kUnselectedColor]);

});
},
// TODO(someone): remove after fixing
// https://github.com/flutter/flutter/issues/133755
leakTrackingTestConfig: const LeakTrackingTestConfig(
notDisposedAllowList: <String, int?>{
'PageController': 1,
},
));

testWidgetsWithLeakTracking('PageSelector indicatorColors', (WidgetTester tester) async {
const Color kRed = Color(0xFFFF0000);
Expand All @@ -205,7 +225,14 @@ void main() {
tabController.index = 0;
await tester.pumpAndSettle();
expect(indicatorColors(tester), const <Color>[kBlue, kRed, kRed]);
});
},
// TODO(someone): remove after fixing
// https://github.com/flutter/flutter/issues/133755
leakTrackingTestConfig: const LeakTrackingTestConfig(
notDisposedAllowList: <String, int?>{
'PageController': 1,
},
));

testWidgets('PageSelector indicatorSize', (WidgetTester tester) async {
final TabController tabController = TabController(
Expand All @@ -228,7 +255,7 @@ void main() {
expect(tester.getSize(find.byType(TabPageSelector)).height, 24.0);
});

testWidgetsWithLeakTracking('PageSelector circle border', (WidgetTester tester) async {
testWidgetsWithLeakTracking('PageSelector circle border', (WidgetTester tester) async {
final TabController tabController = TabController(
vsync: const TestVSync(),
initialIndex: 1,
Expand Down Expand Up @@ -272,5 +299,12 @@ void main() {
for (final TabPageSelectorIndicator indicator in indicators) {
expect(indicator.borderStyle, BorderStyle.solid);
}
});
},
// TODO(someone): remove after fixing
// https://github.com/flutter/flutter/issues/133755
leakTrackingTestConfig: const LeakTrackingTestConfig(
notDisposedAllowList: <String, int?>{
'PageController': 1,
},
));
}
14 changes: 14 additions & 0 deletions packages/flutter/test/material/refresh_indicator_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,8 @@ void main() {
expect(controller.offset, greaterThan(lastScrollOffset));
expect(controller.offset, lessThan(0.0));
expect(refreshCalled, isTrue);

controller.dispose();
}, variant: const TargetPlatformVariant(<TargetPlatform>{ TargetPlatform.iOS, TargetPlatform.macOS }));

testWidgetsWithLeakTracking('RefreshIndicator does not force child to relayout', (WidgetTester tester) async {
Expand Down Expand Up @@ -618,6 +620,8 @@ void main() {
await tester.pump(const Duration(seconds: 1)); // finish the scroll animation
await tester.pump(const Duration(seconds: 1)); // finish the indicator settle animation
expect(tester.getCenter(find.byType(RefreshProgressIndicator)).dy, lessThan(300.0));

scrollController.dispose();
});

testWidgetsWithLeakTracking('Reverse RefreshIndicator(anywhere mode) should be shown when dragging from non-zero scroll position', (WidgetTester tester) async {
Expand Down Expand Up @@ -654,6 +658,8 @@ void main() {
await tester.pump(const Duration(seconds: 1)); // finish the scroll animation
await tester.pump(const Duration(seconds: 1)); // finish the indicator settle animation
expect(tester.getCenter(find.byType(RefreshProgressIndicator)).dy, lessThan(300.0));

scrollController.dispose();
});

// Regression test for https://github.com/flutter/flutter/issues/71936
Expand Down Expand Up @@ -691,6 +697,8 @@ void main() {
await tester.pump(const Duration(seconds: 1)); // finish the scroll animation
await tester.pump(const Duration(seconds: 1)); // finish the indicator settle animation
expect(find.byType(RefreshProgressIndicator), findsNothing);

scrollController.dispose();
});

testWidgetsWithLeakTracking('Top RefreshIndicator(onEdge mode) should not be shown when dragging from non-zero scroll position', (WidgetTester tester) async {
Expand Down Expand Up @@ -725,6 +733,8 @@ void main() {
await tester.pump(const Duration(seconds: 1)); // finish the scroll animation
await tester.pump(const Duration(seconds: 1)); // finish the indicator settle animation
expect(find.byType(RefreshProgressIndicator), findsNothing);

scrollController.dispose();
});

testWidgetsWithLeakTracking('Reverse RefreshIndicator(onEdge mode) should be shown when dragging from non-zero scroll position', (WidgetTester tester) async {
Expand Down Expand Up @@ -760,6 +770,8 @@ void main() {
await tester.pump(const Duration(seconds: 1)); // finish the scroll animation
await tester.pump(const Duration(seconds: 1)); // finish the indicator settle animation
expect(find.byType(RefreshProgressIndicator), findsNothing);

scrollController.dispose();
});

testWidgetsWithLeakTracking('ScrollController.jumpTo should not trigger the refresh indicator', (WidgetTester tester) async {
Expand Down Expand Up @@ -792,6 +804,8 @@ void main() {
await tester.pump(const Duration(seconds: 1)); // finish the indicator hide animation

expect(refreshCalled, false);

scrollController.dispose();
});

testWidgetsWithLeakTracking('RefreshIndicator.adaptive', (WidgetTester tester) async {
Expand Down
Loading

0 comments on commit 414bff1

Please sign in to comment.