Skip to content

Ensure awaitFrame() only awaits a single frame when used from the main thread #3431

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

Closed
wants to merge 1 commit into from
Closed

Ensure awaitFrame() only awaits a single frame when used from the main thread #3431

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions ui/kotlinx-coroutines-android/src/HandlerDispatcher.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,14 @@ public suspend fun awaitFrame(): Long {
postFrameCallback(choreographer, cont)
}
}
// post into looper thread to figure it out
return suspendCancellableCoroutine { cont ->
Dispatchers.Main.dispatch(EmptyCoroutineContext, Runnable {
if (Looper.myLooper() === Looper.getMainLooper()) { // Check if we are already in the main looper thread
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

I'm not yet convinced this should be the solution though.
I would expect that Dispatchers.Main.immediate would be enough, but it is not due to our implementation details.

We'll fix it in the next release, but it is an open question whether we are going to use this particular approach though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No probs!

The only reason I took the Looper approach here is because this code is specific to Android, and I didn't see anything similar for JavaFX or Swing implementations (awaitPulse() was the closest parallel). I see no problems and I am not opposed to using Dispatchers.Main.immediate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to be the way though!

I've opened #3437 with a few changes on top of yours that I'll squash into your commit. Thanks for the contribution, we'll merge it in the next release!

updateChoreographerAndPostFrameCallback(cont)
})
} else { // post into looper thread to figure it out
Dispatchers.Main.dispatch(EmptyCoroutineContext, Runnable {
updateChoreographerAndPostFrameCallback(cont)
})
}
}
}

Expand Down
6 changes: 3 additions & 3 deletions ui/kotlinx-coroutines-android/test/HandlerDispatcherTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,14 @@ class HandlerDispatcherTest : TestBase() {
launch(Dispatchers.Main, start = CoroutineStart.UNDISPATCHED) {
expect(1)
awaitFrame()
expect(5)
expect(3)
}
expect(2)
// Run choreographer detection
mainLooper.runOneTask()
expect(3)
mainLooper.scheduler.advanceBy(50, TimeUnit.MILLISECONDS)
expect(4)
mainLooper.scheduler.advanceBy(50, TimeUnit.MILLISECONDS)
expect(5)
mainLooper.scheduler.advanceBy(51, TimeUnit.MILLISECONDS)
finish(6)
}
Expand Down