Better use of Compatible for ease of container uniquing.

Author: rjrjr

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

@@ -54,7 +54,12 @@ class OverviewDetailContainer(view: View) : LayoutRunner<OverviewDetailScreen> {
       rendering.selectDefault!!.invoke()
     } else {
       val overviewViewEnvironment = viewEnvironment + (OverviewDetailConfig to Overview)
-      overviewStub!!.update(Named(rendering.overviewRendering, "overview"), overviewViewEnvironment)
+
+      // Without this name, the two BackStackScreen containers will try
+      // sign up with SavedStateRegistry with the same id, and crash.
+      val overviewRendering = Named(rendering.overviewRendering, "Overview")
+      overviewStub!!.update(overviewRendering, overviewViewEnvironment)
+
       rendering.detailRendering
         ?.let { detail ->
           detailStub!!.actual.visibility = VISIBLE

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

@@ -111,7 +111,7 @@ internal constructor(
       }
 
     // Create and install a new SavedStateRegistryOwner for the new view and wire it up to the
-    // view's lifecycle. We need to do this even if the StateRegistryHolder hasn't been restored
+    // view's lifecycle. We need to do this even if the StateRegistryAggregator hasn't been restored
     // yet, the view can ask for it as soon as it's attached.
     val registryOwner = KeyedStateRegistryOwner.installAsSavedStateRegistryOwnerOn(newView, newKey)
     currentOwner = registryOwner

workflow-ui/container-android/src/main/java/com/squareup/workflow1/ui/modal/ModalContainer.kt

@@ -13,6 +13,7 @@ import android.view.ViewGroup.LayoutParams.MATCH_PARENT
 import android.widget.FrameLayout
 import androidx.lifecycle.DefaultLifecycleObserver
 import androidx.lifecycle.Lifecycle
+import androidx.lifecycle.Lifecycle.State.INITIALIZED
 import androidx.lifecycle.LifecycleOwner
 import com.squareup.workflow1.ui.Compatible
 import com.squareup.workflow1.ui.ViewEnvironment
@@ -65,9 +66,13 @@ public abstract class ModalContainer<ModalRenderingT : Any> @JvmOverloads constr
     },
     onRestored = { aggregator ->
       dialogs.forEach { ref ->
-        ref.stateRegistryOwner.let {
-          aggregator.restoreRegistryControllerIfReady(it.key, it.controller)
-        }
+        // We're only allowed to restore from an INITIALIZED state, but this callback can also be
+        // invoked while the owner is already CREATED.
+        // https://github.com/square/workflow-kotlin/issues/570
+        ref.stateRegistryOwner.takeIf { it.lifecycle.currentState == INITIALIZED }
+          ?.let {
+            aggregator.restoreRegistryControllerIfReady(it.key, it.controller)
+          }
       }
       // If any keys were not in the list of dialogs, we don't want to restore them ever.
       aggregator.pruneKeys(keysToKeep = emptyList())
@@ -100,7 +105,7 @@ public abstract class ModalContainer<ModalRenderingT : Any> @JvmOverloads constr
             // don't clash with other layers.
             ref.stateRegistryOwner = KeyedStateRegistryOwner.installAsSavedStateRegistryOwnerOn(
               view = dialogView,
-              key = "${this.javaClass.name}[$i]"
+              key = Compatible.keyFor(newScreen, i.toString())
             )
 
             dialogView.addOnAttachStateChangeListener(
@@ -276,7 +281,7 @@ public abstract class ModalContainer<ModalRenderingT : Any> @JvmOverloads constr
 
     constructor(source: Parcel) : super(source) {
       @Suppress("UNCHECKED_CAST")
-      this.dialogBundles = mutableListOf<KeyAndBundle>().apply {
+      dialogBundles = mutableListOf<KeyAndBundle>().apply {
         source.readTypedList(this, KeyAndBundle)
       }
     }

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/KeyedStateRegistryOwner.kt

@@ -43,7 +43,7 @@ public class KeyedStateRegistryOwner private constructor(
       key: String
     ): KeyedStateRegistryOwner {
       val lifecycleOwner = checkNotNull(WorkflowLifecycleOwner.get(view)) {
-        "Expected back stack container view to set a WorkflowLifecycleOwner on its immediate " +
+        "Expected container view to set a WorkflowLifecycleOwner on its immediate " +
           "child views."
       }
       val registryOwner = KeyedStateRegistryOwner(key, lifecycleOwner)

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/StateRegistryAggregator.kt

@@ -82,9 +82,9 @@ public class StateRegistryAggregator(
 
       // These properties are guaranteed to be non-null because this observer is only registered
       // while attached, and these properties are always non-null while attached.
-      val restoredState = parentRegistryOwner!!.savedStateRegistry.takeIf { it.isRestored }
-        ?.consumeRestoredStateForKey(parentKey!!)
-      restoreFromBundle(restoredState)
+      parentRegistryOwner!!.savedStateRegistry.takeIf { it.isRestored }?.let {
+        restoreFromBundle(it.consumeRestoredStateForKey(parentKey!!))
+      }
     }
   }
 
@@ -127,9 +127,10 @@ public class StateRegistryAggregator(
       parentRegistry.registerSavedStateProvider(key, ::saveToBundle)
     } catch (e: IllegalArgumentException) {
       throw IllegalArgumentException(
-        "Error registering StateRegistryHolder as SavedStateProvider with key \"$key\" on parent " +
-          "SavedStateRegistryOwner $parentOwner.\n" +
-          "You might need to wrap your container screen with a Named rendering.",
+        "Error registering SavedStateProvider: key \"$key\" is already in " +
+          "use on parent SavedStateRegistryOwner $parentOwner.\n" +
+          "Give your container screen a unique Compatible.compatibilityKey, perhaps " +
+          "by wrapping it with Named.",
         e
       )
     }

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowAndroidXSupport.kt

@@ -9,6 +9,7 @@ import androidx.lifecycle.ViewTreeLifecycleOwner
 import androidx.savedstate.SavedStateRegistry
 import androidx.savedstate.SavedStateRegistryOwner
 import androidx.savedstate.ViewTreeSavedStateRegistryOwner
+import com.squareup.workflow1.ui.Compatible
 import com.squareup.workflow1.ui.Named
 import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
 import com.squareup.workflow1.ui.getRendering
@@ -77,11 +78,9 @@ public object WorkflowAndroidXSupport {
    */
   @OptIn(WorkflowUiExperimentalApi::class)
   public fun createStateRegistryKeyForContainer(containerView: View): String {
-    val namedKeyOrNull = run {
-      val rendering = containerView.getRendering<Any>() as? Named<*>
-      rendering?.compatibilityKey
-    }
-    val nameSuffix = namedKeyOrNull?.let { "-$it" } ?: ""
+    val nameSuffix = containerView.getRendering<Any>()
+      ?.let { Compatible.keyFor(it) }?.let { "-$it" }
+      ?: ""
     val idSuffix = if (containerView.id == FrameLayout.NO_ID) "" else "-${containerView.id}"
     return containerView.javaClass.name + nameSuffix + idSuffix
   }

workflow-ui/core-android/src/test/java/com/squareup/workflow1/ui/androidx/StateRegistryAggregatorTest.kt

@@ -49,7 +49,7 @@ internal class StateRegistryAggregatorTest {
     }
 
     assertThat(error).hasMessageThat()
-      .contains("Error registering StateRegistryHolder as SavedStateProvider with key \"$key\"")
+      .contains("Error registering SavedStateProvider: key \"$key\" is already in use")
   }
 
   @Test fun `attach observes parent lifecycle`() {
@@ -343,9 +343,9 @@ internal class StateRegistryAggregatorTest {
     aggregator.attachToParentRegistry("parentKey", parent)
 
     parent.lifecycleRegistry.currentState = RESUMED
-    // We used to crash here:
-    // IllegalStateException: You can consumeRestoredStateForKey only after super.onCreate
-    assertThat(restoreCount).isEqualTo(1)
+    // We once crashed here:
+    //   IllegalStateException: You can consumeRestoredStateForKey only after super.onCreate
+    assertThat(restoreCount).isEqualTo(0)
   }
 
   /**