Refactor to ComposeLifecycleOwner for Better Lifecycle Synchronization #1234
Conversation
ekeitho
force-pushed
the
kabdulla/compose-lifecycle-owner
branch
from
0798a0e to
7a8ac55
Compare
workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ComposeLifecycleOwner.kt
Outdated
Show resolved
Hide resolved
workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ComposeLifecycleOwner.kt
Outdated
Show resolved
Hide resolved
workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ComposeLifecycleOwner.kt
Outdated
Show resolved
Hide resolved
workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/WorkflowRendering.kt
Outdated
Show resolved
Hide resolved
| // If we're leaving the composition, ensure the lifecycle is cleaned up | ||
| if (registry.currentState != Lifecycle.State.INITIALIZED) { | ||
| registry.currentState = Lifecycle.State.DESTROYED | ||
| } |
There was a problem hiding this comment.
One behavior change this introduces that we might not want is that it will destroy the child lifecycle before sending a new one, if the parent lifecycle instance changes. I don't think the parent instance should ever change though, unless movableContentOf is used or something, so it's probably not gonna be an issue immediately.
There was a problem hiding this comment.
Interesting, i guess that means we can just not key off parent lifecycle on the remember call
There was a problem hiding this comment.
I made the update to ensure this behavior didn't change since i'd hope it would work with movableContentOf & wrote a test for it too
There was a problem hiding this comment.
I'm guessing you didn't push that yet? It doesn't look changed to me. This should push this code to be even more similar to the View one.
There was a problem hiding this comment.
yeah i removed parentLifecycle from the remember key. check out: https://github.com/square/workflow-kotlin/pull/1234/files#diff-be84e80fcb3d58b96b21bf4311c0bcdc15c6f2d0d90b6628ce1f220bd012301eR76
it will retain the same instance of LifecycleOwner regardless of parent lifecycle changing.
There was a problem hiding this comment.
Oh, that's also incorrect because if the parent changes then we'll be syncing to the wrong one. You need to add some code so the remember function can update the parentLifecycle on this new class, and make sure we remove/add observers correctly when that happens (without necessarily destroying the child).
There was a problem hiding this comment.
i think i got what you mean now: 953e2ab
workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ComposeLifecycleOwner.kt
Outdated
Show resolved
Hide resolved
workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ComposeLifecycleOwner.kt
Outdated
Show resolved
Hide resolved
|
Even if we can't figure out how to test the original bug, we could at least have a test that ensures the a child composable sees the correct lifecycle state immediately. |
|
Also congrats on PR 1234! 😆 |
ekeitho
force-pushed
the
kabdulla/compose-lifecycle-owner
branch
6 times, most recently
from
0202cb2 to
c9d1399
Compare
… management - Extracted anonymous LifecycleOwner and RememberObserver implementation into a reusable ComposeLifecycleOwner class. - ComposeLifecycleOwner synchronizes its lifecycle with the parent Lifecycle and integrates with Compose's memory model via RememberObserver. - Improves code readability, reusability, and aligns lifecycle management with Compose best practices.
ekeitho
force-pushed
the
kabdulla/compose-lifecycle-owner
branch
from
c9d1399 to
bf97796
Compare
workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ComposeLifecycleOwner.kt
Show resolved
Hide resolved
workflow-ui/compose/src/main/java/com/squareup/workflow1/ui/compose/ComposeLifecycleOwner.kt
Show resolved
Hide resolved
ekeitho
force-pushed
the
kabdulla/compose-lifecycle-owner
branch
from
5a0aa6f to
953e2ab
Compare
…t lifecycles if new ones are provided
ekeitho
force-pushed
the
kabdulla/compose-lifecycle-owner
branch
from
3ca095b to
0b62bcc
Compare
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Refactor our creation of
LifecycleOwnerin compose to useComposeLifecycleOwnerfor better lifecycle management & readability.Extracted anonymous
LifecycleOwner& introducedRememberObserverimplementation into a reusableComposeLifecycleOwnerclass. This now synchronizes its lifecycle with the parent Lifecycle & immediately synchornizes on creation ofComposeLifecycleOwnerinstead of where before we were synchronized to the parent AFTER COMPOSITION.