From 4eb11c9f526c1948941dabe8e2f90c137dcd60c8 Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Wed, 17 Jul 2019 10:40:07 -0700 Subject: [PATCH] Fixes RenderSliverFixedExtentBoxAdaptor correctly calculates leadingGarbage and trailingGarbage. (#36302) --- .../rendering/sliver_fixed_extent_list.dart | 26 +++++- .../flutter/test/widgets/slivers_test.dart | 92 +++++++++++++++++++ 2 files changed, 114 insertions(+), 4 deletions(-) diff --git a/packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart b/packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart index 221897f8b7c..131cf382b03 100644 --- a/packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart +++ b/packages/flutter/lib/src/rendering/sliver_fixed_extent_list.dart @@ -142,6 +142,26 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda return childManager.childCount * itemExtent; } + 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; + } + @override void performLayout() { childManager.didStartLayout(); @@ -165,10 +185,8 @@ abstract class RenderSliverFixedExtentBoxAdaptor extends RenderSliverMultiBoxAda getMaxChildIndexForScrollOffset(targetEndScrollOffset, itemExtent) : null; if (firstChild != null) { - final int oldFirstIndex = indexOf(firstChild); - final int oldLastIndex = indexOf(lastChild); - final int leadingGarbage = (firstIndex - oldFirstIndex).clamp(0, childCount); - final int trailingGarbage = targetLastIndex == null ? 0 : (oldLastIndex - targetLastIndex).clamp(0, childCount); + final int leadingGarbage = _calculateLeadingGarbage(firstIndex); + final int trailingGarbage = _calculateTrailingGarbage(targetLastIndex); collectGarbage(leadingGarbage, trailingGarbage); } else { collectGarbage(0, 0); diff --git a/packages/flutter/test/widgets/slivers_test.dart b/packages/flutter/test/widgets/slivers_test.dart index e2c4c9390da..44a942bf8d7 100644 --- a/packages/flutter/test/widgets/slivers_test.dart +++ b/packages/flutter/test/widgets/slivers_test.dart @@ -25,6 +25,37 @@ Future test(WidgetTester tester, double offset, { double anchor = 0.0 }) { ); } +Future testSliverFixedExtentList(WidgetTester tester, List items) { + return tester.pumpWidget( + Directionality( + textDirection: TextDirection.ltr, + child: CustomScrollView( + slivers: [ + SliverFixedExtentList( + itemExtent: 900, + delegate: SliverChildBuilderDelegate( + (BuildContext context, int index) { + return Center( + key: ValueKey(items[index]), + child: KeepAlive( + items[index], + ) + ); + }, + childCount : items.length, + findChildIndexCallback: (Key key) { + final ValueKey valueKey = key; + final String data = valueKey.value; + return items.indexOf(data); + } + ), + ), + ], + ), + ), + ); +} + void verify(WidgetTester tester, List idealPositions, List idealVisibles) { final List actualPositions = tester.renderObjectList(find.byType(SizedBox, skipOffstage: false)).map( (RenderBox target) => target.localToGlobal(const Offset(0.0, 0.0)) @@ -196,6 +227,47 @@ void main() { expect(find.text('BOTTOM'), findsOneWidget); }); + testWidgets('SliverFixedExtentList correctly clears garbage', (WidgetTester tester) async { + final List items = ['1', '2', '3', '4', '5', '6']; + await testSliverFixedExtentList(tester, items); + // Keep alive widgets require 1 frame to notify their parents. Pumps in between + // drags to ensure widgets are kept alive. + await tester.drag(find.byType(CustomScrollView),const Offset(0.0, -1200.0)); + await tester.pump(); + await tester.drag(find.byType(CustomScrollView),const Offset(0.0, -1200.0)); + await tester.pump(); + await tester.drag(find.byType(CustomScrollView),const Offset(0.0, -800.0)); + await tester.pump(); + expect(find.text('1'), findsNothing); + expect(find.text('2'), findsNothing); + expect(find.text('3'), findsNothing); + expect(find.text('4'), findsOneWidget); + expect(find.text('5'), findsOneWidget); + // Indexes [0, 1, 2] are kept alive and [3, 4] are in viewport, thus the sliver + // will need to keep updating the elements at these indexes whenever a rebuild is + // triggered. The current child list in RenderSliverFixedExtentList is + // '4' -> '5' -> null. + // + // With the insertion below, all items will get shifted back 1 position. The sliver + // will have to update indexes [0, 1, 2, 3, 4, 5]. Since this is the first time + // item '0' gets initialized, mounting the element will cause it to attach to + // child list in RenderSliverFixedExtentList. This will create a gap. + // '0' -> '4' -> '5' -> null. + items.insert(0, '0'); + await testSliverFixedExtentList(tester, items); + // Sliver should collect leading and trailing garbage correctly. + // + // The child list update should occur in following order. + // '0' -> '4' -> '5' -> null Started with Original list. + // '4' -> null Removed 1 leading garbage and 1 trailing garbage. + // '3' -> '4' -> null Prepended '3' because viewport is still at [3, 4]. + expect(find.text('0'), findsNothing); + expect(find.text('1'), findsNothing); + expect(find.text('2'), findsNothing); + expect(find.text('3'), findsOneWidget); + expect(find.text('4'), findsOneWidget); + }); + testWidgets('SliverGrid Correctly layout children after rearranging', (WidgetTester tester) async { await tester.pumpWidget(const TestSliverGrid( [ @@ -332,3 +404,23 @@ class TestSliverFixedExtentList extends StatelessWidget { ); } } + +class KeepAlive extends StatefulWidget { + const KeepAlive(this.data); + + final String data; + + @override + KeepAliveState createState() => KeepAliveState(); +} + +class KeepAliveState extends State with AutomaticKeepAliveClientMixin { + @override + bool get wantKeepAlive => true; + + @override + Widget build(BuildContext context) { + super.build(context); + return Text(widget.data); + } +}