Unverified Commit d92d2c12 authored by chunhtai's avatar chunhtai Committed by GitHub

Fix SliverGrid garbage collection issue (#138915)

The code doesn't consider indices of children may not be continuous after rebuild.

fixes https://github.com/flutter/flutter/issues/138749
parent 2663e50a
...@@ -596,14 +596,9 @@ class RenderSliverGrid extends RenderSliverMultiBoxAdaptor { ...@@ -596,14 +596,9 @@ class RenderSliverGrid extends RenderSliverMultiBoxAdaptor {
final int firstIndex = layout.getMinChildIndexForScrollOffset(scrollOffset); final int firstIndex = layout.getMinChildIndexForScrollOffset(scrollOffset);
final int? targetLastIndex = targetEndScrollOffset.isFinite ? final int? targetLastIndex = targetEndScrollOffset.isFinite ?
layout.getMaxChildIndexForScrollOffset(targetEndScrollOffset) : null; layout.getMaxChildIndexForScrollOffset(targetEndScrollOffset) : null;
if (firstChild != null) { if (firstChild != null) {
final int oldFirstIndex = indexOf(firstChild!); final int leadingGarbage = _calculateLeadingGarbage(firstIndex);
final int oldLastIndex = indexOf(lastChild!); final int trailingGarbage = targetLastIndex != null ? _calculateTrailingGarbage(targetLastIndex) : 0;
final int leadingGarbage = (firstIndex - oldFirstIndex).clamp(0, childCount); // ignore_clamp_double_lint
final int trailingGarbage = targetLastIndex == null
? 0
: (oldLastIndex - targetLastIndex).clamp(0, childCount); // ignore_clamp_double_lint
collectGarbage(leadingGarbage, trailingGarbage); collectGarbage(leadingGarbage, trailingGarbage);
} else { } else {
collectGarbage(0, 0); collectGarbage(0, 0);
...@@ -713,4 +708,24 @@ class RenderSliverGrid extends RenderSliverMultiBoxAdaptor { ...@@ -713,4 +708,24 @@ class RenderSliverGrid extends RenderSliverMultiBoxAdaptor {
} }
childManager.didFinishLayout(); childManager.didFinishLayout();
} }
int _calculateLeadingGarbage(int firstIndex) {
RenderBox? walker = firstChild;
int leadingGarbage = 0;
while (walker != null && indexOf(walker) < firstIndex) {
leadingGarbage += 1;
walker = childAfter(walker);
}
return leadingGarbage;
}
int _calculateTrailingGarbage(int targetLastIndex) {
RenderBox? walker = lastChild;
int trailingGarbage = 0;
while (walker != null && indexOf(walker) > targetLastIndex) {
trailingGarbage += 1;
walker = childBefore(walker);
}
return trailingGarbage;
}
} }
...@@ -232,6 +232,69 @@ void main() { ...@@ -232,6 +232,69 @@ void main() {
expect(find.text('BOTTOM'), findsOneWidget); expect(find.text('BOTTOM'), findsOneWidget);
}); });
testWidgetsWithLeakTracking('Sliver grid can replace intermediate items', (WidgetTester tester) async {
// Regression test for https://github.com/flutter/flutter/issues/138749.
// The bug happens when items in between first and last item changed while
// the sliver layout only display a item in the middle of the list.
final List<int> items = <int>[0, 1, 2, 3, 4, 5];
final List<int> replacedItems = <int>[0, 2, 9, 10, 11, 12, 5];
Future<void> pumpSliverGrid(bool replace) async {
await tester.pumpWidget(
Center(
child: SizedBox(
width: 200,
height: 200,
child: Directionality(
textDirection: TextDirection.ltr,
child: CustomScrollView(
slivers: <Widget>[
SliverGrid(
gridDelegate: TestGridDelegate(replace),
delegate: SliverChildBuilderDelegate(
(BuildContext context, int index) {
final int item = replace
? replacedItems[index]
: items[index];
return Container(
key: ValueKey<int>(item),
alignment: Alignment.center,
child: Text('item $item'),
);
},
childCount: replace ? 7 : 6,
findChildIndexCallback: (Key key) {
final int item = (key as ValueKey<int>).value;
final int index = replace
? replacedItems.indexOf(item)
: items.indexOf(item);
return index >= 0 ? index : null;
},
),
),
],
),
),
),
),
);
}
await pumpSliverGrid(false);
expect(find.text('item 0'), findsOneWidget);
expect(find.text('item 1'), findsOneWidget);
expect(find.text('item 2'), findsOneWidget);
expect(find.text('item 3'), findsOneWidget);
expect(find.text('item 4'), findsOneWidget);
await pumpSliverGrid(true);
// The TestGridDelegate only show child at index 1 when not expand.
expect(find.text('item 0'), findsNothing);
expect(find.text('item 1'), findsNothing);
expect(find.text('item 2'), findsOneWidget);
expect(find.text('item 3'), findsNothing);
expect(find.text('item 4'), findsNothing);
});
testWidgetsWithLeakTracking('SliverFixedExtentList correctly clears garbage', (WidgetTester tester) async { testWidgetsWithLeakTracking('SliverFixedExtentList correctly clears garbage', (WidgetTester tester) async {
final List<String> items = <String>['1', '2', '3', '4', '5', '6']; final List<String> items = <String>['1', '2', '3', '4', '5', '6'];
await testSliverFixedExtentList(tester, items); await testSliverFixedExtentList(tester, items);
...@@ -1504,3 +1567,56 @@ class _NullBuildContext implements BuildContext { ...@@ -1504,3 +1567,56 @@ class _NullBuildContext implements BuildContext {
@override @override
dynamic noSuchMethod(Invocation invocation) => throw UnimplementedError(); dynamic noSuchMethod(Invocation invocation) => throw UnimplementedError();
} }
class TestGridDelegate implements SliverGridDelegate {
TestGridDelegate(this.replace);
final bool replace;
@override
SliverGridLayout getLayout(SliverConstraints constraints) {
return TestGridLayout(replace);
}
@override
bool shouldRelayout(covariant TestGridDelegate oldDelegate) {
return true;
}
}
class TestGridLayout implements SliverGridLayout {
TestGridLayout(this.replace);
final bool replace;
@override
double computeMaxScrollOffset(int childCount) {
return 200;
}
@override
SliverGridGeometry getGeometryForChildIndex(int index) {
return SliverGridGeometry(
crossAxisOffset: 20.0 + 20 * index,
crossAxisExtent: 20,
mainAxisExtent: 20,
scrollOffset: 0,
);
}
@override
int getMaxChildIndexForScrollOffset(double scrollOffset) {
if (replace) {
return 1;
}
return 5;
}
@override
int getMinChildIndexForScrollOffset(double scrollOffset) {
if (replace) {
return 1;
}
return 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