Skip to content

Commit

Permalink
Prevent a few unnecessary recompositions in Popup and DesktopMenu. (#…
Browse files Browse the repository at this point in the history 
…1225)
  • Loading branch information
@m-sasha
m-sasha committed Apr 2, 2024
1 parent 28cad83 commit 277215f
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 52 deletions.
8 changes: 4 additions & 4 deletions ...material/material/src/desktopMain/kotlin/androidx/compose/material/DesktopMenu.desktop.kt
View file
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
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
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)
val currentOnKeyEvent by rememberUpdatedState(onKeyEvent)

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

0 comments on commit 277215f

Please sign in to comment.