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 window flashing background with unspecified size #442

Merged
merged 10 commits into from
Mar 24, 2023

Conversation

m-sasha
Copy link

@m-sasha m-sasha commented Mar 20, 2023

Proposed Changes

The original fix to the window background being visible before the context was to add this in the update callback to AwtWindow:

AwtWindow(
    ...
    update = {
        ...
        if (!it.isDisplayable) {
            it.makeDisplayable()
            it.contentPane.paint(it.graphics)
        }
    }
)

Unfortunately, if the window/dialog size is Unspecified, setSizeSafely calls pack() on the window, to determine its preferred size, making is displayable, and so the code above isn't called.

This PR does the following

  • Always call pack() in setSizeSafely to avoid an inconsistent state.
  • Call revalidate instead of pack to force composition+layout.
  • Improve the check for when the window is shown for the first time.
  • Remove window.makeDisplayable() from AwtWindow. Not sure why it was needed.
  • Make setSizeSafely a bit more easy to understand.

Testing

Test: Added new unit tests.

Issues Fixed

Fixes: JetBrains/compose-multiplatform#1794

@m-sasha m-sasha force-pushed the fix-window-flashing-background-with-unspecified-size branch from 4a0907f to da58c9c Compare March 20, 2023 16:21
@igordmn igordmn self-requested a review March 20, 2023 16:29
@Walingar
Copy link
Collaborator

Walingar commented Mar 20, 2023

Please take a look at failing WindowStateTest.set window size by its content when window is on the screen good test case, that is not handled, when size was sepcified and became Unspecified

@m-sasha
Copy link
Author

m-sasha commented Mar 20, 2023

Noticied the changes in setSizeSafely. But setSizeSafely theoretically can be not called. In a case when the window is already this size. So, we won't call pack.

@igordmn setSizeSafely must be called at least once, because the check is

            if (state.size != appliedState.size) {

and the initial value of appliedState.size is null.

@m-sasha m-sasha requested a review from Walingar March 20, 2023 21:00
@m-sasha
Copy link
Author

m-sasha commented Mar 20, 2023

Changes in the latest commit:

  • setSizeSafely now always calls pack(), even if the window is already displayable. That's what was causing the WindowStateTest.set window size by its content when window is on the screen test to fail.
  • setSizeSafely is now responsible for updating the composition size after measuring its preferred size, instead of the Window/Dialog update function. It's better because setSizeSafely is the one responsible for "breaking" it.
  • The Window/Dialog update function is now responsible only for drawing the first frame.

Copy link
Collaborator

@Walingar Walingar left a comment

Choose a reason for hiding this comment

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

LGTM!

@m-sasha m-sasha force-pushed the fix-window-flashing-background-with-unspecified-size branch from 63e7126 to b98ec80 Compare March 22, 2023 18:06
@m-sasha m-sasha changed the base branch from jb-main to fix-window-dispose March 22, 2023 18:06
Base automatically changed from fix-window-dispose to jb-main March 22, 2023 23:24
@m-sasha m-sasha force-pushed the fix-window-flashing-background-with-unspecified-size branch from b98ec80 to ba7e155 Compare March 22, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants