Skip to content

Commit c973fff

Browse files
m-sasham-sasha
committed
Address review feedback.
1 parent 666836b commit c973fff

File tree

6 files changed

+70
-72
lines changed

6 files changed

+70
-72
lines changed

‎compose/ui/ui-test-junit4/src/desktopTest/kotlin/androidx/compose/ui/test/TestBasicsTest.kt

Lines changed: 10 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ class TestBasicsTest {
5656

5757
// See https://github.com/JetBrains/compose-multiplatform/issues/3117
5858
@Test
59-
fun recompositionCompletesBeforeSetContentReturns() = repeat(1000) {
59+
fun recompositionCompletesBeforeSetContentReturns() = repeat(100) {
6060
runSkikoComposeUiTest {
6161
var globalValue by atomic(0)
6262
setContent {
@@ -92,27 +92,6 @@ class TestBasicsTest {
9292
assertTrue(clockAfter > clockBefore, "performClick did not advance the test clock")
9393
}
9494

95-
@Test
96-
fun advancingClockRunsRecomposition() {
97-
rule.mainClock.autoAdvance = false
98-
99-
rule.setContent {
100-
var text by remember { mutableStateOf("1") }
101-
Text(text, modifier = Modifier.testTag("text"))
102-
103-
LaunchedEffect(Unit) {
104-
delay(1_000)
105-
text = "2"
106-
}
107-
}
108-
109-
rule.onNodeWithTag("text").assertTextEquals("1")
110-
rule.mainClock.advanceTimeBy(999, ignoreFrameDuration = true)
111-
rule.onNodeWithTag("text").assertTextEquals("1")
112-
rule.mainClock.advanceTimeBy(1, ignoreFrameDuration = true)
113-
rule.onNodeWithTag("text").assertTextEquals("2")
114-
}
115-
11695
@Test
11796
fun obtainingSemanticsNodeInteractionWaitsUntilIdle() {
11897
var text by mutableStateOf("1")
@@ -251,4 +230,13 @@ class TestBasicsTest {
251230

252231
assertContentEquals(listOf("Composition", "LaunchedEffect"), actions)
253232
}
233+
234+
@Test
235+
fun waitForIdleDoesNotAdvanceClockIfAlreadyIdle() = runComposeUiTest {
236+
setContent { }
237+
238+
val initialTime = mainClock.currentTime
239+
waitForIdle()
240+
assertEquals(initialTime, mainClock.currentTime)
241+
}
254242
}

‎compose/ui/ui-test/src/skikoMain/kotlin/androidx/compose/ui/test/ComposeUiTest.skikoMain.kt

Lines changed: 36 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import androidx.compose.ui.platform.InfiniteAnimationPolicy
2929
import androidx.compose.ui.platform.PlatformContext
3030
import androidx.compose.ui.platform.WindowInfo
3131
import androidx.compose.ui.scene.ComposeScene
32-
import androidx.compose.ui.scene.ComposeSceneContext
3332
import androidx.compose.ui.scene.CanvasLayersComposeScene
3433
import androidx.compose.ui.semantics.SemanticsNode
3534
import androidx.compose.ui.text.input.EditCommand
@@ -189,7 +188,7 @@ class SkikoComposeUiTest @InternalTestApi constructor(
189188
fun <R> runTest(block: SkikoComposeUiTest.() -> R): R {
190189
return composeRootRegistry.withRegistry {
191190
withScene {
192-
renderingAtFrameRate {
191+
withRenderLoop {
193192
block()
194193
}
195194
}
@@ -210,13 +209,15 @@ class SkikoComposeUiTest @InternalTestApi constructor(
210209
}
211210
}
212211

213-
private inline fun <R> renderingAtFrameRate(block: () -> R): R {
212+
private inline fun <R> withRenderLoop(block: () -> R): R {
214213
val scope = CoroutineScope(coroutineContext)
215214
return try {
216215
scope.launch {
217216
while (isActive) {
218217
delay(FRAME_DELAY_MILLIS)
219-
render(mainClock.currentTime)
218+
runOnUiThread {
219+
render(mainClock.currentTime)
220+
}
220221
}
221222
}
222223
block()
@@ -225,20 +226,16 @@ class SkikoComposeUiTest @InternalTestApi constructor(
225226
}
226227
}
227228

229+
/**
230+
* Render the scene at the given time.
231+
*/
228232
private fun render(timeMillis: Long) {
229233
scene.render(
230234
surface.canvas.asComposeCanvas(),
231235
timeMillis * NanoSecondsPerMilliSecond
232236
)
233237
}
234238

235-
private fun renderNextFrame() = runOnUiThread {
236-
render(mainClock.currentTime)
237-
if (mainClock.autoAdvance) {
238-
mainClock.advanceTimeByFrame()
239-
}
240-
}
241-
242239
private fun createUi() = CanvasLayersComposeScene(
243240
density = density,
244241
size = size,
@@ -247,49 +244,54 @@ class SkikoComposeUiTest @InternalTestApi constructor(
247244
invalidate = { }
248245
)
249246

250-
private fun shouldPumpTime(): Boolean {
251-
return mainClock.autoAdvance &&
252-
(Snapshot.current.hasPendingChanges()
253-
|| Snapshot.isApplyObserverNotificationPending
254-
|| scene.hasInvalidations())
247+
private fun advanceIfNeededAndRenderNextFrame() {
248+
if (mainClock.autoAdvance) {
249+
mainClock.advanceTimeByFrame()
250+
// The rendering is done by withRenderLoop
251+
} else {
252+
runOnUiThread {
253+
render(mainClock.currentTime)
254+
}
255+
}
255256
}
256257

257258
@OptIn(InternalComposeUiApi::class)
258259
private fun isIdle(): Boolean {
259-
var i = 0
260-
while (i < 100 && shouldPumpTime()) {
261-
mainClock.advanceTimeByFrame()
262-
++i
260+
if (composeRootRegistry.getComposeRoots().any { it.hasPendingMeasureOrLayout }) {
261+
return false
263262
}
264263

265-
val hasPendingMeasureOrLayout = composeRootRegistry.getComposeRoots().any {
266-
it.hasPendingMeasureOrLayout
264+
if (!mainClock.autoAdvance) {
265+
return true
267266
}
268267

269-
return !shouldPumpTime() && !hasPendingMeasureOrLayout && areAllResourcesIdle()
268+
return !Snapshot.current.hasPendingChanges()
269+
&& !Snapshot.isApplyObserverNotificationPending
270+
&& !scene.hasInvalidations()
271+
&& areAllResourcesIdle()
270272
}
271273

272274
override fun waitForIdle() {
273275
// TODO: consider adding a timeout to avoid an infinite loop?
274-
do {
275-
// always check even if we are idle
276-
uncaughtExceptionHandler.throwUncaught()
277-
renderNextFrame()
276+
// always check even if we are idle
277+
uncaughtExceptionHandler.throwUncaught()
278+
while (!isIdle()) {
279+
advanceIfNeededAndRenderNextFrame()
278280
uncaughtExceptionHandler.throwUncaught()
279281
if (!areAllResourcesIdle()) {
280282
sleep(IDLING_RESOURCES_CHECK_INTERVAL_MS)
281283
}
282-
} while (!isIdle())
284+
}
283285
}
284286

285287
override suspend fun awaitIdle() {
286288
// always check even if we are idle
287289
uncaughtExceptionHandler.throwUncaught()
288290
while (!isIdle()) {
289-
renderNextFrame()
291+
advanceIfNeededAndRenderNextFrame()
290292
uncaughtExceptionHandler.throwUncaught()
291293
if (!areAllResourcesIdle()) {
292-
delay(IDLING_RESOURCES_CHECK_INTERVAL_MS)
294+
sleep(IDLING_RESOURCES_CHECK_INTERVAL_MS)
293295
}
294296
yield()
295297
}
@@ -315,9 +317,11 @@ class SkikoComposeUiTest @InternalTestApi constructor(
315317
val startTime = currentNanoTime()
316318
val timeoutNanos = timeoutMillis * NanoSecondsPerMilliSecond
317319
while (!condition()) {
318-
renderNextFrame()
320+
advanceIfNeededAndRenderNextFrame()
319321
if (currentNanoTime() - startTime > timeoutNanos) {
320-
buildWaitUntilTimeoutMessage(timeoutMillis, conditionDescription)
322+
throw ComposeTimeoutException(
323+
buildWaitUntilTimeoutMessage(timeoutMillis, conditionDescription)
324+
)
321325
}
322326
}
323327

‎compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/awt/ComposeFocusTest.kt

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,10 @@ import androidx.compose.ui.unit.dp
3737
import androidx.compose.ui.window.Popup
3838
import com.google.common.truth.Truth.assertThat
3939
import java.awt.BorderLayout
40-
import java.awt.Component
4140
import java.awt.Dimension
4241
import java.awt.GraphicsEnvironment
4342
import java.awt.KeyboardFocusManager
4443
import java.awt.Window
45-
import java.awt.event.FocusEvent
4644
import java.awt.event.FocusEvent.Cause
4745
import java.awt.event.KeyEvent
4846
import javax.swing.JButton
@@ -51,8 +49,6 @@ import javax.swing.JPanel
5149
import kotlin.random.Random
5250
import kotlin.test.Test
5351
import kotlin.test.assertEquals
54-
import kotlin.test.assertTrue
55-
import kotlin.test.assertFalse
5652
import kotlinx.coroutines.delay
5753
import kotlinx.coroutines.runBlocking
5854
import org.jetbrains.skiko.MainUIDispatcher
@@ -954,15 +950,25 @@ class FocusTestScope {
954950
windows.clear()
955951
}
956952

953+
private fun focusOwner() = KeyboardFocusManager.getCurrentKeyboardFocusManager().focusOwner.also {
954+
if (it == null) {
955+
throw AssertionError(
956+
"The focus owner is `null`; " +
957+
"This can happen due to interference from the user or the window manager. " +
958+
"Try running the test without interacting with any windows."
959+
)
960+
}
961+
}
962+
957963
suspend fun pressNextFocusKey() {
958-
val focusOwner = KeyboardFocusManager.getCurrentKeyboardFocusManager().focusOwner
964+
val focusOwner = focusOwner()
959965
focusOwner.dispatchEvent(KeyEvent(focusOwner, KeyEvent.KEY_PRESSED, 0, 0, KeyEvent.VK_TAB, '\t'))
960966
focusOwner.dispatchEvent(KeyEvent(focusOwner, KeyEvent.KEY_RELEASED, 0, 0, KeyEvent.VK_TAB, '\t'))
961967
awaitEdtAfterDelay()
962968
}
963969

964970
suspend fun pressPreviousFocusKey() {
965-
val focusOwner = KeyboardFocusManager.getCurrentKeyboardFocusManager().focusOwner
971+
val focusOwner = focusOwner()
966972
focusOwner.dispatchEvent(KeyEvent(focusOwner, KeyEvent.KEY_PRESSED, 0, KeyEvent.SHIFT_DOWN_MASK, KeyEvent.VK_TAB, '\t'))
967973
focusOwner.dispatchEvent(KeyEvent(focusOwner, KeyEvent.KEY_RELEASED, 0, KeyEvent.SHIFT_DOWN_MASK, KeyEvent.VK_TAB, '\t'))
968974
awaitEdtAfterDelay()

‎compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/FlushCoroutineDispatcher.skiko.kt

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -44,19 +44,20 @@ internal class FlushCoroutineDispatcher(
4444
private val scope = CoroutineScope(scope.coroutineContext.minusKey(Job))
4545
private var immediateTasks = ArrayDeque<Runnable>()
4646
private val delayedTasks = ArrayDeque<Runnable>()
47-
private val tasksLock = createSynchronizedObject()
47+
private val immediateTasksLock = createSynchronizedObject()
48+
private val delayedTasksLock = createSynchronizedObject()
4849
private var immediateTasksSwap = ArrayDeque<Runnable>()
4950
@Volatile
5051
private var isPerformingRun = false
5152
private val runLock = createSynchronizedObject()
5253

5354
override fun dispatch(context: CoroutineContext, block: Runnable) {
54-
synchronized(tasksLock) {
55+
synchronized(immediateTasksLock) {
5556
immediateTasks.add(block)
5657
}
5758
scope.launch {
5859
performRun {
59-
val isTaskAlive = synchronized(tasksLock) {
60+
val isTaskAlive = synchronized(immediateTasksLock) {
6061
immediateTasks.remove(block)
6162
}
6263
if (isTaskAlive) {
@@ -70,12 +71,12 @@ internal class FlushCoroutineDispatcher(
7071
* Whether the dispatcher has immediate tasks pending.
7172
*/
7273
fun hasImmediateTasks() = isPerformingRun ||
73-
synchronized(tasksLock) { immediateTasks.isNotEmpty() }
74+
synchronized(immediateTasksLock) { immediateTasks.isNotEmpty() }
7475

7576
/**
7677
* Whether the dispatcher has delayed tasks pending.
7778
*/
78-
fun hasDelayedTasks() = synchronized(tasksLock) { delayedTasks.isNotEmpty() }
79+
fun hasDelayedTasks() = synchronized(delayedTasksLock) { delayedTasks.isNotEmpty() }
7980

8081
/**
8182
* Perform all scheduled tasks and wait for the tasks which are already
@@ -85,7 +86,7 @@ internal class FlushCoroutineDispatcher(
8586
// Run tasks until they're empty in order to executed even ones that are added by the tasks
8687
// pending at the start
8788
while (true) {
88-
synchronized(tasksLock) {
89+
synchronized(immediateTasksLock) {
8990
if (immediateTasks.isEmpty())
9091
return@performRun
9192

@@ -112,13 +113,13 @@ internal class FlushCoroutineDispatcher(
112113
@OptIn(ExperimentalCoroutinesApi::class)
113114
override fun scheduleResumeAfterDelay(timeMillis: Long, continuation: CancellableContinuation<Unit>) {
114115
val block = Runnable { continuation.resume(Unit, null) }
115-
synchronized(tasksLock) {
116+
synchronized(delayedTasksLock) {
116117
delayedTasks.add(block)
117118
}
118119
val job = scope.launch {
119120
kotlinx.coroutines.delay(timeMillis)
120121
performRun {
121-
val isTaskAlive = synchronized(tasksLock) {
122+
val isTaskAlive = synchronized(delayedTasksLock) {
122123
delayedTasks.remove(block)
123124
}
124125
if (isTaskAlive) {
@@ -128,7 +129,7 @@ internal class FlushCoroutineDispatcher(
128129
}
129130
continuation.invokeOnCancellation {
130131
job.cancel()
131-
synchronized(tasksLock) {
132+
synchronized(delayedTasksLock) {
132133
delayedTasks.remove(block)
133134
}
134135
}

‎compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/RenderPhasesTest.kt

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,10 +229,9 @@ class RenderPhasesTest {
229229
assertContentEquals(emptyList(), phases)
230230
startTest = true
231231
mainClock.advanceTimeByFrame()
232-
waitForIdle() // Make the effects run
233232
assertContentEquals(
234233
expected = listOf("draw", "launchedWithFrame", "launchedFromComposition", "launchedFromEffect"),
235-
actual = phases.subList(0, 4),
234+
actual = phases
236235
)
237236
}
238237

‎compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/window/DialogTest.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,8 +256,8 @@ class DialogTest {
256256

257257
assertEquals(null, lastValueInComposition)
258258
showDialog = true
259-
mainClock.advanceTimeBy(1000)
260259
waitForIdle()
260+
mainClock.advanceTimeBy(1000)
261261
assertEquals(2, lastValueInComposition)
262262
}
263263
}

0 commit comments

Comments
 (0)