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.

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

Fix the background flashing when closing a window/dialog. #1032

Merged
merged 1 commit into from
Mar 4, 2025

Conversation

m-sasha
Copy link
Member

@m-sasha m-sasha commented Mar 4, 2025

Flushing the transaction causes the window background to be visible for a frame when closing the window.

Fixes: https://youtrack.jetbrains.com/issue/CMP-5651

This reverts #552. I have tested the IDE sample from Jewel, and that issue does not seem to reproduce anymore, even without flushing the transaction.

Screen.Recording.2025-03-04.at.12.07.24.mp4

…552)"

This reverts commit ac324c0.

Flushing the transaction causes the window background to be visible for a frame when closing the window.
https://youtrack.jetbrains.com/issue/CMP-5651
@m-sasha m-sasha requested a review from igordmn March 4, 2025 10:15
Copy link
Contributor

@elijah-semyonov elijah-semyonov left a comment

Choose a reason for hiding this comment

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

Just for more context:
What CATransaction.flush does is committing all changes of CALayer properties and trees to be rendered to be visible during next frame.

CoreAnimation rendering is a blackbox so grouping transactions explicitly or implicitly is the only way to make multiple changes in the tree visually simultaneous.

Implicit transactions are always scoped within main runloop work items so this invocation here introduced a state barrier between before and after potentially allowing CoreAnimation to render two different frames with states from before and after the barrier, which is perceived as flickering.

In order to avoid flickering with guarantees we somehow need to ensure that both changes of CALayer hierarchy by AWT and us are confined within one transaction, which this PR is required for but doesn't guarantee as far as I see it.

@m-sasha
Copy link
Member Author

m-sasha commented Mar 4, 2025

@elijah-semyonov Thanks for the input.

What I understood, but is clearly wrong, is that flushing the transaction is optional. I know it's wrong because removing the flush from other places in MetalRedrawer.mm, such as in resizeLayers, breaks Compose drawing completely (nothing is drawn). So what am I missing?

In order to avoid flickering with guarantees we somehow need to ensure that both changes of CALayer hierarchy by AWT
and us are confined within one transaction, which this PR is required for but doesn't guarantee as far as I see it.

It seems to me that in this particular instance, we're flushing a transaction that isn't our own (there's no [CATransaction begin]; in disposeDevice). Is that even reasonable?

@m-sasha m-sasha merged commit ea19091 into master Mar 4, 2025
6 checks passed
@m-sasha m-sasha deleted the m-sasha/dont-flush-transaction-on-dispose branch March 4, 2025 11:58
@elijah-semyonov
Copy link
Contributor

elijah-semyonov commented Mar 4, 2025

As per doc flush always happens in the end of runloop work item. In other words, any job executed on the macOS executable main thread will be confined within implicit CATransaction.

This API is not marked as @MainActor though, which is super-weird, considering that those rely on global access scope stack-like transaction hierarchy which should be isolated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants