Skip to content
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

Move the effects and synthetic events dispatching to after the draw phase in the render loop. #1260

Merged
merged 4 commits into from
Apr 11, 2024

Conversation

m-sasha
Copy link

@m-sasha m-sasha commented Apr 10, 2024

While investigating this, we've discovered that LazyColumn moves its elements synchronously when scrolled (only if the scrolling causes new items to become visible).

Additionally, we currently run coroutines launched in a composition's rememberCoroutineScope between the layout and draw phases (due to #1034 and our desire to send the synthetic events before the draw phase, to avoid a one-frame delay when scrolling a list that highlights the item under the mouse).

Together these two create a problem (visible in the video linked above) because when the LazyColumn is scrolled from a launched coroutine, it moves its element to the position where they would normally appear only on the next frame, which causes a visible "jump".

Proposed Changes

After much consideration (with @igordmn) we decided to strictly adhere to the ordering of the phases as they are done on Android:

  1. Flush any pending effects.
  2. Composition (which can schedule effects to be run after render completes).
  3. Layout
  4. Schedule synthetic events, if any, to be delivered after render completes.
  5. Draw
  6. (after render) scheduled composition effects.
  7. (after render) scheduled synthetic events.

Testing

Test: Tested the "jumpy" lazy column manually, and added unit tests to verify the order of the render phases.

The manual test:

import androidx.compose.foundation.ScrollState
import androidx.compose.foundation.border
import androidx.compose.foundation.gestures.scrollBy
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.fillMaxHeight
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.lazy.LazyColumn
import androidx.compose.foundation.lazy.LazyListState
import androidx.compose.foundation.rememberScrollState
import androidx.compose.foundation.verticalScroll
import androidx.compose.material.Text
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
import androidx.compose.runtime.withFrameMillis
import androidx.compose.ui.ExperimentalComposeUiApi
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.drawBehind
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.input.pointer.PointerEventType
import androidx.compose.ui.input.pointer.onPointerEvent
import androidx.compose.ui.text.TextStyle
import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp
import androidx.compose.ui.window.singleWindowApplication
import kotlinx.coroutines.launch

val scrollState = ScrollState(0)
val lazyColumnState = LazyListState(0, 0)

fun main() = singleWindowApplication { 
    Box(modifier = Modifier.fillMaxSize()) {
        Row {
            Column(
                modifier = Modifier
                    .fillMaxHeight()
                    .weight(1f)
                    .verticalScroll(scrollState)
            ) {
                (0..100).forEach { index ->
                    Text(
                        "Column item $index",
                        style = TextStyle(fontSize = 30.sp),
                        modifier = Modifier.height(150.dp).fillMaxWidth().border(
                            width = 1.dp,
                            color = Color.Black
                        )
                    )
                }
            }


            LazyColumn(
                state = lazyColumnState,
                modifier = Modifier.fillMaxHeight().weight(1f)
            ) {
                items(100) { index ->
                    Text(
                        "LazyColumn item $index",
                        style = TextStyle(fontSize = 30.sp),
                        modifier = Modifier.height(150.dp).fillMaxWidth().border(
                            width = 1.dp,
                            color = Color.Black
                        )
                    )
                }
            }
        }

        LaunchedEffect(Unit) {
            var direction = 0
            var startTime = 0L
            var frameCount = 0
            while (true) {
                withFrameMillis { millis ->
                    frameCount += 1
                    if (frameCount.mod(1) == 0) {
                        if (direction == 0) {
                            startTime = millis
                            direction = 1
                        }
                        else if (millis - startTime > 10000) {
                            startTime = millis
                            direction = -direction
                        }
                        val scrollDelta = 5 * direction
                        launch {
                            scrollState.scrollBy(scrollDelta.toFloat())
                            lazyColumnState.scrollBy(scrollDelta.toFloat())
                        }
                    }
                }
            }
        }
    }
}

This PR should be verified by QA.

Issues Fixed

Possibly fixes JetBrains/compose-multiplatform#4438

@m-sasha m-sasha requested a review from igordmn April 10, 2024 12:56
… no longer valid and others are in RenderPhasesTest
… send synthetic events, as that now happens automatically via recomposer.hasPendingWork
@m-sasha m-sasha merged commit ef54cd7 into jb-main Apr 11, 2024
6 checks passed
@m-sasha m-sasha deleted the m-sasha/run-effects-after-draw branch April 11, 2024 12:33
igordmn added a commit that referenced this pull request Apr 24, 2024
Fixes JetBrains/compose-multiplatform#4681

The issue was because of this situation:
- the parent RenderNodeLayer is drawn and cached it's `picture`
- the child RenderNodeLayer isn't drawn, but `invalidate` is called

the child's `invalidate` should invalidate parent layer unconditionally, but we called it only if the child was drawn (picture != null), which is incorrect.

It is a regression after #1260, but it just revealed the bug, not introduced it.

## Testing
- new added tests fail before, work after the fix
- the issue is no longer reproducible

This should be tested by QA

## Release Notes
> Fixes - Multiple Platforms
- _(prerelease fix)_ Fix frozen composition with pager and text field
igordmn added a commit that referenced this pull request Apr 25, 2024
Fixes JetBrains/compose-multiplatform#4681

The issue was because of this situation:
- the parent RenderNodeLayer is drawn and cached it's `picture`
- the child RenderNodeLayer isn't drawn, but `invalidate` is called

`invalidate` should have updated the parent layer (removing the
`picture` cache), but didn't do that as we do that conditionally. Now
`invalidate` invalidates the parent layer unconditionally.

It is a regression after
#1260, but
it just revealed the bug, didn't introduce it.

## Testing
- new added tests fail before, work after the fix
- the issue is no longer reproducible

This should be tested by QA

## Release Notes
> Fixes - Multiple Platforms
- _(prerelease fix)_ Fix frozen composition with pager and text field
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants