Skip to content

Commit 3633994

Browse files
rjrjrrjrjr
committed
Tightens up the API related to ViewTreeSavedStateRegistryOwner.
`StateRegistryAggregator` is renamed `WorkflowSavedStateRegistryAggregator`, and now works only with `KeyedSavedStateRegistryOwner`, rather than any old `SavedStateRegistryOwner`. In fact, `WorkflowSavedStateRegistryAggregator` is the now only way to create instances of `KeyedSavedStateRegistryOwner`, which is no longer public API. This eliminates the possibility of forgetting to make the initial call to `restoreChildRegistryOwnerIfReady`. One slightly worrisome change is that `StateRegistryAggregator` now keeps a map of all the `KeyedStateRegistryOwner` instances it creates, theoretically increasing the chances that we'll leak them. Prior to this they were 1:1 with the associated views, with the idea that they wouldn't need to be cleaned up that way. (Note how the new map allows us to eliminate `onWillSave` and `onRestored` from the constructor.) In reality, even before this change we still had to be careful to always call `prune` or `save` as managed views were dropped, so we weren't avoiding any bookkeeping. And there was a decent amount of crucial copy/pasted boilerplate surrounding each use of `KeyedStateRegistryOwner`, which caused real bugs.
1 parent b40a30a commit 3633994

File tree

19 files changed

+604
-673
lines changed

19 files changed

+604
-673
lines changed

‎samples/containers/android/src/main/java/com/squareup/sample/container/overviewdetail/OverviewDetailContainer.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ class OverviewDetailContainer(view: View) : LayoutRunner<OverviewDetailScreen> {
5656
val overviewViewEnvironment = viewEnvironment + (OverviewDetailConfig to Overview)
5757

5858
// Without this name, the two BackStackScreen containers will try
59-
// sign up with SavedStateRegistry with the same id, and crash.
59+
// to sign up with SavedStateRegistry with the same id, and crash.
6060
val overviewRendering = Named(rendering.overviewRendering, "Overview")
6161
overviewStub!!.update(overviewRendering, overviewViewEnvironment)
6262

‎samples/tictactoe/app/src/androidTest/java/com/squareup/sample/TicTacToeEspressoTest.kt

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ import androidx.test.espresso.action.ViewActions.typeText
1111
import androidx.test.espresso.assertion.ViewAssertions.matches
1212
import androidx.test.espresso.matcher.ViewMatchers.hasDescendant
1313
import androidx.test.espresso.matcher.ViewMatchers.isDisplayed
14+
import androidx.test.espresso.matcher.ViewMatchers.withClassName
1415
import androidx.test.espresso.matcher.ViewMatchers.withId
16+
import androidx.test.espresso.matcher.ViewMatchers.withParent
17+
import androidx.test.espresso.matcher.ViewMatchers.withParentIndex
1518
import androidx.test.espresso.matcher.ViewMatchers.withText
1619
import androidx.test.ext.junit.rules.ActivityScenarioRule
1720
import androidx.test.ext.junit.runners.AndroidJUnit4
@@ -29,6 +32,8 @@ import com.squareup.workflow1.ui.internal.test.DetectLeaksAfterTestSuccess
2932
import com.squareup.workflow1.ui.internal.test.IdlingDispatcherRule
3033
import com.squareup.workflow1.ui.internal.test.actuallyPressBack
3134
import com.squareup.workflow1.ui.internal.test.inAnyView
35+
import org.hamcrest.CoreMatchers.allOf
36+
import org.hamcrest.CoreMatchers.endsWith
3237
import org.junit.After
3338
import org.junit.Before
3439
import org.junit.Rule
@@ -190,6 +195,38 @@ class TicTacToeEspressoTest {
190195
inAnyView(withId(R.id.login_email)).check(matches(withText("foo@2fa")))
191196
}
192197

198+
/**
199+
* On tablets this revealed a problem with SavedStateRegistry.
200+
* https://github.com/square/workflow-kotlin/pull/656#issuecomment-1027274391
201+
*/
202+
@Test fun fullJourney() {
203+
inAnyView(withId(R.id.login_email)).type("foo@bar")
204+
inAnyView(withId(R.id.login_password)).type("password")
205+
inAnyView(withId(R.id.login_button)).perform(click())
206+
207+
inAnyView(withId(R.id.start_game)).perform(click())
208+
209+
clickCell(0)
210+
clickCell(3)
211+
clickCell(1)
212+
clickCell(4)
213+
clickCell(2)
214+
215+
inAnyView(withText(R.string.exit)).perform(click())
216+
inAnyView(withId(R.id.start_game)).check(matches(isDisplayed()))
217+
actuallyPressBack()
218+
inAnyView(withId(R.id.login_email)).check(matches(isDisplayed()))
219+
}
220+
221+
private fun clickCell(index: Int) {
222+
inAnyView(
223+
allOf(
224+
withParent(withClassName(endsWith("GridLayout"))),
225+
withParentIndex(index)
226+
)
227+
).perform((click()))
228+
}
229+
193230
private fun ViewInteraction.type(text: String) {
194231
perform(typeText(text), closeSoftKeyboard())
195232
}

‎workflow-ui/compose/src/androidTest/java/com/squareup/workflow1/ui/compose/ComposeViewTreeIntegrationTest.kt

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,7 @@ internal class ComposeViewTreeIntegrationTest {
479479
"Counter[$layer][$screen]: $counter",
480480
Modifier
481481
.clickable { counter++ }
482-
.testTag("[$layer][$screen]")
482+
.testTag("L${layer}S$screen")
483483
)
484484
}
485485

@@ -500,11 +500,14 @@ internal class ComposeViewTreeIntegrationTest {
500500
)
501501
}
502502

503-
composeRule.onNodeWithText("Counter[0][0]: 0")
503+
composeRule.onNodeWithTag("L0S0")
504+
.assertTextEquals("Counter[0][0]: 0")
504505
.assertIsDisplayed()
505506
.performClick()
506507
.assertTextEquals("Counter[0][0]: 1")
507-
composeRule.onNodeWithText("Counter[1][0]: 0")
508+
509+
composeRule.onNodeWithTag("L1S0")
510+
.assertTextEquals("Counter[1][0]: 0")
508511
.assertIsDisplayed()
509512
.performClick()
510513
.assertTextEquals("Counter[1][0]: 1")
@@ -521,15 +524,19 @@ internal class ComposeViewTreeIntegrationTest {
521524
)
522525
}
523526

524-
composeRule.onNodeWithText("Counter[0][0]", substring = true)
527+
composeRule.onNodeWithTag("L0S0")
525528
.assertDoesNotExist()
526-
composeRule.onNodeWithText("Counter[1][0]", substring = true)
529+
composeRule.onNodeWithTag("L1S0")
527530
.assertDoesNotExist()
528-
composeRule.onNodeWithText("Counter[0][1]: 0")
531+
532+
composeRule.onNodeWithTag("L0S1")
533+
.assertTextEquals("Counter[0][1]: 0")
529534
.assertIsDisplayed()
530535
.performClick()
531536
.assertTextEquals("Counter[0][1]: 1")
532-
composeRule.onNodeWithText("Counter[1][1]: 0")
537+
538+
composeRule.onNodeWithTag("L1S1")
539+
.assertTextEquals("Counter[1][1]: 0")
533540
.assertIsDisplayed()
534541
.performClick()
535542
.assertTextEquals("Counter[1][1]: 1")
@@ -538,9 +545,9 @@ internal class ComposeViewTreeIntegrationTest {
538545
scenario.recreate()
539546

540547
// Check that the last-shown screens were restored.
541-
composeRule.onNodeWithText("Counter[0][1]: 1")
548+
composeRule.onNodeWithTag("L0S1")
542549
.assertIsDisplayed()
543-
composeRule.onNodeWithText("Counter[1][1]: 1")
550+
composeRule.onNodeWithTag("L1S1")
544551
.assertIsDisplayed()
545552

546553
// Pop both backstacks and check that screens were restored.

‎workflow-ui/compose/src/androidTest/java/com/squareup/workflow1/ui/compose/RenderAsStateTest.kt

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ import kotlinx.coroutines.job
3636
import kotlinx.coroutines.test.TestCoroutineScope
3737
import okio.ByteString
3838
import okio.ByteString.Companion.decodeBase64
39-
import org.junit.Ignore
4039
import org.junit.Rule
4140
import org.junit.Test
4241
import org.junit.rules.RuleChain
@@ -255,7 +254,6 @@ internal class RenderAsStateTest {
255254
}
256255
}
257256

258-
@Ignore("https://github.com/square/workflow-kotlin/issues/504")
259257
@Test fun runtimeIsCancelledWhenCompositionFails() {
260258
var innerJob: Job? = null
261259
val workflow = Workflow.stateless<Unit, Nothing, Unit> {

‎workflow-ui/container-android/api/container-android.api

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public final class com/squareup/workflow1/ui/backstack/ViewStateCache : android/
3737
public static final field CREATOR Lcom/squareup/workflow1/ui/backstack/ViewStateCache$CREATOR;
3838
public fun <init> ()V
3939
public fun <init> (Ljava/util/Map;)V
40-
public final fun attachToParentRegistry (Ljava/lang/String;Landroidx/savedstate/SavedStateRegistryOwner;)V
40+
public final fun attachToParentRegistryOwner (Ljava/lang/String;Landroidx/savedstate/SavedStateRegistryOwner;)V
4141
public fun describeContents ()I
4242
public final fun detachFromParentRegistry ()V
4343
public final fun getViewStates$wf1_container_android ()Ljava/util/Map;
@@ -130,21 +130,22 @@ public abstract class com/squareup/workflow1/ui/modal/ModalContainer : android/w
130130
}
131131

132132
protected final class com/squareup/workflow1/ui/modal/ModalContainer$DialogRef {
133-
public field stateRegistryOwner Lcom/squareup/workflow1/ui/androidx/KeyedStateRegistryOwner;
133+
public field savedStateRegistryKey Ljava/lang/String;
134134
public fun <init> (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/app/Dialog;Ljava/lang/Object;)V
135135
public synthetic fun <init> (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/app/Dialog;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
136-
public final fun copy (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;)Lcom/squareup/workflow1/ui/modal/ModalContainer$DialogRef;
136+
public final fun copy (Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/app/Dialog;Ljava/lang/Object;)Lcom/squareup/workflow1/ui/modal/ModalContainer$DialogRef;
137+
public static synthetic fun copy$default (Lcom/squareup/workflow1/ui/modal/ModalContainer$DialogRef;Ljava/lang/Object;Lcom/squareup/workflow1/ui/ViewEnvironment;Landroid/app/Dialog;Ljava/lang/Object;ILjava/lang/Object;)Lcom/squareup/workflow1/ui/modal/ModalContainer$DialogRef;
137138
public final fun dismiss$wf1_container_android ()V
138139
public fun equals (Ljava/lang/Object;)Z
139140
public final fun getDialog ()Landroid/app/Dialog;
140141
public final fun getExtra ()Ljava/lang/Object;
141142
public final fun getModalRendering ()Ljava/lang/Object;
142-
public final fun getStateRegistryOwner$wf1_container_android ()Lcom/squareup/workflow1/ui/androidx/KeyedStateRegistryOwner;
143+
public final fun getSavedStateRegistryKey$wf1_container_android ()Ljava/lang/String;
143144
public final fun getViewEnvironment ()Lcom/squareup/workflow1/ui/ViewEnvironment;
144145
public fun hashCode ()I
145146
public final fun restore$wf1_container_android (Lcom/squareup/workflow1/ui/modal/ModalContainer$KeyAndBundle;)V
146147
public final fun save$wf1_container_android ()Lcom/squareup/workflow1/ui/modal/ModalContainer$KeyAndBundle;
147-
public final fun setStateRegistryOwner$wf1_container_android (Lcom/squareup/workflow1/ui/androidx/KeyedStateRegistryOwner;)V
148+
public final fun setSavedStateRegistryKey$wf1_container_android (Ljava/lang/String;)V
148149
}
149150

150151
public final class com/squareup/workflow1/ui/modal/ModalContainer$KeyAndBundle : android/os/Parcelable {

‎workflow-ui/container-android/src/androidTest/java/com/squareup/workflow1/ui/backstack/test/BackstackContainerTest.kt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,10 @@ internal class BackstackContainerTest {
6868
}
6969
}
7070

71+
/**
72+
* Note that this test passes on older AVDs. It fails reliably on API 32 w/o the fix for
73+
* https://github.com/square/workflow-kotlin/issues/570
74+
*/
7175
@Test fun state_restores_after_recreate_without_crashing() {
7276
assertThat(scenario.state).isEqualTo(RESUMED)
7377

‎workflow-ui/container-android/src/androidTest/java/com/squareup/workflow1/ui/backstack/test/ViewStateCacheTest.kt

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import com.google.common.truth.Truth.assertThat
1010
import com.squareup.workflow1.ui.Named
1111
import com.squareup.workflow1.ui.ViewEnvironment
1212
import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
13-
import com.squareup.workflow1.ui.androidx.KeyedStateRegistryOwner
1413
import com.squareup.workflow1.ui.androidx.WorkflowLifecycleOwner
1514
import com.squareup.workflow1.ui.backstack.ViewStateCache
1615
import com.squareup.workflow1.ui.backstack.ViewStateFrame
@@ -65,11 +64,14 @@ internal class ViewStateCacheTest {
6564

6665
// Set some state on the first view that will be saved.
6766
firstView.viewState = "hello world"
68-
// Cache expects there to be a registry owner on old views.
69-
KeyedStateRegistryOwner.installAsSavedStateRegistryOwnerOn(firstView, "first")
67+
68+
// Show the first screen.
69+
cache.update(retainedRenderings = emptyList(), oldViewMaybe = null, newView = firstView)
7070

7171
// "Navigate" to the second screen, saving the first screen.
72-
cache.update(listOf(firstRendering), oldViewMaybe = firstView, newView = secondView)
72+
cache.update(
73+
retainedRenderings = listOf(firstRendering), oldViewMaybe = firstView, newView = secondView
74+
)
7375

7476
// Nothing should read this value again, but clear it to make sure.
7577
firstView.viewState = "ignored"
@@ -89,14 +91,17 @@ internal class ViewStateCacheTest {
8991
// Android requires ID to be set for view hierarchy to be saved or restored.
9092
val firstView = createTestView(firstRendering, id = 1)
9193
val secondView = createTestView(secondRendering)
92-
// Cache expects there to be a registry owner on old views.
93-
KeyedStateRegistryOwner.installAsSavedStateRegistryOwnerOn(firstView, "first")
94+
95+
// Show the first screen.
96+
cache.update(retainedRenderings = emptyList(), oldViewMaybe = null, newView = firstView)
9497

9598
// Set some state on the first view that will be saved.
9699
firstView.viewState = "hello world"
97100

98101
// "Navigate" to the second screen, saving the first screen.
99-
cache.update(listOf(firstRendering), oldViewMaybe = firstView, newView = secondView)
102+
cache.update(
103+
retainedRenderings = listOf(firstRendering), oldViewMaybe = firstView, newView = secondView
104+
)
100105

101106
// Nothing should read this value again, but clear it to make sure.
102107
firstView.viewState = "ignored"
@@ -122,8 +127,9 @@ internal class ViewStateCacheTest {
122127

123128
// Set some state on the first view that will be saved.
124129
firstView.viewState = "hello world"
125-
// Cache expects there to be a registry owner on old views.
126-
KeyedStateRegistryOwner.installAsSavedStateRegistryOwnerOn(firstView, "first")
130+
131+
// Show the first screen.
132+
cache.update(retainedRenderings = emptyList(), oldViewMaybe = null, newView = firstView)
127133

128134
// "Navigate" to the second screen, saving the first screen.
129135
cache.update(listOf(firstRendering), oldViewMaybe = firstView, newView = secondView)

‎workflow-ui/container-android/src/main/java/com/squareup/workflow1/ui/backstack/BackStackContainer.kt

Lines changed: 7 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,18 @@ import android.view.ViewGroup
99
import android.view.ViewGroup.LayoutParams.MATCH_PARENT
1010
import android.view.animation.AccelerateDecelerateInterpolator
1111
import android.widget.FrameLayout
12-
import androidx.savedstate.SavedStateRegistry
13-
import androidx.savedstate.ViewTreeSavedStateRegistryOwner
1412
import androidx.transition.Scene
1513
import androidx.transition.Slide
1614
import androidx.transition.TransitionManager
1715
import androidx.transition.TransitionSet
1816
import com.squareup.workflow1.ui.BuilderViewFactory
17+
import com.squareup.workflow1.ui.Compatible
1918
import com.squareup.workflow1.ui.Named
2019
import com.squareup.workflow1.ui.ViewEnvironment
2120
import com.squareup.workflow1.ui.ViewFactory
2221
import com.squareup.workflow1.ui.ViewRegistry
2322
import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
24-
import com.squareup.workflow1.ui.androidx.WorkflowAndroidXSupport.createStateRegistryKeyForContainer
25-
import com.squareup.workflow1.ui.androidx.WorkflowAndroidXSupport.requireStateRegistryOwnerFromViewTreeOrContext
23+
import com.squareup.workflow1.ui.androidx.WorkflowAndroidXSupport.stateRegistryOwnerFromViewTreeOrContext
2624
import com.squareup.workflow1.ui.androidx.WorkflowLifecycleOwner
2725
import com.squareup.workflow1.ui.backstack.BackStackConfig.First
2826
import com.squareup.workflow1.ui.backstack.BackStackConfig.Other
@@ -31,27 +29,11 @@ import com.squareup.workflow1.ui.buildView
3129
import com.squareup.workflow1.ui.canShowRendering
3230
import com.squareup.workflow1.ui.compatible
3331
import com.squareup.workflow1.ui.container.R
32+
import com.squareup.workflow1.ui.getRendering
3433
import com.squareup.workflow1.ui.showRendering
3534
import com.squareup.workflow1.ui.start
3635

37-
/**
38-
* A container view that can display a stream of [BackStackScreen] instances.
39-
*
40-
* This container supports saving and restoring the view state of each of its subviews corresponding
41-
* to the renderings in its [BackStackScreen]. It supports two distinct state mechanisms:
42-
* 1. Classic view hierarchy state ([View.onSaveInstanceState]/[View.onRestoreInstanceState])
43-
* 2. AndroidX [SavedStateRegistry] via [ViewTreeSavedStateRegistryOwner].
44-
*
45-
* ## A note about `SavedStateRegistry` support.
46-
*
47-
* The [SavedStateRegistry] API involves defining string keys to associate with state bundles. These
48-
* keys must be unique relative to the instance of the registry they are saved in. To support this
49-
* requirement, [BackStackContainer] tries to generate a best-effort unique key by using
50-
* [createStateRegistryKeyForContainer]. See the kdoc on that function for
51-
* more information. If you need to nest multiple [BackStackContainer]s under the same
52-
* `SavedStateRegistry`, just wrap each [BackStackScreen] with a [Named], or give each
53-
* [BackStackContainer] a unique view ID.
54-
*/
36+
/** A container view that can display a stream of [BackStackScreen] instances. */
5537
@WorkflowUiExperimentalApi
5638
public open class BackStackContainer @JvmOverloads constructor(
5739
context: Context,
@@ -179,9 +161,9 @@ public open class BackStackContainer @JvmOverloads constructor(
179161
super.onAttachedToWindow()
180162

181163
// Wire up our viewStateCache to our parent SavedStateRegistry.
182-
val parentRegistryOwner = requireStateRegistryOwnerFromViewTreeOrContext(this)
183-
val key = createStateRegistryKeyForContainer(this)
184-
viewStateCache.attachToParentRegistry(key, parentRegistryOwner)
164+
val parentRegistryOwner = stateRegistryOwnerFromViewTreeOrContext(this)
165+
val key = Compatible.keyFor(this.getRendering()!!)
166+
viewStateCache.attachToParentRegistryOwner(key, parentRegistryOwner)
185167
}
186168

187169
override fun onDetachedFromWindow() {

0 commit comments

Comments
 (0)