diff --git a/DesktopClock.Tests/SettingsTests.cs b/DesktopClock.Tests/SettingsTests.cs index 73e62a2..6167119 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; @@ -70,6 +71,132 @@ 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")); + } + + [Fact] + public void ChangingASetting_ShouldSaveItShortlyAfterwards() + { + using var _ = new TempSettingsFileScope(); + + using var __ = new CanBeSavedScope(); + + var settings = CreateSettingsInstance(); + settings.Format = "saved without exiting"; + + // The save runs on a short timer, so let the dispatcher run for a bit. + PumpDispatcher(TimeSpan.FromSeconds(2)); + + // 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)); + } + + [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] + 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(); + releaser.Join(); + + 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() + { + var canBeSaved = typeof(Settings).GetProperty(nameof(Settings.CanBeSaved), BindingFlags.Public | BindingFlags.Static)!.GetSetMethod(nonPublic: true)!; + var originalCanBeSaved = Settings.CanBeSaved; + + try + { + canBeSaved.Invoke(null, new object[] { false }); + + var loadAndAttemptSave = typeof(Settings).GetMethod("LoadAndAttemptSave", BindingFlags.NonPublic | BindingFlags.Static)!; + return (Settings)loadAndAttemptSave.Invoke(null, null)!; + } + finally + { + canBeSaved.Invoke(null, new object[] { originalCanBeSaved }); + } + } + + 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/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 2616065..a6576bc 100644 --- a/DesktopClock/Properties/Settings.cs +++ b/DesktopClock/Properties/Settings.cs @@ -1,7 +1,9 @@ -using System; +using System; using System.ComponentModel; using System.IO; +using System.Runtime.InteropServices; using System.Windows.Media; +using System.Windows.Threading; using DesktopClock.Utilities; using Newtonsoft.Json; using WpfWindowPlacement; @@ -11,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); @@ -40,6 +44,23 @@ private Settings() EnableRaisingEvents = true, }; _watcher.Changed += 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. @@ -441,7 +462,7 @@ public bool Save() { try { - File.WriteAllText(FilePath, json); + WriteAllTextAtomically(FilePath, json); return true; } catch @@ -463,6 +484,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. /// @@ -480,15 +529,24 @@ private static void Populate(Settings settings) /// private static Settings LoadFromFile() { - try - { - var settings = new Settings(); - Populate(settings); - return settings; - } - catch + 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++) { - return new(); + try + { + Populate(settings); + return settings; + } + catch (IOException) when (attempt < 4 && Exists) + { + System.Threading.Thread.Sleep(250); + } + catch + { + return new(); + } } } @@ -499,7 +557,7 @@ private static Settings LoadAndAttemptSave() { var settings = LoadFromFile(); - if (!File.Exists(FilePath)) + if (!Exists) { settings.ApplySystemThemeDefaultsIfAvailable(); } @@ -514,13 +572,24 @@ private static Settings LoadAndAttemptSave() /// private void FileChanged(object sender, FileSystemEventArgs e) { - try + // 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(() => { - Populate(this); - } - catch - { - } + _saveTimer.Stop(); + + try + { + _populatingFromFile = true; + Populate(this); + } + catch + { + } + finally + { + _populatingFromFile = false; + } + })); } private void ApplySystemThemeDefaultsIfAvailable()