-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
State Restoration support for Veggie Seasons app #433
Conversation
veggieseasons/lib/main.dart
Outdated
// CupertinoApp/MaterialApp. However, in this particular app there are widgets | ||
// above the CupertinoApp that need their state restored, so we are injecting | ||
// the root scope manually above everything. | ||
runApp(RestorationScope.root( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code isn't on master yet is it? Are there non-root constructors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code currently lives in a personal branch of mine. I am in the process of cleaning that up and then will make it available.
There are other constructors. The default one just starts a new child-scope within its parent scope. The root one actually inserts the data received from the engine into the tree.
veggieseasons/lib/main.dart
Outdated
@override | ||
void initState() { | ||
super.initState(); | ||
register(appState, const RestorationId('state')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this name be more tightly scoped? register
sounds really broad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am open to suggestions :)
However, it was meant to have a somewhat broader scope because, after this call the RestorationMixin manages the registered proeprty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @xster's note. Something like registerForRestoration
would help readers know immediately what the call is doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea I just had: restoreProperty
because this is actually the call that retrieves the serialized value from the restoration state, deserializes it, and puts it into the property.
class _SearchScreenState extends State<SearchScreen> { | ||
final controller = TextEditingController(); | ||
class _SearchScreenState extends State<SearchScreen> with RestorationMixin { | ||
RestorableTextEditingController controller = RestorableTextEditingController(TextEditingController()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wow, neat
title: 'Calorie Target', | ||
), | ||
); | ||
Navigator.of(context).restorablePush<void>(_calorieSettingsScreen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could the .of happen to be implemented by a RestorableNavigator? Or it's intended for the user to choose whether each navigation change is restorable or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no RestorableNavigator class. Just like any other built-in widget, the Navigator will attempt to restore its state if it has been configured with a restoration id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really neat. I have a bunch of questions. 😄
- What is your plan for this PR? Are you actually wanting to land it, or is this mostly an experiment as you work through your ideas?
- Does this code need to be in the SDK itself, or could it be distributed as a plugin?
- Is there a design doc for the API?
- Have you worked through how the usage would be for someone building an app with a state management library like redux or BLoC?
veggieseasons/lib/main.dart
Outdated
@override | ||
void initState() { | ||
super.initState(); | ||
register(appState, const RestorationId('state')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @xster's note. Something like registerForRestoration
would help readers know immediately what the call is doing.
controller.addListener(_onTextChanged); | ||
terms = controller.value.text; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to just route the properties directly inside the RestorableTextEditingController
class:
String get text => _value.text;
in order to make terms = controller.value.text
back into terms = controller.text
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting idea. I think that may be possible. Let me experiment with that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this didn't end up being feasible/advisable? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is doable, but ultimately, I thought it was more consistent if a RestorableFoo always wraps a value of type Foo. That's how it has to be for the primitive values int, double, String, etc. So I also extended that to the object type values for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the comment about AppState, merging the two also makes it harder to throw good/proper error messages when you forget to register the class.
Took the liberty of DO NOT SUBMIT => WIP, since that's the check our repo runs. |
@RedBrogdon Here are answers to your questions:
Eventually, I would like to submit this to have one example app with state restoration fully enabled. However, before that can happen, I need to finish the framework side API for state restoration and submit that to the flutter repository. In that process, this PR will probably change a little bit. If having this PR open for a long time is a problem, I am happy to close this. Opening this PR seems like the easiest way to solicit early feedback on API usage.
My plan is to add the state restoration functionality directly to the Flutter SDK. In theory, the functionality itself could be implemented in a plugin, but we also want to enable state restoration for the widgets that ship with flutter. To do that, the SDK would have to depend on this plugin, which is awkward and something we try to avoid. Therefore, I am just going to implement this directly in the framework.
There is a half-backed one. I intent to finish that one up and make it available to the public to collect more feedback soon. I'll let you know.
I have not. If the state held by those libraries is serializable, it should just work™. Do we have an example redux/BLoC app somewhere to which I could add state restoration to prove that theory? |
@goderbauer answers inline
The only issue with a long open PR is the usual maintenance concerns with updates. If you don't mind us pressing the sync button on the PR as we land maintenance changes around this PR, then I have no concerns. This style of early preview on new APIs is gold from my POV.
SGTM
Here's the world's simplest Provider app, which should hopefully be a good stepping stone to things like redux and BLoC: https://github.com/flutter/samples/tree/master/provider_counter |
I've made some updates to incorporate the feedback I've gotten so far and to allow restoring to a different serialized state while the app is running (instead of just initializing the app from serialized state when it is launched). |
veggieseasons/lib/main.dart
Outdated
|
||
// A custom RestorableProperty that saves and restores the data in an [AppState] | ||
// object. | ||
class RestorableAppState extends RestorableListenable<AppState> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is RestorableListenable a RestorableProperty? If so it would be nice if it had Property in the name for clarity.
Ditto RestorableAppState.
/// Current app state. This is used to fetch veggie data. | ||
AppState appState; | ||
|
||
/// The veggie trivia about which to show. | ||
Veggie veggie; | ||
|
||
/// Index of the current trivia question. | ||
int triviaIndex = 0; | ||
RestorableNum<int> triviaIndex = RestorableNum<int>(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend just having RestorableInt and RestorableDouble, it's cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(they can just be defined subclasses of RestorableNum)
@override | ||
void didUpdateValue(PlayerStatus oldValue) { | ||
notifyListeners(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make a RestorableEnum ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally I looked into that, but since enums don't have a common base class other than Object I don't see a good way to do that.
This looks great. What's the testability story? |
The design document for the restoration API is available here: http://flutter.dev/go/state-restoration-design |
f8872fb
to
de329d5
Compare
I updated this PR to the latest implementation of the state restoration framework in Flutter. It requires a version of flutter that's newer than this commit: flutter/flutter@053ebf2. @RedBrogdon Is this something we can land in this repository as an example of how to make an app restorable? If so, I will add some tests to ensure that this doesn't break in the future. I also updated the PR description to explain how one can observe state restoration in this app in action. |
I'm in a quandary. On the one hand, Having state restoration represented in the samples repo would be a clear win, and VeggieSeasons is a great way to do it. On the other, samples in the repo (other than the ones in We could:
WDYT? I would lean toward the second or third one. |
I understand your quandary. :) Given these options, I would prefer number 3:
That way, there continues to be just one canonical copy of the app and we don't have to deal with multiple forks/copies that may become out of date and cause merge conflicts once state restoration fully lands on flutter stable. If you're cool with that, I would adapt the PR to implement this option. |
59e4ba4
to
3cec26c
Compare
@RedBrogdon This one is ready for review now. For a more sane review experience, I'd recommend reviewing the commits separately. The first one makes the app restorable, the second one moves it to experimental. |
3cec26c
to
1136d46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few questions, but that's it. :)
controller.addListener(_onTextChanged); | ||
terms = controller.value.text; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this didn't end up being feasible/advisable? 😄
1136d46
to
c7d3f1a
Compare
This PR enables state restoration for the Veggie Seasons app using Flutter's state restoration framework (Design Doc: http://flutter.dev/go/state-restoration-design).
The changes in this PR require a Flutter version after commit flutter/flutter@053ebf2.
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:
flutter create .
. This generates the boilerplate required to run the veggie seasons app on Android.flutter run
.Part of flutter/flutter#62916.
Background
When an app is backgrounded on e.g. Android, the OS may kill the app at any time to reclaim resources. When the user switches back to the app, the app is relaunched and expected to restore the same state it had when it was killed. For that, apps can serialize out state information before they are killed and that information is handed back to them when they are relaunched.
Summary
In this PR, widget
State
objects that want some of their properties restored when the app is relaunched mixin theRestorationMixin
. They then use (subclasses of)RestorableProperty
to store information that they need restored. When the app is killed, the current value of theRestorableProperty
is serialized out and when the app relaunches, the property magically has its old value again. App developer need to serialize out enough information to rebuild the same widget tree that was active when the app was killed.Those
RestorableProperty
s in aState
object need to be bound to a restoration id by callingRestorationMixin.registerForRestoration
in therestoreState
method. The serialized data of a property is stored under that id and the id is used again to pull the correct data for a given property out of the app's serialized state again when the app is relaunched.The
restoreState
method added toState
objects by theRestorationMixin
is called afterinitState
. In this method,RestorableProperty
should be registered to initialize them with the data from the serialized state. Other initialization code, that depends on the value of aRestorableProperty
, should also run here. TherestoreState
method will be called again if the app is asked to restore from different serialized state while it is running (this is currently uncommon, but may be relevant for some use cases on the web).The framework comes with built-in
RestorableProperty
subclasses to store common values like an int, double, String, TextEditingController, etc. Apps can create their own subclasses to serialize and deserialize more complex custom objects.Widgets, that ship with the framework, will have the functionality to restore state built-in. In order to enable it, app developers need to provide a restoration id to those widgets. The widgets will store their data under this id in the surrounding restoration scope.
Restoration ids need to be unique in their surrounding restoration scope. A new restoration scope can be established by inserting a
RestorationScope
widget into the tree.