[State Restoration] [Reply] Partially State Restorable Pages #368

shihaohong · 2020-11-05T11:00:44Z

This makes most of the pages in the Reply app state restorable.

This PR does not yet include restoration of scrollable offset location, which I will complete in a separate PR.

State Restoration for Flutter is currently only enabled for Android (adding support for iOS is tracked in flutter/flutter#62915). In order to test the state restoration aspect of this app follow these steps:

Enable "Don't keep activities" in the developer settings of your Android device.
Run the app on the Android device via flutter run.
Navigate into the Reply app. Open up the search page or navigate to a different mailbox.
Switch to the home screen or another app via the app switcher. This kills the gallery app.
Switch back the gallery app via the app switcher. This will restart the gallery app.
Observe that the app is still on the original page.

This change required modifications to the email store logic and mail view logic since the state was so tightly coupled that it was difficult to make it state restorable without decoupling it. As nice side effects, the change removes duplicated state in the code, improves starring/unstarring mail handling. For example, before this change, starred emails would not appear in the other mailboxes (inbox, sent, draft), because it had its own unique category. Now, starred emails will still appear on those aforementioned pages.

Outstanding issue

State restoration is blocked on the mail view and compose pages due to the use of OpenContainer, which does not support the restorable Navigator APIs: flutter/flutter#69924. This is causing a crash when using an Android device when:

On the Reply app, open a specific email. (ie. Inbox -> tap on any email by tapping the preview card)
Switch to the home screen or another app via the app switcher. This kills the gallery app.
Navigate around the different mailboxes, which will eventually trigger the crash:


════════ Exception caught by widgets library ═══════════════════════════════════
The following assertion was thrown building Navigator-[LabeledGlobalKey<NavigatorState>#37c7a](dirty, dependencies: [UnmanagedRestorationScope, HeroControllerScope, _EffectiveTickerMode], state: NavigatorState#2a1b0(tickers: tracking 1 ticker)):
'package:flutter/src/widgets/navigator.dart': Failed assertion: line 5104 pos 12: 'ild(BuildContext co': is not true.

Either the assertion indicates an error in the framework itself, or we should provide substantially more information in this error message to help you determine and fix the underlying cause.
In either case, please report this assertion by filing a bug on GitHub:
  https://github.com/flutter/flutter/issues/new?template=2_bug.md

The relevant error-causing widget was
Navigator-[LabeledGlobalKey<NavigatorState>#37c7a]
lib/…/reply/adaptive_nav.dart:1062
When the exception was thrown, this was the stack
#2      NavigatorState.build
package:flutter/…/widgets/navigator.dart:5104
#3      StatefulElement.build
package:flutter/…/widgets/framework.dart:4792
#4      ComponentElement.performRebuild
package:flutter/…/widgets/framework.dart:4675
#5      StatefulElement.performRebuild
package:flutter/…/widgets/framework.dart:4847
#6      Element.rebuild
package:flutter/…/widgets/framework.dart:4369
...

…ation

goderbauer

This is looking great!

Can you also add some tests to ensure that we don't break state restoration for this app in the future? You can probably take some inspirations from the tests I added here: flutter/samples#433

goderbauer · 2020-11-09T18:39:14Z

lib/studies/reply/model/email_model.dart

+
+// A set of the different mailbox pages that the Reply app
+// contains. An enum would be preferable, but only primitives
+// values are state restorable.


I agree, an enum would be better: Can you use an enum and store the index of the enum value (available via the .index getter on an enum value) in the restoration state? To restore the original enum, you can use that index as an index into the values constant of the enum.

goderbauer · 2020-11-09T18:41:22Z

lib/studies/reply/mail_card_preview.dart

-      listen: false,
-    ).isEmailStarred(email);
-
+    // TODO(shihaohong): State restoration of mail view page is


Oh, did you by any chance take a look at OpenContainer to see what it would take to make that restorable?

Discussion continued in flutter/flutter#69924.

Can you add an issue link here?

goderbauer · 2020-11-09T18:57:16Z

I saw you also updated flutter/flutter#62916 with the additional work we need to do! Thanks! Can you also link these migration PRs against the issue so we have them all in one place? Thanks!

goderbauer · 2020-11-09T19:15:44Z

We also need to investigate the crash you are reporting in the PR description. Even if OpenContainer doesn't support state restoration, it shouldn't crash...

…ation

perclasson

LGTM!

perclasson · 2020-11-11T12:56:32Z

lib/studies/reply/adaptive_nav.dart

+          // TODO(shihaohong): State restoration of compose page on mobile is
+          // blocked because OpenContainer does not support restorablePush.


Can you add an issue link here?

perclasson · 2020-11-11T12:56:42Z

lib/studies/reply/mail_card_preview.dart

-      listen: false,
-    ).isEmailStarred(email);
-
+    // TODO(shihaohong): State restoration of mail view page is


Can you add an issue link here?

lib/studies/reply/app.dart

shihaohong added 8 commits November 5, 2020 18:59

Partially restorable pages on Reply app

5210946

Flutter format

1d4f053

decouple email data state from view state

2a2ab31

flutter format

5632e7d

Remove print statement

f18729d

Use ids to track trash and starred emails

52eca77

restorable starred and trash states

8ba73e8

Merge branch 'master' of github.com:flutter/gallery into state-restor…

53f5701

…ation

shihaohong requested a review from goderbauer November 9, 2020 17:47

Fix navigation rail

28d8379

goderbauer reviewed Nov 9, 2020

View reviewed changes

shihaohong mentioned this pull request Nov 10, 2020

☂️ State Restoration Remaining Work flutter/flutter#62916

Closed

24 tasks

shihaohong added 5 commits November 11, 2020 00:24

Code review feedback

b67398d

Merge branch 'master' of github.com:flutter/gallery into state-restor…

92e2a56

…ation

flutter format

f2929fe

Add unit test for state restoration - Reply app

2b3b28d

Flutter format

8d2307c

shihaohong requested a review from rami-a November 10, 2020 17:23

Rename test slightly

3a9d91a

shihaohong requested a review from perclasson November 11, 2020 10:06

perclasson approved these changes Nov 11, 2020

View reviewed changes

shihaohong and others added 2 commits November 11, 2020 21:21

Add issue link

9b2f6bf

Merge branch 'master' into state-restoration

9a58e45

shihaohong merged commit e764888 into flutter:master Nov 12, 2020

shihaohong deleted the state-restoration branch November 12, 2020 00:37

shihaohong changed the title ~~[Reply] Partially State Restorable Pages~~ [State Restoration] [Reply] Partially State Restorable Pages Feb 22, 2021

Renzo-Olivares mentioned this pull request Feb 28, 2021

[Reply] Fade through transition between navigation rail destinations broken #446

Closed

guidezpl mentioned this pull request Dec 9, 2021

Reply UI bugs #567

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[State Restoration] [Reply] Partially State Restorable Pages #368

[State Restoration] [Reply] Partially State Restorable Pages #368

shihaohong commented Nov 5, 2020 •

edited

Loading

goderbauer left a comment

goderbauer Nov 9, 2020

goderbauer Nov 9, 2020

shihaohong Nov 10, 2020

perclasson Nov 11, 2020

goderbauer commented Nov 9, 2020

goderbauer commented Nov 9, 2020

perclasson left a comment

perclasson Nov 11, 2020

perclasson Nov 11, 2020

		// TODO(shihaohong): State restoration of compose page on mobile is
		// blocked because OpenContainer does not support restorablePush.

[State Restoration] [Reply] Partially State Restorable Pages #368

[State Restoration] [Reply] Partially State Restorable Pages #368

Conversation

shihaohong commented Nov 5, 2020 • edited Loading

Outstanding issue

goderbauer left a comment

Choose a reason for hiding this comment

goderbauer Nov 9, 2020

Choose a reason for hiding this comment

goderbauer Nov 9, 2020

Choose a reason for hiding this comment

shihaohong Nov 10, 2020

Choose a reason for hiding this comment

perclasson Nov 11, 2020

Choose a reason for hiding this comment

goderbauer commented Nov 9, 2020

goderbauer commented Nov 9, 2020

perclasson left a comment

Choose a reason for hiding this comment

perclasson Nov 11, 2020

Choose a reason for hiding this comment

perclasson Nov 11, 2020

Choose a reason for hiding this comment

shihaohong commented Nov 5, 2020 •

edited

Loading