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

Prevent a few unnecessary recompositions in Popup and DesktopMenu. #1225

Merged
merged 1 commit into from
Apr 2, 2024
Merged

Prevent a few unnecessary recompositions in Popup and DesktopMenu. #1225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions ...material/material/src/desktopMain/kotlin/androidx/compose/material/DesktopMenu.desktop.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ fun DropdownMenu(
@Deprecated(
level = DeprecationLevel.HIDDEN,
replaceWith = ReplaceWith(
expression = "DropdownMenu(expanded,onDismissRequest, focusable, modifier, offset, " +
expression = "DropdownMenu(state, onDismissRequest, focusable, modifier, offset, " +
"rememberScrollState(), content)",
"androidx.compose.foundation.rememberScrollState"
),
Expand Down Expand Up @@ -298,9 +298,9 @@ private fun OpenDropdownMenu(
focusable: Boolean = true,
modifier: Modifier = Modifier,
content: @Composable ColumnScope.() -> Unit
){
var focusManager: FocusManager? by mutableStateOf(null)
var inputModeManager: InputModeManager? by mutableStateOf(null)
) {
var focusManager: FocusManager? by remember { mutableStateOf(null) }
var inputModeManager: InputModeManager? by remember { mutableStateOf(null) }
Popup(
onDismissRequest = onDismissRequest,
popupPositionProvider = popupPositionProvider,
Expand Down
74 changes: 32 additions & 42 deletions ...ose/material/material/src/desktopTest/kotlin/androidx/compose/material/DesktopMenuTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ import androidx.compose.ui.test.ExperimentalTestApi
import androidx.compose.ui.test.assertLeftPositionInRootIsEqualTo
import androidx.compose.ui.test.getBoundsInRoot
import androidx.compose.ui.test.getUnclippedBoundsInRoot
import androidx.compose.ui.test.junit4.createComposeRule
import androidx.compose.ui.test.onNodeWithTag
import androidx.compose.ui.test.performKeyPress
import androidx.compose.ui.test.performMouseInput
import androidx.compose.ui.test.rightClick
import androidx.compose.ui.test.runComposeUiTest
import androidx.compose.ui.unit.Density
import androidx.compose.ui.unit.DpOffset
import androidx.compose.ui.unit.IntOffset
Expand All @@ -48,18 +48,13 @@ import androidx.compose.ui.unit.LayoutDirection
import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.size
import com.google.common.truth.Truth.assertThat
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlinx.coroutines.runBlocking
import org.junit.Assert
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.JUnit4

@RunWith(JUnit4::class)
@OptIn(ExperimentalTestApi::class)
class DesktopMenuTest {

@get:Rule
val rule = createComposeRule()

private val windowSize = IntSize(200, 200)

Expand Down Expand Up @@ -163,8 +158,8 @@ class DesktopMenuTest {

// (RTL) Anchor right is beyond the right of the window, so align popup to the window right
@Test
fun menu_positioning_rtl_windowRight_belowAnchor() {
rule.setContent {
fun menu_positioning_rtl_windowRight_belowAnchor() = runComposeUiTest {
setContent {
CompositionLocalProvider(LocalLayoutDirection provides LayoutDirection.Rtl) {
Box(Modifier.fillMaxSize().testTag("background")) {
Box(Modifier.offset(x = (-10).dp).size(50.dp)) {
Expand All @@ -175,15 +170,15 @@ class DesktopMenuTest {
}
}
}
val windowSize = rule.onNodeWithTag("background").getBoundsInRoot().size
rule.onNodeWithTag("box")
val windowSize = onNodeWithTag("background").getBoundsInRoot().size
onNodeWithTag("box")
.assertLeftPositionInRootIsEqualTo(windowSize.width - 50.dp)
}

@Test
fun `pressing ESC button invokes onDismissRequest`() {
fun `pressing ESC button invokes onDismissRequest`() = runComposeUiTest {
var dismissCount = 0
rule.setContent {
setContent {
CompositionLocalProvider(LocalDensity provides Density(1f, 1f)) {
DropdownMenu(true, onDismissRequest = {
dismissCount++
Expand All @@ -193,28 +188,24 @@ class DesktopMenuTest {
}
}

rule.onNodeWithTag("dropDownMenu")
onNodeWithTag("dropDownMenu")
.performKeyPress(keyEvent(Key.Escape, KeyEventType.KeyDown))

rule.runOnIdle {
Assert.assertEquals(1, dismissCount)
}
assertEquals(1, dismissCount)

rule.onNodeWithTag("dropDownMenu")
onNodeWithTag("dropDownMenu")
.performKeyPress(keyEvent(Key.Escape, KeyEventType.KeyUp))

rule.runOnIdle {
Assert.assertEquals(1, dismissCount)
}
assertEquals(1, dismissCount)
}

@Test
fun `navigate DropDownMenu using arrows`() {
fun `navigate DropDownMenu using arrows`() = runComposeUiTest {
var item1Clicked = 0
var item2Clicked = 0
var item3Clicked = 0

rule.setContent {
setContent {
CompositionLocalProvider(LocalDensity provides Density(1f, 1f)) {
DropdownMenu(true, onDismissRequest = {},
modifier = Modifier.testTag("dropDownMenu")) {
Expand All @@ -232,14 +223,14 @@ class DesktopMenuTest {
}

fun performKeyDownAndUp(key: Key) {
rule.onNodeWithTag("dropDownMenu").apply {
onNodeWithTag("dropDownMenu").apply {
performKeyPress(keyEvent(key, KeyEventType.KeyDown))
performKeyPress(keyEvent(key, KeyEventType.KeyUp))
}
}

fun assertClicksCount(i1: Int, i2: Int, i3: Int) {
rule.runOnIdle {
runOnIdle {
assertThat(item1Clicked).isEqualTo(i1)
assertThat(item2Clicked).isEqualTo(i2)
assertThat(item3Clicked).isEqualTo(i3)
Expand Down Expand Up @@ -271,11 +262,11 @@ class DesktopMenuTest {
assertClicksCount(2, 2, 2)
}

@OptIn(ExperimentalMaterialApi::class, ExperimentalTestApi::class)
@OptIn(ExperimentalMaterialApi::class)
@Test
fun `right click opens DropdownMenuState`() {
fun `right click opens DropdownMenuState`() = runComposeUiTest {
val state = DropdownMenuState(DropdownMenuState.Status.Closed)
rule.setContent {
setContent {
Box(
modifier = Modifier
.testTag("box")
Expand All @@ -286,18 +277,18 @@ class DesktopMenuTest {
)
}

rule.onNodeWithTag("box").performMouseInput {
onNodeWithTag("box").performMouseInput {
rightClick(Offset(10f, 10f))
}

assertThat(state.status == DropdownMenuState.Status.Open(Offset(10f, 10f)))
}

@OptIn(ExperimentalMaterialApi::class, ExperimentalTestApi::class)
@OptIn(ExperimentalMaterialApi::class)
@Test
fun `right doesn't open DropdownMenuState when disabled`() {
fun `right doesn't open DropdownMenuState when disabled`() = runComposeUiTest {
val state = DropdownMenuState(DropdownMenuState.Status.Closed)
rule.setContent {
setContent {
Box(
modifier = Modifier
.testTag("box")
Expand All @@ -309,18 +300,17 @@ class DesktopMenuTest {
)
}

rule.onNodeWithTag("box").performMouseInput {
onNodeWithTag("box").performMouseInput {
rightClick(Offset(10f, 10f))
}

assertThat(state.status == DropdownMenuState.Status.Closed)
}

@OptIn(ExperimentalTestApi::class)
@Test
fun `pass scroll state`() {
fun `pass scroll state`() = runComposeUiTest {
val scrollState = ScrollState(0)
rule.setContent {
setContent {
DropdownMenu(
true,
onDismissRequest = {},
Expand All @@ -332,23 +322,23 @@ class DesktopMenuTest {
}
}

val initialPosition = rule.onNodeWithTag("box").getUnclippedBoundsInRoot().top
val initialPosition = onNodeWithTag("box").getUnclippedBoundsInRoot().top

runBlocking {
scrollState.scroll {
scrollBy(10000f)
}
}
assertThat(
rule.onNodeWithTag("box").getUnclippedBoundsInRoot().top
onNodeWithTag("box").getUnclippedBoundsInRoot().top
).isLessThan(initialPosition)

rule.onNodeWithTag("menu").performMouseInput {
onNodeWithTag("menu").performMouseInput {
enter(center)
scroll(-10000f)
}
assertThat(
rule.onNodeWithTag("box").getUnclippedBoundsInRoot().top
onNodeWithTag("box").getUnclippedBoundsInRoot().top
).isEqualTo(initialPosition)
}
}
15 changes: 9 additions & 6 deletions compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/window/Popup.skiko.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import androidx.compose.runtime.Immutable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberUpdatedState
import androidx.compose.runtime.setValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.ExperimentalComposeUiApi
Expand All @@ -35,13 +36,10 @@ import androidx.compose.ui.layout.EmptyLayout
import androidx.compose.ui.layout.Layout
import androidx.compose.ui.layout.onGloballyPositioned
import androidx.compose.ui.layout.positionInWindow
import androidx.compose.ui.platform.InsetsConfig
import androidx.compose.ui.platform.LocalLayoutDirection
import androidx.compose.ui.platform.LocalWindowInfo
import androidx.compose.ui.platform.PlatformInsets
import androidx.compose.ui.platform.PlatformInsetsConfig
import androidx.compose.ui.platform.ZeroInsetsConfig
import androidx.compose.ui.platform.union
import androidx.compose.ui.scene.ComposeSceneLayer
import androidx.compose.ui.scene.rememberComposeSceneLayer
import androidx.compose.ui.semantics.popup
Expand Down Expand Up @@ -392,11 +390,15 @@ fun Popup(
onKeyEvent: ((KeyEvent) -> Boolean)? = null,
content: @Composable () -> Unit
) {
val currentOnDismissRequest by rememberUpdatedState(onDismissRequest)
igordmn marked this conversation as resolved.
Show resolved Hide resolved
val currentOnKeyEvent by rememberUpdatedState(onKeyEvent)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to do the same for dialog then


val overriddenOnKeyEvent = if (properties.dismissOnBackPress && onDismissRequest != null) {
// No need to remember this lambda, as it doesn't capture any values that can change.
{ event: KeyEvent ->
val consumed = onKeyEvent?.invoke(event) ?: false
val consumed = currentOnKeyEvent?.invoke(event) ?: false
if (!consumed && event.isDismissRequest()) {
onDismissRequest()
currentOnDismissRequest?.invoke()
true
} else {
consumed
Expand All @@ -406,9 +408,10 @@ fun Popup(
onKeyEvent
}
val onOutsidePointerEvent = if (properties.dismissOnClickOutside && onDismissRequest != null) {
// No need to remember this lambda, as it doesn't capture any values that can change.
{ eventType: PointerEventType ->
if (eventType == PointerEventType.Press) {
onDismissRequest()
currentOnDismissRequest?.invoke()
}
}
} else {
Expand Down