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
Improve docs for [PageStorage] #63634
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
/// [PageStorageKey] is used by [Scrollable] if [ScrollController.keepScrollOffset] | ||
/// is enabled to save their [ScrollPosition]s. When more than one | ||
/// scrollable ([ListView], [SingleChildScrollView], [TextField], etc.) appears | ||
/// in the same route, if you want to save all of their positions, |
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.
Are you sure if it's only when they appear in the same route, or should it be global unique?
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.
Not need global unique, but should be equal between rebuilding.
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.
If you like you can give them the same key, it works well. Because the identifier is a key chain
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.
Actually, "in the same route" is not precise enough, isn't it?
/// in the same route, if you want to save all of their positions, | |
/// within the widget's closest ancestor [PageStorage] (such as within the same route), if you want to save all of their positions, |
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.
Also "if you want to save all of their positions" should be "if you want to save all of their positions independently" followed by "unique [PageStorageKey]s". Otherwise, their positions ARE stored, but overridden by each other.
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.
NICE suggesting.
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.
Done.
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.
LGTM
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.
A few suggestions after a 2nd read through; feel free to correct me if I'm wrong.
(Sorry for deleting your comment by mistake)
/// [PageStorageKey] is used by [Scrollable] if [ScrollController.keepScrollOffset] | ||
/// is enabled to save their [ScrollPosition]s. When more than one | ||
/// scrollable ([ListView], [SingleChildScrollView], [TextField], etc.) appears | ||
/// in the same route, if you want to save all of their positions, |
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.
Actually, "in the same route" is not precise enough, isn't it?
/// in the same route, if you want to save all of their positions, | |
/// within the widget's closest ancestor [PageStorage] (such as within the same route), if you want to save all of their positions, |
/// [PageStorageKey] is used by [Scrollable] if [ScrollController.keepScrollOffset] | ||
/// is enabled to save their [ScrollPosition]s. When more than one | ||
/// scrollable ([ListView], [SingleChildScrollView], [TextField], etc.) appears | ||
/// in the same route, if you want to save all of their positions, |
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.
Also "if you want to save all of their positions" should be "if you want to save all of their positions independently" followed by "unique [PageStorageKey]s". Otherwise, their positions ARE stored, but overridden by each other.
@HansMuller Was it you who wrote |
I have the same doubt, Such an implementation also will cause some strange behavior when modifying the middle of the key chain, like this #63656 |
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.
LGTM, nice job
Let's get this PR merged and discuss the changing proposal in another issue. |
Description
Improve docs for
[PageStorage]
Because the default value of
[ScrollController.keepScrollOffset]
is true, When more than one scrollable ([ListView], [SingleChildScrollView], [TextField], etc.
) appears in the same route, if forgot to givePageStorageKey
or setkeepScrollOffset
false, the behavior of 'PageStorage' is strange. like this #62332For example:
We have three scrollable widgets in one route(The sample code see #63656 )
A -> B -> C
Case 1:
If we only want to save A's state, we should do this:
A(key:PageStorageKey, keepScrollOffset:true) -> B(key:null, keepScrollOffset:false)-> C(key:null, keepScrollOffset:false)
Case 2:
If we want to save all of their states, we should do this:
A(key1:PageStorageKey, keepScrollOffset:true) -> B(key2:PageStorageKey, keepScrollOffset:true)-> C(key3:PageStorageKey, keepScrollOffset:true)
A _StorageEntryIdentifier:[key1]
B _StorageEntryIdentifier:[key2,key1]
C _StorageEntryIdentifier:[key3,key2,key1]
So, in this situation, the key1, key2, and key3 can be equal.
Case 3:
Wrong code demo:
If we only want to save A's state, but we do below(keepScrollOffset default value is true):
A(key:PageStorageKey, keepScrollOffset:true) -> B(key:null, keepScrollOffset:true)-> C(key:null, keepScrollOffset:true)
A _StorageEntryIdentifier:[key1]
B _StorageEntryIdentifier:[key1]
C _StorageEntryIdentifier:[key1]
They have the same _StorageEntryIdentifier, so the save position will be overridden by each other.
Related Issues
#62332
Tests
Not needed.
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.