From bf0897567167a6d83c35b9bd324e4b63a38db9bd Mon Sep 17 00:00:00 2001 From: Daniel Chalmers Date: Tue, 22 Sep 2026 12:48:21 -0500 Subject: [PATCH 1/5] Save settings by swapping in a new file instead of overwriting it Saving truncated the settings file and then wrote it, so an app killed mid-save, such as by a Windows shutdown, could leave the file empty and every setting lost (#7). Killing the app around saves reproduced this in 2 of 150 runs. Now the contents go to a temporary file that is flushed to disk and then renamed over the old one in a single step, and the same test kept the file intact in 150 of 150 runs. File.Replace isn't used because it renames the old file away first, and a kill in between left no settings file at all in testing. A swapped-in file is reported to the watcher as a rename rather than a change, so the watcher now handles renames too. That keeps live reload working for other instances sharing the file, and for editors that save the same way (0 of 8 such edits reloaded before, 8 of 8 now). The file is often still locked for a moment when the event arrives, so reloading retries briefly instead of giving up on the first attempt. --- DesktopClock.Tests/SettingsTests.cs | 19 +++++++++ DesktopClock/Properties/Settings.cs | 60 +++++++++++++++++++++++++---- 2 files changed, 72 insertions(+), 7 deletions(-) diff --git a/DesktopClock.Tests/SettingsTests.cs b/DesktopClock.Tests/SettingsTests.cs index 73e62a2..427928e 100644 --- a/DesktopClock.Tests/SettingsTests.cs +++ b/DesktopClock.Tests/SettingsTests.cs @@ -70,6 +70,25 @@ public void Save_ThenPopulate_ShouldRoundTripWpfAndTimeTypes() Assert.Equal(DateTimeKind.Unspecified, loaded.CountdownTo.Kind); } + [Fact] + public void Save_OverExistingFile_ShouldReplaceItWithoutLeavingTempFile() + { + using var _ = new TempSettingsFileScope(); + + var settings = CreateSettingsInstance(); + settings.Format = "first"; + Assert.True(settings.Save()); + + settings.Format = "second"; + Assert.True(settings.Save()); + + var loaded = CreateSettingsInstance(); + PopulateFromFile(loaded); + + Assert.Equal("second", loaded.Format); + Assert.False(File.Exists(Settings.FilePath + ".tmp")); + } + private static Settings CreateSettingsInstance() => (Settings)Activator.CreateInstance(typeof(Settings), nonPublic: true)!; diff --git a/DesktopClock/Properties/Settings.cs b/DesktopClock/Properties/Settings.cs index 2616065..11fbbb1 100644 --- a/DesktopClock/Properties/Settings.cs +++ b/DesktopClock/Properties/Settings.cs @@ -1,6 +1,7 @@ -using System; +using System; using System.ComponentModel; using System.IO; +using System.Runtime.InteropServices; using System.Windows.Media; using DesktopClock.Utilities; using Newtonsoft.Json; @@ -40,6 +41,9 @@ private Settings() EnableRaisingEvents = true, }; _watcher.Changed += FileChanged; + + // Editors that save by swapping in a new file report it as a rename rather than a change. + _watcher.Renamed += FileChanged; } #pragma warning disable CS0067 // The event 'Settings.PropertyChanged' is never used. Handled by Fody. @@ -441,7 +445,7 @@ public bool Save() { try { - File.WriteAllText(FilePath, json); + WriteAllTextAtomically(FilePath, json); return true; } catch @@ -463,6 +467,34 @@ public bool Save() return false; } + /// + /// Writes to a temporary file and then swaps it in, so the file is never left empty or half-written if the app is killed mid-save, such as during a Windows shutdown (#7). + /// + private static void WriteAllTextAtomically(string path, string contents) + { + var tempPath = path + ".tmp"; + + using (var stream = new FileStream(tempPath, FileMode.Create, FileAccess.Write)) + using (var writer = new StreamWriter(stream)) + { + writer.Write(contents); + writer.Flush(); + + // Make sure the new contents are on disk before they replace the old ones. + stream.Flush(flushToDisk: true); + } + + // Swap it in with a single rename; File.Replace isn't safe here because it renames the old file away first, so being killed in between leaves no settings file at all. + if (!MoveFileEx(tempPath, path, MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH)) + throw new IOException($"Couldn't replace {path}.", Marshal.GetHRForLastWin32Error()); + } + + private const int MOVEFILE_REPLACE_EXISTING = 0x1; + private const int MOVEFILE_WRITE_THROUGH = 0x8; + + [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)] + private static extern bool MoveFileEx(string existingFileName, string newFileName, int flags); + /// /// Populates the given settings with values from the default path. /// @@ -514,12 +546,26 @@ private static Settings LoadAndAttemptSave() /// private void FileChanged(object sender, FileSystemEventArgs e) { - try - { - Populate(this); - } - catch + // A swap-in save also renames the old file away; only the rename that puts the new file in place matters. + if (!string.Equals(e.FullPath, FilePath, StringComparison.OrdinalIgnoreCase)) + return; + + // Right after an edit the file is often still locked, such as while antivirus scans it, so give it a few tries. + for (var i = 0; i < 4; i++) { + try + { + Populate(this); + return; + } + catch (IOException) + { + System.Threading.Thread.Sleep(100); + } + catch + { + return; + } } } From f574529a82fc2a46bb577f0f378e2478b369d60a Mon Sep 17 00:00:00 2001 From: Daniel Chalmers Date: Tue, 22 Sep 2026 12:50:45 -0500 Subject: [PATCH 2/5] Save settings shortly after they change instead of only on exit Settings were written only when the clock closed normally, so a crash, a forced close, or a shutdown that didn't let the app exit lost every change made that session. Now a change is saved a second after the last one, which also groups rapid changes like dragging a slider into one save. The clock's position is saved after dragging or nudging it too, since it used to be read only on exit. Values reloaded from the file don't trigger a save, so editing the file by hand is never rewritten by the app. Verified live: after changing a setting and nudging the clock, an unhandled exception two seconds later kept both on disk (both were lost before), and a hand edit was applied without the file being touched. --- DesktopClock.Tests/SettingsTests.cs | 37 +++++++++++++++++++++++++++++ DesktopClock/MainWindow.xaml.cs | 2 ++ DesktopClock/Properties/Settings.cs | 25 +++++++++++++++++++ 3 files changed, 64 insertions(+) diff --git a/DesktopClock.Tests/SettingsTests.cs b/DesktopClock.Tests/SettingsTests.cs index 427928e..9878931 100644 --- a/DesktopClock.Tests/SettingsTests.cs +++ b/DesktopClock.Tests/SettingsTests.cs @@ -2,6 +2,7 @@ using System.IO; using System.Reflection; using System.Windows.Media; +using System.Windows.Threading; using DesktopClock.Properties; namespace DesktopClock.Tests; @@ -89,6 +90,42 @@ public void Save_OverExistingFile_ShouldReplaceItWithoutLeavingTempFile() Assert.False(File.Exists(Settings.FilePath + ".tmp")); } + [Fact] + public void ChangingASetting_ShouldSaveItShortlyAfterwards() + { + using var _ = new TempSettingsFileScope(); + + var canBeSavedProperty = typeof(Settings).GetProperty(nameof(Settings.CanBeSaved), BindingFlags.Public | BindingFlags.Static)!; + var originalCanBeSaved = Settings.CanBeSaved; + canBeSavedProperty.GetSetMethod(nonPublic: true)!.Invoke(null, new object[] { true }); + + try + { + var settings = CreateSettingsInstance(); + settings.Format = "saved without exiting"; + + // The save runs on a short timer, so let the dispatcher run for a bit. + var frame = new DispatcherFrame(); + var stopTimer = new DispatcherTimer { Interval = TimeSpan.FromSeconds(2) }; + stopTimer.Tick += (_, _) => + { + stopTimer.Stop(); + frame.Continue = false; + }; + stopTimer.Start(); + Dispatcher.PushFrame(frame); + + var loaded = CreateSettingsInstance(); + PopulateFromFile(loaded); + + Assert.Equal("saved without exiting", loaded.Format); + } + finally + { + canBeSavedProperty.GetSetMethod(nonPublic: true)!.Invoke(null, new object[] { originalCanBeSaved }); + } + } + private static Settings CreateSettingsInstance() => (Settings)Activator.CreateInstance(typeof(Settings), nonPublic: true)!; diff --git a/DesktopClock/MainWindow.xaml.cs b/DesktopClock/MainWindow.xaml.cs index 0c54298..95386d1 100644 --- a/DesktopClock/MainWindow.xaml.cs +++ b/DesktopClock/MainWindow.xaml.cs @@ -311,6 +311,7 @@ private void Window_MouseDown(object sender, MouseButtonEventArgs e) DragMove(); PixelShifter?.UpdateBasePosition(this); + Settings.Default.Placement = this.GetPlacement(); UpdateTimeString(); _systemClockTimer.Start(); @@ -452,6 +453,7 @@ private void NudgeWindow(KeyEventArgs e) Top += nudge.Y; PixelShifter?.UpdateBasePosition(this); + Settings.Default.Placement = this.GetPlacement(); e.Handled = true; } diff --git a/DesktopClock/Properties/Settings.cs b/DesktopClock/Properties/Settings.cs index 11fbbb1..cc413c8 100644 --- a/DesktopClock/Properties/Settings.cs +++ b/DesktopClock/Properties/Settings.cs @@ -3,6 +3,7 @@ using System.IO; using System.Runtime.InteropServices; using System.Windows.Media; +using System.Windows.Threading; using DesktopClock.Utilities; using Newtonsoft.Json; using WpfWindowPlacement; @@ -12,6 +13,8 @@ namespace DesktopClock.Properties; public sealed class Settings : INotifyPropertyChanged, IDisposable { private readonly FileSystemWatcher _watcher; + private readonly DispatcherTimer _saveTimer; + private bool _populatingFromFile; private string _resolvedTimeZoneId; private TimeZoneInfo _resolvedTimeZone; private static readonly Lazy _default = new(LoadAndAttemptSave); @@ -44,6 +47,23 @@ private Settings() // Editors that save by swapping in a new file report it as a rename rather than a change. _watcher.Renamed += FileChanged; + + // Save shortly after a change instead of only on exit, so a crash, a forced close, or a shutdown that doesn't let the app exit normally only loses the last moment of changes. The short wait groups rapid changes, like dragging a slider, into one save. + _saveTimer = new DispatcherTimer { Interval = TimeSpan.FromSeconds(1) }; + _saveTimer.Tick += (_, _) => + { + _saveTimer.Stop(); + Save(); + }; + PropertyChanged += (_, _) => + { + // Values that were just read from the file are already saved. + if (!CanBeSaved || _populatingFromFile) + return; + + _saveTimer.Stop(); + _saveTimer.Start(); + }; } #pragma warning disable CS0067 // The event 'Settings.PropertyChanged' is never used. Handled by Fody. @@ -555,6 +575,7 @@ private void FileChanged(object sender, FileSystemEventArgs e) { try { + _populatingFromFile = true; Populate(this); return; } @@ -566,6 +587,10 @@ private void FileChanged(object sender, FileSystemEventArgs e) { return; } + finally + { + _populatingFromFile = false; + } } } From 4d63d86f539a5870c0be8caa1980e987c3e0d7e0 Mon Sep 17 00:00:00 2001 From: Daniel Chalmers Date: Tue, 22 Sep 2026 12:53:32 -0500 Subject: [PATCH 3/5] Keep an unreadable settings file instead of saving defaults over it When the settings file couldn't be read at startup, the app loaded defaults and immediately saved them over it, so a single typo made while editing it by hand wiped every setting with nothing left to recover. Now startup follows one rule: - A missing or empty file starts fresh with colors from the system theme. Older versions could leave an empty file behind when closed mid-save (#7). - A file that can't be read is moved to DesktopClock.settings.bak and a notification says so, then the app starts fresh the same way. - A file that's only locked for a moment, such as while antivirus scans it, is retried briefly instead of being treated as unreadable. Before, that also reset every setting. Reloading after an edit uses the same retry. --- DesktopClock.Tests/SettingsTests.cs | 86 +++++++++++++++++++++++++ DesktopClock/MainWindow.xaml.cs | 5 ++ DesktopClock/Properties/Settings.cs | 99 +++++++++++++++++++++-------- 3 files changed, 163 insertions(+), 27 deletions(-) diff --git a/DesktopClock.Tests/SettingsTests.cs b/DesktopClock.Tests/SettingsTests.cs index 9878931..f96145b 100644 --- a/DesktopClock.Tests/SettingsTests.cs +++ b/DesktopClock.Tests/SettingsTests.cs @@ -126,6 +126,89 @@ public void ChangingASetting_ShouldSaveItShortlyAfterwards() } } + [Fact] + public void Load_WithUnreadableFile_ShouldMoveItToBackupAndUseDefaults() + { + using var _ = new TempSettingsFileScope(); + + // A missing comma, like a typo made while editing the file by hand. + const string brokenJson = "{ \"Format\": \"{HH:mm}\" \"FontFamily\": \"Georgia\" }"; + File.WriteAllText(Settings.FilePath, brokenJson); + + var loaded = LoadAndAttemptSave(out var movedToBackup); + + Assert.True(movedToBackup); + Assert.Equal(brokenJson, File.ReadAllText(Settings.BackupFilePath)); + Assert.Equal(CreateSettingsInstance().Format, loaded.Format); + Assert.True(File.Exists(Settings.FilePath)); + } + + [Fact] + public void Load_WithEmptyFile_ShouldUseDefaultsWithoutBackup() + { + using var _ = new TempSettingsFileScope(); + + File.WriteAllText(Settings.FilePath, ""); + + var loaded = LoadAndAttemptSave(out var movedToBackup); + + Assert.False(movedToBackup); + Assert.False(File.Exists(Settings.BackupFilePath)); + Assert.Equal(CreateSettingsInstance().Format, loaded.Format); + Assert.NotEqual(0, new FileInfo(Settings.FilePath).Length); + } + + [Fact] + public void Load_WithBrieflyLockedFile_ShouldWaitAndKeepSettings() + { + using var _ = new TempSettingsFileScope(); + + var original = CreateSettingsInstance(); + original.Format = "kept through a lock"; + Assert.True(original.Save()); + + // Hold the file like antivirus scanning it, then let go shortly after loading starts. + var lockStream = new FileStream(Settings.FilePath, FileMode.Open, FileAccess.Read, FileShare.None); + var releaser = new System.Threading.Thread(() => + { + System.Threading.Thread.Sleep(150); + lockStream.Dispose(); + }); + releaser.Start(); + + var loaded = LoadAndAttemptSave(out var movedToBackup); + releaser.Join(); + + Assert.False(movedToBackup); + Assert.Equal("kept through a lock", loaded.Format); + } + + /// + /// Runs the app's startup load, restoring the static state it sets so other tests aren't affected. + /// + private static Settings LoadAndAttemptSave(out bool movedToBackup) + { + var canBeSaved = typeof(Settings).GetProperty(nameof(Settings.CanBeSaved), BindingFlags.Public | BindingFlags.Static)!.GetSetMethod(nonPublic: true)!; + var movedUnreadableFileToBackup = typeof(Settings).GetProperty(nameof(Settings.MovedUnreadableFileToBackup), BindingFlags.Public | BindingFlags.Static)!.GetSetMethod(nonPublic: true)!; + var originalCanBeSaved = Settings.CanBeSaved; + + try + { + canBeSaved.Invoke(null, new object[] { false }); + movedUnreadableFileToBackup.Invoke(null, new object[] { false }); + + var loadAndAttemptSave = typeof(Settings).GetMethod("LoadAndAttemptSave", BindingFlags.NonPublic | BindingFlags.Static)!; + var settings = (Settings)loadAndAttemptSave.Invoke(null, null)!; + movedToBackup = Settings.MovedUnreadableFileToBackup; + return settings; + } + finally + { + canBeSaved.Invoke(null, new object[] { originalCanBeSaved }); + movedUnreadableFileToBackup.Invoke(null, new object[] { false }); + } + } + private static Settings CreateSettingsInstance() => (Settings)Activator.CreateInstance(typeof(Settings), nonPublic: true)!; @@ -163,6 +246,9 @@ public void Dispose() { if (File.Exists(Settings.FilePath)) File.Delete(Settings.FilePath); + + if (File.Exists(Settings.BackupFilePath)) + File.Delete(Settings.BackupFilePath); } finally { diff --git a/DesktopClock/MainWindow.xaml.cs b/DesktopClock/MainWindow.xaml.cs index 95386d1..1b08b95 100644 --- a/DesktopClock/MainWindow.xaml.cs +++ b/DesktopClock/MainWindow.xaml.cs @@ -349,6 +349,11 @@ private void Window_SourceInitialized(object sender, EventArgs e) // Start listening for size changes to keep the window right-aligned. SizeChanged += Window_SizeChanged; + if (Settings.MovedUnreadableFileToBackup) + { + _trayIcon?.ShowNotification("Settings reset", $"The settings file couldn't be read, so it was moved to {Path.GetFileName(Settings.BackupFilePath)}."); + } + if (Settings.Default.StartHidden) { _trayIcon?.ShowNotification("Running in the background", "Double-click the tray icon to show the clock."); diff --git a/DesktopClock/Properties/Settings.cs b/DesktopClock/Properties/Settings.cs index cc413c8..471622d 100644 --- a/DesktopClock/Properties/Settings.cs +++ b/DesktopClock/Properties/Settings.cs @@ -88,6 +88,16 @@ private Settings() /// public static bool CanBeSaved { get; private set; } + /// + /// Where a settings file that couldn't be read at startup is moved so defaults can be saved in its place. + /// + public static string BackupFilePath => FilePath + ".bak"; + + /// + /// Indicates the settings file couldn't be read at startup, so it was moved to and defaults were loaded. + /// + public static bool MovedUnreadableFileToBackup { get; private set; } + /// /// Checks if the settings file exists on the disk. /// @@ -527,20 +537,43 @@ private static void Populate(Settings settings) JsonSerializer.Create(_jsonSerializerSettings).Populate(jsonReader, settings); } + /// + /// Populates the given settings from the default path, retrying briefly while the file is locked, which is common right after it's edited while antivirus scans it. + /// + private static void PopulateWithRetries(Settings settings) + { + for (var attempt = 1; ; attempt++) + { + try + { + Populate(settings); + return; + } + catch (IOException) when (attempt < 4) + { + System.Threading.Thread.Sleep(100); + } + } + } + /// /// Loads from the default path in JSON format. /// - private static Settings LoadFromFile() + /// false if the file couldn't be read, in which case holds the defaults. + private static bool TryLoadFromFile(out Settings settings) { + settings = new Settings(); + try { - var settings = new Settings(); - Populate(settings); - return settings; + PopulateWithRetries(settings); + return true; } catch { - return new(); + // Start over so values read before the error don't mix with the defaults. + settings = new(); + return false; } } @@ -549,10 +582,18 @@ private static Settings LoadFromFile() /// private static Settings LoadAndAttemptSave() { - var settings = LoadFromFile(); + Settings settings; - if (!File.Exists(FilePath)) + // An empty file has nothing to keep; older versions could leave one behind when closed mid-save (#7). + if (!Exists || new FileInfo(FilePath).Length == 0) + { + settings = new(); + settings.ApplySystemThemeDefaultsIfAvailable(); + } + else if (!TryLoadFromFile(out settings)) { + // Move a file that couldn't be read, such as after a typo while editing it by hand, out of the way instead of saving defaults over it. + MovedUnreadableFileToBackup = TryMoveToBackup(); settings.ApplySystemThemeDefaultsIfAvailable(); } @@ -561,6 +602,20 @@ private static Settings LoadAndAttemptSave() return settings; } + private static bool TryMoveToBackup() + { + try + { + File.Delete(BackupFilePath); + File.Move(FilePath, BackupFilePath); + return true; + } + catch + { + return false; + } + } + /// /// Occurs after the watcher detects a change in the settings file. /// @@ -570,27 +625,17 @@ private void FileChanged(object sender, FileSystemEventArgs e) if (!string.Equals(e.FullPath, FilePath, StringComparison.OrdinalIgnoreCase)) return; - // Right after an edit the file is often still locked, such as while antivirus scans it, so give it a few tries. - for (var i = 0; i < 4; i++) + try { - try - { - _populatingFromFile = true; - Populate(this); - return; - } - catch (IOException) - { - System.Threading.Thread.Sleep(100); - } - catch - { - return; - } - finally - { - _populatingFromFile = false; - } + _populatingFromFile = true; + PopulateWithRetries(this); + } + catch + { + } + finally + { + _populatingFromFile = false; } } From 90d16cb0341ff0843d26e97d8b9b1fd1fcdcfb2f Mon Sep 17 00:00:00 2001 From: Daniel Chalmers Date: Tue, 22 Sep 2026 13:47:15 -0500 Subject: [PATCH 4/5] Trim the less common settings recovery cases Keep this PR focused on settings surviving crashes and shutdowns. Removed: - Moving an unreadable file to a .bak with a notification. It only helped someone who breaks the file by hand and then restarts the app. - Reloading when the file is renamed into place, along with the retry after that event. That only mattered for editors that save that way, and Notepad edits in place. - Treating an empty file as a fresh start. Saves can't leave empty files anymore. Startup still retries a briefly locked file instead of saving defaults over it, now waiting about as long as saving does. --- DesktopClock.Tests/SettingsTests.cs | 47 ++-------------- DesktopClock/MainWindow.xaml.cs | 5 -- DesktopClock/Properties/Settings.cs | 83 ++++++----------------------- 3 files changed, 18 insertions(+), 117 deletions(-) diff --git a/DesktopClock.Tests/SettingsTests.cs b/DesktopClock.Tests/SettingsTests.cs index f96145b..6055257 100644 --- a/DesktopClock.Tests/SettingsTests.cs +++ b/DesktopClock.Tests/SettingsTests.cs @@ -126,38 +126,6 @@ public void ChangingASetting_ShouldSaveItShortlyAfterwards() } } - [Fact] - public void Load_WithUnreadableFile_ShouldMoveItToBackupAndUseDefaults() - { - using var _ = new TempSettingsFileScope(); - - // A missing comma, like a typo made while editing the file by hand. - const string brokenJson = "{ \"Format\": \"{HH:mm}\" \"FontFamily\": \"Georgia\" }"; - File.WriteAllText(Settings.FilePath, brokenJson); - - var loaded = LoadAndAttemptSave(out var movedToBackup); - - Assert.True(movedToBackup); - Assert.Equal(brokenJson, File.ReadAllText(Settings.BackupFilePath)); - Assert.Equal(CreateSettingsInstance().Format, loaded.Format); - Assert.True(File.Exists(Settings.FilePath)); - } - - [Fact] - public void Load_WithEmptyFile_ShouldUseDefaultsWithoutBackup() - { - using var _ = new TempSettingsFileScope(); - - File.WriteAllText(Settings.FilePath, ""); - - var loaded = LoadAndAttemptSave(out var movedToBackup); - - Assert.False(movedToBackup); - Assert.False(File.Exists(Settings.BackupFilePath)); - Assert.Equal(CreateSettingsInstance().Format, loaded.Format); - Assert.NotEqual(0, new FileInfo(Settings.FilePath).Length); - } - [Fact] public void Load_WithBrieflyLockedFile_ShouldWaitAndKeepSettings() { @@ -176,36 +144,30 @@ public void Load_WithBrieflyLockedFile_ShouldWaitAndKeepSettings() }); releaser.Start(); - var loaded = LoadAndAttemptSave(out var movedToBackup); + var loaded = LoadAndAttemptSave(); releaser.Join(); - Assert.False(movedToBackup); Assert.Equal("kept through a lock", loaded.Format); } /// /// Runs the app's startup load, restoring the static state it sets so other tests aren't affected. /// - private static Settings LoadAndAttemptSave(out bool movedToBackup) + private static Settings LoadAndAttemptSave() { var canBeSaved = typeof(Settings).GetProperty(nameof(Settings.CanBeSaved), BindingFlags.Public | BindingFlags.Static)!.GetSetMethod(nonPublic: true)!; - var movedUnreadableFileToBackup = typeof(Settings).GetProperty(nameof(Settings.MovedUnreadableFileToBackup), BindingFlags.Public | BindingFlags.Static)!.GetSetMethod(nonPublic: true)!; var originalCanBeSaved = Settings.CanBeSaved; try { canBeSaved.Invoke(null, new object[] { false }); - movedUnreadableFileToBackup.Invoke(null, new object[] { false }); var loadAndAttemptSave = typeof(Settings).GetMethod("LoadAndAttemptSave", BindingFlags.NonPublic | BindingFlags.Static)!; - var settings = (Settings)loadAndAttemptSave.Invoke(null, null)!; - movedToBackup = Settings.MovedUnreadableFileToBackup; - return settings; + return (Settings)loadAndAttemptSave.Invoke(null, null)!; } finally { canBeSaved.Invoke(null, new object[] { originalCanBeSaved }); - movedUnreadableFileToBackup.Invoke(null, new object[] { false }); } } @@ -246,9 +208,6 @@ public void Dispose() { if (File.Exists(Settings.FilePath)) File.Delete(Settings.FilePath); - - if (File.Exists(Settings.BackupFilePath)) - File.Delete(Settings.BackupFilePath); } finally { diff --git a/DesktopClock/MainWindow.xaml.cs b/DesktopClock/MainWindow.xaml.cs index 1b08b95..95386d1 100644 --- a/DesktopClock/MainWindow.xaml.cs +++ b/DesktopClock/MainWindow.xaml.cs @@ -349,11 +349,6 @@ private void Window_SourceInitialized(object sender, EventArgs e) // Start listening for size changes to keep the window right-aligned. SizeChanged += Window_SizeChanged; - if (Settings.MovedUnreadableFileToBackup) - { - _trayIcon?.ShowNotification("Settings reset", $"The settings file couldn't be read, so it was moved to {Path.GetFileName(Settings.BackupFilePath)}."); - } - if (Settings.Default.StartHidden) { _trayIcon?.ShowNotification("Running in the background", "Double-click the tray icon to show the clock."); diff --git a/DesktopClock/Properties/Settings.cs b/DesktopClock/Properties/Settings.cs index 471622d..208d389 100644 --- a/DesktopClock/Properties/Settings.cs +++ b/DesktopClock/Properties/Settings.cs @@ -45,9 +45,6 @@ private Settings() }; _watcher.Changed += FileChanged; - // Editors that save by swapping in a new file report it as a rename rather than a change. - _watcher.Renamed += FileChanged; - // Save shortly after a change instead of only on exit, so a crash, a forced close, or a shutdown that doesn't let the app exit normally only loses the last moment of changes. The short wait groups rapid changes, like dragging a slider, into one save. _saveTimer = new DispatcherTimer { Interval = TimeSpan.FromSeconds(1) }; _saveTimer.Tick += (_, _) => @@ -88,16 +85,6 @@ private Settings() /// public static bool CanBeSaved { get; private set; } - /// - /// Where a settings file that couldn't be read at startup is moved so defaults can be saved in its place. - /// - public static string BackupFilePath => FilePath + ".bak"; - - /// - /// Indicates the settings file couldn't be read at startup, so it was moved to and defaults were loaded. - /// - public static bool MovedUnreadableFileToBackup { get; private set; } - /// /// Checks if the settings file exists on the disk. /// @@ -538,42 +525,28 @@ private static void Populate(Settings settings) } /// - /// Populates the given settings from the default path, retrying briefly while the file is locked, which is common right after it's edited while antivirus scans it. + /// Loads from the default path in JSON format. /// - private static void PopulateWithRetries(Settings settings) + private static Settings LoadFromFile() { + var settings = new Settings(); + + // The file can be locked for a moment, such as while antivirus scans it at sign-in, and falling back to defaults here would save them over every setting. Give it about as long as saving does before giving up. for (var attempt = 1; ; attempt++) { try { Populate(settings); - return; + return settings; } - catch (IOException) when (attempt < 4) + catch (IOException) when (attempt < 4 && Exists) { - System.Threading.Thread.Sleep(100); + System.Threading.Thread.Sleep(250); + } + catch + { + return new(); } - } - } - - /// - /// Loads from the default path in JSON format. - /// - /// false if the file couldn't be read, in which case holds the defaults. - private static bool TryLoadFromFile(out Settings settings) - { - settings = new Settings(); - - try - { - PopulateWithRetries(settings); - return true; - } - catch - { - // Start over so values read before the error don't mix with the defaults. - settings = new(); - return false; } } @@ -582,18 +555,10 @@ private static bool TryLoadFromFile(out Settings settings) /// private static Settings LoadAndAttemptSave() { - Settings settings; + var settings = LoadFromFile(); - // An empty file has nothing to keep; older versions could leave one behind when closed mid-save (#7). - if (!Exists || new FileInfo(FilePath).Length == 0) + if (!Exists) { - settings = new(); - settings.ApplySystemThemeDefaultsIfAvailable(); - } - else if (!TryLoadFromFile(out settings)) - { - // Move a file that couldn't be read, such as after a typo while editing it by hand, out of the way instead of saving defaults over it. - MovedUnreadableFileToBackup = TryMoveToBackup(); settings.ApplySystemThemeDefaultsIfAvailable(); } @@ -602,33 +567,15 @@ private static Settings LoadAndAttemptSave() return settings; } - private static bool TryMoveToBackup() - { - try - { - File.Delete(BackupFilePath); - File.Move(FilePath, BackupFilePath); - return true; - } - catch - { - return false; - } - } - /// /// Occurs after the watcher detects a change in the settings file. /// private void FileChanged(object sender, FileSystemEventArgs e) { - // A swap-in save also renames the old file away; only the rename that puts the new file in place matters. - if (!string.Equals(e.FullPath, FilePath, StringComparison.OrdinalIgnoreCase)) - return; - try { _populatingFromFile = true; - PopulateWithRetries(this); + Populate(this); } catch { From bb012ee7b73458a29cd7575c984f7028af8845e3 Mon Sep 17 00:00:00 2001 From: Daniel Chalmers Date: Tue, 22 Sep 2026 13:50:34 -0500 Subject: [PATCH 5/5] Cancel a waiting save when the settings file is edited A change made in the app starts a one-second save timer on the UI thread, but reloading a hand edit ran on the file watcher's thread. If the file was edited in that second, the waiting save could write a half-reloaded file or put the app's values back over the edit. Now the reload runs on the UI thread and stops the waiting save first. The timer ticks at a lower priority than the reload, so a queued reload always runs before a queued save. Added a test that fails without this: a pending save rewrote a hand-edited file with the full settings. --- DesktopClock.Tests/SettingsTests.cs | 76 +++++++++++++++++++---------- DesktopClock/Properties/Settings.cs | 28 ++++++----- 2 files changed, 68 insertions(+), 36 deletions(-) diff --git a/DesktopClock.Tests/SettingsTests.cs b/DesktopClock.Tests/SettingsTests.cs index 6055257..6167119 100644 --- a/DesktopClock.Tests/SettingsTests.cs +++ b/DesktopClock.Tests/SettingsTests.cs @@ -95,35 +95,35 @@ public void ChangingASetting_ShouldSaveItShortlyAfterwards() { using var _ = new TempSettingsFileScope(); - var canBeSavedProperty = typeof(Settings).GetProperty(nameof(Settings.CanBeSaved), BindingFlags.Public | BindingFlags.Static)!; - var originalCanBeSaved = Settings.CanBeSaved; - canBeSavedProperty.GetSetMethod(nonPublic: true)!.Invoke(null, new object[] { true }); + using var __ = new CanBeSavedScope(); - try - { - var settings = CreateSettingsInstance(); - settings.Format = "saved without exiting"; + var settings = CreateSettingsInstance(); + settings.Format = "saved without exiting"; - // The save runs on a short timer, so let the dispatcher run for a bit. - var frame = new DispatcherFrame(); - var stopTimer = new DispatcherTimer { Interval = TimeSpan.FromSeconds(2) }; - stopTimer.Tick += (_, _) => - { - stopTimer.Stop(); - frame.Continue = false; - }; - stopTimer.Start(); - Dispatcher.PushFrame(frame); + // The save runs on a short timer, so let the dispatcher run for a bit. + PumpDispatcher(TimeSpan.FromSeconds(2)); - var loaded = CreateSettingsInstance(); - PopulateFromFile(loaded); + // Read the file directly; populating another instance here would queue its own save and leak into other tests. + Assert.Contains("saved without exiting", File.ReadAllText(Settings.FilePath)); + } - Assert.Equal("saved without exiting", loaded.Format); - } - finally - { - canBeSavedProperty.GetSetMethod(nonPublic: true)!.Invoke(null, new object[] { originalCanBeSaved }); - } + [Fact] + public void EditingTheFile_ShouldCancelASaveStillWaitingFromAnEarlierChange() + { + using var _ = new TempSettingsFileScope(); + using var __ = new CanBeSavedScope(); + + var settings = CreateSettingsInstance(); + settings.Height = 99; + + // Edit the file by hand before that change is saved, then let the watcher report it. + const string handEdit = "{ \"Format\": \"edited by hand\" }"; + File.WriteAllText(Settings.FilePath, handEdit); + typeof(Settings).GetMethod("FileChanged", BindingFlags.NonPublic | BindingFlags.Instance)!.Invoke(settings, new object[] { null, null }); + PumpDispatcher(TimeSpan.FromSeconds(2)); + + Assert.Equal("edited by hand", settings.Format); + Assert.Equal(handEdit, File.ReadAllText(Settings.FilePath)); } [Fact] @@ -171,6 +171,32 @@ private static Settings LoadAndAttemptSave() } } + private static void PumpDispatcher(TimeSpan duration) + { + var frame = new DispatcherFrame(); + var stopTimer = new DispatcherTimer { Interval = duration }; + stopTimer.Tick += (_, _) => + { + stopTimer.Stop(); + frame.Continue = false; + }; + stopTimer.Start(); + Dispatcher.PushFrame(frame); + } + + /// + /// Lets settings save on their own during a test, as they do once the app has confirmed the file is writable. + /// + private sealed class CanBeSavedScope : IDisposable + { + private static readonly MethodInfo _setCanBeSaved = typeof(Settings).GetProperty(nameof(Settings.CanBeSaved), BindingFlags.Public | BindingFlags.Static)!.GetSetMethod(nonPublic: true)!; + private readonly bool _original = Settings.CanBeSaved; + + public CanBeSavedScope() => _setCanBeSaved.Invoke(null, new object[] { true }); + + public void Dispose() => _setCanBeSaved.Invoke(null, new object[] { _original }); + } + private static Settings CreateSettingsInstance() => (Settings)Activator.CreateInstance(typeof(Settings), nonPublic: true)!; diff --git a/DesktopClock/Properties/Settings.cs b/DesktopClock/Properties/Settings.cs index 208d389..a6576bc 100644 --- a/DesktopClock/Properties/Settings.cs +++ b/DesktopClock/Properties/Settings.cs @@ -572,18 +572,24 @@ private static Settings LoadAndAttemptSave() /// private void FileChanged(object sender, FileSystemEventArgs e) { - try - { - _populatingFromFile = true; - Populate(this); - } - catch + // Reload on the thread that saves and cancel any save still waiting from an earlier change, so it can't write a half-reloaded file or put old values back over the edit. + _saveTimer.Dispatcher.BeginInvoke(new Action(() => { - } - finally - { - _populatingFromFile = false; - } + _saveTimer.Stop(); + + try + { + _populatingFromFile = true; + Populate(this); + } + catch + { + } + finally + { + _populatingFromFile = false; + } + })); } private void ApplySystemThemeDefaultsIfAvailable()