make sure events are not assumed and always update to lifecycle obser… #1230
Conversation
…ver event changes
| updateLifecycle(Event.ON_START) | ||
| } | ||
|
|
||
| override fun onViewDetachedFromWindow(v: View) { | ||
| viewIsAttachedToWindow = false | ||
| updateLifecycle() | ||
| updateLifecycle(ON_PAUSE) |
| 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. |
There was a problem hiding this comment.
| // 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. |
| } | ||
|
|
||
| override fun destroyOnDetach() { | ||
| if (!destroyOnDetach) { | ||
| destroyOnDetach = true | ||
| updateLifecycle() | ||
| updateLifecycle(Event.ON_DESTROY) |
There was a problem hiding this comment.
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.
| @@ -225,6 +225,9 @@ internal class RealWorkflowLifecycleOwner( | |||
| } | |||
|
|
|||
| localLifecycle.currentState = when { | |||
| event != null -> { | |||
| event.targetState | |||
There was a problem hiding this comment.
Don't we need to make sure the local lifecycle isn't greater than the parent? E.g. if the parent hasn't STARTED yet, we shouldn't either.
There was a problem hiding this comment.
I think this should be renamed to parentEvent since the events should originate from the parent lifecycle. For example, onStateChanged ensures that the local lifecycle mirrors the parent's state and doesn't exceed it.
ekeitho
force-pushed
the
kabdulla/workflow-lifecycle
branch
2 times, most recently
from
a95006b to
a3d910b
Compare
ekeitho
force-pushed
the
kabdulla/workflow-lifecycle
branch
from
a3d910b to
93ca790
Compare
ekeitho
force-pushed
the
kabdulla/workflow-lifecycle
branch
from
93ca790 to
324945a
Compare
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| } | ||
|
|
||
| override fun destroyOnDetach() { | ||
| if (!destroyOnDetach) { | ||
| destroyOnDetach = true | ||
| updateLifecycle() | ||
| updateLifecycle(Event.ON_DESTROY.targetState) |
There was a problem hiding this comment.
Just to make sure the comment doesn't get lost across updates: we can't ship this way.
-
destroyOnDetach()has to mean "move to destroyed the next timeonViewDetachedFromWindow()is called.
This PR addresses an
IllegalStateExceptioncaused by premature state consumption inWorkflowLifecycleOwner.This issue was uncovered once we started to host views in a compose container.
Before the Change:
Premature Lifecycle Updates:
The lifecycle might have been updated without considering the exact event, leading to situations where Compose tried to consume state before the component was fully initialized.
Event Misalignment:
Without passing explicit events, the lifecycle transitions could have been misaligned with the actual lifecycle of the Android component.
After the Change:
Explicit Event Handling:
By passing specific Lifecycle.Event instances (ON_START, ON_PAUSE, ON_DESTROY) to updateLifecycle, we synchronize the WorkflowLifecycleOwner's state transitions with the Android component's lifecycle events.
Preventing Premature State Consumption:
Ensuring that lifecycle transitions occur after relevant events (like super.onCreate) prevents Compose from attempting to restore state too early.
Consistent Lifecycle State Management:
The RealWorkflowLifecycleOwner now accurately reflects the View's attachment state, maintaining a coherent lifecycle state that aligns with Android's expectations.