Improve docs for [PageStorage] by xu-baolin · Pull Request #63634 · flutter/flutter

xu-baolin · 2020-08-13T07:12:47Z

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 give PageStorageKey or set keepScrollOffset false, the behavior of 'PageStorage' is strange. like this #62332

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

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

flutter-dashboard · 2020-08-13T07:12:49Z

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.

dkwingsmt · 2020-08-13T23:51:01Z

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

+/// [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,


Are you sure if it's only when they appear in the same route, or should it be global unique?

Not need global unique, but should be equal between rebuilding.

If you like you can give them the same key, it works well. Because the identifier is a key chain

Actually, "in the same route" is not precise enough, isn't it?

Suggested change

/// 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,

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.

NICE suggesting.

dkwingsmt

LGTM

dkwingsmt

A few suggestions after a 2nd read through; feel free to correct me if I'm wrong.

(Sorry for deleting your comment by mistake)

dkwingsmt · 2020-08-14T01:42:02Z

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

+/// [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,


Actually, "in the same route" is not precise enough, isn't it?

Suggested change

/// 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,

dkwingsmt · 2020-08-14T01:43:47Z

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

+/// [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,


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.

dkwingsmt · 2020-08-14T01:50:44Z

@HansMuller Was it you who wrote PageStorageKey? I wonder why it isn't the default behavior that each scrollable uses an independent key. I would expect scrolling position overriding each other to be the rare use case here.

xu-baolin · 2020-08-14T02:03:49Z

@HansMuller Was it you who wrote PageStorageKey? I wonder why it isn't the default behavior that each scrollable uses an independent key. I would expect scrolling position overriding each other to be the rare use case here.

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

dkwingsmt

LGTM, nice job

dkwingsmt · 2020-08-14T02:47:46Z

Let's get this PR merged and discuss the changing proposal in another issue.

Improve docs for [PageStorage] flutter#62332

d0c9fde

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 13, 2020

googlebot added the cla: yes label Aug 13, 2020

HansMuller requested a review from dkwingsmt August 13, 2020 23:49

dkwingsmt requested changes Aug 13, 2020

View reviewed changes

xu-baolin requested a review from dkwingsmt August 14, 2020 01:10

dkwingsmt approved these changes Aug 14, 2020

View reviewed changes

flutter deleted a comment from xu-baolin Aug 14, 2020

dkwingsmt requested changes Aug 14, 2020

View reviewed changes

Improve docs

b42b8e9

dkwingsmt approved these changes Aug 14, 2020

View reviewed changes

dkwingsmt added the waiting for tree to go green label Aug 18, 2020

Merge branch 'master' of github.com:flutter/flutter into 0812

e14d521

fluttergithubbot merged commit 2f2130a into flutter:master Aug 21, 2020

smadey pushed a commit to smadey/flutter that referenced this pull request Aug 27, 2020

Improve docs for [PageStorage] (flutter#63634)

462824b

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

Improve docs for [PageStorage] (flutter#63634)

1cfdfbf

markusaksli-nc mentioned this pull request Sep 18, 2020

PageStorage strange behavior without describe in docs #63656

Open

xu-baolin mentioned this pull request Sep 30, 2020

Improve pageStorage behavior #66634

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2021

	/// 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,

Conversation

xu-baolin commented Aug 13, 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!

flutter-dashboard bot commented Aug 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xu-baolin Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dkwingsmt commented Aug 14, 2020

Uh oh!

xu-baolin commented Aug 14, 2020

Uh oh!

dkwingsmt left a comment

Choose a reason for hiding this comment

Uh oh!

dkwingsmt commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xu-baolin commented Aug 13, 2020 •

edited

Loading

xu-baolin Aug 14, 2020 •

edited

Loading

dkwingsmt left a comment •

edited

Loading