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

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

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

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

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

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

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

"...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

"in their names"

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

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

typo in "navigator"

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

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

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

indentation

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

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

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

declare this before first use

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

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

"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

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

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

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

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

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

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

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

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

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

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

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

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

this should probably be before the sample

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

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

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

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

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

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

Copy link

Contributor

Hixie Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi

Hixie reviewed Sep 11, 2020

View reviewed changes

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

Copy link

Contributor

Hixie Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"in their names"

goderbauer reacted with thumbs up emoji

Hixie reviewed Sep 11, 2020

View reviewed changes

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

Copy link

Contributor

Hixie Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in "navigator"

goderbauer reacted with thumbs up emoji

Hixie reviewed Sep 11, 2020

View reviewed changes

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

Copy link

Contributor

Hixie Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

goderbauer reacted with thumbs up emoji

Hixie reviewed Sep 11, 2020

View reviewed changes

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

Copy link

Contributor

Hixie Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declare this before first use

goderbauer reacted with thumbs up emoji

Hixie reviewed Sep 11, 2020

View reviewed changes

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

Copy link

Contributor

Hixie Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should do that in this PR

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 Hixie and chunhtai 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

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