Keep the configure sheet alive between reads - #5
Merged
Merged
Conversation
Pressing Options sometimes did nothing. `configureSheet` built a fresh ConfigureSheetController on every read and stored it in the view's only slot, so the second read dropped the last owner of the window handed out by the first. A host that reads the property twice — or that replaces the ScreenSaverView between reading it and presenting it, which happens on display reconfiguration — was left presenting a deallocated window, and nothing appeared. Exercising the installed bundle the way a host does confirms it: reading the property twice reported the first window DEALLOCATED, and now reports the same pointer, alive. The controller is now built once and kept, and reload() re-reads the stored settings each time the sheet is handed over, so reopening shows what is stored rather than where the last visit was left. Verified by flipping a checkbox, cancelling, and reopening: the control comes back at its stored value. The window also had isReleasedWhenClosed left at its default of true while ARC owned it through the controller, which is an over-release waiting for a host that closes the sheet rather than ordering it out. Turned off.
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.
Pressing Options sometimes did nothing.
Cause
configureSheetbuilt a freshConfigureSheetControlleron every read and storedit in the view's only slot:
The window's only owner was that slot, so the second read deallocated the window
handed out by the first. Two ways that bites:
presenting a deallocated window
ScreenSaverViewbetween reading the property andpresenting it loses the window with the old view. The log shows exactly that
happening around display reconfiguration:
Either way the sheet never appears, and no crash report is filed — the window is
simply gone by the time it is wanted. That matches the "sometimes" in the report.
Separately, the window was left with
isReleasedWhenClosedat its default oftruewhile ARC owned it through the controller: an over-release waiting for ahost that closes the sheet rather than ordering it out.
Fix
One controller, built on first use and kept.
reload()re-reads the storedsettings each time the sheet is handed over, so reopening shows what is stored
rather than where the last visit was left.
isReleasedWhenClosedturned off.Verified
Driving the installed bundle the way a host does, before and after:
Reuse does not leak edits — flip a checkbox, cancel, reopen:
Three present/dismiss rounds stay clean (
attachedSheetback to nil each time).Both schemes build and analyse;
swiftlint --strict,swiftlint analyze --strictand
swiftformat --lintare clean.What this does not prove is that the original symptom is gone — it was
intermittent, and the harness can only show that the mechanism behind it is. That
needs real use in System Settings.
Elsewhere
winebarrel/Macstify has the same
configureSheetshape and the same latent bug.