Make Navigator restorable (inkl. WidgetsApp, MaterialApp, CupertinoApp) #65658

goderbauer · 2020-09-11T20:30:14Z

Description

WidgetsApp, MaterialApp, and CupertinoApp now take a restoration ID to enable state restoration for the app (and the Navigator build by those widgets).

The navigator now takes a restoration id to restore its state by recreating the current history stack of Routes during state restoration and by restoring the internal state of those Routes. However, not all Routes on the stack can be restored:

Page-based routes restore a restoration ID is provided to them.
Routes added with the existing imperative API (push, pushNamed, and friends) can never restore their state.
A Route added with the newly added restorable imperative API (restorablePush, restorablePushNamed, and all other imperative methods with "restorable" in their name) restores its state if all routes below it up to and including the first Page-based route below it are restored. If there is no Page-based route below it, it only restores its state if all routes below it restore theirs.

If a Route is deemed restorable by the Navigator, they will insert their own RestorationScope into the tree that the widgets in the route can use to restore their own state.

Related Issues

#62916

Tests

I added the following tests:

Restoration tests for the Navigator and the newly added restorable API.

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

Hixie · 2020-09-11T23:36:32Z

packages/flutter/lib/src/services/restoration.dart

@@ -317,6 +317,7 @@ class RestorationManager extends ChangeNotifier {
      bucket.finalize();
    }
    _bucketsNeedingSerialization.clear();
+    print(_rootBucket!._rawData); // TODO(goderbauer): revert this debug print before submission.


Hixie · 2020-09-11T23:37:55Z

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

@@ -948,6 +951,25 @@ class WidgetsApp extends StatefulWidget {
  /// {@endtemplate}
  final Map<Type, Action<Intent>> actions;

+  /// {@template flutter.widgets.widgetsApp.restorationScopeId}
+  /// Providing a restoration ID enables state restoration for the app.


there's a line above this, like, "The identifier to use when enabling state restoration for the navigator." or whatever, missing. (The first line of docs should be a noun phrase.)

Hixie · 2020-09-11T23:40:12Z

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

@@ -1470,6 +1492,7 @@ class _WidgetsAppState extends State<WidgetsApp> with WidgetsBindingObserver {
    } else {
      assert(_navigator != null);
      routing = Navigator(
+        restorationScopeId: 'nav',


why "nav"? Why not "n" or "navigator" or "\0x01" or "$Navigator" or something?

Yes. Any of those are valid as long as they are unique within the surrounding restoration scope. Here, the navigator is the only thing in this scope. Children (the individual routes) have their own scope.

So why "nav" specifically?

Hixie · 2020-09-11T23:41:31Z

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

@@ -49,6 +52,8 @@ typedef RouteBuilder<T> = Route<T> Function(BuildContext context, RouteSettings
 /// Used by [Navigator.onGenerateInitialRoutes].
 typedef RouteListFactory = List<Route<dynamic>> Function(NavigatorState navigator, String initialRoute);

+typedef RestorableRouteBuilder<T> = Route<T> Function(BuildContext context, Object arguments);


Interestingly, the analyzer didn't warn about the missing docs here. Filed https://github.com/dart-lang/linter/issues/2238.

Hixie · 2020-09-11T23:42:27Z

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

+  /// for this route.
+  ///
+  /// If the restoration scope ID changes (e.g. because restoration is enabled
+  /// or disabled) throughout the life of the route, the [ValueListenable]


throughout -> during

throughout implies it happening continually, during refers to a particular case of it happening

Hixie · 2020-09-11T23:43:36Z

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

+  /// If the restoration scope ID changes (e.g. because restoration is enabled
+  /// or disabled) throughout the life of the route, the [ValueListenable]
+  /// notifies its listeners. As an example, the ID changes to null while the
+  /// route is transitioning off screen. At that point, the route is considered


"...transitioning off screen, which triggers a notification on this field" or some such. (Right now, you say "as an example" but don't actually then say why what you describe is an example of the previous sentence)

Hixie · 2020-09-11T23:44:43Z

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

+///    friends) can never restore their state.
+///  * A [Route] added with the restorable imperative API ([restorablePush],
+///    [restorablePushNamed], and all other imperative methods with "restorable"
+///    in the name) restores its state if all routes below it up to and


"in their names"

Hixie · 2020-09-11T23:45:23Z

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

+///    is no [Page]-based route below it, it only restores its state if all
+///    routes below it restore theirs.
+///
+/// If a [Route] is deemed restorable, the [Navgiator] will set its


typo in "navigator"

Hixie · 2020-09-11T23:46:10Z

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

+  ///
+  ///  * [RestorationManager], which explains how state restoration works in
+  ///    Flutter.
+  final String restorationScopeId;


should this mention the default id that WidgetsApp uses?

WidgetsApp doesn't use a default id.

It internally specifies an ID for the Navigator it creates. However, that's an implementation detail that's not exposed to the public.

Hixie · 2020-09-11T23:46:19Z

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

@@ -2386,7 +2467,8 @@ class _NotAnnounced extends Route<void> {
 class _RouteEntry extends RouteTransitionRecord {
  _RouteEntry(
    this.route, {
-      @required _RouteLifecycle initialState,
+    @required _RouteLifecycle initialState,
+    this.restorationInformation,


indentation

Hixie · 2020-09-11T23:48:42Z

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

+  /// for it or null if restoration cannot be enabled for this route.
+  String get restorationId {
+    // User-provided restoration ids of Pages are prefixed with 'p'. Generated
+    // ids for pageless routes are prefixed with r to avoid clashes.


put some punctuation between the prefix and the id (or use something other than letters), to avoid the risk of someone being offended if they use the id "rick" or something.

lol, but good point. Added a "+".

Hixie · 2020-09-11T23:50:05Z

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

@@ -2845,6 +2985,11 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
      _effectiveObservers = widget.observers;
  }

+  // Use [_nextPagelessRestorationScopeId] to get the next id.
+  final RestorableNum<int> _rawNextPagelessRestorationScopeId = RestorableNum<int>(0);


declare this before first use

Hixie · 2020-09-11T23:52:51Z

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

+  /// The method returns an opaque ID for the pushed route that can be used by
+  /// the [RestorableRouteFuture] to gain access to the actual [Route] object
+  /// added to the navigator and its return value.
+  /// {@endtempalte}


"endtempalte" is a typo

might be good to have some sample code here

Hixie · 2020-09-11T23:53:54Z

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

@@ -3464,6 +3613,46 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
    return push<T>(_routeNamed<T>(routeName, arguments: arguments));
  }



the restorable* methods should be mentioned in their corresponding non-restorable version

Hixie · 2020-09-11T23:56:02Z

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

+/// When [present] has been called to add a route, it may only be called again
+/// after the previously added route has completed.
+///
+// TODO(goderbauer): Add a code sample that showcases how to use this.


we should do that in this PR

Hixie · 2020-09-11T23:59:29Z

Overall LGTM. Some minor comments.

chunhtai · 2020-09-25T18:20:04Z

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

+  ///
+  /// If non-null, a [RootRestorationScope] is inserted into the widget
+  /// hierarchy, which descendant widgets can use to store and restore their
+  /// state (for example by using the [RestorationMixin]).


Does user have chance to use the RootRestorationScope? isn't this only used by the navigator widget or router widget?

chunhtai · 2020-09-25T20:55:17Z

packages/flutter/lib/src/cupertino/app.dart

@@ -317,6 +319,9 @@ class CupertinoApp extends StatefulWidget {
  /// {@macro flutter.widgets.widgetsApp.actions.seeAlso}
  final Map<Type, Action<Intent>> actions;

+  /// {@macro flutter.widgets.widgetsApp.restorationScopeId}
+  final String restorationScopeId;


should this be nullable String?

here and other places

At the time the PR was created, this file had not been migrated to NNBD, so this was correct. I am going to rebase this to the latest master, though, and then it will have to be nullable, yes.

In other unmigrated files it cannot be marked as nullable just yet.

chunhtai · 2020-09-25T21:51:31Z

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

+/// }
+/// ```
+/// {@end-tool}
+class RestorableRouteFuture<T> extends RestorableProperty<String?> {


Couldn't the restorablePush just return the restorable future? if we asked user to provide their own restorationid to the route, I think we can probably restore this future with the logic similar to the _hookOntoRouteFuture?

After state restoration, how would the entity that pushed the route get access to the restorableRouteFuture again that was returned when the route was initially pushed?

Is this because there is no way to restore the future chain unless all the call back are static functions? Restore the route future is possible I think? but then caller will have to reattach their future chain again.

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

goderbauer

All comments addressed. PTAL

goderbauer · 2020-09-28T22:20:11Z

packages/flutter/lib/src/cupertino/app.dart

@@ -317,6 +319,9 @@ class CupertinoApp extends StatefulWidget {
  /// {@macro flutter.widgets.widgetsApp.actions.seeAlso}
  final Map<Type, Action<Intent>> actions;

+  /// {@macro flutter.widgets.widgetsApp.restorationScopeId}
+  final String restorationScopeId;


At the time the PR was created, this file had not been migrated to NNBD, so this was correct. I am going to rebase this to the latest master, though, and then it will have to be nullable, yes.

goderbauer · 2020-09-28T22:24:32Z

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

+  ///
+  /// If non-null, a [RootRestorationScope] is inserted into the widget
+  /// hierarchy, which descendant widgets can use to store and restore their
+  /// state (for example by using the [RestorationMixin]).


goderbauer · 2020-09-28T22:25:12Z

packages/flutter/lib/src/cupertino/app.dart

@@ -317,6 +319,9 @@ class CupertinoApp extends StatefulWidget {
  /// {@macro flutter.widgets.widgetsApp.actions.seeAlso}
  final Map<Type, Action<Intent>> actions;

+  /// {@macro flutter.widgets.widgetsApp.restorationScopeId}
+  final String restorationScopeId;


In other unmigrated files it cannot be marked as nullable just yet.

goderbauer · 2020-09-28T22:28:01Z

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

+/// }
+/// ```
+/// {@end-tool}
+class RestorableRouteFuture<T> extends RestorableProperty<String?> {


After state restoration, how would the entity that pushed the route get access to the restorableRouteFuture again that was returned when the route was initially pushed?

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

Hixie · 2020-09-30T23:50:53Z

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

+  ///
+  /// ```dart
+  /// void _switchToAudioVolume() {
+  ///   Navigator.restorablePushReplacementNamed(context, '/settings/volume');


If it's the same API as pushReplacementNamed, does it need to be different?

for example, don't we need to be doing something with the return value?

You only have to do something with the return value if you want to get access to the return value of the route or the route object itself. If you don't care about any of this, the usage is exactly the same. See also the flutter.widgets.navigator.restorablePushNamed.returnValue docs.

I also added some clearer language around this to the doc.

Hixie · 2020-09-30T23:51:49Z

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

+  ///
+  /// {@macro flutter.widgets.navigator.restorablePushNamed.returnValue}
+  @optionalTypeArgs
+  static String restorablePush<T extends Object?>(BuildContext context, RestorableRouteBuilder<T> routeBuilder, {Object? arguments}) {


is this supposed to return String?

never mind i didn't see the @macro flutter.widgets.navigator.restorablePushNamed.returnValue.

Instead of a String I could make this one return an opaque RouteId object, if that sounds better...

Hixie · 2020-09-30T23:55:15Z

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

+  /// ```
+  /// {@end-tool}
+  ///
+  /// {@macro flutter.widgets.navigator.restorablePushNamed.returnValue}


this should probably be before the sample

Hixie · 2020-09-30T23:55:48Z

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

+  ///       title: const Text('Sample Code'),
+  ///     ),
+  ///     floatingActionButton: FloatingActionButton(
+  ///       onPressed: () => Navigator.restorablePush(context, _myRouteBuilder),


this seems to discard the return value

which is fine, see new and improved doc on return value.

Hixie · 2020-09-30T23:57:21Z

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

+  ///
+  /// ```dart
+  /// void _openCalendar() {
+  ///   navigator.restorablePushNamedAndRemoveUntil('/calendar', ModalRoute.withName('/'));


this also ignores the return value

which is fine, see new and improved doc on return value.

Hixie

LGTM modulo some docs questions

chunhtai

LGTM

chunhtai · 2020-10-01T18:38:33Z

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

+
+  @override
+  // TODO(goderbauer): remove the kIsWeb check when https://github.com/flutter/flutter/issues/33615 is resolved.
+  bool get isRestorable => !kIsWeb;


Does the restoration framework sends update if the platform is web?

The web embedder currently does not turn on restoration.

fluttergithubbot · 2020-10-02T00:52:03Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Mac framework_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Sep 11, 2020

googlebot added the cla: yes label Sep 11, 2020

goderbauer mentioned this pull request Sep 11, 2020

☂️ State Restoration Remaining Work #62916

Closed

24 tasks

Hixie reviewed Sep 11, 2020

View reviewed changes

goderbauer force-pushed the restorableNavigator branch from 9a16c33 to 16c0487 Compare September 22, 2020 18:39

goderbauer changed the title ~~Make Navigator restorable~~ Make Navigator restorable (inkl. WidgetsApp, MaterialApp, CupertinoApp) Sep 22, 2020

goderbauer force-pushed the restorableNavigator branch 2 times, most recently from 4be8695 to 47d24b6 Compare September 23, 2020 21:07

goderbauer marked this pull request as ready for review September 23, 2020 23:56

goderbauer requested review from chunhtai and Hixie September 23, 2020 23:57

chunhtai reviewed Sep 25, 2020

View reviewed changes

goderbauer force-pushed the restorableNavigator branch from ca06bd5 to b299b10 Compare September 28, 2020 22:29

goderbauer commented Sep 28, 2020

View reviewed changes

goderbauer removed the work in progress; do not review label Sep 29, 2020

Hixie reviewed Sep 30, 2020

View reviewed changes

Hixie approved these changes Sep 30, 2020

View reviewed changes

chunhtai approved these changes Oct 1, 2020

View reviewed changes

goderbauer added 12 commits October 1, 2020 13:18

WIP

1c395d5

whitespace

479ace9

++

d12efe4

++

421d48a

++

e37852e

++

a76417b

++

7735f84

more examples

19d74b1

++

8425584

++

a8a1714

chunhtai review

b179939

Ian review

9f0856a

goderbauer force-pushed the restorableNavigator branch from b299b10 to 9f0856a Compare October 1, 2020 20:22

doc fix

857efb9

goderbauer added the waiting for tree to go green label Oct 1, 2020

fluttergithubbot removed the waiting for tree to go green label Oct 2, 2020

goderbauer added the waiting for tree to go green label Oct 2, 2020

fluttergithubbot merged commit fc85492 into flutter:master Oct 2, 2020

goderbauer deleted the restorableNavigator branch October 2, 2020 19:55

		@@ -3464,6 +3613,46 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
		return push<T>(_routeNamed<T>(routeName, arguments: arguments));
		}

Make Navigator restorable (inkl. WidgetsApp, MaterialApp, CupertinoApp) #65658

Make Navigator restorable (inkl. WidgetsApp, MaterialApp, CupertinoApp) #65658

Uh oh!

Conversation

goderbauer commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Sep 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Sep 11, 2020 •

edited

Loading