Fix dropping back stack retained state on Android Activity rotations #1063
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrisbanes
force-pushed
the
cb/retained-navigation-recreate
branch
from
0161ea8
to
187d090
Compare
chrisbanes
changed the title
chrisbanes
force-pushed
the
cb/retained-navigation-recreate
branch
from
63872fe
to
cabf667
Compare
This reverts commit 190d8e7.
We store the RetainedStateRegistries for each back stack entry into an 'navigation content' RetainedStateRegistry. If we don't do this, those registries would be stored directly in the current LocalRetainedStateRegistry value, which will mostly likely be the continuityRetainedStateRegistry. On Android, that continuityRetainedStateRegistry will drop any 'unclaimed' values when the host Activity is recreated. Since records on the back stack aren't attached to composition, they can't claim their retained registries and thus we drop all of the state for the record. See slackhq#1046. Using this 'navigation content' registry means that _it_ will be stored in the continuityRetainedStateRegistry instead, and any back stack record registries stored within the 'navigation content' registry. The difference is that NavigableCircuitContent will be attached to composition for the entire lifetime that we care about, and thus will be able to save/claim the 'navigation content' registry on recreations. Since any back stack registries are nested in this 'navigation content' registry, everything is saved/claimed correctly.
…ons for good measure
chrisbanes
force-pushed
the
cb/retained-navigation-recreate
branch
from
7b2c8ff
to
5e40c70
Compare
ZacSweers
approved these changes
Dec 20, 2023
| if (backstack.size > 0) { | ||
| if (backstack.isEmpty) return | ||
|
|
||
| /* |
|
I tried porting this to Robolectric but couldn't get the original issue to fail. Rotations/recreations are tricky to simulate so not surprising tbh. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We store the RetainedStateRegistries for each back stack entry into an 'navigation content'
RetainedStateRegistry. If we don't do this, those registries would be stored directly in the currentLocalRetainedStateRegistryvalue, which will mostly likely be thecontinuityRetainedStateRegistry. On Android, thatcontinuityRetainedStateRegistrywill drop any 'unclaimed' values when the host Activity is recreated. Since records on the back stack aren't attached to composition, they can't claim their retained registries and thus we drop all of the state for the record.Using this 'navigation content' registry means that it will be stored in the
continuityRetainedStateRegistryinstead, and any back stack record registries stored within the 'navigation content' registry. The difference is thatNavigableCircuitContentwill be attached to composition for the entire lifetime that we care about, and thus will be able to save/claim the 'navigation content' registry on recreations. Since any back stack registries are nested in this 'navigation content' registry, everything is saved/claimed correctly.Added an Android instrumentation test to stop this regressing in the future.
Fixes #1046