Revert "Fix crash on state reset when preferences are queried"#441
Open
dfabulich wants to merge 1 commit into
Open
Revert "Fix crash on state reset when preferences are queried"#441dfabulich wants to merge 1 commit into
dfabulich wants to merge 1 commit into
Conversation
This reverts commit 23cf094. Fixes skiptools#440
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This reverts commit 23cf094 (PR #418).
Fixes #440
This PR may feel a bit trollish, but, I think it might actually be the right thing to do.
#418 caused #440 by reading preference state (thus declaring a dependency on that state) while constructing a nav entry, triggering a recomposition infinite loop. I've only come up with one good way to accomplish what #418 accomplishes without reading the state… and that's to clean up the state in
Main.kt.Remember, the reason we couldn't reproduce #300 or #417 is that Showcase's
Main.kthadSideEffect { saveableStateHolder.removeState(true) }in it. In #418, @marcprux commented:We already do add that to every project's
Main.kt. Abe updated the project template to add that line in October 2024. skiptools/skipstone@f53a2ce It shipped as part of skip 1.7.0.PR #418 is a heroic attempt to fix #300 / #417 without requiring users to update their
Main.ktfile. But, since that causes #440, I think users should just update theirMain.ktfiles, instead. (Maybe we could/should add askip verifystep to check for this or something.)To be clear, here are my test results:
skip-uiand run my 300 repro test on main Showcase, it still passes, because ofSideEffect { saveableStateHolder.removeState(true) }inMain.kt.SideEffect { saveableStateHolder.removeState(true) }from Showcase'sMain.kt, it still passes, because 418 is doing its job.SideEffect { saveableStateHolder.removeState(true) }, then my 300 repro test fails.Skip Pull Request Checklist:
swift testNot required