From c8e4cbf27d58bec1342d0287f389926fa0e9d9a6 Mon Sep 17 00:00:00 2001 From: Hans Muller Date: Tue, 6 Jun 2017 12:11:21 -0700 Subject: [PATCH] Improved support for saving and restoring the scroll offset, etc (#10517) --- .../catalog/lib/expansion_tile_sample.dart | 2 +- .../flutter/lib/src/widgets/framework.dart | 6 +- .../flutter/lib/src/widgets/page_storage.dart | 103 +++++++++++------- .../flutter/lib/src/widgets/page_view.dart | 25 ++++- .../lib/src/widgets/scroll_controller.dart | 27 ++++- .../lib/src/widgets/scroll_position.dart | 21 +++- .../scroll_position_with_single_context.dart | 13 ++- .../flutter/test/widgets/page_view_test.dart | 6 +- .../test/widgets/scroll_controller_test.dart | 47 ++++++++ 9 files changed, 196 insertions(+), 54 deletions(-) diff --git a/examples/catalog/lib/expansion_tile_sample.dart b/examples/catalog/lib/expansion_tile_sample.dart index 2213ee18c5520..c028b149fdbb4 100644 --- a/examples/catalog/lib/expansion_tile_sample.dart +++ b/examples/catalog/lib/expansion_tile_sample.dart @@ -76,7 +76,7 @@ class EntryItem extends StatelessWidget { if (root.children.isEmpty) return new ListTile(title: new Text(root.title)); return new ExpansionTile( - key: new ValueKey(root), + key: new PageStorageKey(root), title: new Text(root.title), children: root.children.map(_buildTiles).toList(), ); diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index a0f9fd8fee75c..8666889607b98 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -322,10 +322,10 @@ class LabeledGlobalKey> extends GlobalKey { @override String toString() { - final String tag = _debugLabel != null ? ' $_debugLabel' : '#$hashCode'; + final String label = _debugLabel != null ? ' $_debugLabel' : ''; if (runtimeType == LabeledGlobalKey) - return '[GlobalKey$tag]'; - return '[$runtimeType$tag]'; + return '[GlobalKey#$hashCode$label]'; + return '[$runtimeType#$hashCode$label]'; } } diff --git a/packages/flutter/lib/src/widgets/page_storage.dart b/packages/flutter/lib/src/widgets/page_storage.dart index 7d71ac87dbf74..c2611e274cad4 100644 --- a/packages/flutter/lib/src/widgets/page_storage.dart +++ b/packages/flutter/lib/src/widgets/page_storage.dart @@ -6,43 +6,67 @@ import 'package:flutter/foundation.dart'; import 'framework.dart'; +/// A [ValueKey] that defines where [PageStorage] values will be saved. +/// +/// [Scrollable]s ([ScrollPosition]s really) use [PageStorage] to save their +/// scroll offset. Each time a scroll completes, the scrollable's page +/// storage is updated. +/// +/// [PageStorage] is used to save and restore values that can outlive the widget. +/// The values are stored in a per-route [Map] whose keys are defined by the +/// [PageStorageKey]s for the widget and its ancestors. To make it possible +/// for a saved value to be found when a widget is recreated, the key's values +/// must not be objects whose identity will change each time the widget is created. +/// +/// For example, to ensure that the scroll offsets for the scrollable within +/// each `MyScrollableTabView` below are restored when the [TabBarView] +/// is recreated, we've specified [PageStorageKey]s whose values are the the +/// tabs' string labels. +/// +/// ```dart +/// new TabBarView( +/// children: myTabs.map((Tab tab) { +/// new MyScrollableTabView( +/// key: new PageStorageKey(tab.text), // like 'Tab 1' +/// tab: tab, +/// ), +/// }), +///) +/// ``` +class PageStorageKey extends ValueKey { + /// Creates a [ValueKey] that defines where [PageStorage] values will be saved. + const PageStorageKey(T value) : super(value); +} + class _StorageEntryIdentifier { - Type clientType; - List keys; - - void addKey(Key key) { - assert(key != null); - assert(key is! GlobalKey); - keys ??= []; - keys.add(key); + _StorageEntryIdentifier(this.clientType, this.keys) { + assert(clientType != null); + assert(keys != null); } - GlobalKey scopeKey; + final Type clientType; + final List> keys; @override bool operator ==(dynamic other) { - if (other is! _StorageEntryIdentifier) + if (other.runtimeType != runtimeType) return false; final _StorageEntryIdentifier typedOther = other; - if (clientType != typedOther.clientType || - scopeKey != typedOther.scopeKey || - keys?.length != typedOther.keys?.length) + if (clientType != typedOther.clientType || keys.length != typedOther.keys.length) return false; - if (keys != null) { - for (int index = 0; index < keys.length; index += 1) { - if (keys[index] != typedOther.keys[index]) - return false; - } + for (int index = 0; index < keys.length; index += 1) { + if (keys[index] != typedOther.keys[index]) + return false; } return true; } @override - int get hashCode => hashValues(clientType, scopeKey, hashList(keys)); + int get hashCode => hashValues(clientType, hashList(keys)); @override String toString() { - return 'StorageEntryIdentifier($clientType, $scopeKey, ${keys?.join(":")})'; + return 'StorageEntryIdentifier($clientType, ${keys?.join(":")})'; } } @@ -51,27 +75,26 @@ class _StorageEntryIdentifier { /// Useful for storing per-page state that persists across navigations from one /// page to another. class PageStorageBucket { - _StorageEntryIdentifier _computeStorageIdentifier(BuildContext context) { - final _StorageEntryIdentifier result = new _StorageEntryIdentifier(); - result.clientType = context.widget.runtimeType; - Key lastKey = context.widget.key; - if (lastKey is! GlobalKey) { - if (lastKey != null) - result.addKey(lastKey); + bool _maybeAddKey(BuildContext context, List> keys) { + final Widget widget = context.widget; + final Key key = widget.key; + if (key is PageStorageKey) + keys.add(key); + return widget is! PageStorage; + } + + List> _allKeys(BuildContext context) { + final List> keys = >[]; + if (_maybeAddKey(context, keys)) { context.visitAncestorElements((Element element) { - if (element.widget.key is GlobalKey) { - lastKey = element.widget.key; - return false; - } else if (element.widget.key != null) { - result.addKey(element.widget.key); - } - return true; + return _maybeAddKey(element, keys); }); - return result; } - assert(lastKey is GlobalKey); - result.scopeKey = lastKey; - return result; + return keys; + } + + _StorageEntryIdentifier _computeIdentifier(BuildContext context) { + return new _StorageEntryIdentifier(context.widget.runtimeType, _allKeys(context)); } Map _storage; @@ -89,13 +112,13 @@ class PageStorageBucket { /// identifier will change. void writeState(BuildContext context, dynamic data, { Object identifier }) { _storage ??= {}; - _storage[identifier ?? _computeStorageIdentifier(context)] = data; + _storage[identifier ?? _computeIdentifier(context)] = data; } /// Read given data from into this page storage bucket using an identifier /// computed from the given context. More about [identifier] in [writeState]. dynamic readState(BuildContext context, { Object identifier }) { - return _storage != null ? _storage[identifier ?? _computeStorageIdentifier(context)] : null; + return _storage != null ? _storage[identifier ?? _computeIdentifier(context)] : null; } } diff --git a/packages/flutter/lib/src/widgets/page_view.dart b/packages/flutter/lib/src/widgets/page_view.dart index 355deff5e4f10..54dba500b886e 100644 --- a/packages/flutter/lib/src/widgets/page_view.dart +++ b/packages/flutter/lib/src/widgets/page_view.dart @@ -38,17 +38,36 @@ import 'viewport.dart'; class PageController extends ScrollController { /// Creates a page controller. /// - /// The [initialPage] and [viewportFraction] arguments must not be null. + /// The [initialPage], [keepPage], and [viewportFraction] arguments must not be null. PageController({ this.initialPage: 0, + this.keepPage: true, this.viewportFraction: 1.0, }) : assert(initialPage != null), + assert(keepPage != null), assert(viewportFraction != null), assert(viewportFraction > 0.0); /// The page to show when first creating the [PageView]. final int initialPage; + /// Save the current [page] with [PageStorage] and restore it if + /// this controller's scrollable is recreated. + /// + /// If this property is set to false, the current [page] is never saved + /// and [initialPage] is always used to initialize the scroll offset. + /// If true (the default), the initial page is used the first time the + /// controller's scrollable is created, since there's isn't a page to + /// restore yet. Subsequently the saved page is restored and + /// [initialPage] is ignored. + /// + /// See also: + /// + /// * [PageStorageKey], which should be used when more than one + //// scrollable appears in the same route, to distinguish the [PageStorage] + /// locations used to save scroll offsets. + final bool keepPage; + /// The fraction of the viewport that each page should occupy. /// /// Defaults to 1.0, which means each page fills the viewport in the scrolling @@ -116,6 +135,7 @@ class PageController extends ScrollController { physics: physics, context: context, initialPage: initialPage, + keepPage: keepPage, viewportFraction: viewportFraction, oldPosition: oldPosition, ); @@ -150,9 +170,11 @@ class _PagePosition extends ScrollPositionWithSingleContext { ScrollPhysics physics, ScrollContext context, this.initialPage: 0, + bool keepPage: true, double viewportFraction: 1.0, ScrollPosition oldPosition, }) : assert(initialPage != null), + assert(keepPage != null), assert(viewportFraction != null), assert(viewportFraction > 0.0), _viewportFraction = viewportFraction, @@ -161,6 +183,7 @@ class _PagePosition extends ScrollPositionWithSingleContext { physics: physics, context: context, initialPixels: null, + keepScrollOffset: keepPage, oldPosition: oldPosition, ); diff --git a/packages/flutter/lib/src/widgets/scroll_controller.dart b/packages/flutter/lib/src/widgets/scroll_controller.dart index d050414652a22..4f8b268f0b092 100644 --- a/packages/flutter/lib/src/widgets/scroll_controller.dart +++ b/packages/flutter/lib/src/widgets/scroll_controller.dart @@ -39,20 +39,40 @@ import 'scroll_position_with_single_context.dart'; class ScrollController extends ChangeNotifier { /// Creates a controller for a scrollable widget. /// - /// The [initialScrollOffset] must not be null. + /// The values of `initialScrollOffset` and `keepScrollOffset` must not be null. ScrollController({ this.initialScrollOffset: 0.0, + this.keepScrollOffset: true, this.debugLabel, - }) : assert(initialScrollOffset != null); + }) : assert(initialScrollOffset != null), + assert(keepScrollOffset != null); /// The initial value to use for [offset]. /// /// New [ScrollPosition] objects that are created and attached to this - /// controller will have their offset initialized to this value. + /// controller will have their offset initialized to this value + /// if [keepScrollOffset] is false or a scroll offset hasn't been saved yet. /// /// Defaults to 0.0. final double initialScrollOffset; + /// Each time a scroll completes, save the current scroll [offset] with + /// [PageStorage] and restore it if this controller's scrollable is recreated. + /// + /// If this property is set to false, the scroll offset is never saved + /// and [initialScrollOffset] is always used to initialize the scroll + /// offset. If true (the default), the initial scroll offset is used the + /// first time the controller's scrollable is created, since there's no + /// scroll offset to restore yet. Subsequently the saved offset is + /// restored and [initialScrollOffset] is ignored. + /// + /// See also: + /// + /// * [PageStorageKey], which should be used when more than one + //// scrollable appears in the same route, to distinguish the [PageStorage] + /// locations used to save scroll offsets. + final bool keepScrollOffset; + /// A label that is used in the [toString] output. Intended to aid with /// identifying scroll controller instances in debug output. final String debugLabel; @@ -204,6 +224,7 @@ class ScrollController extends ChangeNotifier { physics: physics, context: context, initialPixels: initialScrollOffset, + keepScrollOffset: keepScrollOffset, oldPosition: oldPosition, debugLabel: debugLabel, ); diff --git a/packages/flutter/lib/src/widgets/scroll_position.dart b/packages/flutter/lib/src/widgets/scroll_position.dart index 3785621a75c65..c96ee6ea2fb93 100644 --- a/packages/flutter/lib/src/widgets/scroll_position.dart +++ b/packages/flutter/lib/src/widgets/scroll_position.dart @@ -61,17 +61,22 @@ export 'scroll_activity.dart' show ScrollHoldController; abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { /// Creates an object that determines which portion of the content is visible /// in a scroll view. + /// + /// The [physics], [context], and [keepScrollOffset] parameters must not be null. ScrollPosition({ @required this.physics, @required this.context, + this.keepScrollOffset: true, ScrollPosition oldPosition, this.debugLabel, }) : assert(physics != null), assert(context != null), - assert(context.vsync != null) { + assert(context.vsync != null), + assert(keepScrollOffset != null) { if (oldPosition != null) absorb(oldPosition); - restoreScrollOffset(); + if (keepScrollOffset) + restoreScrollOffset(); } /// How the scroll position should respond to user input. @@ -85,6 +90,15 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { /// Typically implemented by [ScrollableState]. final ScrollContext context; + /// Save the current scroll [offset] with [PageStorage] and restore it if + /// this scroll position's scrollable is recreated. + /// + /// See also: + /// + /// * [ScrollController.keepScrollOffset] and [PageController.keepPage], which + /// create scroll positions and initialize this property. + final bool keepScrollOffset; + /// A label that is used in the [toString] output. Intended to aid with /// identifying animation controller instances in debug output. final String debugLabel; @@ -539,7 +553,8 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { /// This also saves the scroll offset using [saveScrollOffset]. void didEndScroll() { activity.dispatchScrollEndNotification(cloneMetrics(), context.notificationContext); - saveScrollOffset(); + if (keepScrollOffset) + saveScrollOffset(); } /// Called by [setPixels] to report overscroll when an attempt is made to diff --git a/packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart b/packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart index 287f739fdda5c..4d7b81ba6521d 100644 --- a/packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart +++ b/packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart @@ -46,13 +46,24 @@ class ScrollPositionWithSingleContext extends ScrollPosition implements ScrollAc /// imperative that the value be set, using [correctPixels], as soon as /// [applyNewDimensions] is invoked, before calling the inherited /// implementation of that method. + /// + /// If [keepScrollOffset] is true (the default), the current scroll offset is + /// saved with [PageStorage] and restored it if this scroll position's scrollable + /// is recreated. ScrollPositionWithSingleContext({ @required ScrollPhysics physics, @required ScrollContext context, double initialPixels: 0.0, + bool keepScrollOffset: true, ScrollPosition oldPosition, String debugLabel, - }) : super(physics: physics, context: context, oldPosition: oldPosition, debugLabel: debugLabel) { + }) : super( + physics: physics, + context: context, + keepScrollOffset: keepScrollOffset, + oldPosition: oldPosition, + debugLabel: debugLabel, + ) { // If oldPosition is not null, the superclass will first call absorb(), // which may set _pixels and _activity. if (pixels == null && initialPixels != null) diff --git a/packages/flutter/test/widgets/page_view_test.dart b/packages/flutter/test/widgets/page_view_test.dart index 29a4d17b7f81b..0089e2dd2c827 100644 --- a/packages/flutter/test/widgets/page_view_test.dart +++ b/packages/flutter/test/widgets/page_view_test.dart @@ -441,12 +441,14 @@ void main() { ), ); expect(controller.page, 2); + + final PageController controller2 = new PageController(keepPage: false); await tester.pumpWidget( new PageStorage( bucket: bucket, child: new PageView( key: const Key('Check it again against your list and see consistency!'), - controller: controller, + controller: controller2, children: [ const Placeholder(), const Placeholder(), @@ -455,6 +457,6 @@ void main() { ), ), ); - expect(controller.page, 0); + expect(controller2.page, 0); }); } diff --git a/packages/flutter/test/widgets/scroll_controller_test.dart b/packages/flutter/test/widgets/scroll_controller_test.dart index f935edcd57904..5ae0e4d5eb0f4 100644 --- a/packages/flutter/test/widgets/scroll_controller_test.dart +++ b/packages/flutter/test/widgets/scroll_controller_test.dart @@ -259,4 +259,51 @@ void main() { await tester.drag(find.byType(ListView), const Offset(0.0, -130.0)); expect(log, isEmpty); }); + + testWidgets('keepScrollOffset', (WidgetTester tester) async { + final PageStorageBucket bucket = new PageStorageBucket(); + + Widget buildFrame(ScrollController controller) { + return new PageStorage( + bucket: bucket, + child: new ListView( + key: new UniqueKey(), // it's a different ListView every time + controller: controller, + children: new List.generate(50, (int index) { + return new Container(height: 100.0, child: new Text('Item $index')); + }).toList(), + ), + ); + } + + // keepScrollOffset: true (the default). The scroll offset is restored + // when the ListView is recreated with a new ScrollController. + + // The initialScrollOffset is used in this case, because there's no saved + // scroll offset. + ScrollController controller = new ScrollController(initialScrollOffset: 200.0); + await tester.pumpWidget(buildFrame(controller)); + expect(tester.getTopLeft(find.widgetWithText(Container, 'Item 2')), Offset.zero); + + controller.jumpTo(2000.0); + await tester.pump(); + expect(tester.getTopLeft(find.widgetWithText(Container, 'Item 20')), Offset.zero); + + // The initialScrollOffset isn't used in this case, because the scrolloffset + // can be restored. + controller = new ScrollController(initialScrollOffset: 25.0); + await tester.pumpWidget(buildFrame(controller)); + expect(controller.offset, 2000.0); + expect(tester.getTopLeft(find.widgetWithText(Container, 'Item 20')), Offset.zero); + + // keepScrollOffset: false. The scroll offset is -not- restored + // when the ListView is recreated with a new ScrollController and + // the initialScrollOffset is used. + + controller = new ScrollController(keepScrollOffset: false, initialScrollOffset: 100.0); + await tester.pumpWidget(buildFrame(controller)); + expect(controller.offset, 100.0); + expect(tester.getTopLeft(find.widgetWithText(Container, 'Item 1')), Offset.zero); + + }); }