Propagate composition locals to layers in the (re)composition phase #1233
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…mmediately in rememberComposeSceneLayer.
igordmn
approved these changes
Apr 3, 2024
igordmn
reviewed
Apr 3, 2024
| @@ -581,7 +587,14 @@ private class MultiLayerComposeSceneImpl( | |||
| composition?.dispose() | |||
| composition = owner.setContent( | |||
| parent = this@AttachedComposeSceneLayer.compositionContext, | |||
| { this@AttachedComposeSceneLayer.compositionLocalContext } | |||
| { | |||
There was a problem hiding this comment.
Better to remove this lambda completely (it has the same default value), otherwise it is difficult to read without IDE support. The comment itself can be placed above setContent
igordmn
approved these changes
Apr 3, 2024
igordmn
reviewed
Apr 3, 2024
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeSceneLayer.skiko.kt
Show resolved
Hide resolved
igordmn
approved these changes
Apr 3, 2024
MatkovIvan
approved these changes
Apr 3, 2024
| DisposableEffect(Unit) { | ||
| onDispose { | ||
| layer.close() | ||
| } | ||
| } | ||
| SideEffect { |
There was a problem hiding this comment.
It looks like SideEffect was required because of rememberUpdatedState. The current state works fine
There was a problem hiding this comment.
SideEffect was introduced in #562
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the value of a composition local that is provided outside a
Popupchanges, typically, the order of events is:CompositionLocalProvider.CompositionLocalProvideris recomposed, modifying the composition local context, which triggersrememberComposeSceneLayerto recompose, as it readscurrentCompositionLocalContext.rememberComposeSceneLayeris recomposed.SideEffectinrememberComposeSceneLayeris executed, changing thecompositionLocalContextof the layer, which triggers recomposition of the layer (popup) content.However, if the layer content is recomposed (for unrelated reasons) in the same frame as
rememberComposeSceneLayer, it will read an old value of the composition local.The question then is why doesn't the layer content recompose again when the
SideEffectupdates compositionLocalContext. The answer is thatMultiLayerComposeScene.compositionLocalContextis not a Compose state.Proposed Changes
Apply compositionLocalContext, density and layoutDirection to layer immediately in rememberComposeSceneLayer.
Testing
Test: Added a unit test
This PR should be verified by QA.
Issues Fixed
Fixes: JetBrains/compose-multiplatform#4558