From 15a705036039817bb3edb88160b980e9f9f4df0a Mon Sep 17 00:00:00 2001 From: Dirk Date: Mon, 28 Sep 2026 22:43:42 +0200 Subject: [PATCH 1/2] Preserve settings files and report persistence failures --- .../Features/Settings/DolphinSettingsTests.cs | 5 +- .../Settings/SettingsPersistencePathTests.cs | 2 +- .../Settings/SettingsRecoveryTests.cs | 111 ++++++++++ .../Features/Settings/WhWzSettingsTests.cs | 5 +- .../Settings/DolphinSettingManager.cs | 189 ++++++------------ .../Features/Settings/RecompSettingManager.cs | 39 ++-- WheelWizard/Features/Settings/SettingsFile.cs | 18 ++ .../Features/Settings/Types/DolphinSetting.cs | 20 +- .../Features/Settings/Types/Setting.cs | 21 +- .../Features/Settings/Types/VirtualSetting.cs | 30 +-- .../Features/Settings/WhWzSettingManager.cs | 132 +++++------- 11 files changed, 314 insertions(+), 258 deletions(-) create mode 100644 WheelWizard.Test/Features/Settings/SettingsRecoveryTests.cs create mode 100644 WheelWizard/Features/Settings/SettingsFile.cs diff --git a/WheelWizard.Test/Features/Settings/DolphinSettingsTests.cs b/WheelWizard.Test/Features/Settings/DolphinSettingsTests.cs index 4f9808fd..df96506b 100644 --- a/WheelWizard.Test/Features/Settings/DolphinSettingsTests.cs +++ b/WheelWizard.Test/Features/Settings/DolphinSettingsTests.cs @@ -76,7 +76,7 @@ public void LoadSettings_ReadsExistingValue_FromIniFile() } [Fact] - public void LoadSettings_WritesDefaultValue_WhenIniEntryIsMissing() + public void LoadSettings_UsesDefaultWithoutWriting_WhenIniEntryIsMissing() { var fileSystem = new MockFileSystem(); var userFolderPath = $"/wheelwizard-user-{Guid.NewGuid():N}"; @@ -91,7 +91,8 @@ public void LoadSettings_WritesDefaultValue_WhenIniEntryIsMissing() manager.LoadSettings(configFolderPath); var updatedFile = fileSystem.File.ReadAllText(iniPath); - Assert.Contains("NANDRootPath = /default", updatedFile); + Assert.DoesNotContain("NANDRootPath", updatedFile); + Assert.Equal("/default", setting.Get()); } [Fact] diff --git a/WheelWizard.Test/Features/Settings/SettingsPersistencePathTests.cs b/WheelWizard.Test/Features/Settings/SettingsPersistencePathTests.cs index d98c9aee..baf0611d 100644 --- a/WheelWizard.Test/Features/Settings/SettingsPersistencePathTests.cs +++ b/WheelWizard.Test/Features/Settings/SettingsPersistencePathTests.cs @@ -97,7 +97,7 @@ public void RecompSettings_DoNotCreateBackendOwnedFile_WhenMissing() manager.RegisterSetting(setting); manager.LoadSettings(configPath); - manager.SaveSettings(configPath, setting); + Assert.Throws(() => manager.SaveSettings(configPath, setting)); manager.RemoveTomlSetting(configPath, "video", "show_fps"); Assert.False(fs.File.Exists(configPath)); diff --git a/WheelWizard.Test/Features/Settings/SettingsRecoveryTests.cs b/WheelWizard.Test/Features/Settings/SettingsRecoveryTests.cs new file mode 100644 index 00000000..3061ad73 --- /dev/null +++ b/WheelWizard.Test/Features/Settings/SettingsRecoveryTests.cs @@ -0,0 +1,111 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Testably.Abstractions.Testing; +using WheelWizard.Settings; +using WheelWizard.Settings.Types; + +namespace WheelWizard.Test.Features.Settings; + +public class SettingsRecoveryTests +{ + [Fact] + public void DolphinEditPreservesExternalValuesAndReloadDoesNotCarryPreviousProfile() + { + var fs = new MockFileSystem(); + fs.Directory.CreateDirectory("/config"); + fs.File.WriteAllText("/config/GFX.ini", "[Settings]\nShowFPS = False\nInternalResolution = 1\n"); + var manager = new DolphinSettingManager(fs); + var fps = new DolphinSetting(typeof(bool), ("GFX.ini", "Settings", "ShowFPS"), false, s => manager.SaveSettings("/config", s)); + var resolution = new DolphinSetting(typeof(int), ("GFX.ini", "Settings", "InternalResolution"), 1); + manager.RegisterSetting(fps); + manager.RegisterSetting(resolution); + manager.LoadSettings("/config"); + fs.File.WriteAllText("/config/GFX.ini", "[Settings]\nShowFPS = False\nInternalResolution = 4\n"); + Assert.True(fps.Set(true)); + Assert.Contains("InternalResolution = 4", fs.File.ReadAllText("/config/GFX.ini")); + Assert.Contains("ShowFPS = False", fs.File.ReadAllText("/config/GFX.ini.bak")); + manager.ReloadSettings("/config"); + Assert.Equal(4, resolution.Get()); + fs.File.WriteAllText("/config/GFX.ini", "[Settings]\nInternalResolution = invalid\n"); + manager.ReloadSettings("/config"); + Assert.Equal(1, resolution.Get()); + Assert.Equal(false, fps.Get()); + Assert.Contains("invalid", fs.File.ReadAllText("/config/GFX.ini")); + } + + [Fact] + public void RecompReloadMissingKeyUsesDefault() + { + var fs = new MockFileSystem(); + fs.Directory.CreateDirectory("/config"); + fs.File.WriteAllText("/config/Config.toml", "[video]\nshow_fps = false\n"); + var manager = new RecompSettingManager(fs); + var setting = new RecompSetting(typeof(bool), ("video", "show_fps"), true, s => manager.SaveSettings("/config/Config.toml", s)); + manager.RegisterSetting(setting); + manager.LoadSettings("/config/Config.toml"); + fs.File.WriteAllText("/config/Config.toml", "[video]\n"); + manager.ReloadSettings("/config/Config.toml"); + Assert.Equal(true, setting.Get()); + fs.File.Delete("/config/Config.toml"); + Assert.False(setting.Set(false)); + Assert.Equal(true, setting.Get()); + } + + [Fact] + public void FailedSaveRollsBackAndCanBeRetried() + { + var fail = true; + var setting = new WhWzSetting( + typeof(bool), + "Enabled", + false, + _ => + { + if (fail) + throw new IOException("disk full"); + } + ); + var notifications = 0; + setting.Changed += _ => notifications++; + Assert.False(setting.Set(true)); + Assert.Equal(false, setting.Get()); + Assert.Equal(0, notifications); + fail = false; + Assert.True(setting.Set(true)); + Assert.Null(setting.SaveError); + Assert.Equal(1, notifications); + } + + [Fact] + public void JsonRetainsExistingKeysAndUnknownValuesAndPreservesCorruptFile() + { + var fs = new MockFileSystem(); + fs.Directory.CreateDirectory("/config"); + const string path = "/config/config.json"; + const string original = "{\"EnableAnimations\":false,\"FutureSetting\":{\"value\":2}}"; + fs.File.WriteAllText(path, original); + var manager = new WhWzSettingManager(NullLogger.Instance, fs); + var setting = new WhWzSetting(typeof(bool), "EnableAnimations", true, s => manager.SaveSettings(path, s)); + manager.RegisterSetting(setting); + manager.LoadSettings(path); + Assert.Equal(false, setting.Get()); + Assert.True(setting.Set(true)); + Assert.Contains("FutureSetting", fs.File.ReadAllText(path)); + Assert.Equal(original, fs.File.ReadAllText(path + ".bak")); + fs.File.WriteAllText(path, "broken json"); + var corrupt = new WhWzSettingManager(NullLogger.Instance, fs); + corrupt.RegisterSetting(setting); + corrupt.LoadSettings(path); + Assert.Throws(() => corrupt.SaveSettings(path, setting)); + Assert.Equal("broken json", fs.File.ReadAllText(path)); + } + + [Fact] + public void VirtualSettingTracksDependenciesAfterSetterFailure() + { + var source = new WhWzSetting(typeof(int), "source", 1); + using var derived = new VirtualSetting(typeof(int), _ => throw new IOException("save failed"), source.Get).SetDependencies(source); + Assert.False(derived.Set(2)); + source.Set(3); + Assert.Equal(3, derived.Get()); + } +} diff --git a/WheelWizard.Test/Features/Settings/WhWzSettingsTests.cs b/WheelWizard.Test/Features/Settings/WhWzSettingsTests.cs index e550d204..16c91a58 100644 --- a/WheelWizard.Test/Features/Settings/WhWzSettingsTests.cs +++ b/WheelWizard.Test/Features/Settings/WhWzSettingsTests.cs @@ -85,10 +85,11 @@ public void Reset_RestoresForceSave_WhenSavingThrows(bool forceSave) .SetForceSave(forceSave); setting.Set(12, skipSave: true); - Assert.Throws(setting.Reset); + setting.Reset(); + Assert.IsType(setting.SaveError); Assert.Equal(forceSave, setting.Set(6, skipSave: true)); - Assert.Equal(forceSave ? 6 : 5, setting.Get()); + Assert.Equal(forceSave ? 6 : 12, setting.Get()); } [Fact] diff --git a/WheelWizard/Features/Settings/DolphinSettingManager.cs b/WheelWizard/Features/Settings/DolphinSettingManager.cs index cad5d1d9..6aa0cdcb 100644 --- a/WheelWizard/Features/Settings/DolphinSettingManager.cs +++ b/WheelWizard/Features/Settings/DolphinSettingManager.cs @@ -5,174 +5,107 @@ namespace WheelWizard.Settings; public class DolphinSettingManager(IFileSystem fileSystem) : IDolphinSettingManager { - // LOCKS: - // We use locks to keep the settings state and file IO consistent. - // Even though we do not manually create threads in this class, work can still happen concurrently - // (for example the Avalonia UI thread + Task/thread-pool execution), so synchronization is still required. - - // Sync Root: Responsible for synchronizing access to the _settings list and the _loaded flag. - // It ensures that multiple threads don't modify the settings list or the loaded state at the same time - // File IO Sync: Responsible for reading and writing the INI files. It ensures that multiple threads don't read/write at the same time - private readonly object _syncRoot = new(); - private readonly object _fileIoSync = new(); + private readonly object _sync = new(); private bool _loaded; private readonly List _settings = []; public void RegisterSetting(DolphinSetting setting) { - lock (_syncRoot) + lock (_sync) { - if (_loaded) - return; - - _settings.Add(setting); + if (!_loaded) + _settings.Add(setting); } } public void SaveSettings(string configDirectory, DolphinSetting invokingSetting) { - List settingsSnapshot; - lock (_syncRoot) - { - // TODO: This method definitely has to be optimized - if (!_loaded) - return; - - settingsSnapshot = [.. _settings]; - } - - lock (_fileIoSync) + lock (_sync) { - foreach (var setting in settingsSnapshot) + if (!_loaded || !fileSystem.Directory.Exists(configDirectory)) + throw new IOException("The Dolphin configuration directory is not available."); + + var path = fileSystem.Path.Combine(configDirectory, invokingSetting.FileName); + var lines = fileSystem.File.Exists(path) ? fileSystem.File.ReadAllLines(path).ToList() : []; + var header = $"[{invokingSetting.Section}]"; + var section = lines.FindIndex(line => line.Trim() == header); + var replacement = $"{invokingSetting.Name} = {invokingSetting.GetStringValue()}"; + if (section < 0) { - ChangeIniSettings(configDirectory, setting.FileName, setting.Section, setting.Name, setting.GetStringValue()); + lines.Add(header); + lines.Add(replacement); } + else + { + var index = section + 1; + for (; index < lines.Count && !IsSection(lines[index]); index++) + { + if (Key(lines[index]) != invokingSetting.Name) + continue; + lines[index] = replacement; + SettingsFile.WriteLines(fileSystem, path, lines); + return; + } + lines.Insert(index, replacement); + } + SettingsFile.WriteLines(fileSystem, path, lines); } } public void ReloadSettings(string configDirectory) { - lock (_syncRoot) + lock (_sync) { - // TODO: this method could also be optimized by checking if the previously loaded directory - // is still the current ConfigFolderPath and if so, just not run the LoadSettings method again _loaded = false; + LoadSettings(configDirectory); } - - LoadSettings(configDirectory); } public void LoadSettings(string configDirectory) { - List settingsSnapshot; - if (_loaded || !fileSystem.Directory.Exists(configDirectory)) - return; - - lock (_syncRoot) + lock (_sync) { - // Since we are working with concurrency here, we have to check loaded again since it might be changed while we were waiting - // for the lock to open if (_loaded) return; - _loaded = true; - settingsSnapshot = [.. _settings]; - } - - // TODO: This method can maybe be optimized in the future, since now it reads the file for every setting - // and on top of that for reach setting it loops over each line and section and stuff like that. - lock (_fileIoSync) - { - foreach (var setting in settingsSnapshot) + // Read each file once; loading never writes defaults into another application's config. + foreach (var group in _settings.GroupBy(setting => setting.FileName)) { - var value = ReadIniSetting(configDirectory, setting.FileName, setting.Section, setting.Name); - if (value == null) - ChangeIniSettings(configDirectory, setting.FileName, setting.Section, setting.Name, setting.GetStringValue()); - else - setting.SetFromString(value, true); // we read it, which means there is no purpose in saving it again + var path = fileSystem.Path.Combine(configDirectory, group.Key); + string[] lines; + try + { + lines = fileSystem.File.Exists(path) ? fileSystem.File.ReadAllLines(path) : []; + } + catch (Exception exception) when (exception is IOException or UnauthorizedAccessException) + { + lines = []; + } + foreach (var setting in group) + { + var value = ReadValue(lines, setting.Section, setting.Name); + if (value == null || !setting.SetFromString(value, skipSave: true)) + setting.Reset(skipSave: true); + } } + _loaded = true; } } - private string[]? ReadIniFile(string configDirectory, string fileName) - { - var filePath = fileSystem.Path.Combine(configDirectory, fileName); - if (!fileSystem.File.Exists(filePath)) - return null; + private static bool IsSection(string line) => line.Trim() is var text && text.StartsWith('[') && text.EndsWith(']'); - try - { - return fileSystem.File.ReadAllLines(filePath); - } - catch - { - return null; - } - } + private static string? Key(string line) => line.IndexOf('=') is var index && index >= 0 ? line[..index].Trim() : null; - private string? ReadIniSetting(string configDirectory, string fileName, string section, string settingToRead) + private static string? ReadValue(IEnumerable lines, string section, string key) { - var lines = ReadIniFile(configDirectory, fileName); - if (lines == null) - return null; - - var sectionIndex = Array.IndexOf(lines, $"[{section}]"); - if (sectionIndex == -1) - return null; - - // find all the settings related to this section, we dont want to read/influence other sections - var nextSectionName = lines.Skip(sectionIndex + 1).FirstOrDefault(x => x.Trim().StartsWith("[") && x.Trim().EndsWith("]")); - var nextSectionIndex = Array.IndexOf(lines, nextSectionName); - var sectionLines = lines.Skip(sectionIndex + 1); - if (nextSectionIndex != -1) - sectionLines = sectionLines.Take(nextSectionIndex - sectionIndex - 1); - - // finally we can read the setting - foreach (var line in sectionLines) + var inSection = false; + foreach (var line in lines) { - if (!line.StartsWith($"{settingToRead}=") && !line.StartsWith($"{settingToRead} =")) - continue; - //we found the setting, now we need to return the value - var setting = line.Split("="); - return setting[1].Trim(); + if (IsSection(line)) + inSection = line.Trim() == $"[{section}]"; + else if (inSection && Key(line) == key) + return line[(line.IndexOf('=') + 1)..].Trim(); } - return null; } - - // TODO: find out when to use `setting=value` and when to use `setting = value` - private void ChangeIniSettings(string configDirectory, string fileName, string section, string settingToChange, string value) - { - // #todo: replace ini files atomically with a backup instead of overwriting them in place. - var lines = ReadIniFile(configDirectory, fileName)?.ToList(); - if (lines == null) - return; - - var sectionIndex = lines.IndexOf($"[{section}]"); - if (sectionIndex == -1) - { - lines.Add($"[{section}]"); - lines.Add($"{settingToChange} = {value}"); - fileSystem.File.WriteAllLines(fileSystem.Path.Combine(configDirectory, fileName), lines); - return; - } - - for (var i = sectionIndex + 1; i < lines.Count; i++) - { - // - if (lines[i].Trim().StartsWith("[") && lines[i].Trim().EndsWith("]")) - break; // Setting was not found in this section, so we have to append it to the section - - if (!lines[i].StartsWith($"{settingToChange}=") && !lines[i].StartsWith($"{settingToChange} =")) - continue; - - lines[i] = $"{settingToChange} = {value}"; - fileSystem.File.WriteAllLines(fileSystem.Path.Combine(configDirectory, fileName), lines); - return; - } - // you only get here if the setting was not found in the section - - lines.Insert(sectionIndex + 1, $"{settingToChange} = {value}"); - fileSystem.File.WriteAllLines(fileSystem.Path.Combine(configDirectory, fileName), lines); - } } diff --git a/WheelWizard/Features/Settings/RecompSettingManager.cs b/WheelWizard/Features/Settings/RecompSettingManager.cs index 90b512bd..418b249b 100644 --- a/WheelWizard/Features/Settings/RecompSettingManager.cs +++ b/WheelWizard/Features/Settings/RecompSettingManager.cs @@ -30,7 +30,7 @@ public void SaveSettings(string configPath, RecompSetting invokingSetting) lock (_syncRoot) { if (!_loaded) - return; + throw new IOException("The WiiCompiled settings have not been loaded."); } lock (_fileIoSync) @@ -51,7 +51,6 @@ public void ReloadSettings(string configPath) public void RemoveTomlSetting(string configPath, string section, string settingToRemove) { - // #todo: use the same atomic save helper as setting updates so removing a key can't leave a partial config. lock (_fileIoSync) { var lines = ReadTomlFile(configPath)?.ToList(); @@ -73,7 +72,7 @@ public void RemoveTomlSetting(string configPath, string section, string settingT continue; lines.RemoveAt(i); - fileSystem.File.WriteAllLines(configPath, lines); + SettingsFile.WriteLines(fileSystem, configPath, lines); return; } } @@ -82,7 +81,7 @@ public void RemoveTomlSetting(string configPath, string section, string settingT public void LoadSettings(string configPath) { List settingsSnapshot; - if (_loaded || !fileSystem.File.Exists(configPath)) + if (_loaded) return; lock (_syncRoot) @@ -101,9 +100,17 @@ public void LoadSettings(string configPath) // A missing or unparsable key keeps the registered default without writing it back: // the runtime falls back to the very same default, so the file stays untouched until // the user actually changes something. - var value = ReadTomlSetting(configPath, setting.Section, setting.Name); - if (value != null) - setting.SetFromString(value, true); + string? value; + try + { + value = ReadTomlSetting(configPath, setting.Section, setting.Name); + } + catch (Exception exception) when (exception is IOException or UnauthorizedAccessException) + { + value = null; + } + if (value == null || !setting.SetFromString(value, true)) + setting.Reset(skipSave: true); } } } @@ -113,14 +120,7 @@ public void LoadSettings(string configPath) if (!fileSystem.File.Exists(configPath)) return null; - try - { - return fileSystem.File.ReadAllLines(configPath); - } - catch - { - return null; - } + return fileSystem.File.ReadAllLines(configPath); } private string? ReadTomlSetting(string configPath, string section, string settingToRead) @@ -156,13 +156,12 @@ public void LoadSettings(string configPath) private void WriteTomlSetting(string configPath, string section, string settingToChange, string value) { - // #todo: add a shared temp-file-and-backup save helper for toml updates while preserving keys owned by the game. var lines = ReadTomlFile(configPath)?.ToList(); // The backend owns creating Config.toml; a write before it exists would hand the runtime a // file Wheel Wizard invented, so the value simply stays in memory until the next load. if (lines == null) - return; + throw new IOException("The WiiCompiled configuration file is not available."); var sectionIndex = lines.FindIndex(line => line.Trim() == $"[{section}]"); if (sectionIndex == -1) @@ -171,7 +170,7 @@ private void WriteTomlSetting(string configPath, string section, string settingT lines.Add(string.Empty); lines.Add($"[{section}]"); lines.Add($"{settingToChange} = {value}"); - fileSystem.File.WriteAllLines(configPath, lines); + SettingsFile.WriteLines(fileSystem, configPath, lines); return; } @@ -185,12 +184,12 @@ private void WriteTomlSetting(string configPath, string section, string settingT continue; lines[i] = $"{settingToChange} = {value}"; - fileSystem.File.WriteAllLines(configPath, lines); + SettingsFile.WriteLines(fileSystem, configPath, lines); return; } lines.Insert(sectionIndex + 1, $"{settingToChange} = {value}"); - fileSystem.File.WriteAllLines(configPath, lines); + SettingsFile.WriteLines(fileSystem, configPath, lines); } private static bool IsSettingLine(string trimmedLine, string settingName) => diff --git a/WheelWizard/Features/Settings/SettingsFile.cs b/WheelWizard/Features/Settings/SettingsFile.cs new file mode 100644 index 00000000..9fe90371 --- /dev/null +++ b/WheelWizard/Features/Settings/SettingsFile.cs @@ -0,0 +1,18 @@ +using System.IO.Abstractions; +using System.Text; +using WheelWizard.Shared.IO; + +namespace WheelWizard.Settings; + +internal static class SettingsFile +{ + public static void Write(IFileSystem files, string path, string text) + { + var result = files.WriteAllBytesAtomic(path, Encoding.UTF8.GetBytes(text)); + if (result.IsFailure) + throw new IOException(result.Error.Message, result.Error.Exception); + } + + public static void WriteLines(IFileSystem files, string path, IEnumerable lines) => + Write(files, path, string.Join(Environment.NewLine, lines) + Environment.NewLine); +} diff --git a/WheelWizard/Features/Settings/Types/DolphinSetting.cs b/WheelWizard/Features/Settings/Types/DolphinSetting.cs index 4bc67365..ace57169 100644 --- a/WheelWizard/Features/Settings/Types/DolphinSetting.cs +++ b/WheelWizard/Features/Settings/Types/DolphinSetting.cs @@ -1,3 +1,5 @@ +using System.Globalization; + namespace WheelWizard.Settings.Types; public class DolphinSetting : Setting @@ -62,7 +64,7 @@ public string GetStringValue() if (ValueType.IsEnum) return ((int)Value).ToString(); - return Value?.ToString() ?? "null"; + return Convert.ToString(Value, CultureInfo.InvariantCulture) ?? "null"; } public bool SetFromString(string newValue, bool skipSave = false) @@ -72,12 +74,16 @@ public bool SetFromString(string newValue, bool skipSave = false) return ValueType switch { { } t when t == typeof(string) => Set(newValue, skipSave), - { } t when t == typeof(int) => Set(int.Parse(newValue), skipSave), - { } t when t == typeof(long) => Set(long.Parse(newValue), skipSave), - { } t when t == typeof(float) => Set(float.Parse(newValue), skipSave), - { } t when t == typeof(double) => Set(double.Parse(newValue), skipSave), - { } t when t == typeof(bool) => Set(bool.Parse(newValue), skipSave), - { IsEnum: true } t => Set(Enum.ToObject(t, int.Parse(newValue)), skipSave), + { } t when t == typeof(int) => int.TryParse(newValue, CultureInfo.InvariantCulture, out var number) && Set(number, skipSave), + { } t when t == typeof(long) => long.TryParse(newValue, CultureInfo.InvariantCulture, out var number) && Set(number, skipSave), + { } t when t == typeof(float) => float.TryParse(newValue, CultureInfo.InvariantCulture, out var number) + && Set(number, skipSave), + { } t when t == typeof(double) => double.TryParse(newValue, CultureInfo.InvariantCulture, out var number) + && Set(number, skipSave), + { } t when t == typeof(bool) => bool.TryParse(newValue, out var flag) && Set(flag, skipSave), + { IsEnum: true } t => int.TryParse(newValue, out var number) + && Enum.IsDefined(t, number) + && Set(Enum.ToObject(t, number), skipSave), _ => throw new InvalidOperationException($"Unsupported type: {ValueType.Name}"), }; } diff --git a/WheelWizard/Features/Settings/Types/Setting.cs b/WheelWizard/Features/Settings/Types/Setting.cs index a838f42d..b5b39d0a 100644 --- a/WheelWizard/Features/Settings/Types/Setting.cs +++ b/WheelWizard/Features/Settings/Types/Setting.cs @@ -20,16 +20,29 @@ protected Setting(Type type, string name, object defaultValue) protected Func? ValidationFunc { get; set; } protected bool SaveEvenIfNotValid { get; set; } public Type ValueType { get; protected set; } + public Exception? SaveError { get; private set; } public bool Set(object newValue, bool skipSave = false) { + SaveError = null; if (newValue.GetType() != ValueType) return false; - if (Value?.Equals(newValue) == true) + if (Value.Equals(newValue)) return true; - var succeeded = SetInternal(newValue, skipSave); + var previousValue = Value; + bool succeeded; + try + { + succeeded = SetInternal(newValue, skipSave); + } + catch (Exception exception) when (exception is IOException or UnauthorizedAccessException) + { + Value = previousValue; + SaveError = exception; + return false; + } if (succeeded) SignalChange(); @@ -40,13 +53,13 @@ public bool Set(object newValue, bool skipSave = false) public abstract object Get(); - public void Reset() + public void Reset(bool skipSave = false) { var s = SaveEvenIfNotValid; SaveEvenIfNotValid = true; try { - Set(DefaultValue); + Set(DefaultValue, skipSave); } finally { diff --git a/WheelWizard/Features/Settings/Types/VirtualSetting.cs b/WheelWizard/Features/Settings/Types/VirtualSetting.cs index c47445ae..638b81a7 100644 --- a/WheelWizard/Features/Settings/Types/VirtualSetting.cs +++ b/WheelWizard/Features/Settings/Types/VirtualSetting.cs @@ -22,20 +22,26 @@ protected override bool SetInternal(object newValue, bool skipSave = false) { // we don't use skipSave here since its a virtual setting, and so there is nothing to save _acceptsSignals = false; - var oldValue = Value; - Value = newValue; - var newIsValid = SaveEvenIfNotValid || IsValid(); - var succeeded = false; - if (newIsValid) + try { - _setter(newValue); - succeeded = true; - } - else - Value = oldValue; + var oldValue = Value; + Value = newValue; + var newIsValid = SaveEvenIfNotValid || IsValid(); + var succeeded = false; + if (newIsValid) + { + _setter(newValue); + succeeded = true; + } + else + Value = oldValue; - _acceptsSignals = true; - return succeeded; + return succeeded; + } + finally + { + _acceptsSignals = true; + } } public override object Get() => Value; diff --git a/WheelWizard/Features/Settings/WhWzSettingManager.cs b/WheelWizard/Features/Settings/WhWzSettingManager.cs index 545b833d..1b84198d 100644 --- a/WheelWizard/Features/Settings/WhWzSettingManager.cs +++ b/WheelWizard/Features/Settings/WhWzSettingManager.cs @@ -7,123 +7,91 @@ namespace WheelWizard.Settings; public class WhWzSettingManager(ILogger logger, IFileSystem fileSystem) : IWhWzSettingManager { - // LOCKS: - // We are working with locks. This is to ensure that we always have accurate information in our settings / application. - // We do not create multiple threads. However, some of our features run through Tasks. Those are executed asynchronously, therefore still require locks. - - // Sync Root: Responsible for synchronizing access to the _settings list and the _loaded flag. - // It ensures that multiple threads don't modify the settings list or the loaded state at the same time - // File IO Sync: Responsible for reading and writing the INI files. It ensures that multiple threads don't read/write at the same time - private readonly object _syncRoot = new(); - private readonly object _fileIoSync = new(); + private readonly object _sync = new(); private bool _loaded; + private Exception? _loadError; private readonly Dictionary _settings = new(); + private readonly Dictionary _unknownSettings = new(); public void RegisterSetting(WhWzSetting setting) { - lock (_syncRoot) + lock (_sync) { - if (_loaded) - return; - - _settings[setting.Name] = setting; + if (!_loaded) + _settings[setting.Name] = setting; } } public void SaveSettings(string configPath, WhWzSetting invokingSetting) { - // #todo: write to a temp file and swap it in with a backup so an interrupted save can't leave a broken config. - Dictionary settingsSnapshot; - lock (_syncRoot) + lock (_sync) { if (!_loaded) - return; - - settingsSnapshot = new(_settings); - } + throw new IOException("Application settings have not been loaded."); + if (_loadError != null) + throw new IOException("The settings file could not be loaded; the original file has been preserved.", _loadError); - var settingsToSave = new Dictionary(); + var values = _unknownSettings.ToDictionary(pair => pair.Key, pair => (object?)pair.Value); + foreach (var (name, setting) in _settings) + values[name] = setting.Get(); - foreach (var (name, setting) in settingsSnapshot) - { - settingsToSave[name] = setting.Get(); - } - - var jsonString = JsonSerializer.Serialize(settingsToSave, new JsonSerializerOptions { WriteIndented = true }); - lock (_fileIoSync) - { try { - var directoryPath = fileSystem.Path.GetDirectoryName(configPath); - if (!string.IsNullOrWhiteSpace(directoryPath) && !fileSystem.Directory.Exists(directoryPath)) - fileSystem.Directory.CreateDirectory(directoryPath); - - fileSystem.File.WriteAllText(configPath, jsonString); + SettingsFile.Write( + fileSystem, + configPath, + JsonSerializer.Serialize(values, new JsonSerializerOptions { WriteIndented = true }) + ); } - catch (Exception ex) + catch (Exception exception) { - logger.LogError(ex, "Failed to save settings file: {Path}", configPath); + logger.LogError(exception, "Failed to save settings file: {Path}", configPath); + throw; } } } public void LoadSettings(string configPath) { - Dictionary settingsSnapshot; - lock (_syncRoot) + lock (_sync) { if (_loaded) return; - - _loaded = true; - settingsSnapshot = new(_settings); - } - - // Even if it now returns early, loading has been considered complete. - string? jsonString; - lock (_fileIoSync) - { try { - jsonString = fileSystem.File.Exists(configPath) ? fileSystem.File.ReadAllText(configPath) : null; + if (!fileSystem.File.Exists(configPath)) + return; + var values = JsonSerializer.Deserialize>(fileSystem.File.ReadAllText(configPath)); + if (values == null) + throw new JsonException("Expected a settings object."); + foreach (var (name, value) in values) + { + if (!_settings.TryGetValue(name, out var setting)) + { + _unknownSettings[name] = value; + continue; + } + try + { + if (!setting.SetFromJson(value, skipSave: true)) + setting.Reset(skipSave: true); + } + catch (Exception exception) + { + logger.LogWarning(exception, "Invalid value for setting {SettingName}; resetting to default.", name); + setting.Reset(skipSave: true); + } + } } - catch (Exception ex) + catch (Exception exception) when (exception is IOException or UnauthorizedAccessException or JsonException) { - logger.LogError(ex, "Failed to read settings file: {Path}", configPath); - jsonString = null; + _loadError = exception; + logger.LogError(exception, "Failed to load settings file: {Path}", configPath); } - } - - if (jsonString == null) - return; - - try - { - var loadedSettings = JsonSerializer.Deserialize>(jsonString); - if (loadedSettings == null) - return; - - foreach (var kvp in loadedSettings) + finally { - if (!settingsSnapshot.TryGetValue(kvp.Key, out var setting)) - continue; - - try - { - var success = setting.SetFromJson(kvp.Value, skipSave: true); - if (!success) - setting.Set(setting.DefaultValue, skipSave: true); - } - catch (Exception ex) - { - logger.LogWarning(ex, "Invalid value for setting {SettingName}; resetting to default.", setting.Name); - setting.Set(setting.DefaultValue, skipSave: true); - } + _loaded = true; } } - catch (JsonException e) - { - logger.LogError(e, "Failed to deserialize the JSON config"); - } } } From 6156eaf502240158199f6255ce3e9d9e83c354ba Mon Sep 17 00:00:00 2001 From: Dirk Date: Fri, 2 Oct 2026 21:57:56 +0200 Subject: [PATCH 2/2] Propagate recommended settings save failures --- .../Features/Settings/SettingsTests.cs | 40 +++++++++++++++++++ .../Features/Settings/SettingsManager.cs | 15 +++++-- .../Pages/Settings/VideoSettings.axaml.cs | 13 +++++- 3 files changed, 63 insertions(+), 5 deletions(-) diff --git a/WheelWizard.Test/Features/Settings/SettingsTests.cs b/WheelWizard.Test/Features/Settings/SettingsTests.cs index 31fd9ec1..d207ca44 100644 --- a/WheelWizard.Test/Features/Settings/SettingsTests.cs +++ b/WheelWizard.Test/Features/Settings/SettingsTests.cs @@ -21,6 +21,46 @@ public sealed class SettingsFeatureCollection; [Collection("SettingsFeature")] public class SettingsManagerTests { + [Theory] + [InlineData("ShaderCompilationMode")] +#if WINDOWS + [InlineData("WaitForShadersBeforeStarting")] +#endif + [InlineData("MSAA")] + [InlineData("SSAA")] + public void RecommendedSettings_PropagatesChildSaveFailureAndCanRetry(string failedSetting) + { + using var manager = CreateManager(new MockFileSystem(), out _, out var dolphinManager, out _); + var children = dolphinManager + .ReceivedCalls() + .Where(call => call.GetMethodInfo().Name == nameof(IDolphinSettingManager.RegisterSetting)) + .Select(call => (DolphinSetting)call.GetArguments()[0]!) + .ToDictionary(setting => setting.Name); + children["SSAA"].Set(true, skipSave: true); + var failure = new IOException("disk full"); + var fail = true; + dolphinManager + .When(m => m.SaveSettings(Arg.Any(), Arg.Is(s => s.Name == failedSetting))) + .Do(_ => + { + if (fail) + throw failure; + }); + var notifications = 0; + manager.RECOMMENDED_SETTINGS.Changed += _ => notifications++; + + Assert.False(manager.Set(manager.RECOMMENDED_SETTINGS, true)); + Assert.Same(failure, manager.RECOMMENDED_SETTINGS.SaveError); + Assert.False(manager.Get(manager.RECOMMENDED_SETTINGS)); + Assert.Equal(0, notifications); + + fail = false; + Assert.True(manager.Set(manager.RECOMMENDED_SETTINGS, true)); + Assert.True(manager.Get(manager.RECOMMENDED_SETTINGS)); + Assert.Null(manager.RECOMMENDED_SETTINGS.SaveError); + Assert.Equal(1, notifications); + } + [Fact] public void Get_Throws_WhenRequestedTypeDoesNotMatchSettingType() { diff --git a/WheelWizard/Features/Settings/SettingsManager.cs b/WheelWizard/Features/Settings/SettingsManager.cs index 3eb604f5..d2461d79 100644 --- a/WheelWizard/Features/Settings/SettingsManager.cs +++ b/WheelWizard/Features/Settings/SettingsManager.cs @@ -280,15 +280,22 @@ IUnixCommandService commands typeof(bool), value => { + void SetDolphinSetting(Setting setting, object newValue) + { + if (!setting.Set(newValue)) + throw setting.SaveError ?? new IOException($"Failed to save Dolphin setting '{setting.Name}'."); + } + var newValue = (bool)value!; - _dolphinCompilationMode.Set( + SetDolphinSetting( + _dolphinCompilationMode, newValue ? DolphinShaderCompilationMode.HybridUberShaders : DolphinShaderCompilationMode.Default ); #if WINDOWS - _dolphinCompileShadersAtStart.Set(newValue); + SetDolphinSetting(_dolphinCompileShadersAtStart, newValue); #endif - _dolphinMsaa.Set(newValue ? "0x00000002" : "0x00000001"); - _dolphinSsaa.Set(false); + SetDolphinSetting(_dolphinMsaa, newValue ? "0x00000002" : "0x00000001"); + SetDolphinSetting(_dolphinSsaa, false); }, () => { diff --git a/WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs b/WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs index 87c9e441..7343b879 100644 --- a/WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs +++ b/WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs @@ -101,7 +101,18 @@ private void VSync_OnClick(object? sender, RoutedEventArgs e) private void Recommended_OnClick(object? sender, RoutedEventArgs e) { - SettingsService.Set(SettingsService.RECOMMENDED_SETTINGS, RecommendedButton.IsChecked == true); + if (!SettingsService.Set(SettingsService.RECOMMENDED_SETTINGS, RecommendedButton.IsChecked == true)) + { + RecommendedButton.IsCheckedChanged -= Recommended_OnClick; + try + { + RecommendedButton.IsChecked = SettingsService.Get(SettingsService.RECOMMENDED_SETTINGS); + } + finally + { + RecommendedButton.IsCheckedChanged += Recommended_OnClick; + } + } } private void ShowFPS_OnClick(object? sender, RoutedEventArgs e)