-
Notifications
You must be signed in to change notification settings - Fork 113
Use dedicated coroutine context for Compose scene #2646
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.
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
Conversation
| private var activeStateListener: SceneActiveStateListener? = null | ||
| val composeCoroutineContext: CoroutineContext = coroutineContext + motionDurationScale | ||
|
|
||
| private var sceneJob: Job = Job().also { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this class doesn't operate with the scene directly, I think it's more correct to call it composeJob instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scene's lifetime tight to the sceneJob - the variable name highlights this fact. The composeJob is too generic imo.
| // The initial state of the container considered as "not active". | ||
| // The `initializeComposeScene` must be called to set the active `sceneJob`. | ||
| it.cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it important? What goes wrong in the case of creating it during class initialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment explains it. This scope also used for running animations and other tasks outside the ComposeContainer. It's needed to prevent running any of them before the initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says nothing about animations or reasoning in general
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it's a good idea because comment will become outdated pretty soon as it isn't connected with the place of use directly.
Use dedicated coroutine context with separate Job for compose scene.
Use nested coroutine scope tight to the Compose scene context for frame animations.
Make
ComposeSceneMediator's lifetime tight to the context.Fixes https://youtrack.jetbrains.com/issue/CMP-7727/Use-dedicated-coroutine-scopes-for-compose-scenes
Release Notes
N/A