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

Update WindowInfo.containerSize on window resize to keep it correct when window is moved between displays #1333

Merged
merged 1 commit into from
May 6, 2024

Conversation

m-sasha
Copy link

@m-sasha m-sasha commented Apr 30, 2024

ComposeContainter currently registers the listener for window size and position on the windowContainer, which is a JLayeredPane. It ends up not being called at all when the window is resized or moved. According to @MatkovIvan, however, this is intentional and merely poorly named (e.g. onChangeWindowPosition is meant to be called when the windowContainer's position changes, not the window's).

Because of this, when a window is moved to another screen with a different density, the size we store for it in WindowInfo.containerSize isn't updated. This, in turn, causes issues with popup positioning.

This PR registers an additional size listener on the window, in order to update WindowInfo.containerSize when the window size changes. It also renames all the related listener functions.

Fixes JetBrains/compose-multiplatform#4697
Possibly fixes additional issues where the window size was not being updated after moving it to a different screen.

Testing

Tested manually with this code:

@Composable
fun TestButton(text: String) {
    val displayHover = remember { mutableStateOf(false) }

    Column(
        modifier = Modifier
    ) {
        Button(
            onClick = {
                displayHover.value = true
            },
            modifier = Modifier.pointerHoverIcon(PointerIcon.Hand)
        ) {
            Text(text)
        }
        DropdownMenu(
            displayHover.value,
            onDismissRequest = {
                displayHover.value = false
            },
            modifier = Modifier.background(MaterialTheme.colors.background),
        ) {
            Row(Modifier.width(200.dp), horizontalArrangement = Arrangement.SpaceBetween) {
                Text(
                    text = "My tooltip",
                )
                Text(
                    text = "2 tooltip",
                )
            }

        }
    }
}

fun main() = singleWindowApplication {
    MaterialTheme {
        Box(Modifier.fillMaxSize(), contentAlignment = Alignment.TopEnd) {
            TestButton("Hello, World!")
        }
    }
}

To simulate a 2nd screen, I've used this app: https://github.com/Stengo/DeskPad and configured the 2nd screen to be of a very low resolution (such that the density is 1.0).

This could be tested by QA

Release Notes

Fixes - Desktop

  • Fix dropdown/popup positioning when a window is moved to a screen with a different density

@m-sasha m-sasha requested a review from MatkovIvan April 30, 2024 14:19
@m-sasha m-sasha requested a review from MatkovIvan April 30, 2024 14:41
@@ -512,7 +512,10 @@ private fun rememberPopupMeasurePolicy(
-platformInsets.top.roundToPx()
)
val positionInWindow = popupPositionProvider.calculatePosition(
boundsWithoutInsets, sizeWithoutInsets, layoutDirection, contentSize
Copy link
Member

Choose a reason for hiding this comment

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

Please revert unrelated change

Copy link
Author

Choose a reason for hiding this comment

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

It's not entirely unrelated, and it improves readability, and I don't want to waste time creating a separate PR for such a tiny change.

@m-sasha m-sasha force-pushed the m-sasha/fix-window-position-and-size-listener branch from 161fa84 to 400d90f Compare April 30, 2024 19:03
@m-sasha m-sasha changed the title In ComposeContainer, register the window move/resize listener on the window, rather than on the layered pane. Update WindowInfo.containerSize on window resize to keep it correct when window is moved between displays Apr 30, 2024
@m-sasha m-sasha requested a review from MatkovIvan April 30, 2024 19:10
… listener functions related to ComposeContainer.
@m-sasha m-sasha force-pushed the m-sasha/fix-window-position-and-size-listener branch from 400d90f to 323be90 Compare May 1, 2024 08:33
@m-sasha m-sasha requested a review from MatkovIvan May 1, 2024 08:36
Comment on lines +154 to +156
// When the window is moved to a display with a different density, componentResized is
// called on the window, but not on `windowContainer`, so we need to update the size
// here too.
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to have a test for this case?

Copy link
Author

Choose a reason for hiding this comment

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

I can't think of an easy way.

@m-sasha m-sasha merged commit 383239d into jb-main May 6, 2024
6 checks passed
@m-sasha m-sasha deleted the m-sasha/fix-window-position-and-size-listener branch May 6, 2024 09:33
MatkovIvan pushed a commit that referenced this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants