Add Scene method to force synchronize compose state#2952
Add Scene method to force synchronize compose state#2952
Conversation
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/BaseComposeScene.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/BaseComposeScene.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/BaseComposeScene.skiko.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/iosMain/kotlin/androidx/compose/ui/scene/ComposeSceneMediator.ios.kt
Outdated
Show resolved
Hide resolved
| fun render(canvas: Canvas, nanoTime: Long) | ||
|
|
||
| fun update(nanoTime: Long) |
There was a problem hiding this comment.
In my draft of phase separation I did the separation like this:
| fun render(canvas: Canvas, nanoTime: Long) | |
| fun update(nanoTime: Long) | |
| fun render(canvas: Canvas, nanoTime: Long) { | |
| onFrame(nanoTime) | |
| measureAndLayout() | |
| draw(canvas) | |
| } | |
| fun onFrame(frameTimeNanos: Long) | |
| fun measureAndLayout() | |
| fun draw(canvas: Canvas) |
update is too generic name here. Also, I'd like to reuse the code, not duplicate parts of it.
Maybe we can do this split like this now without extracting recomposition yet? Just to get the right API shape here
There was a problem hiding this comment.
Separating measureAndLayout also works in this PR.
update is too generic name here
I don't like onFrame as well. I would avoid on, because it sounds like we react on something, but the API of ComposeScene is imperative, so at least we should name it doFrame.
Will we only have these after separation?
recomposer.performScheduledEffects()
recomposer.performScheduledRecomposerTasks()
frameClock.sendFrame(nanoTime)
Then we can call this function recompose, and performing the effects is just a requirement for recomposition.
There was a problem hiding this comment.
Recomposition won't be a part of ComposeScene - It's supposed to be shared for all scenes in the app
See CMP-9758
There was a problem hiding this comment.
@MatkovIvan , due to the postponeInvalidation it's hard to split this method as proposed because the performSnapshotChangesSynchronously also used as a synchronization point. To do so - refactoring is needed. It means that we have to bring additional methods to the ComposeScene, which I'm trying to avoid.
There was a problem hiding this comment.
Recomposition won't be a part of ComposeScene
We can move it as part of that task, and for now just describe the current way of recomposing. I suspect that when we fix CMP-9758, we either:
- don't have recompose/doFrame method
- have it, but it recomposes the scene subcomposition (for example, via
currentRecomposeScope)
On a second thought, besides recomposition, we perform sendFrame that can be considered additional to recomposition. sendFrame looks also as a good name.
postponeInvalidation
Initially this was added only as just an optimization of calling the invalidate callback once instead on each change inside Compose.
sendAndPerformSnapshotChanges were added later, to ensure that the state that was changed in background threads is actual during recomposition. Calling it in between recompostion -> layout, and layout -> draw seems can break "state transactions" within a single event loop tick 🤔:
var state by remember { mutableStateOf(2) }
LaunchedEffect(Unit) {
withContext(Dispatchers.Default) {
state = 3
}
}
Layout {
// if we call `sendAndPerformSnapshotChanges` before layout, we can see 3 instead of 2
something(state)
}
Maybe we can call sendAndPerformSnapshotChanges only in the recompose function 🤔, but we can decide that when we need to separate them. For now, having recomposeAndLayout looks fine to me to fix the PR issue.
We still need to decide the name of it:
- recomposeAndLayout
- doFrameAndLayout
- sendFrameAndLayout
- update (maybe it is generic, but it also a conventional name in rendering: update + draw)
I vote for 4 and 3
There was a problem hiding this comment.
// if we call
sendAndPerformSnapshotChangesbefore layout, we can see 3 instead of 2
But if we wrap everything in the performSnapshotChangesSynchronously, does it guarantee that all background changes will be called after the performSnapshotChangesSynchronously ends, and the order of the sendAndPerformSnapshotChanges and layout does not really matters for this outcome?
We still need to decide the name of it:
The idea of the new function - to bring the Scene in the new advances state that occurs immediately after submitting changes. Unfortunately, current render method does more complicated list of action, rather then doing a lightweight update. And it must be synchronized with Android - that's why I cannot say the function must be a part of the rendering pipeline: In this case its functionality will be mistreated.
For this reason, I vote for the recomposeAndLayout. The doFrame, sendFrame and update methods assume a much wider range of functionality.
There was a problem hiding this comment.
But if we wrap everything in the performSnapshotChangesSynchronously, does it guarantee that all background changes will be called after the performSnapshotChangesSynchronously ends
After looking at the code again, I realized that I was mistaken, and it doesn't guarantee that, it just postpones the snapshotObserver callbacks:


It is fine to call them in between recompose-layout, layout-draw. Maybe even beneficial 🤔
Then it looks like a separate issue, and we can scope it out of this PR. So, it looks okay to split a single postponeInvalidation into 3, so initial suggestion is returned to the discussion - to have separate methods with its own postponeInvalidation's.
The idea of the new function - to bring the Scene in the new advances state that occurs immediately after submitting changes
It is better to keep API semantically minimal. "advance the state" looks like a new semantics, which makes it more difficult to understand from the ComposeScene user perspective, and can affect user applications (for example, we performed layout without sending a synthetic event - that could break the apps). Instead of bringing new semantics, we can extract existing part from render. I would keep this part semantically whole and the same, even if we have to call additional actions that are not needed to solve the specific issue with text input. I see only advantages of doing this, and don't see disadvantages (there shouldn't be overhead, because not needed operations should be skipped).
I.e, if we need recomposition, we need to:
- drive
LaunchedEffect's - call
sendFrame(it callswithFrameNanoscallback). It is currently not possible to decouple recomposition from it.
If we need relayout, we have to send the synthetic event.
A question: do we actually need relayout to fix the text input issue?
| frameClock.sendFrame(nanoTime) // withFrameMillis/Nanos and recomposition | ||
|
|
||
| doMeasureAndLayout() // Layout | ||
| doMeasureAndLayout() |
There was a problem hiding this comment.
The lines 179-195 (updatePointerPosition and the second layout) are part of the layout step - we need to include them into it.
| fun render(canvas: Canvas, nanoTime: Long) | ||
|
|
||
| fun update(nanoTime: Long) |
There was a problem hiding this comment.
Recomposition won't be a part of ComposeScene
We can move it as part of that task, and for now just describe the current way of recomposing. I suspect that when we fix CMP-9758, we either:
- don't have recompose/doFrame method
- have it, but it recomposes the scene subcomposition (for example, via
currentRecomposeScope)
On a second thought, besides recomposition, we perform sendFrame that can be considered additional to recomposition. sendFrame looks also as a good name.
postponeInvalidation
Initially this was added only as just an optimization of calling the invalidate callback once instead on each change inside Compose.
sendAndPerformSnapshotChanges were added later, to ensure that the state that was changed in background threads is actual during recomposition. Calling it in between recompostion -> layout, and layout -> draw seems can break "state transactions" within a single event loop tick 🤔:
var state by remember { mutableStateOf(2) }
LaunchedEffect(Unit) {
withContext(Dispatchers.Default) {
state = 3
}
}
Layout {
// if we call `sendAndPerformSnapshotChanges` before layout, we can see 3 instead of 2
something(state)
}
Maybe we can call sendAndPerformSnapshotChanges only in the recompose function 🤔, but we can decide that when we need to separate them. For now, having recomposeAndLayout looks fine to me to fix the PR issue.
We still need to decide the name of it:
- recomposeAndLayout
- doFrameAndLayout
- sendFrameAndLayout
- update (maybe it is generic, but it also a conventional name in rendering: update + draw)
I vote for 4 and 3
This method is designed to address the issue of synchronising the state of text fields between Compose and the iOS text input service. The iOS keyboard API expectes all changes to be applied immediately, but this is not the case for Compose, where all changes are first stored in the state and updated during recomposition.
In order to resolve various issues that arise during text input, the Compose state must be synchronised with the text input to provide an accurate result after each editing command.
Release Notes
N/A