Skip to content

Call Modifier.onGloballyPositioned when window moves #2163

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

Jump to bottom
Merged
merged 9 commits into from
Jun 12, 2025
Merged

Call Modifier.onGloballyPositioned when window moves #2163

merged 9 commits into from
Jun 12, 2025

Conversation

m-sasha
Copy link
Member

@m-sasha m-sasha commented Jun 11, 2025 •
edited
Loading

When the window moves or becomes iconified/deiconified, call Modifier.onGloballyPositioned.

Fixes https://youtrack.jetbrains.com/issue/CMP-8337/Modifier.onGloballyPositioned-should-be-called-when-window-is-moved

Testing

Tested manually with

fun main() = singleWindowApplication {
    Box(Modifier.fillMaxSize(), contentAlignment = Alignment.Center) {
        Column(horizontalAlignment = Alignment.CenterHorizontally) {
            var rect: Rect? by remember { mutableStateOf(null) }
            Box(
                Modifier
                    .size(100.dp)
                    .background(Color.Red)
                    .onGloballyPositioned { layout ->
                        rect = layout.boundsInParent().translate(layout.positionOnScreen()).also {
                            println(it)
                        }
                    }
            )
            Text("$rect", Modifier.size(400.dp, 40.dp), textAlign = TextAlign.Center)
        }
    }
}

and also added unit tests.

This should be tested by QA

Release Notes

Fixes - Desktop

  • When the window moves or becomes iconified/de-iconified, all instances of Modifier.onGloballyPositioned will be called.

Migration Notes - Desktop

  • When the window is iconified, converting to/from screen coordinates (with e.g.LayoutCoordinates.positionOnScreen()) will return Offset.Unspecified.

@m-sasha m-sasha requested a review from igordmn June 11, 2025 08:27
@MatkovIvan
Copy link
Member

I don't think that it should be a part of Breaking Changes category in release notes.
If I miss something and it's really BREAKING, let's avoid such changes completely.

@m-sasha
Copy link
Member Author

m-sasha commented Jun 11, 2025

I don't think that it should be a part of Breaking Changes category in release notes.

I agree it's maybe weaker than BREAKING CHANGE, but it's still something to be aware of. We don't have a suitable category, so I preferred to warn users rather than say nothing.

If I miss something and it's really BREAKING, let's avoid such changes completely.

It's a new value you can receive when calling positionOnScreen(), so if you're not expecting it, something bad can happen.

MatkovIvan
MatkovIvan reviewed Jun 11, 2025
View reviewed changes
...e/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/platform/PlatformWindowContext.desktop.kt
*/
private inline fun withLocationOnScreenOrUnspecified(
component: Component,
block: (locationOnScreen: Offset) -> Offset
Copy link
Member

Choose a reason for hiding this comment

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

It's the last action in the function. There is no reason to introduce another lambda instead of just returning location on screen for the component

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean. Without a lambda it would not be possible to directly return Offset.Unspecified from the calling function without checking the return value of the helper function.

compose/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/scene/ComposeScene.skiko.kt
* @see PlatformContext.convertLocalToScreenPosition
* @see PlatformContext.convertScreenToLocalPosition
*/
fun invalidatePositionOnScreen()
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need a new API here? It seems calling invalidatePositionInWindow should work just fine here. So having two functions with the same effect is confusing from public contract POV. Narrowing set of invalidation is an implementation detail, sot sure that it's actually worth it.

If you don't like explicit InWindow mentioning in the name we probably can rename it to something like
invalidateLocalCoordinates or so. It should look better than two methods.

🤔 but on another hand it might make sense. So I'm not really sure here

Copy link
Member Author

Choose a reason for hiding this comment

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

Position in window and position on screen are categorically different, as evidenced by the fact that we forgot to even consider changes in position on screen as important for the scene.

So I disagree that it's an implementation detail.

@MatkovIvan
Copy link
Member

I agree it's maybe weaker than BREAKING CHANGE, but it's still something to be aware of. We don't have a suitable category

We do. "Migration Notes" is exactly what you're looking for.

@m-sasha
Copy link
Member Author

m-sasha commented Jun 11, 2025

I agree it's maybe weaker than BREAKING CHANGE, but it's still something to be aware of. We don't have a suitable category

We do. "Migration Notes" is exactly what you're looking for.

Changed to Migration Notes

@m-sasha m-sasha requested a review from MatkovIvan June 11, 2025 09:25
@m-sasha m-sasha merged commit 9c68570 into jb-main Jun 12, 2025
10 checks passed
@m-sasha m-sasha deleted the m-sasha/call-onGloballyPositioned-when-window-moves branch June 12, 2025 07:18
@m-sasha m-sasha mentioned this pull request Jun 12, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MatkovIvan MatkovIvan MatkovIvan approved these changes

@igordmn igordmn igordmn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@m-sasha @MatkovIvan @igordmn