Skip to content

only attempt to restore View saved state while INITIALIZED #635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 20, 2022
Merged

only attempt to restore View saved state while INITIALIZED #635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions workflow-testing/api/workflow-testing.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public final class com/squareup/workflow1/testing/RealRenderTester : com/squareu
public fun getActionSink ()Lcom/squareup/workflow1/Sink;
public fun render (Lkotlin/jvm/functions/Function1;)Lcom/squareup/workflow1/testing/RenderTestResult;
public fun renderChild (Lcom/squareup/workflow1/Workflow;Ljava/lang/Object;Ljava/lang/String;Lkotlin/jvm/functions/Function1;)Ljava/lang/Object;
public fun requireExplicitSideEffectExpectations ()Lcom/squareup/workflow1/testing/RenderTester;
public fun requireExplicitWorkerExpectations ()Lcom/squareup/workflow1/testing/RenderTester;
public fun runningSideEffect (Ljava/lang/String;Lkotlin/jvm/functions/Function2;)V
public fun send (Lcom/squareup/workflow1/WorkflowAction;)V
Expand Down
55 changes: 55 additions & 0 deletions ...d/src/androidTest/java/com/squareup/workflow1/ui/backstack/test/BackstackContainerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.squareup.workflow1.ui.backstack.test

import android.os.Build
import android.view.View
import android.view.ViewGroup
import androidx.lifecycle.Lifecycle.State.CREATED
import androidx.lifecycle.Lifecycle.State.RESUMED
import androidx.lifecycle.Lifecycle.State.STARTED
Expand All @@ -12,6 +13,7 @@ import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
import com.squareup.workflow1.ui.backstack.test.fixtures.BackStackContainerLifecycleActivity
import com.squareup.workflow1.ui.backstack.test.fixtures.BackStackContainerLifecycleActivity.TestRendering.LeafRendering
import com.squareup.workflow1.ui.backstack.test.fixtures.BackStackContainerLifecycleActivity.TestRendering.RecurseRendering
import com.squareup.workflow1.ui.backstack.test.fixtures.ViewStateTestView
import com.squareup.workflow1.ui.backstack.test.fixtures.viewForScreen
import com.squareup.workflow1.ui.backstack.test.fixtures.waitForScreen
import org.junit.Rule
Expand Down Expand Up @@ -60,6 +62,59 @@ internal class BackstackContainerTest {
}
}

@Test fun state_restores_after_recreate_without_crashing() {
assertThat(scenario.state).isEqualTo(RESUMED)

fun BackStackContainerLifecycleActivity.nestedTestView(): ViewStateTestView {
val root = rootRenderedView as ViewGroup
val backStack = root.getChildAt(0) as ViewGroup
return backStack.getChildAt(0) as ViewStateTestView
}

scenario.onActivity {
it.consumeLifecycleEvents()
it.setRendering(RecurseRendering(listOf(LeafRendering("nested"))))
}

scenario.onActivity {
assertThat(it.consumeLifecycleEvents())
.containsExactly(
"nested onViewCreated viewState=",
"nested onShowRendering viewState=",
"nested onAttach viewState=",
"LeafView nested ON_CREATE",
"LeafView nested ON_START",
"LeafView nested ON_RESUME"
).inOrder()
}

scenario.onActivity {
// Set some view state to be saved and restored.
it.nestedTestView().viewState = "some state"
}

// Recreating the activity sends one ON_CREATE event,
// but the views will step through both INITIALIZED and CREATED before the observer is removed.
// We're testing that we don't try to restore SavedState when the view is already CREATED.
scenario.recreate()

scenario.onActivity {
// If we get through this, nothing crashed while recreating.
assertThat(it.consumeLifecycleEvents())
.containsExactly(
"activity onCreate",
"activity onStart",
"activity onResume"
).inOrder()
}

scenario.onActivity {
// We didn't crash, which means we didn't try to restore while CREATED,
// but make sure that we still restored while the view was in the INITIALIZED state.
assertThat(it.nestedTestView().viewState).isEqualTo("some state")
}
}

@Test fun restores_current_view_after_config_change() {
val firstScreen = LeafRendering("initial")

Expand Down
20 changes: 20 additions & 0 deletions .../com/squareup/workflow1/ui/backstack/test/fixtures/BackStackContainerLifecycleActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import com.squareup.workflow1.ui.WorkflowUiExperimentalApi
import com.squareup.workflow1.ui.WorkflowViewStub
import com.squareup.workflow1.ui.backstack.BackStackScreen
import com.squareup.workflow1.ui.backstack.test.fixtures.BackStackContainerLifecycleActivity.TestRendering.LeafRendering
import com.squareup.workflow1.ui.backstack.test.fixtures.BackStackContainerLifecycleActivity.TestRendering.OuterRendering
import com.squareup.workflow1.ui.backstack.test.fixtures.BackStackContainerLifecycleActivity.TestRendering.RecurseRendering
import com.squareup.workflow1.ui.bindShowRendering
import com.squareup.workflow1.ui.internal.test.AbstractLifecycleTestActivity
Expand Down Expand Up @@ -49,6 +50,11 @@ internal class BackStackContainerLifecycleActivity : AbstractLifecycleTestActivi
}

data class RecurseRendering(val wrappedBackstack: List<TestRendering>) : TestRendering()

@OptIn(WorkflowUiExperimentalApi::class)
data class OuterRendering(val name: String) : TestRendering() {
val backStack = BackStackScreen(LeafRendering("nested leaf in $name"))
}
}

private val viewObserver =
Expand Down Expand Up @@ -122,6 +128,20 @@ internal class BackStackContainerLifecycleActivity : AbstractLifecycleTestActivi
}
}
},
BuilderViewFactory(OuterRendering::class) { initialRendering,
initialViewEnvironment,
contextForNewView, _ ->
FrameLayout(contextForNewView).also { container ->

val stub = WorkflowViewStub(contextForNewView)
container.addView(stub)
container.bindShowRendering(
initialRendering, initialViewEnvironment
) { rendering, env ->
stub.update(rendering.backStack, env)
}
}
},
)

/** Returns the view that is the current screen. */
Expand Down
12 changes: 9 additions & 3 deletions ...-ui/container-android/src/main/java/com/squareup/workflow1/ui/backstack/ViewStateCache.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import android.view.View
import android.view.View.BaseSavedState
import androidx.annotation.VisibleForTesting
import androidx.annotation.VisibleForTesting.PRIVATE
import androidx.lifecycle.Lifecycle
import androidx.savedstate.SavedStateRegistryOwner
import androidx.savedstate.ViewTreeSavedStateRegistryOwner
import com.squareup.workflow1.ui.Named
Expand Down Expand Up @@ -50,9 +51,14 @@ internal constructor(
}
},
onRestored = { aggregator ->
currentOwner?.let { owner ->
aggregator.restoreRegistryControllerIfReady(owner.key, owner.controller)
}
currentOwner
// 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
?.takeIf { it.lifecycle.currentState == Lifecycle.State.INITIALIZED }
Copy link
Contributor

@steve-the-edwards steve-the-edwards Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @rjrjr originally tried this solution but decided against it? I can't remember now.

Copy link
Contributor Author

@RBusarow RBusarow Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it was just a matter of being able to reproduce the bug first.

If this line is removed, everything still passes except for the new test, even if you call scenario.recreate(), or change orientation or something.

Copy link
Collaborator

@rjrjr rjrjr Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I feel fine about the fix, just didn't want to do it w/o proof.

?.let { owner ->
aggregator.restoreRegistryControllerIfReady(owner.key, owner.controller)
}
}
)

Expand Down