-
Notifications
You must be signed in to change notification settings - Fork 81
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 WindowState and DialogState savers to not crash on unspecified window/dialog size. #1394
Conversation
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/window/WindowState.desktop.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/window/WindowState.desktop.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/window/WindowState.desktop.kt
Outdated
Show resolved
Hide resolved
Hi, issue submitter here. Amazing turnaround! It looks like you'll need to apply the same fix in at least |
4a492c1
to
2c97866
Compare
There doesn't seem to be documentation that |
DialogState indeed doesn't contain it compare to WindowState 🤔. It was an oversight, and it should be in KDoc |
|
||
index = 1 | ||
waitForIdle() | ||
index = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test doesn't check that state can be restored correctly.
To check that, we need to change the size, and switch back to index = 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the need to switch back to index = 0
, but why change the size?
The latest commit seems to work (I purposely "broke" restoring in order to test it, and the test fails correctly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why change the size?
If we don't change the size, then we can't be sure that value was restored instead of reusing the initial passed value (or reused the value saved in index=1).
To check it correctly, it seems we need to have the initial size not Unspecified:
var index by mutableIntStateOf(0)
lateinit var lastRecomposedState: WindowState
setContent {
val saveableStateHolder = rememberSaveableStateHolder()
saveableStateHolder.SaveableStateProvider(index) {
val state = rememberWindowState(size = DpSize(10f, 10f))
lastRecomposedState = sate
}
}
state.size = DpSize.Unspecified
waitForIdle()
assertWindowStateEquals(WindowState(size = DpSize.Unspecified), lastRecomposedState)
index = 1
waitForIdle()
assertWindowStateEquals(WindowState(size = DpSize(10f, 10f)), lastRecomposedState)
index = 0
waitForIdle()
assertWindowStateEquals(WindowState(size = DpSize.Unspecified), lastRecomposedState)
…ndow/dialog size.
2c97866
to
d53e952
Compare
Fix WindowState.Saver to correctly save/restore unspecified window size.
Fixes JetBrains/compose-multiplatform#4945
Testing
Manually with
Release Notes
Fixes - Desktop
WindowState
with unspecifiedsize
instead of crashing.