Skip to content

Commit 3182f42

Browse files
committed
fix(profiles): prompt before applying display spacing
Clicking Apply on Displays > "Menu Bar Item Spacing" while a profile is active triggered a relaunch wave for menu bar apps, after which `DisplaySettingsManager.applyActiveDisplaySpacing` called `ProfileManager.reapplyActiveProfile()`. The reapplied profile's stored `displayConfigurations` still held the previous offset, so the just-applied spacing was silently reverted, then re-applied by a second relaunch wave once the value flipped back. Reported in #619. The Apply and inline ↺ reset buttons now route through a single confirmation gate in `DisplaySettingsPane`. When the change targets the active menu bar display, the alert names the relaunch wave; when a profile is active, the alert lets the user pick `Update Active Profile` (saves into the active profile via `updateProfile(scope:.configurationOnly)`) or `Update All Profiles` (writes the spacing into every profile via the new `ProfileManager.updateAllProfilesItemSpacingOffset(displayUUID:offset:)` helper). The profile file(s) are written synchronously before the Combine sink dispatches `applyActiveDisplaySpacing`, so the subsequent `reapplyActiveProfile` loads the new spacing instead of the old one and the value sticks. Cases without an active profile and a non-active display still apply immediately, preserving the prior fast path. Cancelling the alert snaps the slider back to the saved value. New strings are added to `Localizable.xcstrings` for Crowdin pickup. `DisplaySettingsManager.loadInitialState` also now adopts externally configured system spacing on first launch. When no per-display configurations exist yet and the user has set `NSStatusItemSpacing` outside Thaw (e.g. via a `defaults write` in Terminal), the manager seeds each connected display's `itemSpacingOffset` from that on-disk value and persists the seed inline. Previously Thaw read its own default offset of 0, computed target = 16, and fired a relaunch wave on every startup that overwrote the user's manual setting. Addresses the recurring restart loops in #602 and #598. Fixes #598, #602, #619 Signed-off-by: Amir Zarrinkafsh <3339418+nightah@users.noreply.github.com>
1 parent a47970c commit 3182f42

4 files changed

Lines changed: 275 additions & 9 deletions

File tree

Thaw/Resources/Localizable.xcstrings

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43989,6 +43989,30 @@
4398943989
}
4399043990
}
4399143991
}
43992+
},
43993+
"Apply spacing change?" : {
43994+
"comment" : "Title of the alert shown before applying a menu bar item spacing change.",
43995+
"localizations" : {}
43996+
},
43997+
"Applying this spacing change will briefly relaunch all apps with menu bar items." : {
43998+
"comment" : "Alert message variant shown when applying a spacing change for the active menu bar display and no profile is active.",
43999+
"localizations" : {}
44000+
},
44001+
"Applying this spacing change will briefly relaunch all apps with menu bar items. Save the new spacing to the active profile \"%@\", or save it to every profile." : {
44002+
"comment" : "Alert message variant shown when applying a spacing change for the active menu bar display while a profile is active. %@ is the active profile name.",
44003+
"localizations" : {}
44004+
},
44005+
"Save the new spacing to the active profile \"%@\", or save it to every profile." : {
44006+
"comment" : "Alert message variant shown when applying a spacing change for a non-active display while a profile is active. %@ is the active profile name.",
44007+
"localizations" : {}
44008+
},
44009+
"Update Active Profile" : {
44010+
"comment" : "Button in the spacing-change confirmation alert that writes the new spacing into the active profile.",
44011+
"localizations" : {}
44012+
},
44013+
"Update All Profiles" : {
44014+
"comment" : "Button in the spacing-change confirmation alert that writes the new spacing into every profile.",
44015+
"localizations" : {}
4399244016
}
4399344017
},
4399444018
"version" : "1.1"

Thaw/Settings/Models/DisplaySettingsManager.swift

Lines changed: 79 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,15 @@ final class DisplaySettingsManager: ObservableObject {
4646
/// Internal access so unit tests in ThawTests can seed and assert it.
4747
var lastAppliedActiveDisplayUUID: String?
4848

49+
/// UUID of the display that currently owns the menu bar, or nil if it
50+
/// cannot be determined. Exposed for views that need to decide whether
51+
/// a spacing change will trigger the relaunch wave (only writes against
52+
/// the active display do, because applyActiveDisplaySpacing only reads
53+
/// configurationForActiveDisplay()).
54+
var activeMenuBarDisplayUUID: String? {
55+
Bridging.getActiveMenuBarDisplayUUID()
56+
}
57+
4958
/// Performs the initial setup of the manager.
5059
func performSetup(with appState: AppState) {
5160
self.appState = appState
@@ -84,7 +93,17 @@ final class DisplaySettingsManager: ObservableObject {
8493

8594
// MARK: - Loading
8695

87-
/// Loads saved configurations from Defaults.
96+
/// Default baseline for NSStatusItemSpacing and NSStatusItemSelectionPadding,
97+
/// kept in sync with MenuBarItemSpacingManager.Key.defaultValue. Used to
98+
/// translate on-disk system spacing into Thaw's relative offset model.
99+
private static let systemSpacingDefault = 16
100+
101+
/// Loads saved configurations from Defaults. On a truly first launch
102+
/// (no persisted per-display configurations) with externally configured
103+
/// system spacing, adopts the on-disk value as the seed offset for each
104+
/// connected display so Thaw does not overwrite a user's manual
105+
/// defaults write NSStatusItemSpacing and trigger a startup relaunch
106+
/// wave. See issue #602.
88107
private func loadInitialState() {
89108
if let data = Defaults.data(forKey: .displayIceBarConfigurations) {
90109
do {
@@ -94,6 +113,9 @@ final class DisplaySettingsManager: ObservableObject {
94113
diagLog.error("Failed to decode per-display configurations: \(error)")
95114
}
96115
}
116+
if configurations.isEmpty {
117+
seedConfigurationsFromSystemSpacing()
118+
}
97119
if let data = Defaults.data(forKey: .knownDisplays) {
98120
do {
99121
let decoded = try decoder.decode([String: KnownDisplay].self, from: data)
@@ -115,6 +137,62 @@ final class DisplaySettingsManager: ObservableObject {
115137
}
116138
}
117139

140+
/// Reads the current system value for NSStatusItemSpacing from the byHost
141+
/// global domain. Returns nil when the key is unset, letting callers
142+
/// distinguish "user has explicitly configured spacing" from "macOS
143+
/// default applies".
144+
private static func currentSystemSpacing() -> Int? {
145+
CFPreferencesCopyValue(
146+
"NSStatusItemSpacing" as CFString,
147+
kCFPreferencesAnyApplication,
148+
kCFPreferencesCurrentUser,
149+
kCFPreferencesCurrentHost
150+
) as? Int
151+
}
152+
153+
/// When the user has manually set NSStatusItemSpacing outside of Thaw
154+
/// (e.g. via a defaults write in Terminal), seed an entry for each
155+
/// connected display whose itemSpacingOffset corresponds to that on-disk
156+
/// value. Without this, applyActiveDisplaySpacing on first launch reads
157+
/// the default offset of 0, computes target = 16, sees on-disk = N, and
158+
/// fires a relaunch wave that rewrites the user's manual setting back to
159+
/// 16. The seeded entries are written to Defaults inline because the
160+
/// persistence sink is not yet wired at loadInitialState time; without
161+
/// the explicit save, subsequent launches would re-seed on every start
162+
/// instead of remembering the adopted value. The padding key is not
163+
/// consulted because Thaw drives both keys from a single offset; users
164+
/// whose padding diverges from spacing will see one normalising relaunch
165+
/// on first launch but no recurring waves thereafter.
166+
private func seedConfigurationsFromSystemSpacing() {
167+
guard let onDisk = Self.currentSystemSpacing(),
168+
onDisk != Self.systemSpacingDefault
169+
else {
170+
return
171+
}
172+
let offset = Double(onDisk - Self.systemSpacingDefault)
173+
var seeded = configurations
174+
for screen in NSScreen.screens {
175+
guard let uuid = Bridging.getDisplayUUIDString(for: screen.displayID) else {
176+
continue
177+
}
178+
if seeded[uuid] != nil { continue }
179+
seeded[uuid] = DisplayIceBarConfiguration
180+
.defaultConfiguration
181+
.withItemSpacingOffset(offset)
182+
}
183+
guard seeded != configurations else { return }
184+
configurations = seeded
185+
do {
186+
let data = try encoder.encode(seeded)
187+
Defaults.set(data, forKey: .displayIceBarConfigurations)
188+
diagLog.info(
189+
"Seeded itemSpacingOffset=\(offset) from external NSStatusItemSpacing=\(onDisk) for \(seeded.count) display(s)"
190+
)
191+
} catch {
192+
diagLog.error("Failed to persist seeded per-display configurations: \(error)")
193+
}
194+
}
195+
118196
// MARK: - Persistence
119197

120198
/// Configures Combine sinks to persist configurations on change.

Thaw/Settings/Models/ProfileManager.swift

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -667,6 +667,23 @@ final class ProfileManager: ObservableObject {
667667
try saveProfileAndUpdateManifest(profile)
668668
}
669669

670+
/// Writes the given itemSpacingOffset into every profile's stored
671+
/// displayConfigurations entry for the specified display UUID. Creates
672+
/// a default entry when a profile has no prior configuration for that
673+
/// display. Used by the spacing-apply confirmation in DisplaySettingsPane
674+
/// so a freshly applied spacing change is not reverted by the next
675+
/// profile reapply.
676+
func updateAllProfilesItemSpacingOffset(displayUUID: String, offset: Double) throws {
677+
let now = Date()
678+
for meta in profiles {
679+
var profile = try loadProfile(id: meta.id)
680+
let base = profile.displayConfigurations[displayUUID] ?? .defaultConfiguration
681+
profile.displayConfigurations[displayUUID] = base.withItemSpacingOffset(offset)
682+
profile.modifiedAt = now
683+
try saveProfileAndUpdateManifest(profile)
684+
}
685+
}
686+
670687
// MARK: - Profile Hooks
671688

672689
/// Returns the automation config (pre/post hooks) attached to the

Thaw/Settings/SettingsPanes/DisplaySettingsPane.swift

Lines changed: 155 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,31 @@
99
import SwiftUI
1010

1111
struct DisplaySettingsPane: View {
12+
@EnvironmentObject var appState: AppState
1213
@ObservedObject var displaySettings: DisplaySettingsManager
1314

1415
@State private var maxSliderLabelWidth: CGFloat = 0
1516
/// Per-display draft of the spacing slider, keyed by display UUID.
1617
/// Until the user clicks Apply, dragging the slider only updates this
17-
/// dictionary it does not touch the saved configuration or trigger
18+
/// dictionary, it does not touch the saved configuration or trigger
1819
/// any relaunches.
1920
@State private var draftSpacing: [String: CGFloat] = [:]
21+
/// Pending spacing apply held while the confirmation alert is shown.
22+
/// Set by requestSpacingApply when a prompt is required; the alert binds
23+
/// to its non-nil state. Nil when no alert is showing.
24+
@State private var pendingSpacingApply: PendingSpacingApply?
25+
@State private var errorMessage: String?
26+
@State private var showingError = false
27+
28+
/// A spacing apply request awaiting user confirmation.
29+
private struct PendingSpacingApply: Equatable {
30+
let displayID: String
31+
let displayName: String
32+
let offset: Double
33+
let isActiveDisplay: Bool
34+
let activeProfileID: UUID?
35+
let activeProfileName: String?
36+
}
2037

2138
var body: some View {
2239
IceForm {
@@ -26,6 +43,21 @@ struct DisplaySettingsPane: View {
2643
}
2744
}
2845
}
46+
.alert(
47+
String(localized: "Apply spacing change?"),
48+
isPresented: Binding(
49+
get: { pendingSpacingApply != nil },
50+
set: { if !$0 { pendingSpacingApply = nil } }
51+
),
52+
presenting: pendingSpacingApply,
53+
actions: { pending in spacingConfirmationButtons(for: pending) },
54+
message: { pending in Text(spacingConfirmationMessage(for: pending)) }
55+
)
56+
.alert("Error", isPresented: $showingError) {
57+
Button("OK") { errorMessage = nil }
58+
} message: {
59+
if let errorMessage { Text(errorMessage) }
60+
}
2961
}
3062

3163
@ViewBuilder
@@ -204,18 +236,13 @@ struct DisplaySettingsPane: View {
204236
} label: {
205237
LabeledContent {
206238
Button("Apply") {
207-
displaySettings.updateConfiguration(forDisplayUUID: display.id) { config in
208-
config.withItemSpacingOffset(Double(draft))
209-
}
239+
requestSpacingApply(for: display, offset: Double(draft))
210240
}
211241
.help(Text("Apply the spacing for this display"))
212242
.disabled(!canApply)
213243

214244
Button {
215-
draftSpacing[display.id] = 0
216-
displaySettings.updateConfiguration(forDisplayUUID: display.id) { config in
217-
config.withItemSpacingOffset(0)
218-
}
245+
requestSpacingApply(for: display, offset: 0)
219246
} label: {
220247
Image(systemName: "arrow.counterclockwise.circle.fill")
221248
}
@@ -235,4 +262,124 @@ struct DisplaySettingsPane: View {
235262
draftSpacing[display.id] = CGFloat(newValue)
236263
}
237264
}
265+
266+
// MARK: - Spacing Apply Confirmation
267+
268+
/// Routes both the Apply button and the inline reset button through a
269+
/// single decision point. When no profile is active and the change is
270+
/// for a non-active display, applies immediately (matches prior
271+
/// behaviour). Otherwise stages a PendingSpacingApply so the .alert
272+
/// can ask the user to choose between updating the active profile,
273+
/// updating every profile, or cancelling.
274+
private func requestSpacingApply(
275+
for display: DisplaySettingsManager.DisplayInfo,
276+
offset: Double
277+
) {
278+
let activeID = appState.profileManager.activeProfileID
279+
let isActiveDisplay = displaySettings.activeMenuBarDisplayUUID == display.id
280+
281+
if activeID == nil, !isActiveDisplay {
282+
commitSpacing(displayID: display.id, offset: offset)
283+
return
284+
}
285+
286+
let activeName = activeID.flatMap { id in
287+
appState.profileManager.profiles.first(where: { $0.id == id })?.name
288+
}
289+
pendingSpacingApply = PendingSpacingApply(
290+
displayID: display.id,
291+
displayName: display.name,
292+
offset: offset,
293+
isActiveDisplay: isActiveDisplay,
294+
activeProfileID: activeID,
295+
activeProfileName: activeName
296+
)
297+
}
298+
299+
/// Writes the new spacing to displaySettings.configurations. The
300+
/// Combine sink in DisplaySettingsManager picks this up and drives the
301+
/// relaunch wave on the next main-queue dispatch, so the caller is
302+
/// expected to have already written the profile file when persisting
303+
/// to a profile is desired.
304+
private func commitSpacing(displayID: String, offset: Double) {
305+
draftSpacing[displayID] = CGFloat(offset)
306+
displaySettings.updateConfiguration(forDisplayUUID: displayID) { config in
307+
config.withItemSpacingOffset(offset)
308+
}
309+
}
310+
311+
@ViewBuilder
312+
private func spacingConfirmationButtons(for pending: PendingSpacingApply) -> some View {
313+
if pending.activeProfileID != nil {
314+
Button(String(localized: "Update Active Profile"), role: .destructive) {
315+
if let id = pending.activeProfileID {
316+
// Save to the profile FIRST so the spacing value is on disk
317+
// before the Combine sink dispatches applyActiveDisplaySpacing,
318+
// which would otherwise trigger reapplyActiveProfile and
319+
// overwrite the new spacing with the profile's old value.
320+
// updateProfile(scope:.configurationOnly) captures live state,
321+
// so we update the in-memory configuration first, then save.
322+
commitSpacing(displayID: pending.displayID, offset: pending.offset)
323+
do {
324+
try appState.profileManager.updateProfile(
325+
id: id,
326+
scope: .configurationOnly,
327+
appState: appState
328+
)
329+
} catch {
330+
errorMessage = error.localizedDescription
331+
showingError = true
332+
}
333+
} else {
334+
commitSpacing(displayID: pending.displayID, offset: pending.offset)
335+
}
336+
}
337+
Button(String(localized: "Update All Profiles"), role: .destructive) {
338+
commitSpacing(displayID: pending.displayID, offset: pending.offset)
339+
do {
340+
try appState.profileManager.updateAllProfilesItemSpacingOffset(
341+
displayUUID: pending.displayID,
342+
offset: pending.offset
343+
)
344+
} catch {
345+
errorMessage = error.localizedDescription
346+
showingError = true
347+
}
348+
}
349+
Button(String(localized: "Cancel"), role: .cancel) {
350+
draftSpacing[pending.displayID] = CGFloat(
351+
displaySettings.configuration(forUUID: pending.displayID).itemSpacingOffset
352+
)
353+
}
354+
} else {
355+
Button(String(localized: "Apply"), role: .destructive) {
356+
commitSpacing(displayID: pending.displayID, offset: pending.offset)
357+
}
358+
Button(String(localized: "Cancel"), role: .cancel) {
359+
draftSpacing[pending.displayID] = CGFloat(
360+
displaySettings.configuration(forUUID: pending.displayID).itemSpacingOffset
361+
)
362+
}
363+
}
364+
}
365+
366+
private func spacingConfirmationMessage(for pending: PendingSpacingApply) -> String {
367+
let profileName = pending.activeProfileName ?? ""
368+
switch (pending.isActiveDisplay, pending.activeProfileID != nil) {
369+
case (true, true):
370+
return String(
371+
format: String(localized: "Applying this spacing change will briefly relaunch all apps with menu bar items. Save the new spacing to the active profile \"%@\", or save it to every profile."),
372+
profileName
373+
)
374+
case (false, true):
375+
return String(
376+
format: String(localized: "Save the new spacing to the active profile \"%@\", or save it to every profile."),
377+
profileName
378+
)
379+
case (true, false):
380+
return String(localized: "Applying this spacing change will briefly relaunch all apps with menu bar items.")
381+
case (false, false):
382+
return ""
383+
}
384+
}
238385
}

0 commit comments

Comments
 (0)