fix(display): skip relaunch wave on resolution-only screen-parameters changes#569
Merged
Merged
Conversation
… changes PR stonerl#529 (commit `1ed2ce67`, "feat: per-display menu bar spacing with dynamic apply") wired `DisplaySettingsManager.applyActiveDisplaySpacing` to `NSApplication.didChangeScreenParametersNotification` so each display's saved spacing follows the active menu bar around. That notification fires for every screen-parameter event though, including resolution changes, lid open/close, and GPU/sleep transitions on a display that is already connected. Each fire armed a preflight settling window and ran `applyOffset`, and any time the on-disk byHost values disagreed with the target (Control Center restart, sleep/wake clobber, mid-flight oscillation across profiles) the call escalated to a full relaunch wave even though the user had only changed a setting of the display, not changed to a different display. `DisplaySettingsManager` now records the active menu bar display UUID inside `applyActiveDisplaySpacing` and the `didChangeScreenParametersNotification` sink compares the current UUID against that cache via a new `shouldSkipSpacingApply(currentActiveDisplayUUID:lastAppliedActiveDisplayUUID:)` static predicate. When the UUIDs match (the resolution/lid/GPU case) the sink logs "Active menu bar display unchanged" and returns; when they differ (laptop docked, active menu bar moved to another monitor, first event after launch) the existing apply path runs as before. The `$configurations` and external Settings-URI paths are untouched: those reflect deliberate user intent and continue to rely on the no-op guard in `MenuBarItemSpacingManager.applyOffset` to short-circuit when target equals on-disk. Adds `ThawTests/DisplaySettingsManagerSpacingGateTests` with eight cases pinning the predicate (matching, differing, either-nil, both-nil, repeated-stable) and the field semantics on a fresh `DisplaySettingsManager` instance. The new file is picked up automatically by the `ThawTests` synchronized-folder reference so no project edits are required. Fixes stonerl#551 Signed-off-by: Amir Zarrinkafsh <3339418+nightah@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughAdded a display UUID tracking mechanism to ChangesDisplay Spacing Re-apply Gate
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Thaw/Settings/Models/DisplaySettingsManager.swift (1)
233-266:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefer cache update until spacing apply succeeds.
Line 233 marks spacing as “applied” before
applyOffset()completes. IfapplyOffset()fails, later same-display notifications will be skipped and retry is suppressed.Suggested fix
private func applyActiveDisplaySpacing(reason: String) { guard let appState else { return } - lastAppliedActiveDisplayUUID = Bridging.getActiveMenuBarDisplayUUID() + let activeUUIDAtApplyStart = Bridging.getActiveMenuBarDisplayUUID() let desired = Int(configurationForActiveDisplay().itemSpacingOffset.rounded()) appState.spacingManager.offset = desired Task { [weak self] in guard let self else { return } appState.itemManager.startSettlingPeriod(reason: "spacingRelaunch:\(reason):preflight") do { let outcome = try await appState.spacingManager.applyOffset() + lastAppliedActiveDisplayUUID = activeUUIDAtApplyStart if outcome.didRelaunch { appState.itemManager.startSettlingPeriod( reason: "spacingRelaunch:\(reason)", expectedBundleIDs: outcome.recoveredBundleIDs ) appState.profileManager.reapplyActiveProfile() } else { appState.itemManager.cancelSettlingPeriod( reason: "spacingRelaunch:\(reason):noOp" ) } } catch { appState.itemManager.cancelSettlingPeriod( reason: "spacingRelaunch:\(reason):error" ) diagLog.error("applyActiveDisplaySpacing(\(reason)) failed: \(error)") } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Thaw/Settings/Models/DisplaySettingsManager.swift` around lines 233 - 266, The code marks spacing as applied by setting lastAppliedActiveDisplayUUID before awaiting appState.spacingManager.applyOffset(), which means if applyOffset() throws the cache is updated incorrectly; remove the early assignment to lastAppliedActiveDisplayUUID and instead set lastAppliedActiveDisplayUUID = Bridging.getActiveMenuBarDisplayUUID() only after applyOffset() succeeds (inside the do block, after handling outcome.didRelaunch/no-op but before leaving the Task), so both successful branches update the cache and the catch block does not; update references inside the Task that rely on lastAppliedActiveDisplayUUID accordingly and keep the existing weak self guard and error handling intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Thaw/Settings/Models/DisplaySettingsManager.swift`:
- Around line 233-266: The code marks spacing as applied by setting
lastAppliedActiveDisplayUUID before awaiting
appState.spacingManager.applyOffset(), which means if applyOffset() throws the
cache is updated incorrectly; remove the early assignment to
lastAppliedActiveDisplayUUID and instead set lastAppliedActiveDisplayUUID =
Bridging.getActiveMenuBarDisplayUUID() only after applyOffset() succeeds (inside
the do block, after handling outcome.didRelaunch/no-op but before leaving the
Task), so both successful branches update the cache and the catch block does
not; update references inside the Task that rely on lastAppliedActiveDisplayUUID
accordingly and keep the existing weak self guard and error handling intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42ad854e-0485-42e5-92c5-4dbc3c46c3df
📒 Files selected for processing (2)
Thaw/Settings/Models/DisplaySettingsManager.swiftThawTests/DisplaySettingsManagerSpacingGateTests.swift
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
2 participants
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.
What does this PR do?
Stops
DisplaySettingsManager.applyActiveDisplaySpacingfrom firing onNSApplication.didChangeScreenParametersNotificationevents that only reflect resolution or other-parameter changes on the same display, so a resolution flip no longer risks triggering a full menu-bar-app relaunch wave.PR Type
Does this PR introduce a breaking change?
If yes, please describe the impact and migration path:
N/A
What is the current behavior?
PR #529 (commit
1ed2ce67) wired the spacing apply todidChangeScreenParametersNotification. That notification fires on every screen-parameter event, including resolution changes, lid open/close, and GPU/sleep transitions on an already-connected display. Each fire armed a preflight settling window and ranapplyOffset. When the on-diskNSStatusItembyHost values disagreed with the target (Control Center restart, sleep/wake clobber, mid-flight oscillation across profiles) the call escalated to a full relaunch wave even though the user had only changed a setting of the display, not changed to a different one.Issue Number: #551
What is the new behavior?
DisplaySettingsManagernow caches the active menu bar display UUID at everyapplyActiveDisplaySpacingcall and thedidChangeScreenParametersNotificationsink consults a pure static predicateshouldSkipSpacingApply(currentActiveDisplayUUID:lastAppliedActiveDisplayUUID:)before falling through. When the UUIDs match (resolution change, lid event, or GPU transition on the same display) the sink logs "Active menu bar display unchanged" and returns. When they differ (laptop docked, active menu bar moved to another monitor, first event after launch) the existing apply path runs as before. The$configurationssink and external Settings-URI path are unchanged because those reflect deliberate user intent and already rely on the no-op guard insideMenuBarItemSpacingManager.applyOffsetto short-circuit when target equals on-disk.PR Checklist
Other information
Adds
ThawTests/DisplaySettingsManagerSpacingGateTestswith eight cases pinning the predicate (matching, differing, either-nil, both-nil, repeated-stable) and the field semantics on a freshDisplaySettingsManagerinstance. The file is picked up automatically by theThawTestssynchronized-folder reference, soThaw.xcodeproj/project.pbxprojrequires no edits.Notes for reviewers
The predicate is intentionally a static pure function so it can be unit-tested without
AppState,Bridging, or liveNSScreenstate. End-to-end driving of the notification sink would depend on the host machine's display topology and the 1 s.debounce, so the manual verification steps below cover that surface instead:Bridging.getActiveMenuBarDisplayUUID()returns the same UUID, so no redundant wave on reconnect.Fixes #551
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests