From 98ad574d9be6a4876962fd44d646da47a91c42e1 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 10 Sep 2018 21:01:10 -0400 Subject: [PATCH] Fix keepalive for large jumps on tabs and lists. (#21350) * Ensure that the _childElements map is properly traversed as a sparse list and not inflated with garbage collected children. * Add tests to ensure Lists/Tabs with KeepAlive children can make large jumps, don't lose children (including after rebuild). --- .../rendering/sliver_multi_box_adaptor.dart | 6 +- .../lib/src/widgets/automatic_keep_alive.dart | 4 + packages/flutter/lib/src/widgets/sliver.dart | 20 ++-- packages/flutter/test/material/tabs_test.dart | 69 +++++++++++ .../widgets/automatic_keep_alive_test.dart | 38 ++++++ .../flutter/test/widgets/list_view_test.dart | 109 ++++++++++++++++-- 6 files changed, 227 insertions(+), 19 deletions(-) diff --git a/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart b/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart index e48e633528b..4df703ec6f6 100644 --- a/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart +++ b/packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart @@ -130,8 +130,9 @@ class SliverMultiBoxAdaptorParentData extends SliverLogicalParentData with Conta /// Whether to keep the child alive even when it is no longer visible. bool keepAlive = false; - /// Whether the widget is currently in the - /// [RenderSliverMultiBoxAdaptor._keepAliveBucket]. + /// Whether the widget is currently being kept alive, i.e. has [keepAlive] set + /// to true and is offscreen. + bool get keptAlive => _keptAlive; bool _keptAlive = false; @override @@ -206,6 +207,7 @@ abstract class RenderSliverMultiBoxAdaptor extends RenderSliver @override void insert(RenderBox child, { RenderBox after }) { + assert(!_keepAliveBucket.containsValue(child)); super.insert(child, after: after); assert(firstChild != null); assert(() { diff --git a/packages/flutter/lib/src/widgets/automatic_keep_alive.dart b/packages/flutter/lib/src/widgets/automatic_keep_alive.dart index 0a5fccbe731..25dbde45e7a 100644 --- a/packages/flutter/lib/src/widgets/automatic_keep_alive.dart +++ b/packages/flutter/lib/src/widgets/automatic_keep_alive.dart @@ -89,6 +89,9 @@ class _AutomaticKeepAliveState extends State { // build of this subtree. Wait until the end of the frame to update // the child when the child is guaranteed to be present. SchedulerBinding.instance.addPostFrameCallback((Duration timeStamp) { + if (!mounted) { + return; + } final ParentDataElement childElement = _getChildElement(); assert(childElement != null); _updateParentDataOfChild(childElement); @@ -103,6 +106,7 @@ class _AutomaticKeepAliveState extends State { /// While this widget is guaranteed to have a child, this may return null if /// the first build of that child has not completed yet. ParentDataElement _getChildElement() { + assert(mounted); final Element element = context; Element childElement; // We use Element.visitChildren rather than context.visitChildElements diff --git a/packages/flutter/lib/src/widgets/sliver.dart b/packages/flutter/lib/src/widgets/sliver.dart index ae5a61be657..721ef20e29c 100644 --- a/packages/flutter/lib/src/widgets/sliver.dart +++ b/packages/flutter/lib/src/widgets/sliver.dart @@ -756,24 +756,24 @@ class SliverMultiBoxAdaptorElement extends RenderObjectElement implements Render _currentBeforeChild = null; assert(_currentlyUpdatingChildIndex == null); try { - int firstIndex = _childElements.firstKey(); - int lastIndex = _childElements.lastKey(); - if (_childElements.isEmpty) { - firstIndex = 0; - lastIndex = 0; - } else if (_didUnderflow) { - lastIndex += 1; - } - for (int index = firstIndex; index <= lastIndex; ++index) { + void processElement(int index) { _currentlyUpdatingChildIndex = index; final Element newChild = updateChild(_childElements[index], _build(index), index); if (newChild != null) { _childElements[index] = newChild; - _currentBeforeChild = newChild.renderObject; + final SliverMultiBoxAdaptorParentData parentData = newChild.renderObject.parentData; + if (!parentData.keptAlive) + _currentBeforeChild = newChild.renderObject; } else { _childElements.remove(index); } } + // processElement may modify the Map - need to do a .toList() here. + _childElements.keys.toList().forEach(processElement); + if (_didUnderflow) { + final int lastKey = _childElements.lastKey() ?? -1; + processElement(lastKey + 1); + } } finally { _currentlyUpdatingChildIndex = null; } diff --git a/packages/flutter/test/material/tabs_test.dart b/packages/flutter/test/material/tabs_test.dart index 605e3b3c037..827a6de2128 100644 --- a/packages/flutter/test/material/tabs_test.dart +++ b/packages/flutter/test/material/tabs_test.dart @@ -48,6 +48,23 @@ class StateMarkerState extends State { } } +class AlwaysKeepAliveWidget extends StatefulWidget { + static String text = 'AlwaysKeepAlive'; + @override + AlwaysKeepAliveState createState() => new AlwaysKeepAliveState(); +} + +class AlwaysKeepAliveState extends State + with AutomaticKeepAliveClientMixin { + @override + bool get wantKeepAlive => true; + + @override + Widget build(BuildContext context) { + return new Text(AlwaysKeepAliveWidget.text); + } +} + Widget buildFrame({ Key tabBarKey, List tabs, @@ -1754,4 +1771,56 @@ void main() { }); + testWidgets('Skipping tabs with a KeepAlive child works', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/11895 + final List tabs = [ + 'Tab1', + 'Tab2', + 'Tab3', + 'Tab4', + 'Tab5', + ]; + final TabController controller = new TabController( + vsync: const TestVSync(), + length: tabs.length, + ); + await tester.pumpWidget( + new MaterialApp( + home: new Align( + alignment: Alignment.topLeft, + child: new SizedBox( + width: 300.0, + height: 200.0, + child: new Scaffold( + appBar: new AppBar( + title: const Text('tabs'), + bottom: new TabBar( + controller: controller, + tabs: tabs.map((String tab) => new Tab(text: tab)).toList(), + ), + ), + body: new TabBarView( + controller: controller, + children: [ + new AlwaysKeepAliveWidget(), + const Text('2'), + const Text('3'), + const Text('4'), + const Text('5'), + ], + ), + ), + ), + ), + ), + ); + expect(find.text(AlwaysKeepAliveWidget.text), findsOneWidget); + expect(find.text('4'), findsNothing); + await tester.tap(find.text('Tab4')); + await tester.pumpAndSettle(); + await tester.pump(); + expect(controller.index, 3); + expect(find.text(AlwaysKeepAliveWidget.text, skipOffstage: false), findsOneWidget); + expect(find.text('4'), findsOneWidget); + }); } diff --git a/packages/flutter/test/widgets/automatic_keep_alive_test.dart b/packages/flutter/test/widgets/automatic_keep_alive_test.dart index d8286929c02..ecc8781d4eb 100644 --- a/packages/flutter/test/widgets/automatic_keep_alive_test.dart +++ b/packages/flutter/test/widgets/automatic_keep_alive_test.dart @@ -496,6 +496,44 @@ void main() { expect(find.text('FooBar 1'), findsNothing); expect(find.text('FooBar 2'), findsNothing); }); + + testWidgets('AutomaticKeepAlive with keepAlive set to true before initState and widget goes out of scope', (WidgetTester tester) async { + await tester.pumpWidget(new Directionality( + textDirection: TextDirection.ltr, + child: new ListView.builder( + itemCount: 250, + itemBuilder: (BuildContext context, int index){ + if (index % 2 == 0){ + return new _AlwaysKeepAlive( + key: GlobalObjectKey<_AlwaysKeepAliveState>(index), + ); + } + return new Container( + height: 44.0, + child: new Text('FooBar $index'), + ); + }, + ), + )); + + expect(find.text('keep me alive'), findsNWidgets(7)); + expect(find.text('FooBar 1'), findsOneWidget); + expect(find.text('FooBar 3'), findsOneWidget); + + expect(find.byKey(const GlobalObjectKey<_AlwaysKeepAliveState>(0)), findsOneWidget); + + final ScrollableState state = tester.state(find.byType(Scrollable)); + final ScrollPosition position = state.position; + position.jumpTo(3025.0); + + await tester.pump(); + expect(find.byKey(const GlobalObjectKey<_AlwaysKeepAliveState>(0), skipOffstage: false), findsOneWidget); + + expect(find.text('keep me alive', skipOffstage: false), findsNWidgets(23)); + expect(find.text('FooBar 1'), findsNothing); + expect(find.text('FooBar 3'), findsNothing); + expect(find.text('FooBar 73'), findsOneWidget); + }); } class _AlwaysKeepAlive extends StatefulWidget { diff --git a/packages/flutter/test/widgets/list_view_test.dart b/packages/flutter/test/widgets/list_view_test.dart index 678e3c7c2b7..965483e15e0 100644 --- a/packages/flutter/test/widgets/list_view_test.dart +++ b/packages/flutter/test/widgets/list_view_test.dart @@ -18,6 +18,61 @@ class TestSliverChildListDelegate extends SliverChildListDelegate { } } +class Alive extends StatefulWidget { + const Alive(this.alive, this.index); + final bool alive; + final int index; + + @override + AliveState createState() => new AliveState(); + + @override + String toString({DiagnosticLevel minLevel}) => '$index $alive'; +} + +class AliveState extends State with AutomaticKeepAliveClientMixin { + @override + bool get wantKeepAlive => widget.alive; + + @override + Widget build(BuildContext context) => + new Text('${widget.index}:$wantKeepAlive'); +} + +typedef WhetherToKeepAlive = bool Function(int); +class _StatefulListView extends StatefulWidget { + const _StatefulListView(this.aliveCallback); + + final WhetherToKeepAlive aliveCallback; + @override + _StatefulListViewState createState() => new _StatefulListViewState(); +} + +class _StatefulListViewState extends State<_StatefulListView> { + @override + Widget build(BuildContext context) { + return new GestureDetector( + // force a rebuild - the test(s) using this are verifying that the list is + // still correct after rebuild + onTap: () => setState, + child: new Directionality( + textDirection: TextDirection.ltr, + child: new ListView( + children: new List.generate(200, (int i) { + return new Builder( + builder: (BuildContext context) { + return new Container( + child: new Alive(widget.aliveCallback(i), i), + ); + }, + ); + }), + ), + ), + ); + } +} + void main() { testWidgets('ListView default control', (WidgetTester tester) async { await tester.pumpWidget( @@ -91,7 +146,7 @@ void main() { return new Container( child: new Text('$i'), ); - } + }, ); }), ), @@ -120,6 +175,43 @@ void main() { log.clear(); }); + testWidgets('ListView large scroll jump and keepAlive first child not keepAlive', (WidgetTester tester) async { + Future checkAndScroll([String zero = '0:false']) async { + expect(find.text(zero), findsOneWidget); + expect(find.text('1:false'), findsOneWidget); + expect(find.text('2:false'), findsOneWidget); + expect(find.text('3:true'), findsOneWidget); + expect(find.text('116:false'), findsNothing); + final ScrollableState state = tester.state(find.byType(Scrollable)); + final ScrollPosition position = state.position; + position.jumpTo(1025.0); + + await tester.pump(); + + expect(find.text(zero), findsNothing); + expect(find.text('1:false'), findsNothing); + expect(find.text('2:false'), findsNothing); + expect(find.text('3:true', skipOffstage: false), findsOneWidget); + expect(find.text('116:false'), findsOneWidget); + + await tester.tapAt(const Offset(100.0, 100.0)); + position.jumpTo(0.0); + await tester.pump(); + await tester.pump(); + + expect(find.text(zero), findsOneWidget); + expect(find.text('1:false'), findsOneWidget); + expect(find.text('2:false'), findsOneWidget); + expect(find.text('3:true'), findsOneWidget); + } + + await tester.pumpWidget(new _StatefulListView((int i) => i > 2 && i % 3 == 0)); + await checkAndScroll(); + + await tester.pumpWidget(new _StatefulListView((int i) => i % 3 == 0)); + await checkAndScroll('0:true'); + }); + testWidgets('ListView can build out of underflow', (WidgetTester tester) async { await tester.pumpWidget( new Directionality( @@ -225,11 +317,14 @@ void main() { testWidgets('didFinishLayout has correct indices', (WidgetTester tester) async { final TestSliverChildListDelegate delegate = new TestSliverChildListDelegate( - new List.generate(20, (int i) { - return new Container( - child: new Text('$i', textDirection: TextDirection.ltr), - ); - }) + new List.generate( + 20, + (int i) { + return new Container( + child: new Text('$i', textDirection: TextDirection.ltr), + ); + }, + ) ); await tester.pumpWidget( @@ -345,7 +440,7 @@ void main() { ), ), ), - ); + ); expect(find.byType(Viewport), isNot(paints..clipRect())); });