Make Scrollables restorable #63131
Make Scrollables restorable #63131
Conversation
b96f1d2 to
84e6337
Compare
84e6337 to
a3d72db
Compare
| void _doSerialization() { | ||
| Future<void> _doSerialization() { | ||
| if (!_serializationScheduled) { | ||
| return Future<void>.value(null); |
There was a problem hiding this comment.
just make the function async and return;.
There was a problem hiding this comment.
though... why do you not return the same future as the other call to doSerialization?
Or throw? Seems weird to do nothing when called redundantly, and return a future that completes before the earlier "real" call...
|
|
||
| for (final RestorationBucket bucket in _bucketsNeedingSerialization) { | ||
| bucket.finalize(); | ||
| } | ||
| _bucketsNeedingSerialization.clear(); | ||
| sendToEngine(_encodeRestorationData(_rootBucket!._rawData)); | ||
| final Future<void> result = sendToEngine(_encodeRestorationData(_rootBucket!._rawData)); |
|
|
||
| for (final RestorationBucket bucket in _bucketsNeedingSerialization) { | ||
| bucket.finalize(); | ||
| } | ||
| _bucketsNeedingSerialization.clear(); | ||
| sendToEngine(_encodeRestorationData(_rootBucket!._rawData)); | ||
| final Future<void> result = sendToEngine(_encodeRestorationData(_rootBucket!._rawData)); | ||
|
|
||
| assert(() { | ||
| _debugDoingUpdate = false; |
There was a problem hiding this comment.
making this async is... exciting
| /// Restoration data is automatically flushed to the engine at the end of a | ||
| /// frame. Since a change in restoration data is usually accompanied by | ||
| /// scheduling a frame (e.g. because the restoration data is modified inside a | ||
| /// [State.setState] call) it is uncommon to call this method directly. |
There was a problem hiding this comment.
took me a while to understand these two sentences. How about:
A change in restoration data is usually accompanied by scheduling a frame (because the restoration data is modified inside a [State.setState] call, because it is usually something that affects the interface). Restoration data is automatically flushed to the engine at the end of a frame. As a result, it is uncommon to need to call this method directly.
| /// frame. Since a change in restoration data is usually accompanied by | ||
| /// scheduling a frame (e.g. because the restoration data is modified inside a | ||
| /// [State.setState] call) it is uncommon to call this method directly. | ||
| /// However if restoration data is changed without triggering a frame this |
| /// scheduling a frame (e.g. because the restoration data is modified inside a | ||
| /// [State.setState] call) it is uncommon to call this method directly. | ||
| /// However if restoration data is changed without triggering a frame this | ||
| /// method must be called to ensure that the updated restoration data is send |
| /// [State.setState] call) it is uncommon to call this method directly. | ||
| /// However if restoration data is changed without triggering a frame this | ||
| /// method must be called to ensure that the updated restoration data is send | ||
| /// to the engine in a timely manner. An example for such a use case is the |
| /// However if restoration data is changed without triggering a frame this | ||
| /// method must be called to ensure that the updated restoration data is send | ||
| /// to the engine in a timely manner. An example for such a use case is the | ||
| /// [Scrollable], where the final to be persisted scroll offset after a scroll |
There was a problem hiding this comment.
"final to be persisted scroll offset" is dubious, not sure what you mean
| /// Calling this method is a no-op if a frame is already scheduled. In that | ||
| /// case, the restoration data will be flushed to the engine at the end of | ||
| /// that frame. If this method is called and no frame is scheduled, the | ||
| /// current restoration data is directly send to the engine. |
| /// that frame. If this method is called and no frame is scheduled, the | ||
| /// current restoration data is directly send to the engine. | ||
| Future<void> flushData() async { | ||
| assert(!_debugDoingUpdate); |
There was a problem hiding this comment.
It just ensures that you don't call flushData from a RestorationBucket's finalize method (which is called during serialization).
There was a problem hiding this comment.
ah now that it's not async that works
|
|
||
| @override | ||
| void didUpdateValue(double oldValue) { | ||
| notifyListeners(); |
There was a problem hiding this comment.
RestorableValue only calls didUpdateValue if old and new value are different.
|
LGTM modulo the async logic in the restoration code |
|
I removed the async logic in the restoration code. It's not necessary and was just leftover from a different approach. |
|
I'll intend to land this when the tree goes green unless I hear otherwise. |
|
This pull request is not suitable for automatic merging in its current state.
|
Description
Enabled the Scrollable widgets (ListView, GridView, PageView, etc) to restore their scroll offset with the state restoration framework.
The change is slightly awkward because Scrollables already had a restoration mechanism with
PageStorage. The plan is to eventually replace that mechanism with the new state restoration approach once state restoration supports keeping state information around even when the owning widget is getting disposed.Related Issues
#62916
Tests
I added the following tests:
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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.