Skip to content

Commit 6db1da3

Browse files
authored
fix(display): skip relaunch wave on resolution-only screen-parameters changes (#569)
PR #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 #551 Signed-off-by: Amir Zarrinkafsh <3339418+nightah@users.noreply.github.com>
1 parent 81fa976 commit 6db1da3

2 files changed

Lines changed: 112 additions & 0 deletions

File tree

Thaw/Settings/Models/DisplaySettingsManager.swift

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,12 @@ final class DisplaySettingsManager: ObservableObject {
4040
/// active-display configuration changes. Held weakly to avoid retain cycles.
4141
private weak var appState: AppState?
4242

43+
/// UUID of the active menu bar display the last time spacing was applied.
44+
/// Used to skip didChangeScreenParametersNotification fires that only
45+
/// reflect a resolution or other-parameter change on the same display.
46+
/// Internal access so unit tests in ThawTests can seed and assert it.
47+
var lastAppliedActiveDisplayUUID: String?
48+
4349
/// Performs the initial setup of the manager.
4450
func performSetup(with appState: AppState) {
4551
self.appState = appState
@@ -162,6 +168,14 @@ final class DisplaySettingsManager: ObservableObject {
162168
guard let self else { return }
163169
diagLog.info("Screen parameters changed — \(NSScreen.screens.count) screen(s) connected")
164170
captureCurrentlyConnectedDisplays()
171+
let currentUUID = Bridging.getActiveMenuBarDisplayUUID()
172+
if Self.shouldSkipSpacingApply(
173+
currentActiveDisplayUUID: currentUUID,
174+
lastAppliedActiveDisplayUUID: lastAppliedActiveDisplayUUID
175+
) {
176+
diagLog.info("Active menu bar display unchanged (\(currentUUID ?? "nil")); skipping spacing apply")
177+
return
178+
}
165179
applyActiveDisplaySpacing(reason: "screenParametersChanged")
166180
}
167181
.store(in: &c)
@@ -191,6 +205,22 @@ final class DisplaySettingsManager: ObservableObject {
191205
cancellables = c
192206
}
193207

208+
/// Returns true when a didChangeScreenParametersNotification fire should
209+
/// be ignored because the active menu bar display has not changed
210+
/// identity since the last spacing apply. A resolution change, lid
211+
/// open/close, GPU/sleep transition, or other display-parameter event
212+
/// that leaves the active display UUID the same is not a reason to
213+
/// re-apply spacing (and risk a relaunch wave when on-disk values drift).
214+
///
215+
/// Pure on its inputs, separated from the sink so it can be unit tested
216+
/// without spinning up AppState or driving real screen events.
217+
static func shouldSkipSpacingApply(
218+
currentActiveDisplayUUID currentUUID: String?,
219+
lastAppliedActiveDisplayUUID lastUUID: String?
220+
) -> Bool {
221+
currentUUID == lastUUID
222+
}
223+
194224
/// Reads the active display's spacing offset, syncs it into
195225
/// spacingManager.offset, and triggers applyOffset. The no-op guard
196226
/// inside applyOffset skips when on-disk values already match, so this
@@ -200,6 +230,7 @@ final class DisplaySettingsManager: ObservableObject {
200230
/// moving them.
201231
private func applyActiveDisplaySpacing(reason: String) {
202232
guard let appState else { return }
233+
lastAppliedActiveDisplayUUID = Bridging.getActiveMenuBarDisplayUUID()
203234
let desired = Int(configurationForActiveDisplay().itemSpacingOffset.rounded())
204235
appState.spacingManager.offset = desired
205236
Task { [weak self] in
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
//
2+
// DisplaySettingsManagerSpacingGateTests.swift
3+
// Project: Thaw
4+
//
5+
// Copyright (Thaw) © 2026 Toni Förster
6+
// Licensed under the GNU GPLv3
7+
//
8+
9+
@testable import Thaw
10+
import XCTest
11+
12+
@MainActor
13+
final class DisplaySettingsManagerSpacingGateTests: XCTestCase {
14+
15+
// MARK: - Predicate
16+
17+
func testPredicateSkipsWhenUUIDsMatch() {
18+
XCTAssertTrue(DisplaySettingsManager.shouldSkipSpacingApply(
19+
currentActiveDisplayUUID: "UUID-A",
20+
lastAppliedActiveDisplayUUID: "UUID-A"
21+
))
22+
}
23+
24+
func testPredicateDoesNotSkipWhenUUIDsDiffer() {
25+
XCTAssertFalse(DisplaySettingsManager.shouldSkipSpacingApply(
26+
currentActiveDisplayUUID: "UUID-B",
27+
lastAppliedActiveDisplayUUID: "UUID-A"
28+
))
29+
}
30+
31+
func testPredicateDoesNotSkipOnFirstApply() {
32+
XCTAssertFalse(DisplaySettingsManager.shouldSkipSpacingApply(
33+
currentActiveDisplayUUID: "UUID-A",
34+
lastAppliedActiveDisplayUUID: nil
35+
))
36+
}
37+
38+
func testPredicateDoesNotSkipWhenCurrentBecomesNil() {
39+
XCTAssertFalse(DisplaySettingsManager.shouldSkipSpacingApply(
40+
currentActiveDisplayUUID: nil,
41+
lastAppliedActiveDisplayUUID: "UUID-A"
42+
))
43+
}
44+
45+
func testPredicateSkipsWhenBothNil() {
46+
XCTAssertTrue(DisplaySettingsManager.shouldSkipSpacingApply(
47+
currentActiveDisplayUUID: nil,
48+
lastAppliedActiveDisplayUUID: nil
49+
))
50+
}
51+
52+
func testPredicateIsStableAcrossRepeatedCalls() {
53+
for _ in 0..<10 {
54+
XCTAssertTrue(DisplaySettingsManager.shouldSkipSpacingApply(
55+
currentActiveDisplayUUID: "UUID-A",
56+
lastAppliedActiveDisplayUUID: "UUID-A"
57+
))
58+
}
59+
}
60+
61+
// MARK: - Field semantics
62+
63+
func testFreshManagerHasNilLastAppliedUUID() {
64+
let manager = DisplaySettingsManager()
65+
XCTAssertNil(manager.lastAppliedActiveDisplayUUID)
66+
}
67+
68+
func testSeededFieldDrivesPredicate() {
69+
let manager = DisplaySettingsManager()
70+
manager.lastAppliedActiveDisplayUUID = "UUID-A"
71+
72+
XCTAssertTrue(DisplaySettingsManager.shouldSkipSpacingApply(
73+
currentActiveDisplayUUID: "UUID-A",
74+
lastAppliedActiveDisplayUUID: manager.lastAppliedActiveDisplayUUID
75+
))
76+
XCTAssertFalse(DisplaySettingsManager.shouldSkipSpacingApply(
77+
currentActiveDisplayUUID: "UUID-B",
78+
lastAppliedActiveDisplayUUID: manager.lastAppliedActiveDisplayUUID
79+
))
80+
}
81+
}

0 commit comments

Comments
 (0)