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

Jump to bottom

Compose as `LifecycleOwner` #1198

Merged

MatkovIvan merged 6 commits into jb-main from ivan.matkov/compose-lifecycle-owner

Mar 22, 2024

Member

MatkovIvan commented Mar 16, 2024 •

edited

Loading

Proposed Changes

Cherry-pick 2950349: Move LocalLifecycleOwner to common
Implement LifecycleOwner for Desktop, iOS and Web

Testing

Test: Observe lifecycle events in mpp

Fixed Issues

Fixes JetBrains/compose-multiplatform#2915

MatkovIvan requested a review from igordmn

March 16, 2024 12:02

This comment was marked as resolved.

Sign in to view

MatkovIvan added the changes in common label

MatkovIvan force-pushed the ivan.matkov/compose-lifecycle-owner branch 2 times, most recently from f8fdda2 to 8aa573d Compare

March 18, 2024 13:04

igordmn requested changes

View reviewed changes

Collaborator

igordmn left a comment

Awesome, thanks!

I agree with the states that we need to set on different events, but we need to set them carefully to avoid bugs. See my comments, I suggest to create a common class that routes events the right way.

...i/src/uikitMain/kotlin/androidx/compose/ui/window/ViewControllerBasedLifecycleOwner.uikit.kt

+                  }
+                  companion object {
+                      fun logWarning(message: String) {

Collaborator

igordmn Mar 20, 2024

Suggested change

      
                    fun logWarning(message: String) {
          
                    private fun logWarning(message: String) {

...i/src/uikitMain/kotlin/androidx/compose/ui/window/ViewControllerBasedLifecycleOwner.uikit.kt

+                                      fail("Invalid '$action' for 'State.Disposed'")
+                                  }
+                                  Action.APPLICATION_DID_ENTER_BACKGROUND, Action.APPLICATION_WILL_ENTER_FOREGROUND -> {
+                                      // no-op

Collaborator

igordmn Mar 20, 2024

Should we fail as well? These actions shouldn't be fired because we unsubscribe from them:

    fun dispose() {
        handleAction(Action.DISPOSE)
        applicationStateListener.dispose()
    }

...i/src/uikitMain/kotlin/androidx/compose/ui/window/ViewControllerBasedLifecycleOwner.uikit.kt

+              import androidx.lifecycle.LifecycleRegistry
+              import kotlin.test.fail
+              internal class ViewControllerBasedLifecycleOwner : LifecycleOwner {

Collaborator

igordmn Mar 20, 2024

We should write a unit test for this class, as here can be lot of bugs regarding non-symmetric events.

...i/src/uikitMain/kotlin/androidx/compose/ui/window/ViewControllerBasedLifecycleOwner.uikit.kt

Comment on lines +56 to +62

+                                      if (isApplicationForeground) {
+                                          lifecycle.handleLifecycleEvent(Lifecycle.Event.ON_PAUSE)
+                                      }
+                                      lifecycle.handleLifecycleEvent(Lifecycle.Event.ON_STOP)
+                                      Created(isApplicationForeground = isApplicationForeground, lifecycle = lifecycle)

Collaborator

igordmn Mar 20, 2024

Suggested change

      
                                    if (isApplicationForeground) {
          
                                        lifecycle.handleLifecycleEvent(Lifecycle.Event.ON_PAUSE)
          
                                    }
          
                                    lifecycle.handleLifecycleEvent(Lifecycle.Event.ON_STOP)
          
                                    Created(isApplicationForeground = isApplicationForeground, lifecycle = lifecycle)
          
                                    this

It doesn't sound right to emit Pause/Stop from Created state. If we in this state, it means we didn't receive VIEW_WILL_APPEAR, and so - didn't emit Start/Resume before.

...i/src/uikitMain/kotlin/androidx/compose/ui/window/ViewControllerBasedLifecycleOwner.uikit.kt

+              import androidx.lifecycle.Lifecycle
+              import androidx.lifecycle.LifecycleOwner
+              import androidx.lifecycle.LifecycleRegistry
+              import kotlin.test.fail

Collaborator

igordmn Mar 20, 2024

Why is the test library in the dependencies?

We should remove it, and use error() instead

...i/src/uikitMain/kotlin/androidx/compose/ui/window/ViewControllerBasedLifecycleOwner.uikit.kt

+                  }
+                  init {
+                      lifecycle.handleLifecycleEvent(Lifecycle.Event.ON_CREATE)

Collaborator

igordmn Mar 20, 2024

What if the class itself is created after view is visible? We need to send ON_START or ON_RESUME

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/scene/ComposeContainer.desktop.kt Show resolved Hide resolved

compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/scene/ComposeContainer.desktop.kt

@@ @@ -227,9 +255,13 @@ internal class ComposeContainer( @@
                       // Re-checking the actual size if it wasn't available during init.
                       onChangeWindowSize()
                       onChangeWindowPosition()
+                      lifecycle.handleLifecycleEvent(Lifecycle.Event.ON_START)

Collaborator

igordmn Mar 20, 2024

We set started state on 2 different states:

when it is iconified (minimized)
when it is added to window hierarchy.

So, we have 2 sources of truth and races between them.

To avoid that, we should either choose one source of truth, or combine isIconified and isAddedToHirarchy states.

compose/ui/ui/src/webCommonW3C/kotlin/androidx/compose/ui/window/ComposeWindow.js.kt

+                          lifecycleOwner.handleLifecycleEvent(Lifecycle.Event.ON_PAUSE)
+                      })
+                      lifecycleOwner.handleLifecycleEvent(Lifecycle.Event.ON_CREATE)

Collaborator

igordmn Mar 20, 2024

Let's not call ON_CREATE and ON_START in the same method init, and just call ON_START straight away in the end of init, if JS doesn't have meaning for CREATED state.

compose/ui/ui/build.gradle

@@ @@ -170,6 +170,8 @@ if(AndroidXComposePlugin.isMultiplatformEnabled(project)) { @@
                               api project(":compose:ui:ui-unit")
                               api project(":compose:ui:ui-util")
                               implementation(project(":annotation:annotation"))
+                              implementation(project(":lifecycle:lifecycle-common"))

Collaborator

igordmn Mar 20, 2024

Because we add a new dependency, this change requires 2 approves, please ask someone else to review this commit (about adding dependency).

It looks fine for me, because we either will release a stable version of lifecycle, or we just depend on stable API of it right now.

Collaborator

igordmn Mar 20, 2024

@eymar, no need to review the whole PR, only this commit

Member

eymar Mar 20, 2024

Having these dependencies added, we can't avoid publishing lifecycle libraries (there was an idea that maybe we don't need to publish lifecyclce right now - #1204 (comment)). User projects won't work with this ui library if we don't publish lifecycle.

Collaborator

igordmn Mar 20, 2024

there was an idea

If you talk about my comment:

If we don't need it urgently

I meant that "if don't need it right now". But we definitely need it for the 1.6.10 release

Member

eymar Mar 20, 2024

even dev builds won't work untill lifecycle is published if this PR is merged before that.

Collaborator

igordmn Mar 20, 2024

Indeed. Then let's wait until we publish

MatkovIvan requested review from terrakok and eymar

March 20, 2024 15:18

Collaborator

igordmn commented Mar 20, 2024 •

edited

Loading

We agreed to push it as is, and fix comments separately

Collaborator

igordmn commented Mar 20, 2024

We agreed to push it as is, and fix comments separately

I will create separate tasks for each platform later.

igordmn approved these changes

View reviewed changes

terrakok approved these changes

View reviewed changes

eymar approved these changes

View reviewed changes

MatkovIvan force-pushed the ivan.matkov/compose-lifecycle-owner branch from 8aa573d to 926ec3f Compare

March 21, 2024 07:59

MatkovIvan and others added 6 commits

March 22, 2024 09:49


          Move LocalLifecycleOwner to common

3b54183

After lifecycle modules commonization, `LifecycleOwner` is available in the common source set. Move it via `expect`/`actual` to keep binary compatibility on Android, which is based on filename - `AndroidCompositionLocals_androidKt`.

Test: N/A
Change-Id: I4b0ed6087e494a0b30cb6a890496fa17ebfda19c


          Add lifecycle dependency to mpp demo

cfbb45b


          Initial implementation of iOS bindings to Lifecycle (#1043)

8a01245

Initial implementation of iOS bindings to Lifecycle.

<img width="836" alt="ios_lifecycle"
src="https://github.com/JetBrains/compose-multiplatform-core/assets/4167681/ec73ed26-5435-43e7-a4cf-5d3b9fe91693">


          Provide LocalLifecycleOwner on Desktop

605e874


          add initial implementation of Web bindings for Lifecycle

3522b73


          Update API dump

4c4b1ac

MatkovIvan force-pushed the ivan.matkov/compose-lifecycle-owner branch from 926ec3f to 4c4b1ac Compare

March 22, 2024 08:49

MatkovIvan merged commit 4c4b1ac into jb-main

6 checks passed

MatkovIvan deleted the ivan.matkov/compose-lifecycle-owner branch

March 22, 2024 09:33

MatkovIvan mentioned this pull request

LocalLifecycleOwner not declared on Desktop JetBrains/compose-multiplatform#2915

Closed

Sanlorng mentioned this pull request

Support for Google's Viewmodel which recently went multi-platform Tlaster/PreCompose#289

Open

This was referenced Apr 9, 2024

polish ViewControllerBasedLifecycleOwner #1248

Merged

remove useless ON_CREATED event for WASM Lifecycle handling #1251

Merged

kropp added a commit that referenced this pull request


          remove useless ON_CREATED event for WASM Lifecycle handling (#1251)

049afe9

It cannot be handled anyway because ON_STARTED/ON_RESUME will be called
immediately after and web target is single-threaded

Fixes comment from #1198

## Testing

Test: added ComposeWindowLifecycleTest

MatkovIvan pushed a commit that referenced this pull request


          polish ViewControllerBasedLifecycleOwner (#1248)

9f63260

## Proposed Changes

Fixed iOS-related comments from #1198

## Testing

Test: added ViewControllerBasedLifecycleOwnerTest

ryanw-mobile mentioned this pull request

Compose as LifecycleOwner ryanw-mobile/OctoMeter#335

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes in common