Merge pull request #1202 from square/ray/view-state-is-trash

Stop pretending everyone implements view state right.

Author: rjrjr

workflow-ui/core-android/src/androidTest/java/com/squareup/workflow1/ui/navigation/ViewStateCacheTest.kt

@@ -106,6 +106,38 @@ internal class ViewStateCacheTest {
     assertThat(firstViewRestored.viewState).isEqualTo("hello world")
   }
 
+  @Test fun doesnt_restore_child_states_on_error() {
+    val cache = ViewStateCache()
+    val firstRendering = NamedScreen(content = AScreen, name = "first")
+    val secondRendering = NamedScreen(content = AScreen, name = "second")
+    // Android requires ID to be set for view hierarchy to be saved or restored.
+    val firstView = createTestView(firstRendering, id = 1)
+    val secondView = createTestView(secondRendering)
+
+    // Set some state on the first view that will be saved.
+    firstView.viewState = "hello world"
+
+    // Show the first screen.
+    cache.update(retainedRenderings = emptyList(), oldHolderMaybe = null, newHolder = firstView)
+
+    // "Navigate" to the second screen, saving the first screen.
+    cache.update(
+      retainedRenderings = listOf(firstRendering),
+      oldHolderMaybe = firstView,
+      newHolder = secondView
+    )
+
+    // "Navigate" back to the first screen, restoring state.
+    val firstViewRestored = createTestView(firstRendering, id = 1, crashOnRestore = true)
+    firstViewRestored.viewState = "should not be clobbered"
+
+    cache.update(listOf(), oldHolderMaybe = secondView, newHolder = firstViewRestored)
+    // We didn't crash, that's already a good sign.
+
+    // Check that the "hard coded" view state is in tact due to the swallowed exception.
+    assertThat(firstViewRestored.viewState).isEqualTo("should not be clobbered")
+  }
+
   @Test fun doesnt_restore_state_when_restored_view_id_is_different() {
     val cache = ViewStateCache()
     val firstRendering = NamedScreen(content = AScreen, name = "first")
@@ -198,9 +230,10 @@ internal class ViewStateCacheTest {
 
   private fun createTestView(
     firstRendering: NamedScreen<*>,
-    id: Int? = null
+    id: Int? = null,
+    crashOnRestore: Boolean = false
   ): ScreenViewHolder<NamedScreen<*>> {
-    val view = ViewStateTestView(instrumentation.context).also { view ->
+    val view = ViewStateTestView(instrumentation.context, crashOnRestore).also { view ->
       id?.let { view.id = id }
       WorkflowLifecycleOwner.installOn(view, fakeOnBack)
     }

workflow-ui/core-android/src/androidTest/java/com/squareup/workflow1/ui/navigation/fixtures/ViewStateTestView.kt

@@ -13,7 +13,10 @@ import com.squareup.workflow1.ui.navigation.fixtures.BackStackContainerLifecycle
  * [onSaveInstanceState] and [onRestoreInstanceState] methods.
  */
 @OptIn(WorkflowUiExperimentalApi::class)
-internal class ViewStateTestView(context: Context) : LeafView<LeafRendering>(context) {
+internal class ViewStateTestView(
+  context: Context,
+  private val crashOnRestore: Boolean = false
+) : LeafView<LeafRendering>(context) {
 
   var viewState: String = ""
 
@@ -23,6 +26,7 @@ internal class ViewStateTestView(context: Context) : LeafView<LeafRendering>(con
   }
 
   override fun onRestoreInstanceState(state: Parcelable?) {
+    if (crashOnRestore) throw IllegalArgumentException("Crashing on restore.")
     (state as? SavedState)?.let {
       viewState = it.viewState
       super.onRestoreInstanceState(state.superState)
@@ -43,7 +47,10 @@ internal class ViewStateTestView(context: Context) : LeafView<LeafRendering>(con
       viewState = source.readString()!!
     }
 
-    override fun writeToParcel(out: Parcel, flags: Int) {
+    override fun writeToParcel(
+      out: Parcel,
+      flags: Int
+    ) {
       super.writeToParcel(out, flags)
       out.writeString(viewState)
     }

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

@@ -6,6 +6,7 @@ import android.os.Parcel
 import android.os.Parcelable
 import android.os.Parcelable.Creator
 import android.util.AttributeSet
+import android.util.Log
 import android.util.SparseArray
 import android.widget.FrameLayout
 import androidx.lifecycle.Lifecycle
@@ -65,7 +66,11 @@ public class WorkflowLayout(
     showing.show(rootScreen, environment.withOnBackDispatcher())
     restoredChildState?.let { restoredState ->
       restoredChildState = null
-      showing.actual.restoreHierarchyState(restoredState)
+      try {
+        showing.actual.restoreHierarchyState(restoredState)
+      } catch (e: Exception) {
+        Log.w("Workflow", "WorkflowLayout failed to restore view state", e)
+      }
     }
   }
 

workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/navigation/ViewStateCache.kt

@@ -5,6 +5,7 @@ import android.os.Build.VERSION_CODES
 import android.os.Parcel
 import android.os.Parcelable
 import android.os.Parcelable.Creator
+import android.util.Log
 import android.util.SparseArray
 import android.view.View
 import androidx.annotation.VisibleForTesting
@@ -57,8 +58,8 @@ internal constructor(
 
   /**
    * @param retainedRenderings the renderings to be considered hidden after this update. Any
-   * associated view state will be retained in the cache, possibly to be restored to [newView]
-   * on a succeeding call to his method. Any other cached view state will be dropped.
+   * associated view state will be retained in the cache, possibly to be restored to the view
+   * of [newHolder] on a succeeding call to his method. Any other cached view state will be dropped.
    *
    * @param oldHolderMaybe the view that is being removed, if any, which is expected to be showing
    * a [NamedScreen] rendering. If that rendering is
@@ -90,7 +91,13 @@ internal constructor(
     stateRegistryAggregator.installChildRegistryOwnerOn(newHolder.view, newKey)
 
     viewStates.remove(newKey)
-      ?.let { newHolder.view.restoreHierarchyState(it.viewState) }
+      ?.let {
+        try {
+          newHolder.view.restoreHierarchyState(it.viewState)
+        } catch (e: Exception) {
+          Log.w("Workflow", "ViewStateCache failed to restore view state for $newKey", e)
+        }
+      }
 
     // Save both the view state and state registry of the view that's going away, as long as it's
     // still in the backstack.

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

@@ -5,10 +5,12 @@ import android.os.Bundle
 import android.os.Parcelable
 import android.util.SparseArray
 import android.view.View
+import androidx.activity.OnBackPressedDispatcherOwner
 import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.testing.TestLifecycleOwner
 import androidx.test.core.app.ApplicationProvider
 import com.google.common.truth.Truth.assertThat
+import com.squareup.workflow1.ui.androidx.OnBackPressedDispatcherOwnerKey
 import com.squareup.workflow1.ui.navigation.WrappedScreen
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.flow.flowOf
@@ -29,23 +31,49 @@ internal class WorkflowLayoutTest {
   private val workflowLayout = WorkflowLayout(context).apply { id = 42 }
 
   @Test fun ignoresAlienViewState() {
-    var saved = false
-    val weirdView = object : View(context) {
-      override fun onSaveInstanceState(): Parcelable {
-        saved = true
-        super.onSaveInstanceState()
-        return Bundle()
-      }
-    }.apply { id = 42 }
+    val weirdView = BundleSavingView(context)
 
     val viewState = SparseArray<Parcelable>()
     weirdView.saveHierarchyState(viewState)
-    assertThat(saved).isTrue()
+    assertThat(weirdView.saved).isTrue()
 
     workflowLayout.restoreHierarchyState(viewState)
     // No crash, no bug.
   }
 
+  @Test fun ignoresNestedViewStateMistakes() {
+    class AScreen : AndroidScreen<AScreen> {
+      override val viewFactory =
+        ScreenViewFactory.fromCode<AScreen> { _, initialEnvironment, context, _ ->
+          ScreenViewHolder(initialEnvironment, BundleSavingView(context)) { _, _ -> }
+        }
+    }
+
+    class BScreen : AndroidScreen<BScreen> {
+      override val viewFactory =
+        ScreenViewFactory.fromCode<BScreen> { _, initialEnvironment, context, _ ->
+          ScreenViewHolder(initialEnvironment, SaveStateSavingView(context)) { _, _ -> }
+        }
+    }
+
+    val onBack = object : OnBackPressedDispatcherOwner {
+      override fun getOnBackPressedDispatcher() = error("nope")
+      override val lifecycle: Lifecycle get() = error("also nope")
+    }
+
+    val env = ViewEnvironment.EMPTY + (OnBackPressedDispatcherOwnerKey to onBack)
+
+    val original = WorkflowLayout(context).apply { id = 1 }
+    original.show(AScreen(), env)
+
+    val viewState = SparseArray<Parcelable>()
+    original.saveHierarchyState(viewState)
+
+    val unoriginal = WorkflowLayout(context).apply { id = 1 }
+    unoriginal.restoreHierarchyState(viewState)
+    unoriginal.show(BScreen(), env)
+  }
+
   @Test fun usesLifecycleDispatcher() {
     val lifecycleDispatcher = UnconfinedTestDispatcher()
     val collectionContext: CoroutineContext = UnconfinedTestDispatcher()
@@ -62,4 +90,24 @@ internal class WorkflowLayoutTest {
 
     // No crash then we safely removed the dispatcher.
   }
+
+  private class BundleSavingView(context: Context) : View(context) {
+    var saved = false
+
+    init {
+      id = 42
+    }
+
+    override fun onSaveInstanceState(): Parcelable {
+      saved = true
+      super.onSaveInstanceState()
+      return Bundle()
+    }
+  }
+
+  private class SaveStateSavingView(context: Context) : View(context) {
+    init {
+      id = 42
+    }
+  }
 }