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.

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

Fix consuming events of undecorated resizable window #124

Merged
merged 2 commits into from
Nov 26, 2021
Merged

Fix consuming events of undecorated resizable window #124

merged 2 commits into from
Nov 26, 2021

Conversation

igordmn
Copy link
Collaborator

@igordmn igordmn commented Nov 25, 2021

Fixes JetBrains/compose-multiplatform#1432
Fixes JetBrains/compose-multiplatform#1444

Rewrote to Compose, so Compose can handle all events by itself.
I haven't managed to write code properly with AWT (branch feature/fix_drag_undecorated)

@igordmn
Fix consuming events of undecorated resizable window
58f8b2e 
Fixes JetBrains/compose-multiplatform#1432
Fixes JetBrains/compose-multiplatform#1444

Rewrote to Compose, so Compose can handle all events by itself.
I haven't managed to write code properly with AWT (branch feature/fix_drag_undecorated)
@igordmn igordmn requested a review from Rsedaikin November 25, 2021 14:55
igordmn added a commit that referenced this pull request Nov 25, 2021
@igordmn
Async events is cause why sometimes interaction is so clunky:
4204031 
- when we resize a heavy undecorated window from #124, it continue resizing, even if we release the mouse
- when we scroll CodeViewer, it continues to scroll for 1-2 seconds

Async events removed freezes for us for scrolling heavy lazy lists, but instead it adds unresponsive UI that lives its own live.

Also, i checked heavy lazy list (codeviewer), and it doesn't freeze anymore during scrolling

Partially fixes JetBrains/compose-multiplatform#1345

Without that we can't merge #124
@igordmn
Merge remote-tracking branch 'origin/release/1.0' into feature/fix_dr…
02261c9 
…ag_undecorated2

# Conflicts:
#	compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeWindow.desktop.kt
igordmn added a commit that referenced this pull request Nov 25, 2021
@igordmn
Remove async events
f21e6d5 
Async events is cause why sometimes interaction is so clunky:
- when we resize a heavy undecorated window from #124, it continue resizing, even if we release the mouse
- when we scroll CodeViewer, it continues to scroll for 1-2 seconds

Async events removed freezes for us for scrolling heavy lazy lists, but instead it adds unresponsive UI that lives its own live.

Also, i checked heavy lazy list (codeviewer), and it doesn't freeze anymore during scrolling

Partially fixes JetBrains/compose-multiplatform#1345

Without that we can't merge #124
@igordmn igordmn mentioned this pull request Nov 25, 2021
Merged
@igordmn igordmn merged commit 9bbf022 into release/1.0 Nov 26, 2021
igordmn added a commit that referenced this pull request Jan 14, 2022
@igordmn
Events
0593438 
Call enter/exit events if we hover another popup

Fixes JetBrains/compose-multiplatform#841

Consume key events

Don't send event to focusable popup if there is no hovered popup

Don't scroll outside of focusable popup

Fixes JetBrains/compose-multiplatform#1346

Fix crash when we press right mouse button during dragging with left button

Fixes JetBrains/compose-multiplatform#1426
Fixes JetBrains/compose-multiplatform#1176

The weird behaviour doesn't reproduce anymore after cherry-picks
(this fix was reverted a few days ago here: #89)

Fix lazy scrollbar

Fixes JetBrains/compose-multiplatform#1430

Fix ScrollbarTest

Make real synthetic move events

I encountered code, where users doesn't use Compose events at all. They just listen to awtEvent, and check its type.

The feature "send synthetic move event on relayout", which we recently implemented, will break such use case, as synthetic event type would not reflect reality. For example, we would resend the latest native event, which was MousePressed. The application checks it and determines, that that cursor is over some button and presses it.

To fix that, the platform should provide a factory-method to create synthetic move events.

Fix build

Desktop. Fix problems with Enter/Exit events

Resend events similar to Android:
https://android-review.googlesource.com/c/platform/frameworks/support/+/1856856

On desktop we don't change the native event, as we don't know the nature of it, it can be not only MouseEvent in the future.
We change only PointerEvent.
This can lead to some confusion, if user reads the native event type instead of Compose event type.

Fixes JetBrains/compose-multiplatform#523
Fixes JetBrains/compose-multiplatform#594

Manual test:
```
import androidx.compose.foundation.background
import androidx.compose.foundation.gestures.scrollBy
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.rememberScrollState
import androidx.compose.foundation.verticalScroll
import androidx.compose.material.Text
import androidx.compose.runtime.Composable
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.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.input.pointer.PointerEventType
import androidx.compose.ui.input.pointer.pointerInput
import androidx.compose.ui.platform.LocalDensity
import androidx.compose.ui.unit.dp
import androidx.compose.ui.window.singleWindowApplication
import kotlinx.coroutines.delay

@composable
private fun App() {
    val state = rememberScrollState()
    Column(Modifier.size(300.dp).verticalScroll(state)) {
        repeat(20) {
            Item(it)
        }
    }
}

@composable
private fun Item(num: Int) {
    var isHovered by remember { mutableStateOf(false) }
    Box(
        Modifier
            .fillMaxWidth()
            .height(50.dp)
            .padding(8.dp)
            .background(if (isHovered) Color.Red else Color.Green)
            .pointerInput(Unit) {
                while (true) {
                    awaitPointerEventScope<Unit> {
                        val event = awaitPointerEvent()
                        when (event.type) {
                            PointerEventType.Enter -> {
                                isHovered = true
                            }
                            PointerEventType.Exit -> {
                                isHovered = false
                            }
                        }
                    }
                }
            }
    ) {
        Text(num.toString())
    }
}

fun main() {
    singleWindowApplication {
        App()
    }
}
```

Change-Id: I667c206bd08568fa0cb78208037c797bb8298702
Test: manual and ./gradlew jvmTest desktopTest -Pandroidx.compose.multiplatformEnabled=true

# Conflicts:
#	compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/input/pointer/PointerEvent.desktop.kt
#	compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/ComposeScene.skiko.kt

Get rid of mousePressed from ComposeScene

mousePressed is unreliable on Windows (we can miss the release event), and doesn't work well with multiple buttons.

After this fix, mouseClickable only reacts to the first pressed button. Right button click doesn't trigger callback, if there is already left mouse button is pressed.

`Clickable` shouldn't be able to handle these cases. If users would want simultaneously handle multiple buttons, they have to use low-level api:
```
Modifier.pointerInput(Unit) {
    while (true) {
        val event = awaitPointerEventScope { awaitPointerEvent() }

        if (event.type == PointerEventType.Press && event.buttons.isPrimaryPressed) {
            // do something
        } else if (event.type == PointerEventType.Press && event.buttons.isSecondaryPressed) {
            // do something
        }
    }
}
```
(it is verbose, there is a field for improvement)

Also pass PointerButtons and PointerKeyboardModifiers to ComposeScene, instead of reading them from AWT event.

Test: ./gradlew jvmTest desktopTest -Pandroidx.compose.multiplatformEnabled=true
Test: the snippet from JetBrains/compose-multiplatform#1149, because changed the code for that fix

Revert "Remove pointerId from ComposeScene"

Remove pointerId from ComposeScene

pointerId was indroduced in https://android-review.googlesource.com/c/platform/frameworks/support/+/1402607, because double click didn't work (see https://jetbrains.slack.com/archives/GT449QBCK/p1597328095373000)

But it messes with hover and clicking multiple mouse buttons at the same time.

Double clicking still works after removing it:
```
import androidx.compose.foundation.ExperimentalFoundationApi
import androidx.compose.foundation.background
import androidx.compose.foundation.combinedClickable
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.size
import androidx.compose.ui.Modifier
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.unit.dp
import androidx.compose.ui.window.singleWindowApplication

@OptIn(ExperimentalFoundationApi::class)
fun main() = singleWindowApplication {
    Box(
        Modifier
        .size(300.dp)
        .background(Color.Red)
            .combinedClickable(onDoubleClick = {
                println("onDoubleClick")
            }, onClick = {
                println("onClick")
            })
    ) {
    }
}
```

Fixes JetBrains/compose-multiplatform#1176

Test: ./gradlew jvmTest desktopTest -Pandroidx.compose.multiplatformEnabled=true
Test: manual (see the snippet)

Events

Fix sending events from AWT

Fix disposing window in event callback

Fixes JetBrains/compose-multiplatform#1448

The sequence of calls:
mouseReleased
  window.dispose
    scene.dispose
  cancelRippleEffect
    scene.invalidate
      layer.needRedraw

Fix losing Exit event on scroll

Fixes JetBrains/compose-multiplatform#1324 (comment)

Compose doesn't work well if we send an event with different coordinates without sending Move event before it:
```
Column {
  Box(size=10)
  Box(size=10)
}
```
If we send these events:
Move(5,5) -> Scroll(5,15) -> Move(5,15)

Then during the scroll event, HitPathTracker forgets about the first Box, and never send Exit to it. Instead it sends the Scroll event to it.

We should send events in this order:
Move(5,5) -> Move(5,15) -> Scroll(5,15) -> Move(5,15)

With synthetic events things more complicated, as we always send events with the same position. I suppose the proper fix should be in Compose core itself, but it would a very huge fix.

Reproducer:
```
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.size
import androidx.compose.ui.ExperimentalComposeUiApi
import androidx.compose.ui.ImageComposeScene
import androidx.compose.ui.Modifier
import androidx.compose.ui.geometry.Offset
import androidx.compose.ui.input.pointer.PointerEventType
import androidx.compose.ui.input.pointer.onPointerEvent
import androidx.compose.ui.unit.Density
import androidx.compose.ui.unit.dp
import androidx.compose.ui.use
import com.google.common.truth.Truth.assertThat
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.JUnit4

@OptIn(ExperimentalComposeUiApi::class)
@RunWith(JUnit4::class)
class MouseHoverFilterTest {
    // bug: JetBrains/compose-multiplatform#1324 (comment)
    @test
    fun `move from one component to another, between another non-move event`() = ImageComposeScene(
        width = 100,
        height = 100,
        density = Density(1f)
    ).use { scene ->
        var enterCount1 = 0
        var exitCount1 = 0
        var enterCount2 = 0
        var exitCount2 = 0

        scene.setContent {
            Column {
                Box(
                    modifier = Modifier
                        .pointerMove(
                            onEnter = { enterCount1++ },
                            onExit = { exitCount1++ }
                        )
                        .size(10.dp, 10.dp)
                )
                Box(
                    modifier = Modifier
                        .pointerMove(
                            onEnter = { enterCount2++ },
                            onExit = { exitCount2++ }
                        )
                        .size(10.dp, 10.dp)
                )
            }
        }

        scene.sendPointerEvent(PointerEventType.Enter, Offset(5f, 5f))
        assertThat(enterCount1).isEqualTo(1)
        assertThat(exitCount1).isEqualTo(0)
        assertThat(enterCount2).isEqualTo(0)
        assertThat(exitCount2).isEqualTo(0)

        // Compose doesn't work well if we send an event with different type and different coordinates, without sending move event before it
        scene.sendPointerEvent(PointerEventType.Scroll, Offset(5f, 15f))
        scene.sendPointerEvent(PointerEventType.Move, Offset(5f, 15f))
        assertThat(enterCount1).isEqualTo(1)
        assertThat(exitCount1).isEqualTo(1)
        assertThat(enterCount2).isEqualTo(1)
        assertThat(exitCount2).isEqualTo(0)
    }
}
```

Another issue:
JetBrains/compose-multiplatform#1480

Fix losing hover events, speed up scroll

Fixes JetBrains/compose-multiplatform#1324

Fix crash when AWT event is sent after the window is disposed

Fixes JetBrains/compose-multiplatform#1448 (comment)

AWT can send event even after calling `window.dipose`

I checked, and it nothing to do with scheduleSyntheticMoveEvent - the crash still reproducible without it.

Remove async events

Async events is cause why sometimes interaction is so clunky:
- when we resize a heavy undecorated window from #124, it continue resizing, even if we release the mouse
- when we scroll CodeViewer, it continues to scroll for 1-2 seconds

Async events removed freezes for us for scrolling heavy lazy lists, but instead it adds unresponsive UI that lives its own live.

Also, i checked heavy lazy list (codeviewer), and it doesn't freeze anymore during scrolling

Partially fixes JetBrains/compose-multiplatform#1345

Without that we can't merge #124

1
igordmn added a commit that referenced this pull request Jan 18, 2022
@igordmn
Get rid of async
69725b2 
- Get rid of async
- consume
- check if disposed

JetBrains/compose-multiplatform#1448

JetBrains/compose-multiplatform#1345

The sequence of calls:
mouseReleased
  window.dispose
    scene.dispose
  cancelRippleEffect
    scene.invalidate
      layer.needRedraw

Fix crash when AWT event is sent after the window is disposed

Fixes JetBrains/compose-multiplatform#1448 (comment)

AWT can send event even after calling `window.dipose`

Remove async events

Async events is cause why sometimes interaction is so clunky:
- when we resize a heavy undecorated window from #124, it continue resizing, even if we release the mouse
- when we scroll CodeViewer, it continues to scroll for 1-2 seconds

Async events removed freezes for us for scrolling heavy lazy lists, but instead it adds unresponsive UI that lives its own live.

Also, i checked heavy lazy list (codeviewer), and it doesn't freeze anymore during scrolling
igordmn added a commit that referenced this pull request Jan 20, 2022
@igordmn
Desktop. Remove scheduling events on the next event loop frame
1c08b7f 
Because of that we sometimes have a clunky unresponsive UI that lives its own live:
- when we resize a heavy undecorated window, it continues to resize, even if we release the mouse (after we upstream this feature #124)
- when we scroll CodeViewer, it continues to scroll for 1-2 seconds. when we scroll fast up/down, it will also be scrolling up/down for some time.

We added it in the past to remove freezes when we scroll heavy lazy lists:
- the user scrolls very fast
- we receive 30-50 events in one frame
- we handle each scroll event (each event can be processed for 50ms), and freeze rendering for some time (for 1-2 seconds)

The better fix for this issue would be either to:
1. handle only the first scroll event, and skip the others in the current frame (this was already implemented here: https://android-review.googlesource.com/c/platform/frameworks/support/+/1837304/33/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/Scrollable.kt#233)
2. or batch multiple scroll events into the one big event (it is planned in JetBrains/compose-multiplatform#1345)

The code inside MouseScrollFilter.kt isn't used anywhere in Compose, it just committed to maintain backward compatibility and to keep codebase in sync with jb-main. It is marked as Experimental in Compose 1.0.0 MPP, so it is probably safe to remove it in 1.2.0

Additional fix:

Fix crash when AWT event is sent after the window is disposed (we check isDisposed on every event now). AWT can send event even after calling `window.dipose`

Fixes JetBrains/compose-multiplatform#1448 (comment)

Test: ./gradlew jvmTest desktopTest -Pandroidx.compose.multiplatformEnabled=true
igordmn added a commit that referenced this pull request Jan 20, 2022
@igordmn
Desktop. Remove scheduling events on the next event loop frame
c4ad787 
Because of that we sometimes have a clunky unresponsive UI that lives its own live:
- when we resize a heavy undecorated window, it continues to resize, even if we release the mouse (after we upstream this feature #124)
- when we scroll CodeViewer, it continues to scroll for 1-2 seconds. when we scroll fast up/down, it will also be scrolling up/down for some time.

We added it in the past to remove freezes when we scroll heavy lazy lists:
- the user scrolls very fast
- we receive 30-50 events in one frame
- we handle each scroll event (each event can be processed for 50ms), and freeze rendering for some time (for 1-2 seconds)

The better fix for this issue would be either to:
1. handle only the first scroll event, and skip the others in the current frame (this was already implemented here: https://android-review.googlesource.com/c/platform/frameworks/support/+/1837304/33/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/Scrollable.kt#233)
2. or batch multiple scroll events into the one big event (it is planned in JetBrains/compose-multiplatform#1345)

The code inside MouseScrollFilter.kt isn't used anywhere in Compose, it just committed to maintain backward compatibility and to keep codebase in sync with jb-main. It is marked as Experimental in Compose 1.0.0 MPP, so it is probably safe to remove it in 1.2.0. It breaks tests though (not functional itself). Mark it @ignored for now, will remove when we upstream ImageComposeScene.

Additional fix:

Fix crash when AWT event is sent after the window is disposed (we check isDisposed on every event now). AWT can send event even after calling `window.dipose`

Fixes JetBrains/compose-multiplatform#1448 (comment)

Test: ./gradlew jvmTest desktopTest -Pandroidx.compose.multiplatformEnabled=true
copybara-service bot pushed a commit to androidx/androidx that referenced this pull request Feb 1, 2022
@igordmn
Desktop. Remove scheduling events on the next event loop frame
0a818ac 
Because of that we sometimes have a clunky unresponsive UI that lives its own live:
- when we resize a heavy undecorated window, it continues to resize, even if we release the mouse (after we upstream this feature JetBrains#124)
- when we scroll CodeViewer, it continues to scroll for 1-2 seconds

We added it in the past to remove freezes when we scroll heavy lazy lists:
- the user scrolls very fast
- we receive 30-50 events in one frame
- we handle each scroll event (each event can be processed for 50ms), and freeze rendering for some time (for 1-2 seconds)

The better fix for this issue would be either to:
1. handle only the first scroll event, and skip the others in the current frame (this was already implemented here: https://android-review.googlesource.com/c/platform/frameworks/support/+/1837304/33/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/Scrollable.kt#233)
2. or batch multiple scroll events into the one big event (it is planned in JetBrains/compose-multiplatform#1345)

The code inside MouseScrollFilter.kt isn't used anywhere in Compose, it just committed to maintain backward compatibility and to keep codebase in sync with jb-main. It is marked as Experimental in Compose 1.0.0 MPP, so it is probably safe to remove it in 1.2.0. It breaks tests though (not functional itself). I mark it @ignored for now, will remove when we upstream ImageComposeScene.

Additional fix:

Fix crash when AWT event is sent after the window is disposed (we check isDisposed on every event now). AWT can send event even after calling `window.dispose`

Fixes JetBrains/compose-multiplatform#1448 (comment)

Test: ./gradlew jvmTest desktopTest -Pandroidx.compose.multiplatformEnabled=true
Change-Id: I2ffac68c0e9d13bbfb51dbcc980a139c1ec701bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rsedaikin Rsedaikin Rsedaikin approved these changes

@olonho olonho olonho approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@igordmn @olonho @Rsedaikin