From 9337e478a169fb8fcd4c2c00d448e0d9982f4591 Mon Sep 17 00:00:00 2001 From: Keith Abdulla Date: Mon, 11 Nov 2024 10:57:08 -0800 Subject: [PATCH 1/3] make sure events are not assumed and always update to lifecycle observer event changes --- .../ui/androidx/WorkflowLifecycleOwner.kt | 50 ++++++------------- 1 file changed, 15 insertions(+), 35 deletions(-) diff --git a/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt b/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt index 3f3f29cc7..022e82a02 100644 --- a/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt +++ b/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt @@ -8,6 +8,8 @@ import androidx.annotation.VisibleForTesting import androidx.annotation.VisibleForTesting.Companion.PRIVATE import androidx.lifecycle.Lifecycle import androidx.lifecycle.Lifecycle.Event +import androidx.lifecycle.Lifecycle.Event.ON_DESTROY +import androidx.lifecycle.Lifecycle.Event.ON_PAUSE import androidx.lifecycle.Lifecycle.State.DESTROYED import androidx.lifecycle.Lifecycle.State.INITIALIZED import androidx.lifecycle.Lifecycle.State.RESUMED @@ -190,12 +192,12 @@ internal class RealWorkflowLifecycleOwner( oldLifecycle?.removeObserver(this) parentLifecycle?.addObserver(this) } - updateLifecycle() + updateLifecycle(Event.ON_START) } override fun onViewDetachedFromWindow(v: View) { viewIsAttachedToWindow = false - updateLifecycle() + updateLifecycle(ON_PAUSE) } /** Called when the [parentLifecycle] changes state. */ @@ -203,53 +205,31 @@ internal class RealWorkflowLifecycleOwner( source: LifecycleOwner, event: Event ) { - updateLifecycle() + // we should always consume what the new event is from onStateChanged. if we try to + // assume the event like we did in the past, we can get into scenarios where we try to put + // a view in created, when it's parent hasn't completed fully it's super.onCreate yet. + updateLifecycle(event) } override fun destroyOnDetach() { if (!destroyOnDetach) { destroyOnDetach = true - updateLifecycle() + updateLifecycle(Event.ON_DESTROY) } } @VisibleForTesting(otherwise = PRIVATE) - internal fun updateLifecycle() { - val parentState = parentLifecycle?.currentState - val localState = localLifecycle.currentState + internal fun updateLifecycle(event: Event) { + val targetState = event.targetState - if (localState == DESTROYED || hasBeenDestroyed) { + if (localLifecycle.currentState == DESTROYED || hasBeenDestroyed) { view = null // Local lifecycle is in a terminal state. return } - localLifecycle.currentState = when { - destroyOnDetach && !viewIsAttachedToWindow -> { - // We've been enqueued for destruction. - // Stay attached to the parent's lifecycle until we re-attach, since the parent could be - // destroyed while we're detached. - DESTROYED - } - parentState != null -> { - // We may or may not be attached, but we have a parent lifecycle so we just blindly follow - // it. - parentState - } - localState == INITIALIZED -> { - // We have no parent and we're not destroyed, which means we have never been attached, so - // the only valid state we can be in is INITIALIZED. - INITIALIZED - } - else -> { - // We don't have a parent and we're neither in DESTROYED or INITIALIZED: this is an invalid - // state. Throw an AssertionError instead of IllegalStateException because there's no API to - // get into this state, so this means the library has a bug. - throw AssertionError( - "Must have a parent lifecycle after attaching and until being destroyed." - ) - } - }.let { newState -> + localLifecycle.currentState = targetState + targetState.let { newState -> if (newState == DESTROYED) { hasBeenDestroyed = true @@ -273,7 +253,7 @@ internal class RealWorkflowLifecycleOwner( // out of the INITIALIZED state. That's an invalid state transition, and so setCurrentState // will throw if we do that. That exception can mask actual test failures, so to avoid that // here we just stay in the initialized state forever. - if (localState == INITIALIZED) { + if (localLifecycle.currentState == INITIALIZED) { INITIALIZED } else { DESTROYED From 324945a26e2043acde836b3b3176242aac34ffce Mon Sep 17 00:00:00 2001 From: Keith Abdulla Date: Mon, 11 Nov 2024 11:42:48 -0800 Subject: [PATCH 2/3] revert a bit to please some tests --- .../ui/androidx/WorkflowLifecycleOwner.kt | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt b/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt index 022e82a02..c7718764d 100644 --- a/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt +++ b/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt @@ -8,8 +8,7 @@ import androidx.annotation.VisibleForTesting import androidx.annotation.VisibleForTesting.Companion.PRIVATE import androidx.lifecycle.Lifecycle import androidx.lifecycle.Lifecycle.Event -import androidx.lifecycle.Lifecycle.Event.ON_DESTROY -import androidx.lifecycle.Lifecycle.Event.ON_PAUSE +import androidx.lifecycle.Lifecycle.State import androidx.lifecycle.Lifecycle.State.DESTROYED import androidx.lifecycle.Lifecycle.State.INITIALIZED import androidx.lifecycle.Lifecycle.State.RESUMED @@ -192,12 +191,18 @@ internal class RealWorkflowLifecycleOwner( oldLifecycle?.removeObserver(this) parentLifecycle?.addObserver(this) } - updateLifecycle(Event.ON_START) + + // if (parentLifecycle?.currentState == INITIALIZED) { + // // updateLifecycle(Event.ON_RESUME.targetState) + // } else { + // + // } + updateLifecycle(Event.ON_RESUME.targetState) } override fun onViewDetachedFromWindow(v: View) { viewIsAttachedToWindow = false - updateLifecycle(ON_PAUSE) + updateLifecycle(Event.ON_PAUSE.targetState) } /** Called when the [parentLifecycle] changes state. */ @@ -205,31 +210,28 @@ internal class RealWorkflowLifecycleOwner( source: LifecycleOwner, event: Event ) { - // we should always consume what the new event is from onStateChanged. if we try to - // assume the event like we did in the past, we can get into scenarios where we try to put - // a view in created, when it's parent hasn't completed fully it's super.onCreate yet. - updateLifecycle(event) + updateLifecycle(event.targetState) } override fun destroyOnDetach() { if (!destroyOnDetach) { destroyOnDetach = true - updateLifecycle(Event.ON_DESTROY) + updateLifecycle(Event.ON_DESTROY.targetState) } } @VisibleForTesting(otherwise = PRIVATE) - internal fun updateLifecycle(event: Event) { - val targetState = event.targetState + internal fun updateLifecycle(parentTargetState: State) { + //val parentState = parentLifecycle?.currentState + val localState = localLifecycle.currentState - if (localLifecycle.currentState == DESTROYED || hasBeenDestroyed) { + if (localState == DESTROYED || hasBeenDestroyed) { view = null // Local lifecycle is in a terminal state. return } - localLifecycle.currentState = targetState - targetState.let { newState -> + localLifecycle.currentState = parentTargetState.let { newState -> if (newState == DESTROYED) { hasBeenDestroyed = true @@ -253,7 +255,7 @@ internal class RealWorkflowLifecycleOwner( // out of the INITIALIZED state. That's an invalid state transition, and so setCurrentState // will throw if we do that. That exception can mask actual test failures, so to avoid that // here we just stay in the initialized state forever. - if (localLifecycle.currentState == INITIALIZED) { + if (localState == INITIALIZED) { INITIALIZED } else { DESTROYED From 2adf1464a5c6689278f4ef9ea07d8485f332aa65 Mon Sep 17 00:00:00 2001 From: ekeitho Date: Tue, 12 Nov 2024 22:21:33 +0000 Subject: [PATCH 3/3] Apply changes from ktLintFormat Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt b/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt index c7718764d..728b432e0 100644 --- a/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt +++ b/workflow-ui/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt @@ -222,7 +222,7 @@ internal class RealWorkflowLifecycleOwner( @VisibleForTesting(otherwise = PRIVATE) internal fun updateLifecycle(parentTargetState: State) { - //val parentState = parentLifecycle?.currentState + // val parentState = parentLifecycle?.currentState val localState = localLifecycle.currentState if (localState == DESTROYED || hasBeenDestroyed) {