Skip to content
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

make sure events are not assumed and always update to lifecycle obser… #1230

Closed
wants to merge 3 commits into from
Closed

make sure events are not assumed and always update to lifecycle obser… #1230

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9337e47
make sure events are not assumed and always update to lifecycle obser…
ekeitho Nov 11, 2024
324945a
revert a bit to please some tests
ekeitho Nov 11, 2024
2adf146
Apply changes from ktLintFormat
ekeitho Nov 12, 2024
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
50 changes: 15 additions & 35 deletions ...i/core-android/src/main/java/com/squareup/workflow1/ui/androidx/WorkflowLifecycleOwner.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -190,66 +192,44 @@ 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels really good.

}

/** Called when the [parentLifecycle] changes state. */
override fun onStateChanged(
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// a view in created, when it's parent hasn't completed fully it's super.onCreate yet.
// a view in created, when its parent hasn't completed fully its super.onCreate yet.

updateLifecycle(event)
}

override fun destroyOnDetach() {
if (!destroyOnDetach) {
destroyOnDetach = true
updateLifecycle()
updateLifecycle(Event.ON_DESTROY)
Copy link
Contributor

Choose a reason for hiding this comment

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

This subverts the purpose of destroyOnDetach(), which is to make sure things are still alive while the view is visible. I think instead you want to change onViewDetachedFromWindow to look at destroyOnDetach and use that to choose between ON_PAUSE and ON_DESTROY. Maybe also need to change the guard clause at the top of onViewAttachedToWindow to look directly at destroyOnDetach.

}
}

@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

Expand All @@ -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
Expand Down
Loading