Revert "Improved support for saving and restoring the scroll offset, etc (#10517)" (#10575)

This reverts commit c8e4cbf2.

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 PageStorageKey<Entry>(root),
+      key: new ValueKey<Entry>(root),
       title: new Text(root.title),
       children: root.children.map(_buildTiles).toList(),
     );

packages/flutter/lib/src/widgets/framework.dart

@@ -322,10 +322,10 @@ class LabeledGlobalKey<T extends State<StatefulWidget>> extends GlobalKey<T> {
 
   @override
   String toString() {
-    final String label = _debugLabel != null ? ' $_debugLabel' : '';
+    final String tag = _debugLabel != null ? ' $_debugLabel' : '#$hashCode';
     if (runtimeType == LabeledGlobalKey)
-      return '[GlobalKey#$hashCode$label]';
-    return '[$runtimeType#$hashCode$label]';
+      return '[GlobalKey$tag]';
+    return '[$runtimeType$tag]';
   }
 }
 

packages/flutter/lib/src/widgets/page_storage.dart

@@ -6,67 +6,43 @@ 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<String>(tab.text), // like 'Tab 1'
-///       tab: tab,
-///    ),
-///  }),
-///)
-/// ```
-class PageStorageKey<T> extends ValueKey<T> {
-  /// Creates a [ValueKey] that defines where [PageStorage] values will be saved.
-  const PageStorageKey(T value) : super(value);
-}
-
 class _StorageEntryIdentifier {
-  _StorageEntryIdentifier(this.clientType, this.keys) {
-    assert(clientType != null);
-    assert(keys != null);
+  Type clientType;
+  List<Key> keys;
+
+  void addKey(Key key) {
+    assert(key != null);
+    assert(key is! GlobalKey);
+    keys ??= <Key>[];
+    keys.add(key);
   }
 
-  final Type clientType;
-  final List<PageStorageKey<dynamic>> keys;
+  GlobalKey scopeKey;
 
   @override
   bool operator ==(dynamic other) {
-    if (other.runtimeType != runtimeType)
+    if (other is! _StorageEntryIdentifier)
       return false;
     final _StorageEntryIdentifier typedOther = other;
-    if (clientType != typedOther.clientType || keys.length != typedOther.keys.length)
+    if (clientType != typedOther.clientType ||
+        scopeKey != typedOther.scopeKey ||
+        keys?.length != typedOther.keys?.length)
       return false;
-    for (int index = 0; index < keys.length; index += 1) {
-      if (keys[index] != typedOther.keys[index])
-        return false;
+    if (keys != null) {
+      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, hashList(keys));
+  int get hashCode => hashValues(clientType, scopeKey, hashList(keys));
 
   @override
   String toString() {
-    return 'StorageEntryIdentifier($clientType, ${keys?.join(":")})';
+    return 'StorageEntryIdentifier($clientType, $scopeKey, ${keys?.join(":")})';
   }
 }
 
@@ -75,26 +51,27 @@ class _StorageEntryIdentifier {
 /// Useful for storing per-page state that persists across navigations from one
 /// page to another.
 class PageStorageBucket {
-  bool _maybeAddKey(BuildContext context, List<PageStorageKey<dynamic>> keys) {
-    final Widget widget = context.widget;
-    final Key key = widget.key;
-    if (key is PageStorageKey)
-      keys.add(key);
-    return widget is! PageStorage;
-  }
-
-  List<PageStorageKey<dynamic>> _allKeys(BuildContext context) {
-    final List<PageStorageKey<dynamic>> keys = <PageStorageKey<dynamic>>[];
-    if (_maybeAddKey(context, keys)) {
+  _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);
       context.visitAncestorElements((Element element) {
-        return _maybeAddKey(element, keys);
+        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 result;
     }
-    return keys;
-  }
-
-  _StorageEntryIdentifier _computeIdentifier(BuildContext context) {
-    return new _StorageEntryIdentifier(context.widget.runtimeType, _allKeys(context));
+    assert(lastKey is GlobalKey);
+    result.scopeKey = lastKey;
+    return result;
   }
 
   Map<Object, dynamic> _storage;
@@ -112,13 +89,13 @@ class PageStorageBucket {
   /// identifier will change.
   void writeState(BuildContext context, dynamic data, { Object identifier }) {
     _storage ??= <Object, dynamic>{};
-    _storage[identifier ?? _computeIdentifier(context)] = data;
+    _storage[identifier ?? _computeStorageIdentifier(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 ?? _computeIdentifier(context)] : null;
+    return _storage != null ? _storage[identifier ?? _computeStorageIdentifier(context)] : null;
   }
 }
 

packages/flutter/lib/src/widgets/page_view.dart

@@ -38,36 +38,17 @@ import 'viewport.dart';
 class PageController extends ScrollController {
   /// Creates a page controller.
   ///
-  /// The [initialPage], [keepPage], and [viewportFraction] arguments must not be null.
+  /// The [initialPage] 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
@@ -135,7 +116,6 @@ class PageController extends ScrollController {
       physics: physics,
       context: context,
       initialPage: initialPage,
-      keepPage: keepPage,
       viewportFraction: viewportFraction,
       oldPosition: oldPosition,
     );
@@ -170,11 +150,9 @@ 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,
@@ -183,7 +161,6 @@ class _PagePosition extends ScrollPositionWithSingleContext {
          physics: physics,
          context: context,
          initialPixels: null,
-         keepScrollOffset: keepPage,
          oldPosition: oldPosition,
        );
 

packages/flutter/lib/src/widgets/scroll_controller.dart

@@ -39,40 +39,20 @@ import 'scroll_position_with_single_context.dart';
 class ScrollController extends ChangeNotifier {
   /// Creates a controller for a scrollable widget.
   ///
-  /// The values of `initialScrollOffset` and `keepScrollOffset` must not be null.
+  /// The [initialScrollOffset] must not be null.
   ScrollController({
     this.initialScrollOffset: 0.0,
-    this.keepScrollOffset: true,
     this.debugLabel,
-  }) : assert(initialScrollOffset != null),
-       assert(keepScrollOffset != null);
+  }) : assert(initialScrollOffset != 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
-  /// if [keepScrollOffset] is false or a scroll offset hasn't been saved yet.
+  /// controller will have their offset initialized to this value.
   ///
   /// 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;
@@ -224,7 +204,6 @@ class ScrollController extends ChangeNotifier {
       physics: physics,
       context: context,
       initialPixels: initialScrollOffset,
-      keepScrollOffset: keepScrollOffset,
       oldPosition: oldPosition,
       debugLabel: debugLabel,
     );

packages/flutter/lib/src/widgets/scroll_position.dart

@@ -61,22 +61,17 @@ 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(keepScrollOffset != null) {
+       assert(context.vsync != null) {
     if (oldPosition != null)
       absorb(oldPosition);
-    if (keepScrollOffset)
-      restoreScrollOffset();
+    restoreScrollOffset();
   }
 
   /// How the scroll position should respond to user input.
@@ -90,15 +85,6 @@ 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;
@@ -553,8 +539,7 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics {
   /// This also saves the scroll offset using [saveScrollOffset].
   void didEndScroll() {
     activity.dispatchScrollEndNotification(cloneMetrics(), context.notificationContext);
-    if (keepScrollOffset)
-      saveScrollOffset();
+    saveScrollOffset();
   }
 
   /// Called by [setPixels] to report overscroll when an attempt is made to

packages/flutter/lib/src/widgets/scroll_position_with_single_context.dart

@@ -46,24 +46,13 @@ 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,
-         keepScrollOffset: keepScrollOffset,
-         oldPosition: oldPosition,
-         debugLabel: debugLabel,
-       ) {
+  }) : super(physics: physics, context: context, 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)

packages/flutter/test/widgets/page_view_test.dart

@@ -441,14 +441,12 @@ 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: controller2,
+          controller: controller,
           children: <Widget>[
             const Placeholder(),
             const Placeholder(),
@@ -457,6 +455,6 @@ void main() {
         ),
       ),
     );
-    expect(controller2.page, 0);
+    expect(controller.page, 0);
   });
 }

packages/flutter/test/widgets/scroll_controller_test.dart

@@ -259,51 +259,4 @@ 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<Widget>.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);
-
-  });
 }