From 65e43f8d5465e109f81509b51d8c920e4757c7f8 Mon Sep 17 00:00:00 2001 From: Dirk Date: Mon, 28 Sep 2026 22:43:42 +0200 Subject: [PATCH 1/6] 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 4f9808fd8..df96506b7 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 d98c9aee1..baf0611d1 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 000000000..3061ad73a --- /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 e550d2043..16c91a58c 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 cad5d1d9a..6aa0cdcb1 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 90b512bd6..418b249b4 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 000000000..9fe903718 --- /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 4bc673659..ace57169b 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 a838f42d0..b5b39d0a6 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 c47445ae7..638b81a76 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 545b833d4..1b84198d1 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 6f1a5379b2f73bfe62a351c0aae1164edd56e216 Mon Sep 17 00:00:00 2001 From: Dirk Date: Mon, 28 Sep 2026 22:49:30 +0200 Subject: [PATCH 2/6] Make settings values type safe and centralize path reloads --- .../Dolphin/FeaturePathOwnershipTests.cs | 10 +- .../Launching/DolphinLaunchServiceTests.cs | 2 +- .../Features/Launching/LaunchPathTests.cs | 4 +- .../RetroRewindLaunchServiceTests.cs | 10 +- .../Features/MiiRepositoryServiceTests.cs | 6 +- .../Features/Settings/DolphinSettingsTests.cs | 24 ++- .../Settings/SettingsPersistencePathTests.cs | 8 +- .../Settings/SettingsRecoveryTests.cs | 15 +- .../Features/Settings/SettingsTests.cs | 35 ++-- .../Features/Settings/VirtualSettingsTests.cs | 18 +- .../Features/Settings/WhWzSettingsTests.cs | 37 ++-- .../Views/HomeDolphinLaunchTests.cs | 2 +- .../Settings/DolphinSettingManager.cs | 6 +- .../Features/Settings/ISettingsServices.cs | 79 ++++----- .../Features/Settings/RecompSettingManager.cs | 8 +- .../Features/Settings/SettingsManager.cs | 160 +++++++++--------- .../Features/Settings/Types/DolphinSetting.cs | 112 +++++------- .../Features/Settings/Types/RecompSetting.cs | 57 +++---- .../Features/Settings/Types/Setting.cs | 106 ++++++------ .../Settings/Types/SettingConstants.cs | 4 +- .../Features/Settings/Types/VirtualSetting.cs | 38 +---- .../Features/Settings/Types/WhWzSetting.cs | 77 +++------ .../Features/Settings/WhWzSettingManager.cs | 8 +- WheelWizard/Views/Layout.axaml.cs | 2 +- .../Pages/Settings/OtherSettings.axaml.cs | 4 +- .../Pages/Settings/RecompSettings.axaml.cs | 8 +- .../Views/Pages/Settings/SettingsEditing.cs | 19 +++ .../Pages/Settings/VideoSettings.axaml.cs | 14 +- .../Pages/Settings/WhWzSettings.axaml.cs | 19 +-- 29 files changed, 408 insertions(+), 484 deletions(-) create mode 100644 WheelWizard/Views/Pages/Settings/SettingsEditing.cs diff --git a/WheelWizard.Test/Features/Dolphin/FeaturePathOwnershipTests.cs b/WheelWizard.Test/Features/Dolphin/FeaturePathOwnershipTests.cs index 3eda063f0..cc3637859 100644 --- a/WheelWizard.Test/Features/Dolphin/FeaturePathOwnershipTests.cs +++ b/WheelWizard.Test/Features/Dolphin/FeaturePathOwnershipTests.cs @@ -94,19 +94,19 @@ public void DistributionWithDolphin_UsesEffectiveLoadDirectory_WhileDownloadsFol private static (ISettingsManager, DolphinPaths) CreateDolphinPaths(MockFileSystem fs, string userFolder) { var settings = Substitute.For(); - settings.USER_FOLDER_PATH.Returns(new WhWzSetting(typeof(string), "UserFolderPath", userFolder)); - settings.DOLPHIN_LOCATION.Returns(new WhWzSetting(typeof(string), "DolphinLocation", "dolphin-emu")); + settings.USER_FOLDER_PATH.Returns(new WhWzSetting("UserFolderPath", userFolder)); + settings.DOLPHIN_LOCATION.Returns(new WhWzSetting("DolphinLocation", "dolphin-emu")); settings.LOAD_PATH.Returns( - new WhWzSetting(typeof(string), "LoadPath", "").SetValidation(value => + new WhWzSetting("LoadPath", "").SetValidation(value => value is string directory && !string.IsNullOrWhiteSpace(directory) && fs.Directory.Exists(directory) ) ); settings.NAND_ROOT_PATH.Returns( - new WhWzSetting(typeof(string), "NandRootPath", "").SetValidation(value => + new WhWzSetting("NandRootPath", "").SetValidation(value => value is string directory && !string.IsNullOrWhiteSpace(directory) && fs.Directory.Exists(directory) ) ); - settings.Get(Arg.Any()).Returns(call => (string)call.Arg().Get()); + settings.Get(Arg.Any>()).Returns(call => (string)call.Arg>().Get()); return (settings, new DolphinPaths(settings, new DolphinPathResolver(fs, Substitute.For()), fs)); } } diff --git a/WheelWizard.Test/Features/Launching/DolphinLaunchServiceTests.cs b/WheelWizard.Test/Features/Launching/DolphinLaunchServiceTests.cs index ac8d5a4b3..d9c7dbd4b 100644 --- a/WheelWizard.Test/Features/Launching/DolphinLaunchServiceTests.cs +++ b/WheelWizard.Test/Features/Launching/DolphinLaunchServiceTests.cs @@ -154,7 +154,7 @@ private sealed class Fixture public IDolphinVersionService Versions { get; } = Substitute.For(); public ILinuxDolphinInstaller Installer { get; } = Substitute.For(); public IDolphinLaunchPresentation Presentation { get; } = Substitute.For(); - public WhWzSetting GamePath { get; } = new(typeof(string), "GamePath", "/game.iso"); + public WhWzSetting GamePath { get; } = new("GamePath", "/game.iso"); public DolphinLaunchService Service { get; } public Fixture(bool windows = false, string command = "dolphin-emu") diff --git a/WheelWizard.Test/Features/Launching/LaunchPathTests.cs b/WheelWizard.Test/Features/Launching/LaunchPathTests.cs index bb10d20dc..584ef393f 100644 --- a/WheelWizard.Test/Features/Launching/LaunchPathTests.cs +++ b/WheelWizard.Test/Features/Launching/LaunchPathTests.cs @@ -94,8 +94,8 @@ public async Task BlockedDistributionPreflight_DoesNotKillPrepareOrWrite(bool be private static ISettingsManager CreateSettings() { var settings = Substitute.For(); - settings.GAME_LOCATION.Returns(new WhWzSetting(typeof(string), "GamePath", "/game.iso")); - settings.Get(Arg.Any()).Returns(call => (string)call.Arg().Get()); + settings.GAME_LOCATION.Returns(new WhWzSetting("GamePath", "/game.iso")); + settings.Get(Arg.Any>()).Returns(call => (string)call.Arg>().Get()); return settings; } } diff --git a/WheelWizard.Test/Features/Launching/RetroRewindLaunchServiceTests.cs b/WheelWizard.Test/Features/Launching/RetroRewindLaunchServiceTests.cs index e18842286..3d7492d40 100644 --- a/WheelWizard.Test/Features/Launching/RetroRewindLaunchServiceTests.cs +++ b/WheelWizard.Test/Features/Launching/RetroRewindLaunchServiceTests.cs @@ -122,11 +122,11 @@ public Fixture() { Fs.File.WriteAllText("/game.iso", "game"); var settings = Substitute.For(); - settings.GAME_LOCATION.Returns(new WhWzSetting(typeof(string), "Game", "/game.iso")); - settings.FORCE_WIIMOTE.Returns(new WhWzSetting(typeof(bool), "Force", true)); - settings.LAUNCH_WITH_DOLPHIN.Returns(new WhWzSetting(typeof(bool), "Dolphin", false)); - settings.Get(Arg.Any()).Returns(call => (string)call.Arg().Get()); - settings.Get(Arg.Any()).Returns(call => (bool)call.Arg().Get()); + settings.GAME_LOCATION.Returns(new WhWzSetting("Game", "/game.iso")); + settings.FORCE_WIIMOTE.Returns(new WhWzSetting("Force", true)); + settings.LAUNCH_WITH_DOLPHIN.Returns(new WhWzSetting("Dolphin", false)); + settings.Get(Arg.Any>()).Returns(call => (string)call.Arg>().Get()); + settings.Get(Arg.Any>()).Returns(call => (bool)call.Arg>().Get()); var paths = Substitute.For(); paths.PatchesFolderPath.Returns("/stable-patches"); paths.BetaPatchesFolderPath.Returns("/beta-patches"); diff --git a/WheelWizard.Test/Features/MiiRepositoryServiceTests.cs b/WheelWizard.Test/Features/MiiRepositoryServiceTests.cs index 0c909ca84..886851c35 100644 --- a/WheelWizard.Test/Features/MiiRepositoryServiceTests.cs +++ b/WheelWizard.Test/Features/MiiRepositoryServiceTests.cs @@ -26,9 +26,9 @@ public sealed class MiiRepositoryServiceTests public MiiRepositoryServiceTests() { _settings = SettingsTestUtils.CreateSettingsStub(Path.GetDirectoryName(_sourceNand)!); - var nandSetting = new WhWzSetting(typeof(string), "NandRoot", _sourceNand); - var copySetting = new WhWzSetting(typeof(bool), "CopyNand", false); - var useSetting = new WhWzSetting(typeof(bool), "UseDolphinData", true); + var nandSetting = new WhWzSetting("NandRoot", _sourceNand); + var copySetting = new WhWzSetting("CopyNand", false); + var useSetting = new WhWzSetting("UseDolphinData", true); _settings.NAND_ROOT_PATH.Returns(nandSetting); _settings.RECOMP_COPY_DOLPHIN_NAND.Returns(copySetting); _settings.RECOMP_USE_DOLPHIN_DATA.Returns(useSetting); diff --git a/WheelWizard.Test/Features/Settings/DolphinSettingsTests.cs b/WheelWizard.Test/Features/Settings/DolphinSettingsTests.cs index df96506b7..01fde950f 100644 --- a/WheelWizard.Test/Features/Settings/DolphinSettingsTests.cs +++ b/WheelWizard.Test/Features/Settings/DolphinSettingsTests.cs @@ -10,7 +10,7 @@ public class DolphinSettingTests [Fact] public void Constructor_Throws_WhenFileNameIsNotIni() { - var action = () => new DolphinSetting(typeof(string), ("Dolphin.cfg", "General", "NANDRootPath"), "value"); + var action = () => new DolphinSetting(("Dolphin.cfg", "General", "NANDRootPath"), "value"); Assert.Throws(action); } @@ -18,8 +18,7 @@ public void Constructor_Throws_WhenFileNameIsNotIni() [Fact] public void SetFromString_ParsesEnumAndFormatsAsIntegerString() { - var setting = new DolphinSetting( - typeof(DolphinShaderCompilationMode), + var setting = new DolphinSetting( ("GFX.ini", "Settings", "ShaderCompilationMode"), DolphinShaderCompilationMode.Default ); @@ -34,9 +33,7 @@ public void SetFromString_ParsesEnumAndFormatsAsIntegerString() [Fact] public void Set_ReturnsFalseAndKeepsOldValue_WhenValidationFails() { - var setting = new DolphinSetting(typeof(int), ("GFX.ini", "Settings", "InternalResolution"), 1).SetValidation(value => - (int)value! >= 0 - ); + var setting = new DolphinSetting(("GFX.ini", "Settings", "InternalResolution"), 1).SetValidation(value => value >= 0); setting.Set(2); var result = setting.Set(-1); @@ -46,11 +43,12 @@ public void Set_ReturnsFalseAndKeepsOldValue_WhenValidationFails() } [Fact] - public void SetFromString_Throws_WhenTypeIsUnsupported() + public void SetFromString_ParsesTypedNumbers() { - var setting = new DolphinSetting(typeof(decimal), ("GFX.ini", "Settings", "Price"), 1m); + var setting = new DolphinSetting(("GFX.ini", "Settings", "Price"), 1m); - Assert.Throws(() => setting.SetFromString("3.14")); + Assert.True(setting.SetFromString("3.14")); + Assert.Equal(3.14m, setting.Value); } } @@ -67,7 +65,7 @@ public void LoadSettings_ReadsExistingValue_FromIniFile() fileSystem.Directory.CreateDirectory(configFolderPath); fileSystem.File.WriteAllLines(iniPath, ["[General]", "NANDRootPath = /persisted"]); var manager = new DolphinSettingManager(fileSystem); - var setting = new DolphinSetting(typeof(string), ("Dolphin.ini", "General", "NANDRootPath"), "/default"); + var setting = new DolphinSetting(("Dolphin.ini", "General", "NANDRootPath"), "/default"); manager.RegisterSetting(setting); manager.LoadSettings(configFolderPath); @@ -85,7 +83,7 @@ public void LoadSettings_UsesDefaultWithoutWriting_WhenIniEntryIsMissing() fileSystem.Directory.CreateDirectory(configFolderPath); fileSystem.File.WriteAllLines(iniPath, ["[General]", "OtherSetting = 1"]); var manager = new DolphinSettingManager(fileSystem); - var setting = new DolphinSetting(typeof(string), ("Dolphin.ini", "General", "NANDRootPath"), "/default"); + var setting = new DolphinSetting(("Dolphin.ini", "General", "NANDRootPath"), "/default"); manager.RegisterSetting(setting); manager.LoadSettings(configFolderPath); @@ -105,7 +103,7 @@ public void SaveSettings_UpdatesExistingSettingLine_InIniFile() fileSystem.Directory.CreateDirectory(configFolderPath); fileSystem.File.WriteAllLines(iniPath, ["[General]", "NANDRootPath = /old"]); var manager = new DolphinSettingManager(fileSystem); - var setting = new DolphinSetting(typeof(string), ("Dolphin.ini", "General", "NANDRootPath"), "/default"); + var setting = new DolphinSetting(("Dolphin.ini", "General", "NANDRootPath"), "/default"); manager.RegisterSetting(setting); manager.LoadSettings(configFolderPath); @@ -127,7 +125,7 @@ public void ReloadSettings_ReReadsFile_AfterItChangesOnDisk() fileSystem.Directory.CreateDirectory(configFolderPath); fileSystem.File.WriteAllLines(iniPath, ["[General]", "NANDRootPath = /first"]); var manager = new DolphinSettingManager(fileSystem); - var setting = new DolphinSetting(typeof(string), ("Dolphin.ini", "General", "NANDRootPath"), "/default"); + var setting = new DolphinSetting(("Dolphin.ini", "General", "NANDRootPath"), "/default"); manager.RegisterSetting(setting); manager.LoadSettings(configFolderPath); diff --git a/WheelWizard.Test/Features/Settings/SettingsPersistencePathTests.cs b/WheelWizard.Test/Features/Settings/SettingsPersistencePathTests.cs index baf0611d1..2eb207c45 100644 --- a/WheelWizard.Test/Features/Settings/SettingsPersistencePathTests.cs +++ b/WheelWizard.Test/Features/Settings/SettingsPersistencePathTests.cs @@ -17,7 +17,7 @@ public void ApplicationSettings_SaveToRequestedDestination_WithoutChangingOrigin const string originalJson = "{\"Volume\":12}"; fs.File.WriteAllText(original, originalJson); var manager = new WhWzSettingManager(NullLogger.Instance, fs); - var setting = new WhWzSetting(typeof(int), "Volume", 5); + var setting = new WhWzSetting("Volume", 5); manager.RegisterSetting(setting); manager.LoadSettings(original); @@ -43,7 +43,7 @@ public void DolphinSettings_ReloadAndSave_UseNewUserDirectory() fs.File.WriteAllText(firstFile, originalIni); fs.File.WriteAllText(secondFile, "[General]\nNANDRootPath = /second-nand\nOther = keep\n"); var manager = new DolphinSettingManager(fs); - var setting = new DolphinSetting(typeof(string), ("Dolphin.ini", "General", "NANDRootPath"), ""); + var setting = new DolphinSetting(("Dolphin.ini", "General", "NANDRootPath"), ""); manager.RegisterSetting(setting); manager.LoadSettings(first); @@ -72,7 +72,7 @@ public void RecompSettings_ReloadSaveAndRemove_PreserveOtherFileAndUnrelatedCont ["# keep comment", "[paths]", "nand_root = \"second\"", "other = true", "[video]", "show_fps = false"] ); var manager = new RecompSettingManager(fs); - var setting = new RecompSetting(typeof(string), ("paths", "nand_root"), "", _ => { }); + var setting = new RecompSetting(("paths", "nand_root"), "", _ => { }); manager.RegisterSetting(setting); manager.LoadSettings(first); @@ -93,7 +93,7 @@ public void RecompSettings_DoNotCreateBackendOwnedFile_WhenMissing() var fs = new MockFileSystem(); var configPath = fs.Path.GetFullPath("/missing/Config.toml"); var manager = new RecompSettingManager(fs); - var setting = new RecompSetting(typeof(bool), ("video", "show_fps"), false, _ => { }); + var setting = new RecompSetting(("video", "show_fps"), false, _ => { }); manager.RegisterSetting(setting); manager.LoadSettings(configPath); diff --git a/WheelWizard.Test/Features/Settings/SettingsRecoveryTests.cs b/WheelWizard.Test/Features/Settings/SettingsRecoveryTests.cs index 3061ad73a..bcd54e20e 100644 --- a/WheelWizard.Test/Features/Settings/SettingsRecoveryTests.cs +++ b/WheelWizard.Test/Features/Settings/SettingsRecoveryTests.cs @@ -14,8 +14,8 @@ public void DolphinEditPreservesExternalValuesAndReloadDoesNotCarryPreviousProfi 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); + var fps = new DolphinSetting(("GFX.ini", "Settings", "ShowFPS"), false, s => manager.SaveSettings("/config", s)); + var resolution = new DolphinSetting(("GFX.ini", "Settings", "InternalResolution"), 1); manager.RegisterSetting(fps); manager.RegisterSetting(resolution); manager.LoadSettings("/config"); @@ -39,7 +39,7 @@ public void RecompReloadMissingKeyUsesDefault() 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)); + var setting = new RecompSetting(("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"); @@ -54,8 +54,7 @@ public void RecompReloadMissingKeyUsesDefault() public void FailedSaveRollsBackAndCanBeRetried() { var fail = true; - var setting = new WhWzSetting( - typeof(bool), + var setting = new WhWzSetting( "Enabled", false, _ => @@ -84,7 +83,7 @@ public void JsonRetainsExistingKeysAndUnknownValuesAndPreservesCorruptFile() 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)); + var setting = new WhWzSetting("EnableAnimations", true, s => manager.SaveSettings(path, s)); manager.RegisterSetting(setting); manager.LoadSettings(path); Assert.Equal(false, setting.Get()); @@ -102,8 +101,8 @@ public void JsonRetainsExistingKeysAndUnknownValuesAndPreservesCorruptFile() [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); + var source = new WhWzSetting("source", 1); + using var derived = new VirtualSetting(_ => 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/SettingsTests.cs b/WheelWizard.Test/Features/Settings/SettingsTests.cs index 31fd9ec1b..3eda65fc6 100644 --- a/WheelWizard.Test/Features/Settings/SettingsTests.cs +++ b/WheelWizard.Test/Features/Settings/SettingsTests.cs @@ -22,11 +22,22 @@ public sealed class SettingsFeatureCollection; public class SettingsManagerTests { [Fact] - public void Get_Throws_WhenRequestedTypeDoesNotMatchSettingType() + public void Get_InfersTheTypeFromTheSetting() { var manager = CreateManager(new MockFileSystem(), out _, out _, out _); - Assert.Throws(() => manager.Get(manager.WW_LANGUAGE)); + string language = manager.Get(manager.WW_LANGUAGE); + Assert.Equal("en", language); + } + + [Fact] + public void SettingApiExposesOnlyItsDeclaredValueType() + { + var setter = typeof(Setting).GetMethod("Set")!; + Assert.Equal(typeof(bool), setter.GetParameters()[0].ParameterType); + Assert.Equal(typeof(bool), typeof(Setting).GetProperty("Value")!.PropertyType); + Assert.Null(typeof(Setting).GetMethod("Set")); + Assert.Null(typeof(Setting).GetMethod("Get")); } [Fact] @@ -181,7 +192,7 @@ public class SettingsSignalBusTests public void Publish_NotifiesActiveSubscribers() { var signalBus = SettingsTestUtils.CreateSettingsSignalBus(); - var setting = new WhWzSetting(typeof(int), "Volume", 10); + var setting = new WhWzSetting("Volume", 10); SettingChangedSignal? receivedSignal = null; using var _ = signalBus.Subscribe(signal => receivedSignal = signal); @@ -195,7 +206,7 @@ public void Publish_NotifiesActiveSubscribers() public void DisposeSubscription_StopsReceivingSignals() { var signalBus = SettingsTestUtils.CreateSettingsSignalBus(); - var setting = new WhWzSetting(typeof(int), "Volume", 10); + var setting = new WhWzSetting("Volume", 10); var receiveCount = 0; var subscription = signalBus.Subscribe(_ => receiveCount++); @@ -285,9 +296,9 @@ public void Initialize_SetsCurrentCulture_FromLanguageSetting() var originalLanguage = LocalizationProvider.Current.CurrentLanguage; var signalBus = SettingsTestUtils.CreateSettingsSignalBus(); var settingsManager = Substitute.For(); - var languageSetting = new WhWzSetting(typeof(string), "WW_Language", "fr"); + var languageSetting = new WhWzSetting("WW_Language", "fr"); settingsManager.WW_LANGUAGE.Returns(languageSetting); - settingsManager.Get(Arg.Any()).Returns(_ => (string)languageSetting.Get()); + settingsManager.Get(Arg.Any>()).Returns(_ => (string)languageSetting.Get()); var yamlLocalizationService = new EmbeddedYamlLocalizationService(); using var localizationService = new SettingsLocalizationService(settingsManager, signalBus, yamlLocalizationService); @@ -323,9 +334,9 @@ public void PublishLanguageSignal_UpdatesCulture_WhenLanguageChanges() var originalLanguage = LocalizationProvider.Current.CurrentLanguage; var signalBus = SettingsTestUtils.CreateSettingsSignalBus(); var settingsManager = Substitute.For(); - var languageSetting = new WhWzSetting(typeof(string), "WW_Language", "en"); + var languageSetting = new WhWzSetting("WW_Language", "en"); settingsManager.WW_LANGUAGE.Returns(languageSetting); - settingsManager.Get(Arg.Any()).Returns(_ => (string)languageSetting.Get()); + settingsManager.Get(Arg.Any>()).Returns(_ => (string)languageSetting.Get()); var yamlLocalizationService = new EmbeddedYamlLocalizationService(); using var localizationService = new SettingsLocalizationService(settingsManager, signalBus, yamlLocalizationService); @@ -438,17 +449,17 @@ public static string GetValidDolphinLocation(IFileSystem fileSystem) public static ISettingsManager CreateSettingsStub(string userFolderPath, string dolphinLocation = "dolphin-emu") { var settings = Substitute.For(); - var userFolderSetting = new WhWzSetting(typeof(string), "UserFolderPath", userFolderPath); - var dolphinLocationSetting = new WhWzSetting(typeof(string), "DolphinLocation", dolphinLocation); + var userFolderSetting = new WhWzSetting("UserFolderPath", userFolderPath); + var dolphinLocationSetting = new WhWzSetting("DolphinLocation", dolphinLocation); settings.USER_FOLDER_PATH.Returns(userFolderSetting); settings.DOLPHIN_LOCATION.Returns(dolphinLocationSetting); settings - .Get(Arg.Is(setting => ReferenceEquals(setting, userFolderSetting))) + .Get(Arg.Is>(setting => ReferenceEquals(setting, userFolderSetting))) .Returns(_ => (string)userFolderSetting.Get()); settings - .Get(Arg.Is(setting => ReferenceEquals(setting, dolphinLocationSetting))) + .Get(Arg.Is>(setting => ReferenceEquals(setting, dolphinLocationSetting))) .Returns(_ => (string)dolphinLocationSetting.Get()); return settings; diff --git a/WheelWizard.Test/Features/Settings/VirtualSettingsTests.cs b/WheelWizard.Test/Features/Settings/VirtualSettingsTests.cs index 475a1c7aa..a2ea05773 100644 --- a/WheelWizard.Test/Features/Settings/VirtualSettingsTests.cs +++ b/WheelWizard.Test/Features/Settings/VirtualSettingsTests.cs @@ -10,7 +10,7 @@ public class VirtualSettingTests public void Set_StoresValueAndInvokesSetter_WhenValueIsValid() { var backingValue = 1; - var setting = new VirtualSetting(typeof(int), value => backingValue = (int)value, () => backingValue); + var setting = new VirtualSetting(value => backingValue = (int)value, () => backingValue); var result = setting.Set(5); @@ -23,9 +23,7 @@ public void Set_StoresValueAndInvokesSetter_WhenValueIsValid() public void Set_ReturnsFalseAndKeepsOldValue_WhenValidationFails() { var backingValue = 2; - var setting = new VirtualSetting(typeof(int), value => backingValue = (int)value, () => backingValue).SetValidation(value => - (int)value! >= 0 - ); + var setting = new VirtualSetting(value => backingValue = (int)value, () => backingValue).SetValidation(value => value >= 0); var result = setting.Set(-1); @@ -37,8 +35,8 @@ public void Set_ReturnsFalseAndKeepsOldValue_WhenValidationFails() [Fact] public void SetDependencies_RecalculatesValue_WhenDependencySignalsChange() { - var dependency = new WhWzSetting(typeof(int), "Dependency", 1); - var setting = new VirtualSetting(typeof(int), _ => { }, () => (int)dependency.Get()).SetDependencies(dependency); + var dependency = new WhWzSetting("Dependency", 1); + var setting = new VirtualSetting(_ => { }, () => (int)dependency.Get()).SetDependencies(dependency); dependency.Set(7, skipSave: true); @@ -48,8 +46,8 @@ public void SetDependencies_RecalculatesValue_WhenDependencySignalsChange() [Fact] public void Dispose_StopsRecalculationWithoutAnyGlobalRuntime() { - var dependency = new WhWzSetting(typeof(int), "Dependency", 1); - var setting = new VirtualSetting(typeof(int), _ => { }, () => dependency.Get()).SetDependencies(dependency); + var dependency = new WhWzSetting("Dependency", 1); + var setting = new VirtualSetting(_ => { }, () => dependency.Get()).SetDependencies(dependency); setting.Dispose(); dependency.Set(7, skipSave: true); @@ -60,8 +58,8 @@ public void Dispose_StopsRecalculationWithoutAnyGlobalRuntime() [Fact] public void SetDependencies_Throws_WhenCalledTwice() { - var dependency = new WhWzSetting(typeof(int), "Dependency", 1); - var setting = new VirtualSetting(typeof(int), _ => { }, () => 1).SetDependencies(dependency); + var dependency = new WhWzSetting("Dependency", 1); + var setting = new VirtualSetting(_ => { }, () => 1).SetDependencies(dependency); Assert.Throws(() => setting.SetDependencies(dependency)); } diff --git a/WheelWizard.Test/Features/Settings/WhWzSettingsTests.cs b/WheelWizard.Test/Features/Settings/WhWzSettingsTests.cs index 16c91a58c..13d848f19 100644 --- a/WheelWizard.Test/Features/Settings/WhWzSettingsTests.cs +++ b/WheelWizard.Test/Features/Settings/WhWzSettingsTests.cs @@ -13,7 +13,7 @@ public class WhWzSettingTests public void Set_StoresValueAndCallsSaveAction_WhenValueIsValid() { var saveCalls = 0; - var setting = new WhWzSetting(typeof(int), "Volume", 10, _ => saveCalls++); + var setting = new WhWzSetting("Volume", 10, _ => saveCalls++); var result = setting.Set(20); @@ -25,7 +25,7 @@ public void Set_StoresValueAndCallsSaveAction_WhenValueIsValid() [Fact] public void Set_ReturnsFalseAndKeepsOldValue_WhenValidationFails() { - var setting = new WhWzSetting(typeof(int), "Volume", 10).SetValidation(value => (int)value! >= 0); + var setting = new WhWzSetting("Volume", 10).SetValidation(value => value >= 0); setting.Set(5); var result = setting.Set(-1); @@ -38,7 +38,7 @@ public void Set_ReturnsFalseAndKeepsOldValue_WhenValidationFails() public void Reset_AppliesDefaultValue_EvenIfDefaultDoesNotPassValidation() { var saveCalls = 0; - var setting = new WhWzSetting(typeof(int), "Threshold", 5, _ => saveCalls++).SetValidation(value => (int)value! >= 10); + var setting = new WhWzSetting("Threshold", 5, _ => saveCalls++).SetValidation(value => value >= 10); setting.Set(12); setting.Reset(); @@ -50,7 +50,7 @@ public void Reset_AppliesDefaultValue_EvenIfDefaultDoesNotPassValidation() [Fact] public void Set_NotifiesLaterHandlers_WhenAChangedHandlerThrows() { - var setting = new WhWzSetting(typeof(int), "Volume", 10); + var setting = new WhWzSetting("Volume", 10); Setting? received = null; setting.Changed += _ => throw new InvalidOperationException("Subscriber failed"); setting.Changed += changed => received = changed; @@ -64,7 +64,7 @@ public void Set_NotifiesLaterHandlers_WhenAChangedHandlerThrows() [Fact] public void Reset_RestoresValidation_WhenAChangedHandlerThrows() { - var setting = new WhWzSetting(typeof(int), "Threshold", 5).SetValidation(value => (int)value! >= 10); + var setting = new WhWzSetting("Threshold", 5).SetValidation(value => value >= 10); setting.Set(12); setting.Changed += _ => throw new InvalidOperationException("Subscriber failed"); @@ -80,8 +80,8 @@ public void Reset_RestoresValidation_WhenAChangedHandlerThrows() [InlineData(true)] public void Reset_RestoresForceSave_WhenSavingThrows(bool forceSave) { - var setting = new WhWzSetting(typeof(int), "Threshold", 5, _ => throw new IOException("Save failed")) - .SetValidation(value => (int)value! >= 10) + var setting = new WhWzSetting("Threshold", 5, _ => throw new IOException("Save failed")) + .SetValidation(value => value >= 10) .SetForceSave(forceSave); setting.Set(12, skipSave: true); @@ -95,8 +95,8 @@ public void Reset_RestoresForceSave_WhenSavingThrows(bool forceSave) [Fact] public void SetFromJson_ParsesEnumAndArrayValues() { - var enumSetting = new WhWzSetting(typeof(DayOfWeek), "Day", DayOfWeek.Monday); - var arraySetting = new WhWzSetting(typeof(string[]), "Names", Array.Empty()); + var enumSetting = new WhWzSetting("Day", DayOfWeek.Monday); + var arraySetting = new WhWzSetting("Names", Array.Empty()); using var enumDocument = JsonDocument.Parse("2"); using var arrayDocument = JsonDocument.Parse("[\"A\", \"B\"]"); @@ -110,12 +110,13 @@ public void SetFromJson_ParsesEnumAndArrayValues() } [Fact] - public void SetFromJson_Throws_WhenTypeIsUnsupported() + public void SetFromJson_ReadsTypedDecimalValues() { - var setting = new WhWzSetting(typeof(decimal), "Price", 1m); + var setting = new WhWzSetting("Price", 1m); using var document = JsonDocument.Parse("2"); - Assert.Throws(() => setting.SetFromJson(document.RootElement, skipSave: true)); + Assert.True(setting.SetFromJson(document.RootElement, skipSave: true)); + Assert.Equal(2m, setting.Value); } } @@ -128,8 +129,8 @@ public void LoadSettings_AppliesPersistedValues_ToRegisteredSettings() var fileSystem = new MockFileSystem(); var logger = Substitute.For>(); var manager = new WhWzSettingManager(logger, fileSystem); - var volume = new WhWzSetting(typeof(int), "Volume", 5).SetValidation(value => (int)value! >= 0); - var language = new WhWzSetting(typeof(string), "Language", "en"); + var volume = new WhWzSetting("Volume", 5).SetValidation(value => value >= 0); + var language = new WhWzSetting("Language", "en"); var configPath = fileSystem.Path.GetFullPath("/settings/config.json"); var configFolderPath = fileSystem.Path.GetDirectoryName(configPath)!; fileSystem.Directory.CreateDirectory(configFolderPath); @@ -149,7 +150,7 @@ public void LoadSettings_ResetsInvalidPersistedValues_ToDefaults() var fileSystem = new MockFileSystem(); var logger = Substitute.For>(); var manager = new WhWzSettingManager(logger, fileSystem); - var volume = new WhWzSetting(typeof(int), "Volume", 5).SetValidation(value => (int)value! >= 0); + var volume = new WhWzSetting("Volume", 5).SetValidation(value => value >= 0); var configPath = fileSystem.Path.GetFullPath("/settings/config.json"); var configFolderPath = fileSystem.Path.GetDirectoryName(configPath)!; fileSystem.Directory.CreateDirectory(configFolderPath); @@ -167,7 +168,7 @@ public void SaveSettings_PersistsRegisteredValues_AfterLoad() var fileSystem = new MockFileSystem(); var logger = Substitute.For>(); var manager = new WhWzSettingManager(logger, fileSystem); - var volume = new WhWzSetting(typeof(int), "Volume", 5); + var volume = new WhWzSetting("Volume", 5); var configPath = fileSystem.Path.GetFullPath("/settings/config.json"); manager.RegisterSetting(volume); @@ -185,8 +186,8 @@ public void RegisterSetting_IsIgnoredAfterLoad() var fileSystem = new MockFileSystem(); var logger = Substitute.For>(); var manager = new WhWzSettingManager(logger, fileSystem); - var registeredBeforeLoad = new WhWzSetting(typeof(int), "Volume", 1); - var ignoredAfterLoad = new WhWzSetting(typeof(string), "Future", "initial"); + var registeredBeforeLoad = new WhWzSetting("Volume", 1); + var ignoredAfterLoad = new WhWzSetting("Future", "initial"); var configPath = fileSystem.Path.GetFullPath("/settings/config.json"); manager.RegisterSetting(registeredBeforeLoad); diff --git a/WheelWizard.Test/Views/HomeDolphinLaunchTests.cs b/WheelWizard.Test/Views/HomeDolphinLaunchTests.cs index a053a9a79..1709e38b5 100644 --- a/WheelWizard.Test/Views/HomeDolphinLaunchTests.cs +++ b/WheelWizard.Test/Views/HomeDolphinLaunchTests.cs @@ -117,7 +117,7 @@ private sealed class Fixture public IDolphinVersionService Versions { get; } = Substitute.For(); public ILinuxDolphinInstaller Installer { get; } = Substitute.For(); public IDolphinLaunchPresentation Presentation { get; } = Substitute.For(); - public WhWzSetting GamePath { get; } = new(typeof(string), "GamePath", "/game.iso"); + public WhWzSetting GamePath { get; } = new("GamePath", "/game.iso"); public DolphinLaunchService Service { get; } public Fixture(bool windows = false, string command = "dolphin-emu") diff --git a/WheelWizard/Features/Settings/DolphinSettingManager.cs b/WheelWizard/Features/Settings/DolphinSettingManager.cs index 6aa0cdcb1..c8d7436cb 100644 --- a/WheelWizard/Features/Settings/DolphinSettingManager.cs +++ b/WheelWizard/Features/Settings/DolphinSettingManager.cs @@ -7,9 +7,9 @@ public class DolphinSettingManager(IFileSystem fileSystem) : IDolphinSettingMana { private readonly object _sync = new(); private bool _loaded; - private readonly List _settings = []; + private readonly List _settings = []; - public void RegisterSetting(DolphinSetting setting) + public void RegisterSetting(IDolphinSetting setting) { lock (_sync) { @@ -18,7 +18,7 @@ public void RegisterSetting(DolphinSetting setting) } } - public void SaveSettings(string configDirectory, DolphinSetting invokingSetting) + public void SaveSettings(string configDirectory, IDolphinSetting invokingSetting) { lock (_sync) { diff --git a/WheelWizard/Features/Settings/ISettingsServices.cs b/WheelWizard/Features/Settings/ISettingsServices.cs index 736cf20cf..5effc07c2 100644 --- a/WheelWizard/Features/Settings/ISettingsServices.cs +++ b/WheelWizard/Features/Settings/ISettingsServices.cs @@ -1,26 +1,27 @@ +using WheelWizard.Models.Enums; using WheelWizard.Settings.Types; namespace WheelWizard.Settings; public interface IWhWzSettingManager { - void RegisterSetting(WhWzSetting setting); - void SaveSettings(string configPath, WhWzSetting invokingSetting); + void RegisterSetting(IWhWzSetting setting); + void SaveSettings(string configPath, IWhWzSetting invokingSetting); void LoadSettings(string configPath); } public interface IDolphinSettingManager { - void RegisterSetting(DolphinSetting setting); - void SaveSettings(string configDirectory, DolphinSetting invokingSetting); + void RegisterSetting(IDolphinSetting setting); + void SaveSettings(string configDirectory, IDolphinSetting invokingSetting); void ReloadSettings(string configDirectory); void LoadSettings(string configDirectory); } public interface IRecompSettingManager { - void RegisterSetting(RecompSetting setting); - void SaveSettings(string configPath, RecompSetting invokingSetting); + void RegisterSetting(IRecompSetting setting); + void SaveSettings(string configPath, IRecompSetting invokingSetting); void ReloadSettings(string configPath); void LoadSettings(string configPath); @@ -33,45 +34,45 @@ public interface IRecompSettingManager public interface ISettingsProperties { - Setting USER_FOLDER_PATH { get; } - Setting DOLPHIN_LOCATION { get; } - Setting GAME_LOCATION { get; } - Setting FORCE_WIIMOTE { get; } - Setting LAUNCH_WITH_DOLPHIN { get; } - Setting LAUNCH_RR_ON_STARTUP { get; } - Setting ENABLE_RECOMP { get; } - Setting RECOMP_USE_DOLPHIN_DATA { get; } - Setting RECOMP_COPY_DOLPHIN_NAND { get; } - Setting PREFERS_MODS_ROW_VIEW { get; } - Setting USE_PATCHES_SYSTEM { get; } - Setting FOCUSED_USER { get; } - Setting ENABLE_ANIMATIONS { get; } - Setting TESTING_MODE_ENABLED { get; } - Setting SAVED_WINDOW_SCALE { get; } - Setting RR_REGION { get; } - Setting WW_LANGUAGE { get; } - Setting NAND_ROOT_PATH { get; } - Setting LOAD_PATH { get; } - Setting VSYNC { get; } - Setting INTERNAL_RESOLUTION { get; } - Setting SHOW_FPS { get; } - Setting GFX_BACKEND { get; } - Setting MACADDRESS { get; } - Setting WINDOW_SCALE { get; } - Setting RECOMMENDED_SETTINGS { get; } - Setting RECOMP_RESOLUTION_MULTIPLIER { get; } - Setting RECOMP_GRAPHICS_API { get; } - Setting RECOMP_SHOW_FPS { get; } - Setting RECOMP_PREVENT_STUTTERS { get; } - Setting RECOMP_NAND_ROOT { get; } + Setting USER_FOLDER_PATH { get; } + Setting DOLPHIN_LOCATION { get; } + Setting GAME_LOCATION { get; } + Setting FORCE_WIIMOTE { get; } + Setting LAUNCH_WITH_DOLPHIN { get; } + Setting LAUNCH_RR_ON_STARTUP { get; } + Setting ENABLE_RECOMP { get; } + Setting RECOMP_USE_DOLPHIN_DATA { get; } + Setting RECOMP_COPY_DOLPHIN_NAND { get; } + Setting PREFERS_MODS_ROW_VIEW { get; } + Setting USE_PATCHES_SYSTEM { get; } + Setting FOCUSED_USER { get; } + Setting ENABLE_ANIMATIONS { get; } + Setting TESTING_MODE_ENABLED { get; } + Setting SAVED_WINDOW_SCALE { get; } + Setting RR_REGION { get; } + Setting WW_LANGUAGE { get; } + Setting NAND_ROOT_PATH { get; } + Setting LOAD_PATH { get; } + Setting VSYNC { get; } + Setting INTERNAL_RESOLUTION { get; } + Setting SHOW_FPS { get; } + Setting GFX_BACKEND { get; } + Setting MACADDRESS { get; } + Setting WINDOW_SCALE { get; } + Setting RECOMMENDED_SETTINGS { get; } + Setting RECOMP_RESOLUTION_MULTIPLIER { get; } + Setting RECOMP_GRAPHICS_API { get; } + Setting RECOMP_SHOW_FPS { get; } + Setting RECOMP_PREVENT_STUTTERS { get; } + Setting RECOMP_NAND_ROOT { get; } } public interface ISettingsManager : ISettingsProperties { OperationResult ValidateCorePathSettings(); - T Get(Setting setting); - bool Set(Setting setting, T value, bool skipSave = false); + T Get(Setting setting); + bool Set(Setting setting, T value, bool skipSave = false); bool PathsSetupCorrectly(); bool DolphinPathsSetupCorrectly(); diff --git a/WheelWizard/Features/Settings/RecompSettingManager.cs b/WheelWizard/Features/Settings/RecompSettingManager.cs index 418b249b4..5fed8732d 100644 --- a/WheelWizard/Features/Settings/RecompSettingManager.cs +++ b/WheelWizard/Features/Settings/RecompSettingManager.cs @@ -12,9 +12,9 @@ public class RecompSettingManager(IFileSystem fileSystem) : IRecompSettingManage private readonly object _syncRoot = new(); private readonly object _fileIoSync = new(); private bool _loaded; - private readonly List _settings = []; + private readonly List _settings = []; - public void RegisterSetting(RecompSetting setting) + public void RegisterSetting(IRecompSetting setting) { lock (_syncRoot) { @@ -25,7 +25,7 @@ public void RegisterSetting(RecompSetting setting) } } - public void SaveSettings(string configPath, RecompSetting invokingSetting) + public void SaveSettings(string configPath, IRecompSetting invokingSetting) { lock (_syncRoot) { @@ -80,7 +80,7 @@ public void RemoveTomlSetting(string configPath, string section, string settingT public void LoadSettings(string configPath) { - List settingsSnapshot; + List settingsSnapshot; if (_loaded) return; diff --git a/WheelWizard/Features/Settings/SettingsManager.cs b/WheelWizard/Features/Settings/SettingsManager.cs index c5d1542e2..2bf9af833 100644 --- a/WheelWizard/Features/Settings/SettingsManager.cs +++ b/WheelWizard/Features/Settings/SettingsManager.cs @@ -25,10 +25,10 @@ public class SettingsManager : ISettingsManager, IDisposable private readonly IRuntimeEnvironment _environment; private bool IsFlatpakSandboxed => _environment.IsFlatpakSandboxed(_fileSystem); - private readonly Setting _dolphinCompilationMode; - private readonly Setting _dolphinCompileShadersAtStart; - private readonly Setting _dolphinSsaa; - private readonly Setting _dolphinMsaa; + private readonly Setting _dolphinCompilationMode; + private readonly Setting _dolphinCompileShadersAtStart; + private readonly Setting _dolphinSsaa; + private readonly Setting _dolphinMsaa; private bool _hasLoadedSettings; private double _internalScale = -1.0; @@ -77,7 +77,7 @@ IUnixCommandService commands // The Dolphin location setting is ignored with the Flatpak since it bundles a separate Dolphin return true; } - var pathOrCommand = value as string ?? string.Empty; + var pathOrCommand = value; if (string.IsNullOrWhiteSpace(pathOrCommand)) return IsRecompModeActive(); @@ -95,7 +95,7 @@ IUnixCommandService commands "", value => { - var userFolderPath = value as string ?? string.Empty; + var userFolderPath = value; if (string.IsNullOrWhiteSpace(userFolderPath)) return IsRecompModeActive(); if (!_fileSystem.Directory.Exists(userFolderPath)) @@ -205,36 +205,28 @@ IUnixCommandService commands } ); - GAME_LOCATION = RegisterWhWz("GameLocation", "", value => _fileSystem.File.Exists(value as string ?? string.Empty)); + GAME_LOCATION = RegisterWhWz("GameLocation", "", value => _fileSystem.File.Exists(value)); FORCE_WIIMOTE = RegisterWhWz("ForceWiimote", false); LAUNCH_WITH_DOLPHIN = RegisterWhWz("LaunchWithDolphin", false); LAUNCH_RR_ON_STARTUP = RegisterWhWz("LaunchRrOnStartup", false); PREFERS_MODS_ROW_VIEW = RegisterWhWz("PrefersModsRowView", true); USE_PATCHES_SYSTEM = RegisterWhWz("UsePatchesSystem", false); - FOCUSED_USER = RegisterWhWz("FavoriteUser", 0, value => (int)(value ?? -1) >= 0 && (int)(value ?? -1) < 4); + FOCUSED_USER = RegisterWhWz("FavoriteUser", 0, value => value >= 0 && value < 4); ENABLE_ANIMATIONS = RegisterWhWz("EnableAnimations", true); TESTING_MODE_ENABLED = RegisterWhWz("TestingModeEnabled", false); SAVED_WINDOW_SCALE = RegisterWhWz("WindowScale", 1.0, SettingValues.IsValidWindowScale); RR_REGION = RegisterWhWz("RR_Region", MarioKartWiiEnums.Regions.None); - WW_LANGUAGE = RegisterWhWz("WW_Language", "en", value => SettingValues.WhWzLanguages.ContainsKey((string)value!)); + WW_LANGUAGE = RegisterWhWz("WW_Language", "en", value => SettingValues.WhWzLanguages.ContainsKey(value)); #endregion #region Dolphin settings - NAND_ROOT_PATH = RegisterDolphin( - ("Dolphin.ini", "General", "NANDRootPath"), - "", - value => _fileSystem.Directory.Exists(value as string ?? string.Empty) - ); + NAND_ROOT_PATH = RegisterDolphin(("Dolphin.ini", "General", "NANDRootPath"), "", value => _fileSystem.Directory.Exists(value)); - LOAD_PATH = RegisterDolphin( - ("Dolphin.ini", "General", "LoadPath"), - "", - value => _fileSystem.Directory.Exists(value as string ?? string.Empty) - ); + LOAD_PATH = RegisterDolphin(("Dolphin.ini", "General", "LoadPath"), "", value => _fileSystem.Directory.Exists(value)); VSYNC = RegisterDolphin(("GFX.ini", "Hardware", "VSync"), false); - INTERNAL_RESOLUTION = RegisterDolphin(("GFX.ini", "Settings", "InternalResolution"), 1, value => (int)(value ?? -1) >= 0); + INTERNAL_RESOLUTION = RegisterDolphin(("GFX.ini", "Settings", "InternalResolution"), 1, value => value >= 0); SHOW_FPS = RegisterDolphin(("GFX.ini", "Settings", "ShowFPS"), false); GFX_BACKEND = RegisterDolphin(("Dolphin.ini", "Core", "GFXBackend"), SettingValues.GFXRenderers.Values.First()); @@ -245,7 +237,7 @@ IUnixCommandService commands _dolphinMsaa = RegisterDolphin( ("GFX.ini", "Settings", "MSAA"), "0x00000001", - value => (value?.ToString() ?? "") is "0x00000001" or "0x00000002" or "0x00000004" or "0x00000008" + value => (value) is "0x00000001" or "0x00000002" or "0x00000004" or "0x00000008" ); // Readonly settings @@ -266,27 +258,26 @@ IUnixCommandService commands #endregion #region Virtual settings - var windowScale = new VirtualSetting( - typeof(double), - value => _internalScale = (double)value!, + var windowScale = new VirtualSetting( + value => _internalScale = value, () => _internalScale == -1.0 ? SAVED_WINDOW_SCALE.Get() : _internalScale ); windowScale.SetValidation(SettingValues.IsValidWindowScale); WINDOW_SCALE = windowScale.SetDependencies(SAVED_WINDOW_SCALE); - RECOMMENDED_SETTINGS = new VirtualSetting( - typeof(bool), + RECOMMENDED_SETTINGS = new VirtualSetting( value => { - var newValue = (bool)value!; - _dolphinCompilationMode.Set( + var newValue = value; + SaveRecommended( + _dolphinCompilationMode, newValue ? DolphinShaderCompilationMode.HybridUberShaders : DolphinShaderCompilationMode.Default ); #if WINDOWS - _dolphinCompileShadersAtStart.Set(newValue); + SaveRecommended(_dolphinCompileShadersAtStart, newValue); #endif - _dolphinMsaa.Set(newValue ? "0x00000002" : "0x00000001"); - _dolphinSsaa.Set(false); + SaveRecommended(_dolphinMsaa, newValue ? "0x00000002" : "0x00000001"); + SaveRecommended(_dolphinSsaa, false); }, () => { @@ -308,58 +299,64 @@ IUnixCommandService commands } #endregion + private static void SaveRecommended(Setting setting, T value) + { + if (!setting.Set(value)) + throw new IOException($"Could not save {setting.Name}.", setting.SaveError); + } + #region Settings Properties - public Setting USER_FOLDER_PATH { get; } - public Setting DOLPHIN_LOCATION { get; } - public Setting GAME_LOCATION { get; } - public Setting FORCE_WIIMOTE { get; } - public Setting LAUNCH_WITH_DOLPHIN { get; } - public Setting LAUNCH_RR_ON_STARTUP { get; } - public Setting ENABLE_RECOMP { get; } - public Setting RECOMP_USE_DOLPHIN_DATA { get; } - public Setting RECOMP_COPY_DOLPHIN_NAND { get; } - public Setting PREFERS_MODS_ROW_VIEW { get; } - public Setting USE_PATCHES_SYSTEM { get; } - public Setting FOCUSED_USER { get; } - public Setting ENABLE_ANIMATIONS { get; } - public Setting TESTING_MODE_ENABLED { get; } - public Setting SAVED_WINDOW_SCALE { get; } - public Setting RR_REGION { get; } - public Setting WW_LANGUAGE { get; } - - public Setting NAND_ROOT_PATH { get; } - public Setting LOAD_PATH { get; } - public Setting VSYNC { get; } - public Setting INTERNAL_RESOLUTION { get; } - public Setting SHOW_FPS { get; } - public Setting GFX_BACKEND { get; } - public Setting MACADDRESS { get; } - public Setting WINDOW_SCALE { get; } - public Setting RECOMMENDED_SETTINGS { get; } - public Setting RECOMP_RESOLUTION_MULTIPLIER { get; } - public Setting RECOMP_GRAPHICS_API { get; } - public Setting RECOMP_SHOW_FPS { get; } - public Setting RECOMP_PREVENT_STUTTERS { get; } - public Setting RECOMP_NAND_ROOT { get; } + public Setting USER_FOLDER_PATH { get; } + public Setting DOLPHIN_LOCATION { get; } + public Setting GAME_LOCATION { get; } + public Setting FORCE_WIIMOTE { get; } + public Setting LAUNCH_WITH_DOLPHIN { get; } + public Setting LAUNCH_RR_ON_STARTUP { get; } + public Setting ENABLE_RECOMP { get; } + public Setting RECOMP_USE_DOLPHIN_DATA { get; } + public Setting RECOMP_COPY_DOLPHIN_NAND { get; } + public Setting PREFERS_MODS_ROW_VIEW { get; } + public Setting USE_PATCHES_SYSTEM { get; } + public Setting FOCUSED_USER { get; } + public Setting ENABLE_ANIMATIONS { get; } + public Setting TESTING_MODE_ENABLED { get; } + public Setting SAVED_WINDOW_SCALE { get; } + public Setting RR_REGION { get; } + public Setting WW_LANGUAGE { get; } + + public Setting NAND_ROOT_PATH { get; } + public Setting LOAD_PATH { get; } + public Setting VSYNC { get; } + public Setting INTERNAL_RESOLUTION { get; } + public Setting SHOW_FPS { get; } + public Setting GFX_BACKEND { get; } + public Setting MACADDRESS { get; } + public Setting WINDOW_SCALE { get; } + public Setting RECOMMENDED_SETTINGS { get; } + public Setting RECOMP_RESOLUTION_MULTIPLIER { get; } + public Setting RECOMP_GRAPHICS_API { get; } + public Setting RECOMP_SHOW_FPS { get; } + public Setting RECOMP_PREVENT_STUTTERS { get; } + public Setting RECOMP_NAND_ROOT { get; } #endregion #region Public API - public T Get(Setting setting) - { - // #todo: make setting keys generic so reading and writing the wrong value type fails at compile time. - var value = setting.Get(); - if (value is not T typedValue) - throw new InvalidOperationException($"Setting '{setting.Name}' does not match expected type '{typeof(T).Name}'."); - - return typedValue; - } + public T Get(Setting setting) => setting.Value; - public bool Set(Setting setting, T value, bool skipSave = false) + public bool Set(Setting setting, T value, bool skipSave = false) { if (value == null) throw new ArgumentNullException(nameof(value)); - return setting.Set(value, skipSave); + var previous = setting.Value; + if (!setting.Set(value, skipSave)) + return false; + if ( + !EqualityComparer.Default.Equals(previous, value) + && (ReferenceEquals(setting, USER_FOLDER_PATH) || ReferenceEquals(setting, DOLPHIN_LOCATION)) + ) + _dolphinSettingManager.ReloadSettings(_dolphinPaths.Resolve(DOLPHIN_LOCATION.Value, USER_FOLDER_PATH.Value).ConfigFolderPath); + return true; } public bool PathsSetupCorrectly() @@ -427,10 +424,9 @@ public void LoadSettings() #endregion #region Registration Helpers - private WhWzSetting RegisterWhWz(string name, T defaultValue, Func? validation = null) + private WhWzSetting RegisterWhWz(string name, T defaultValue, Func? validation = null) { - var setting = new WhWzSetting( - typeof(T), + var setting = new WhWzSetting( name, defaultValue!, setting => _whWzSettingManager.SaveSettings(_fileSystem.Path.Combine(_applicationData.DirectoryPath, "config.json"), setting) @@ -443,10 +439,9 @@ private WhWzSetting RegisterWhWz(string name, T defaultValue, Func((string, string, string) location, T defaultValue, Func? validation = null) + private DolphinSetting RegisterDolphin((string, string, string) location, T defaultValue, Func? validation = null) { - var setting = new DolphinSetting( - typeof(T), + var setting = new DolphinSetting( location, defaultValue!, setting => @@ -463,10 +458,9 @@ private DolphinSetting RegisterDolphin((string, string, string) location, T d return setting; } - private RecompSetting RegisterRecomp((string, string) location, T defaultValue, Func? validation = null) + private RecompSetting RegisterRecomp((string, string) location, T defaultValue, Func? validation = null) { - var setting = new RecompSetting( - typeof(T), + var setting = new RecompSetting( location, defaultValue!, setting => _recompSettingManager.SaveSettings(_recompPaths.ConfigFilePath, setting) diff --git a/WheelWizard/Features/Settings/Types/DolphinSetting.cs b/WheelWizard/Features/Settings/Types/DolphinSetting.cs index ace57169b..c6abcda45 100644 --- a/WheelWizard/Features/Settings/Types/DolphinSetting.cs +++ b/WheelWizard/Features/Settings/Types/DolphinSetting.cs @@ -2,89 +2,69 @@ namespace WheelWizard.Settings.Types; -public class DolphinSetting : Setting +public interface IDolphinSetting { - private readonly Action _saveAction; - - public string FileName { get; private set; } - public string Section { get; private set; } + string Name { get; } + string FileName { get; } + string Section { get; } + string GetStringValue(); + bool SetFromString(string value, bool skipSave = false); + void Reset(bool skipSave = false); +} - public DolphinSetting(Type type, (string, string, string) location, object defaultValue) - : this(type, location, defaultValue, _ => { }) { } +public class DolphinSetting : Setting, IDolphinSetting +{ + private readonly Action? _save; + public string FileName { get; } + public string Section { get; } - public DolphinSetting(Type type, (string, string, string) location, object defaultValue, Action saveAction) - : base(type, location.Item3, defaultValue) + public DolphinSetting((string File, string Section, string Key) location, T defaultValue, Action? saveAction = null) + : base(location.Key, defaultValue) { - _saveAction = saveAction ?? throw new ArgumentNullException(nameof(saveAction)); - FileName = location.Item1; - Section = location.Item2; - // name/key = location.Item3 - - // I rather not translate this message, makes it easier to check where a given error came from - if (!FileName.EndsWith(".ini")) - throw new ArgumentException( - $"FileName for dolphin setting '[{Section}]{Name}' must end with .ini (given file is '{FileName}')" - ); + if (!location.File.EndsWith(".ini", StringComparison.OrdinalIgnoreCase)) + throw new ArgumentException("Dolphin settings must use an .ini file."); + FileName = location.File; + Section = location.Section; + _save = saveAction; } - protected override bool SetInternal(object newValue, bool skipSave = false) + protected override void ApplyValue(bool skipSave) { - var oldValue = Value; - Value = newValue; - var newIsValid = SaveEvenIfNotValid || IsValid(); - if (newIsValid) - { - if (!skipSave) - _saveAction(this); - } - else - Value = oldValue; - - return newIsValid; + if (!skipSave) + _save?.Invoke(this); } - public override object Get() => Value; - - public override bool IsValid() => ValidationFunc == null || ValidationFunc(Value); + public string GetStringValue() => + typeof(T).IsEnum + ? Convert.ToInt32(Value, CultureInfo.InvariantCulture).ToString(CultureInfo.InvariantCulture) + : Convert.ToString(Value, CultureInfo.InvariantCulture) ?? string.Empty; - public new DolphinSetting SetValidation(Func validationFunc) + public bool SetFromString(string value, bool skipSave = false) { - base.SetValidation(validationFunc); - return this; + try + { + // Runtime conversion belongs at the untrusted file boundary, never at a setting call site. + if (typeof(T).IsEnum) + return int.TryParse(value, out var number) + && Enum.IsDefined(typeof(T), number) + && Set((T)Enum.ToObject(typeof(T), number), skipSave); + return Set((T)Convert.ChangeType(value, typeof(T), CultureInfo.InvariantCulture), skipSave); + } + catch (Exception exception) when (exception is FormatException or OverflowException or InvalidCastException) + { + return false; + } } - public new DolphinSetting SetForceSave(bool saveEvenIfNotValid) + public new DolphinSetting SetValidation(Func validation) { - base.SetForceSave(saveEvenIfNotValid); + base.SetValidation(validation); return this; } - public string GetStringValue() + public new DolphinSetting SetForceSave(bool enabled) { - if (ValueType.IsEnum) - return ((int)Value).ToString(); - - return Convert.ToString(Value, CultureInfo.InvariantCulture) ?? "null"; - } - - public bool SetFromString(string newValue, bool skipSave = false) - { - // That these are the only types currently supported does not mean that these are all the Dolphin settings types - // feel free to add more types if you find them - return ValueType switch - { - { } t when t == typeof(string) => Set(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}"), - }; + base.SetForceSave(enabled); + return this; } } diff --git a/WheelWizard/Features/Settings/Types/RecompSetting.cs b/WheelWizard/Features/Settings/Types/RecompSetting.cs index f09c72b32..f5a0b3f27 100644 --- a/WheelWizard/Features/Settings/Types/RecompSetting.cs +++ b/WheelWizard/Features/Settings/Types/RecompSetting.cs @@ -8,42 +8,30 @@ namespace WheelWizard.Settings.Types; /// in-game settings bar. Values are formatted exactly the way the runtime's own writer formats /// them: booleans bare and lowercase, strings double-quoted, numbers invariant. /// -public class RecompSetting : Setting +public interface IRecompSetting { - private readonly Action _saveAction; - - public string Section { get; } + string Name { get; } + string Section { get; } + string GetStringValue(); + bool SetFromString(string value, bool skipSave = false); + void Reset(bool skipSave = false); +} - public RecompSetting(Type type, (string Section, string Key) location, object defaultValue, Action saveAction) - : base(type, location.Key, defaultValue) - { - _saveAction = saveAction ?? throw new ArgumentNullException(nameof(saveAction)); - Section = location.Section; - } +public class RecompSetting((string Section, string Key) location, T defaultValue, Action saveAction) + : Setting(location.Key, defaultValue), + IRecompSetting +{ + public string Section { get; } = location.Section; - protected override bool SetInternal(object newValue, bool skipSave = false) + protected override void ApplyValue(bool skipSave) { - var oldValue = Value; - Value = newValue; - var newIsValid = SaveEvenIfNotValid || IsValid(); - if (newIsValid) - { - if (!skipSave) - _saveAction(this); - } - else - Value = oldValue; - - return newIsValid; + if (!skipSave) + saveAction(this); } - public override object Get() => Value; - - public override bool IsValid() => ValidationFunc == null || ValidationFunc(Value); - - public new RecompSetting SetValidation(Func validationFunc) + public new RecompSetting SetValidation(Func validation) { - base.SetValidation(validationFunc); + base.SetValidation(validation); return this; } @@ -70,13 +58,14 @@ public string GetStringValue() => public bool SetFromString(string tomlValue, bool skipSave = false) { var literal = tomlValue.Trim(); - return ValueType switch + return typeof(T) switch { - { } t when t == typeof(string) => Set(Unquote(literal), skipSave), - { } t when t == typeof(bool) => bool.TryParse(literal, out var flag) && Set(flag, skipSave), + { } t when t == typeof(string) => Set((T)(object)Unquote(literal), skipSave), + { } t when t == typeof(bool) => bool.TryParse(literal, out var flag) && Set((T)(object)flag, skipSave), { } t when t == typeof(double) => double.TryParse(literal, NumberStyles.Float, CultureInfo.InvariantCulture, out var number) - && Set(number, skipSave), - _ => throw new InvalidOperationException($"Unsupported type: {ValueType.Name}"), + && double.IsFinite(number) + && Set((T)(object)number, skipSave), + _ => throw new InvalidOperationException($"Unsupported type: {typeof(T).Name}"), }; } diff --git a/WheelWizard/Features/Settings/Types/Setting.cs b/WheelWizard/Features/Settings/Types/Setting.cs index b5b39d0a6..3beee03c9 100644 --- a/WheelWizard/Features/Settings/Types/Setting.cs +++ b/WheelWizard/Features/Settings/Types/Setting.cs @@ -2,60 +2,74 @@ namespace WheelWizard.Settings.Types; -public abstract class Setting +// The non-generic surface is only for change notifications and heterogeneous registries. +public abstract class Setting(string name) { + public string Name { get; } = name; public event Action? Changed; + public Exception? SaveError { get; protected set; } + public abstract bool IsValid(); + public abstract void Reset(bool skipSave = false); - protected Setting(Type type, string name, object defaultValue) + protected void SignalChange() { - Name = name; - DefaultValue = defaultValue; - Value = defaultValue; - ValueType = type; + if (Changed is not { } handlers) + return; + foreach (Action handler in handlers.GetInvocationList()) + { + try + { + handler(this); + } + catch (Exception exception) + { + Trace.TraceError($"A subscriber threw while handling a change to setting '{Name}': {exception}"); + } + } } +} - public string Name { get; protected set; } - public object DefaultValue { get; protected set; } - protected object Value { get; set; } - protected Func? ValidationFunc { get; set; } - protected bool SaveEvenIfNotValid { get; set; } - public Type ValueType { get; protected set; } - public Exception? SaveError { get; private set; } +public abstract class Setting(string name, T defaultValue) : Setting(name) +{ + public T DefaultValue { get; } = defaultValue; + public T Value { get; protected set; } = defaultValue; + protected bool SaveEvenIfNotValid { get; private set; } + private Func? _validation; + + public T Get() => Value; - public bool Set(object newValue, bool skipSave = false) + public bool Set(T newValue, bool skipSave = false) { + ArgumentNullException.ThrowIfNull(newValue); SaveError = null; - if (newValue.GetType() != ValueType) - return false; - - if (Value.Equals(newValue)) + if (EqualityComparer.Default.Equals(Value, newValue)) return true; + if (!SaveEvenIfNotValid && _validation?.Invoke(newValue) == false) + return false; - var previousValue = Value; - bool succeeded; + var previous = Value; + Value = newValue; try { - succeeded = SetInternal(newValue, skipSave); + ApplyValue(skipSave); } catch (Exception exception) when (exception is IOException or UnauthorizedAccessException) { - Value = previousValue; + Value = previous; SaveError = exception; return false; } - if (succeeded) - SignalChange(); - - return succeeded; + SignalChange(); + return true; } - protected abstract bool SetInternal(object newValue, bool skipSave = false); + protected abstract void ApplyValue(bool skipSave); - public abstract object Get(); + public override bool IsValid() => _validation?.Invoke(Value) ?? true; - public void Reset(bool skipSave = false) + public override void Reset(bool skipSave = false) { - var s = SaveEvenIfNotValid; + var previous = SaveEvenIfNotValid; SaveEvenIfNotValid = true; try { @@ -63,41 +77,19 @@ public void Reset(bool skipSave = false) } finally { - SaveEvenIfNotValid = s; + SaveEvenIfNotValid = previous; } } - public abstract bool IsValid(); - - public Setting SetValidation(Func validationFunc) + public Setting SetValidation(Func validation) { - ValidationFunc = validationFunc; + _validation = validation; return this; } - public Setting SetForceSave(bool saveEvenIfNotValid) + public Setting SetForceSave(bool enabled) { - SaveEvenIfNotValid = saveEvenIfNotValid; + SaveEvenIfNotValid = enabled; return this; } - - protected void SignalChange() - { - var handlers = Changed; - if (handlers is null) - return; - - foreach (Action handler in handlers.GetInvocationList()) - { - try - { - handler(this); - } - catch (Exception exception) - { - // A subscriber failure must not interrupt the mutation or delivery to other subscribers. - Trace.TraceError($"A subscriber threw while handling a change to setting '{Name}': {exception}"); - } - } - } } diff --git a/WheelWizard/Features/Settings/Types/SettingConstants.cs b/WheelWizard/Features/Settings/Types/SettingConstants.cs index ebf3f6473..021d9a90e 100644 --- a/WheelWizard/Features/Settings/Types/SettingConstants.cs +++ b/WheelWizard/Features/Settings/Types/SettingConstants.cs @@ -21,9 +21,9 @@ public static class SettingValues public static readonly double[] WindowScales = [0.7, 0.8, 0.9, 1.0, 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.8, 2]; - public static bool IsValidWindowScale(object? value) + public static bool IsValidWindowScale(double scale) { - return value is double scale && scale >= MinWindowScale && scale <= MaxWindowScale; + return scale >= MinWindowScale && scale <= MaxWindowScale; } public static readonly Dictionary GFXRenderers = new() //Display name, value diff --git a/WheelWizard/Features/Settings/Types/VirtualSetting.cs b/WheelWizard/Features/Settings/Types/VirtualSetting.cs index 638b81a76..c3f30a387 100644 --- a/WheelWizard/Features/Settings/Types/VirtualSetting.cs +++ b/WheelWizard/Features/Settings/Types/VirtualSetting.cs @@ -2,41 +2,27 @@ namespace WheelWizard.Settings.Types; -public class VirtualSetting : Setting, IDisposable +public class VirtualSetting : Setting, IDisposable { - private Setting[] _dependencies; - private readonly Action _setter; - private readonly Func _getter; + private Setting[] _dependencies = []; + private readonly Action _setter; + private readonly Func _getter; private bool _acceptsSignals = true; private bool _dependenciesAssigned; - public VirtualSetting(Type type, Action setter, Func getter) - : base(type, "virtual", getter()) + public VirtualSetting(Action setter, Func getter) + : base("virtual", getter()) { - _dependencies = []; _setter = setter; _getter = getter; } - protected override bool SetInternal(object newValue, bool skipSave = false) + protected override void ApplyValue(bool skipSave) { - // we don't use skipSave here since its a virtual setting, and so there is nothing to save _acceptsSignals = false; try { - var oldValue = Value; - Value = newValue; - var newIsValid = SaveEvenIfNotValid || IsValid(); - var succeeded = false; - if (newIsValid) - { - _setter(newValue); - succeeded = true; - } - else - Value = oldValue; - - return succeeded; + _setter(Value); } finally { @@ -44,13 +30,7 @@ protected override bool SetInternal(object newValue, bool skipSave = false) } } - public override object Get() => Value; - - // We dont have to constantly recalculate the value, since if they didn't change, the value is still the same - // and they only change when the dependencies change, or when the users sets a new value - public override bool IsValid() => ValidationFunc == null || ValidationFunc(Value); - - public VirtualSetting SetDependencies(params Setting[] dependencies) + public VirtualSetting SetDependencies(params Setting[] dependencies) { // I rather not translate this message, makes it easier to check where a given error came from if (_dependenciesAssigned) diff --git a/WheelWizard/Features/Settings/Types/WhWzSetting.cs b/WheelWizard/Features/Settings/Types/WhWzSetting.cs index 43a4c582e..4186f6448 100644 --- a/WheelWizard/Features/Settings/Types/WhWzSetting.cs +++ b/WheelWizard/Features/Settings/Types/WhWzSetting.cs @@ -2,74 +2,41 @@ namespace WheelWizard.Settings.Types; -public class WhWzSetting : Setting +public interface IWhWzSetting { - private readonly Action _saveAction; - - public WhWzSetting(Type type, string name, object defaultValue) - : this(type, name, defaultValue, _ => { }) { } - - public WhWzSetting(Type type, string name, object defaultValue, Action saveAction) - : base(type, name, defaultValue) - { - _saveAction = saveAction ?? throw new ArgumentNullException(nameof(saveAction)); - } + string Name { get; } + object? GetValue(); + bool SetFromJson(JsonElement value, bool skipSave = false); + void Reset(bool skipSave = false); +} - protected override bool SetInternal(object newValue, bool skipSave = false) +public class WhWzSetting(string name, T defaultValue, Action? saveAction = null) + : Setting(name, defaultValue), + IWhWzSetting +{ + protected override void ApplyValue(bool skipSave) { - var oldValue = Value; - Value = newValue; - var newIsValid = SaveEvenIfNotValid || IsValid(); - if (newIsValid) - { - if (!skipSave) - _saveAction(this); - } - else - Value = oldValue; - - return newIsValid; + if (!skipSave) + saveAction?.Invoke(this); } - public override object Get() => Value; + object? IWhWzSetting.GetValue() => Value; - public override bool IsValid() => ValidationFunc == null || ValidationFunc(Value); - - public new WhWzSetting SetValidation(Func validationFunc) + public bool SetFromJson(JsonElement value, bool skipSave = false) { - base.SetValidation(validationFunc); - return this; + var parsed = value.Deserialize(); + return parsed is not null && (!typeof(T).IsEnum || Enum.IsDefined(typeof(T), parsed)) && Set(parsed, skipSave); } - public new WhWzSetting SetForceSave(bool saveEvenIfNotValid) + public new WhWzSetting SetValidation(Func validation) { - base.SetForceSave(saveEvenIfNotValid); + base.SetValidation(validation); return this; } - public bool SetFromJson(JsonElement newValue, bool skipSave = false) - { - // Feel free to add more types if you find them - return ValueType switch - { - { } t when t == typeof(bool) => Set(newValue.GetBoolean(), skipSave), - { } t when t == typeof(int) => Set(newValue.GetInt32(), skipSave), - { } t when t == typeof(long) => Set(newValue.GetInt64(), skipSave), - { } t when t == typeof(float) => Set((float)newValue.GetDouble(), skipSave), - { } t when t == typeof(double) => Set(newValue.GetDouble(), skipSave), - { } t when t == typeof(string) => Set(newValue.GetString()!, skipSave), - { } t when t == typeof(DateTime) => Set(newValue.GetDateTime(), skipSave), - { IsEnum: true } t => Set(Enum.ToObject(t, newValue.GetInt32()), skipSave), - { IsArray: true } t => SetArray(newValue, t.GetElementType()!, skipSave), - _ => throw new InvalidOperationException($"Unsupported type: {ValueType.Name}"), - }; - } - - private bool SetArray(JsonElement value, Type elementType, bool skipSave = false) + public new WhWzSetting SetForceSave(bool enabled) { - var json = value.GetRawText().Trim('\0'); - var arrayType = Array.CreateInstance(elementType, 0).GetType(); - var array = (Array)JsonSerializer.Deserialize(json, arrayType)!; - return Set(array, skipSave); + base.SetForceSave(enabled); + return this; } } diff --git a/WheelWizard/Features/Settings/WhWzSettingManager.cs b/WheelWizard/Features/Settings/WhWzSettingManager.cs index 1b84198d1..8013ba2b2 100644 --- a/WheelWizard/Features/Settings/WhWzSettingManager.cs +++ b/WheelWizard/Features/Settings/WhWzSettingManager.cs @@ -10,10 +10,10 @@ public class WhWzSettingManager(ILogger logger, IFileSystem private readonly object _sync = new(); private bool _loaded; private Exception? _loadError; - private readonly Dictionary _settings = new(); + private readonly Dictionary _settings = new(); private readonly Dictionary _unknownSettings = new(); - public void RegisterSetting(WhWzSetting setting) + public void RegisterSetting(IWhWzSetting setting) { lock (_sync) { @@ -22,7 +22,7 @@ public void RegisterSetting(WhWzSetting setting) } } - public void SaveSettings(string configPath, WhWzSetting invokingSetting) + public void SaveSettings(string configPath, IWhWzSetting invokingSetting) { lock (_sync) { @@ -33,7 +33,7 @@ public void SaveSettings(string configPath, WhWzSetting invokingSetting) var values = _unknownSettings.ToDictionary(pair => pair.Key, pair => (object?)pair.Value); foreach (var (name, setting) in _settings) - values[name] = setting.Get(); + values[name] = setting.GetValue(); try { diff --git a/WheelWizard/Views/Layout.axaml.cs b/WheelWizard/Views/Layout.axaml.cs index dbfe085fa..181a0919b 100644 --- a/WheelWizard/Views/Layout.axaml.cs +++ b/WheelWizard/Views/Layout.axaml.cs @@ -176,7 +176,7 @@ private void OnSettingChanged(Setting setting) // Note that this method will also be called whenever the setting changes if (setting == SettingsService.WINDOW_SCALE || setting == SettingsService.SAVED_WINDOW_SCALE) { - var scaleFactor = GetUsableWindowScale((double)setting.Get()); + var scaleFactor = GetUsableWindowScale(SettingsService.WINDOW_SCALE.Value); CompleteGrid.Resources["SettingsRowGap"] = 2d / scaleFactor; Height = WindowHeight * scaleFactor; Width = WindowWidth * scaleFactor; diff --git a/WheelWizard/Views/Pages/Settings/OtherSettings.axaml.cs b/WheelWizard/Views/Pages/Settings/OtherSettings.axaml.cs index 29c84b83a..13748af66 100644 --- a/WheelWizard/Views/Pages/Settings/OtherSettings.axaml.cs +++ b/WheelWizard/Views/Pages/Settings/OtherSettings.axaml.cs @@ -81,12 +81,12 @@ private void RefreshRetroRewindVersion() private void ClickLaunchRrOnStartup(object? sender, RoutedEventArgs e) { - SettingsService.Set(SettingsService.LAUNCH_RR_ON_STARTUP, LaunchRrOnStartup.IsChecked == true); + SettingsEditing.Set(SettingsService, SettingsService.LAUNCH_RR_ON_STARTUP, LaunchRrOnStartup.IsChecked == true); } private void ClickEnableRecomp(object? sender, RoutedEventArgs e) { - SettingsService.Set(SettingsService.ENABLE_RECOMP, EnableRecomp.IsChecked == true); + SettingsEditing.Set(SettingsService, SettingsService.ENABLE_RECOMP, EnableRecomp.IsChecked == true); } private async void Reinstall_RetroRewind(object sender, RoutedEventArgs e) diff --git a/WheelWizard/Views/Pages/Settings/RecompSettings.axaml.cs b/WheelWizard/Views/Pages/Settings/RecompSettings.axaml.cs index 3a62a7542..f2a50fe2e 100644 --- a/WheelWizard/Views/Pages/Settings/RecompSettings.axaml.cs +++ b/WheelWizard/Views/Pages/Settings/RecompSettings.axaml.cs @@ -160,7 +160,7 @@ private void Resolution_OnChanged(object? sender, SelectionChangedEventArgs e) if (_loading || index < 0 || index >= RecompVideoConfig.ResolutionMultipliers.Count) return; - SettingsService.Set(SettingsService.RECOMP_RESOLUTION_MULTIPLIER, RecompVideoConfig.ResolutionMultipliers[index]); + SettingsEditing.Set(SettingsService, SettingsService.RECOMP_RESOLUTION_MULTIPLIER, RecompVideoConfig.ResolutionMultipliers[index]); } private void GraphicsApi_OnChanged(object? sender, SelectionChangedEventArgs e) @@ -169,7 +169,7 @@ private void GraphicsApi_OnChanged(object? sender, SelectionChangedEventArgs e) if (_loading || index < 0 || index >= RecompVideoConfig.OfferedGraphicsApis.Count) return; - SettingsService.Set(SettingsService.RECOMP_GRAPHICS_API, RecompVideoConfig.OfferedGraphicsApis[index]); + SettingsEditing.Set(SettingsService, SettingsService.RECOMP_GRAPHICS_API, RecompVideoConfig.OfferedGraphicsApis[index]); } private void ShowFps_OnChanged(object? sender, RoutedEventArgs e) @@ -177,7 +177,7 @@ private void ShowFps_OnChanged(object? sender, RoutedEventArgs e) if (_loading) return; - SettingsService.Set(SettingsService.RECOMP_SHOW_FPS, ShowFps.IsChecked == true); + SettingsEditing.Set(SettingsService, SettingsService.RECOMP_SHOW_FPS, ShowFps.IsChecked == true); } private void PreventStutters_OnChanged(object? sender, RoutedEventArgs e) @@ -185,7 +185,7 @@ private void PreventStutters_OnChanged(object? sender, RoutedEventArgs e) if (_loading) return; - SettingsService.Set(SettingsService.RECOMP_PREVENT_STUTTERS, PreventStutters.IsChecked == true); + SettingsEditing.Set(SettingsService, SettingsService.RECOMP_PREVENT_STUTTERS, PreventStutters.IsChecked == true); } #endregion diff --git a/WheelWizard/Views/Pages/Settings/SettingsEditing.cs b/WheelWizard/Views/Pages/Settings/SettingsEditing.cs new file mode 100644 index 000000000..7d4243949 --- /dev/null +++ b/WheelWizard/Views/Pages/Settings/SettingsEditing.cs @@ -0,0 +1,19 @@ +using WheelWizard.Settings; +using WheelWizard.Settings.Types; +using WheelWizard.Shared.MessageTranslations; + +namespace WheelWizard.Views.Pages.Settings; + +internal static class SettingsEditing +{ + public static bool Set(ISettingsManager settings, Setting setting, T value) + { + if (settings.Set(setting, value)) + return true; + if (setting.SaveError is { } error) + MessageTranslationHelper.ShowMessage(Fail(error)); + else + MessageTranslationHelper.ShowMessage(MessageTranslation.Warning_InvalidPathSettings); + return false; + } +} diff --git a/WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs b/WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs index 87c9e441d..117a2a260 100644 --- a/WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs +++ b/WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs @@ -51,12 +51,12 @@ public VideoSettings(ISettingsManager settingsService) private void ClickForceWiimote(object? sender, RoutedEventArgs e) { - SettingsService.Set(SettingsService.FORCE_WIIMOTE, DisableForce.IsChecked == true); + SettingsEditing.Set(SettingsService, SettingsService.FORCE_WIIMOTE, DisableForce.IsChecked == true); } private void ClickLaunchWithDolphinWindow(object? sender, RoutedEventArgs e) { - SettingsService.Set(SettingsService.LAUNCH_WITH_DOLPHIN, LaunchWithDolphin.IsChecked == true); + SettingsEditing.Set(SettingsService, SettingsService.LAUNCH_WITH_DOLPHIN, LaunchWithDolphin.IsChecked == true); } private void LoadSettings() @@ -91,22 +91,22 @@ private void ForceLoadSettings() private void ResolutionDropdown_OnSelectionChanged(object? sender, SelectionChangedEventArgs e) { if (ResolutionDropdown.SelectedIndex >= 0) - SettingsService.Set(SettingsService.INTERNAL_RESOLUTION, ResolutionDropdown.SelectedIndex + 1); + SettingsEditing.Set(SettingsService, SettingsService.INTERNAL_RESOLUTION, ResolutionDropdown.SelectedIndex + 1); } private void VSync_OnClick(object? sender, RoutedEventArgs e) { - SettingsService.Set(SettingsService.VSYNC, VSyncButton.IsChecked == true); + SettingsEditing.Set(SettingsService, SettingsService.VSYNC, VSyncButton.IsChecked == true); } private void Recommended_OnClick(object? sender, RoutedEventArgs e) { - SettingsService.Set(SettingsService.RECOMMENDED_SETTINGS, RecommendedButton.IsChecked == true); + SettingsEditing.Set(SettingsService, SettingsService.RECOMMENDED_SETTINGS, RecommendedButton.IsChecked == true); } private void ShowFPS_OnClick(object? sender, RoutedEventArgs e) { - SettingsService.Set(SettingsService.SHOW_FPS, ShowFPSButton.IsChecked == true); + SettingsEditing.Set(SettingsService, SettingsService.SHOW_FPS, ShowFPSButton.IsChecked == true); } private void RendererDropdown_OnSelectionChanged(object? sender, SelectionChangedEventArgs e) @@ -117,7 +117,7 @@ private void RendererDropdown_OnSelectionChanged(object? sender, SelectionChange if (SettingValues.GFXRenderers.TryGetValue(selectedDisplayName, out var actualValue)) { - SettingsService.Set(SettingsService.GFX_BACKEND, actualValue); + SettingsEditing.Set(SettingsService, SettingsService.GFX_BACKEND, actualValue); } else { diff --git a/WheelWizard/Views/Pages/Settings/WhWzSettings.axaml.cs b/WheelWizard/Views/Pages/Settings/WhWzSettings.axaml.cs index 604359ff3..fffa0a47d 100644 --- a/WheelWizard/Views/Pages/Settings/WhWzSettings.axaml.cs +++ b/WheelWizard/Views/Pages/Settings/WhWzSettings.axaml.cs @@ -350,22 +350,17 @@ private async void DolphinUserPathBrowse_OnClick(object sender, RoutedEventArgs } } - private async Task ApplyLocationSettingAsync(Setting setting, string path) + private async Task ApplyLocationSettingAsync(Setting setting, string path) { - var normalizedPath = - setting == SettingsService.USER_FOLDER_PATH ? path.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) : path; - var previousPath = (string)setting.Get(); + var normalizedPath = setting == SettingsService.USER_FOLDER_PATH ? Path.TrimEndingDirectorySeparator(path) : path; - if (!setting.Set(normalizedPath)) + if (!SettingsEditing.Set(SettingsService, setting, normalizedPath)) { - await MessageTranslationHelper.AwaitMessageAsync(MessageTranslation.Warning_InvalidPathSettings); UpdateLocationRows(); return false; } UpdateLocationRows(); - if (!string.Equals(previousPath, normalizedPath, StringComparison.Ordinal) && SettingsService.PathsSetupCorrectly()) - DolphinSettingsService.ReloadSettings(DolphinPaths.ConfigFolderPath); await MessageTranslationHelper.AwaitMessageAsync(MessageTranslation.Success_PathSettingsSaved); return true; @@ -739,7 +734,7 @@ private async void WindowScaleDropdown_OnSelectionChanged(object sender, Selecti WindowScaleDropdown.Items.Add(selectedItemText); WindowScaleDropdown.SelectedItem = selectedItemText; - if (!SettingsService.WINDOW_SCALE.Set(scale)) + if (!SettingsEditing.Set(SettingsService, SettingsService.WINDOW_SCALE, scale)) { WindowScaleDropdown.SelectedItem = ScaleToString((double)SettingsService.WINDOW_SCALE.Get()); _editingScale = false; @@ -769,7 +764,7 @@ private async void WindowScaleDropdown_OnSelectionChanged(object sender, Selecti var yesNoAnswer = await yesNoWindow.AwaitAnswer(); if (yesNoAnswer) - SettingsService.SAVED_WINDOW_SCALE.Set(SettingsService.WINDOW_SCALE.Get()); + SettingsEditing.Set(SettingsService, SettingsService.SAVED_WINDOW_SCALE, SettingsService.WINDOW_SCALE.Value); else { SettingsService.WINDOW_SCALE.Set(SettingsService.SAVED_WINDOW_SCALE.Get()); @@ -838,7 +833,7 @@ private async void WhWzLanguageDropdown_OnSelectionChanged(object? sender, Selec return; // We only want to change the setting if we really apply this change } - if (SettingsService.WW_LANGUAGE.Set(selectedLanguage.Key)) + if (SettingsEditing.Set(SettingsService, SettingsService.WW_LANGUAGE, selectedLanguage.Key)) { LocalizationService.ApplyCurrentLanguage(); RefreshLanguageDropdown(); @@ -848,5 +843,5 @@ private async void WhWzLanguageDropdown_OnSelectionChanged(object? sender, Selec } private void EnableAnimations_OnClick(object sender, RoutedEventArgs e) => - SettingsService.ENABLE_ANIMATIONS.Set(EnableAnimations.IsChecked == true); + SettingsEditing.Set(SettingsService, SettingsService.ENABLE_ANIMATIONS, EnableAnimations.IsChecked == true); } From 15a705036039817bb3edb88160b980e9f9f4df0a Mon Sep 17 00:00:00 2001 From: Dirk Date: Mon, 28 Sep 2026 22:43:42 +0200 Subject: [PATCH 3/6] 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 4f9808fd8..df96506b7 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 d98c9aee1..baf0611d1 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 000000000..3061ad73a --- /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 e550d2043..16c91a58c 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 cad5d1d9a..6aa0cdcb1 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 90b512bd6..418b249b4 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 000000000..9fe903718 --- /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 4bc673659..ace57169b 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 a838f42d0..b5b39d0a6 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 c47445ae7..638b81a76 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 545b833d4..1b84198d1 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 4/6] 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 31fd9ec1b..d207ca44a 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 3eb604f53..d2461d796 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 87c9e441d..7343b8796 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) From ccbbe062ce90691c6a5d12b682c6584b8b7244fc Mon Sep 17 00:00:00 2001 From: Dirk Date: Mon, 28 Sep 2026 22:52:25 +0200 Subject: [PATCH 5/6] Add cardinal plural categories and preserve legacy translations --- .../Features/Localization/PluralRulesTests.cs | 77 +++++++++++++++++++ .../Features/Settings/SettingsTests.cs | 4 +- .../Resources/Languages/plural-fixture.yml | 16 ++++ WheelWizard.Test/WheelWizard.Test.csproj | 3 + .../EmbeddedYamlLocalizationService.cs | 19 ++++- .../Localization/ILocalizationService.cs | 1 + .../Features/Localization/PluralRules.cs | 58 ++++++++++++++ .../Localization/TranslationFunctions.cs | 52 +++++++++++-- WheelWizard/Views/Layout.axaml.cs | 4 +- 9 files changed, 221 insertions(+), 13 deletions(-) create mode 100644 WheelWizard.Test/Features/Localization/PluralRulesTests.cs create mode 100644 WheelWizard.Test/Resources/Languages/plural-fixture.yml create mode 100644 WheelWizard/Features/Localization/PluralRules.cs diff --git a/WheelWizard.Test/Features/Localization/PluralRulesTests.cs b/WheelWizard.Test/Features/Localization/PluralRulesTests.cs new file mode 100644 index 000000000..39056ae3e --- /dev/null +++ b/WheelWizard.Test/Features/Localization/PluralRulesTests.cs @@ -0,0 +1,77 @@ +using System.Globalization; +using WheelWizard.Localization; + +namespace WheelWizard.Test.Features.Localization; + +[Collection("SettingsFeature")] +public class PluralRulesTests +{ + [Theory] + [InlineData("en", "1", PluralCategory.One)] + [InlineData("en", "1.0", PluralCategory.Other)] + [InlineData("en", "0", PluralCategory.Other)] + [InlineData("nl", "1", PluralCategory.One)] + [InlineData("de", "2", PluralCategory.Other)] + [InlineData("fi", "1.5", PluralCategory.Other)] + [InlineData("fr", "0", PluralCategory.One)] + [InlineData("fr", "1.5", PluralCategory.One)] + [InlineData("fr", "1000000", PluralCategory.Many)] + [InlineData("pt", "0", PluralCategory.One)] + [InlineData("pt-PT", "0", PluralCategory.Other)] + [InlineData("es", "1.0", PluralCategory.One)] + [InlineData("it", "1.0", PluralCategory.Other)] + [InlineData("it", "1000000", PluralCategory.Many)] + [InlineData("tr", "1.0", PluralCategory.One)] + [InlineData("ja", "1", PluralCategory.Other)] + [InlineData("ko", "2", PluralCategory.Other)] + [InlineData("cs", "3", PluralCategory.Few)] + [InlineData("cs", "1.0", PluralCategory.Many)] + [InlineData("ru", "21", PluralCategory.One)] + [InlineData("ru", "22", PluralCategory.Few)] + [InlineData("ru", "12", PluralCategory.Many)] + [InlineData("ru", "1.5", PluralCategory.Other)] + [InlineData("ru", "-22", PluralCategory.Few)] + [InlineData("pl", "21", PluralCategory.Many)] + [InlineData("pl", "22", PluralCategory.Few)] + [InlineData("pl", "12", PluralCategory.Many)] + public void SelectsCardinalCategory(string language, string count, PluralCategory expected) => + Assert.Equal(expected, PluralRules.Select(language, decimal.Parse(count, CultureInfo.InvariantCulture))); + + [Fact] + public void GlobalTranslationSupportsNamedAndPositionalCountsAndLanguageFallback() + { + var previous = LocalizationProvider.Current; + var service = new EmbeddedYamlLocalizationService(typeof(PluralRulesTests).Assembly); + LocalizationProvider.Use(service); + try + { + Assert.Equal("One item for Alex", TranslationFunctions.t("items", count: 1, "Alex")); + Assert.Equal("2 items for Alex", TranslationFunctions.t("items", 2, "Alex")); + Assert.Equal("Value 3", TranslationFunctions.t("plain", 3)); + service.SetLanguage("ru"); + Assert.Equal("ru few", TranslationFunctions.t("items", count: 22)); + Assert.Equal("English other", TranslationFunctions.t("fallback", count: 22)); + Assert.Equal("English other", TranslationFunctions.t("en.fallback", count: 0)); + } + finally + { + LocalizationProvider.Use(previous); + } + } + + [Fact] + public void LegacyNumericKeysRemainUnchanged() + { + var previous = LocalizationProvider.Current; + LocalizationProvider.Use(new EmbeddedYamlLocalizationService()); + try + { + Assert.Equal("1 day", TranslationFunctions.t_legacy("en.time.days.n", 1)); + Assert.Equal("2 days", TranslationFunctions.t_legacy("en.time.days.n", 2)); + } + finally + { + LocalizationProvider.Use(previous); + } + } +} diff --git a/WheelWizard.Test/Features/Settings/SettingsTests.cs b/WheelWizard.Test/Features/Settings/SettingsTests.cs index 0316dd016..716a2cb58 100644 --- a/WheelWizard.Test/Features/Settings/SettingsTests.cs +++ b/WheelWizard.Test/Features/Settings/SettingsTests.cs @@ -316,8 +316,8 @@ public void TranslationFunction_UsesSpecificNumberVariant_WhenItExists() { localizationService.SetLanguage("en"); - Assert.Equal("1 day", TranslationFunctions.t("time.days.n", 1)); - Assert.Equal("2 days", TranslationFunctions.t("time.days.n", 2)); + Assert.Equal("1 day", TranslationFunctions.t_legacy("time.days.n", 1)); + Assert.Equal("2 days", TranslationFunctions.t_legacy("time.days.n", 2)); } finally { diff --git a/WheelWizard.Test/Resources/Languages/plural-fixture.yml b/WheelWizard.Test/Resources/Languages/plural-fixture.yml new file mode 100644 index 000000000..b820e5b1a --- /dev/null +++ b/WheelWizard.Test/Resources/Languages/plural-fixture.yml @@ -0,0 +1,16 @@ +en: + items: + one: "One item for {$2}" + other: "{$1} items for {$2}" + fallback: + one: "English one" + other: "English other" + plain: "Value {$1}" +ru: + items: + one: "ru one" + few: "ru few" + many: "ru many" + other: "ru other" + fallback: + one: "ru one" diff --git a/WheelWizard.Test/WheelWizard.Test.csproj b/WheelWizard.Test/WheelWizard.Test.csproj index 191070897..a7fc24521 100644 --- a/WheelWizard.Test/WheelWizard.Test.csproj +++ b/WheelWizard.Test/WheelWizard.Test.csproj @@ -34,4 +34,7 @@ + + + diff --git a/WheelWizard/Features/Localization/EmbeddedYamlLocalizationService.cs b/WheelWizard/Features/Localization/EmbeddedYamlLocalizationService.cs index d0897b193..6f8de6f88 100644 --- a/WheelWizard/Features/Localization/EmbeddedYamlLocalizationService.cs +++ b/WheelWizard/Features/Localization/EmbeddedYamlLocalizationService.cs @@ -13,7 +13,7 @@ public sealed class EmbeddedYamlLocalizationService : ILocalizationService public EmbeddedYamlLocalizationService() : this(typeof(EmbeddedYamlLocalizationService).Assembly) { } - internal EmbeddedYamlLocalizationService(Assembly resourceAssembly) + public EmbeddedYamlLocalizationService(Assembly resourceAssembly) { _translations = LoadTranslations(resourceAssembly); if (_translations.Count == 0) @@ -49,6 +49,23 @@ public string Translate(string key) return TranslateForLanguage(key, CurrentLanguage); } + public string TranslatePlural(string key, decimal count, string? languageCode = null) + { + var language = NormalizeLanguage(languageCode ?? CurrentLanguage); + // Select again in English on fallback; a Russian 'few' must not request English 'few'. + foreach (var candidate in new[] { language, DefaultLanguage }.Distinct()) + { + var category = PluralRules.Select(candidate, count).ToString().ToLowerInvariant(); + if ( + TryGetValue(candidate, $"{key}.{category}", out var value) + || TryGetValue(candidate, $"{key}.other", out value) + || TryGetValue(candidate, key, out value) + ) + return value; + } + return key; + } + public string TranslateForLanguage(string key, string languageCode) { if (string.IsNullOrWhiteSpace(key)) diff --git a/WheelWizard/Features/Localization/ILocalizationService.cs b/WheelWizard/Features/Localization/ILocalizationService.cs index 85a49afe9..5c3540aff 100644 --- a/WheelWizard/Features/Localization/ILocalizationService.cs +++ b/WheelWizard/Features/Localization/ILocalizationService.cs @@ -7,6 +7,7 @@ public interface ILocalizationService void SetLanguage(string languageCode); string Translate(string key); + string TranslatePlural(string key, decimal count, string? languageCode = null); string TranslateForLanguage(string key, string languageCode); bool TryTranslateForLanguage(string key, string languageCode, out string value); bool HasLanguage(string languageCode); diff --git a/WheelWizard/Features/Localization/PluralRules.cs b/WheelWizard/Features/Localization/PluralRules.cs new file mode 100644 index 000000000..6b2d077bb --- /dev/null +++ b/WheelWizard/Features/Localization/PluralRules.cs @@ -0,0 +1,58 @@ +namespace WheelWizard.Localization; + +public enum PluralCategory +{ + Zero, + One, + Two, + Few, + Many, + Other, +} + +public static class PluralRules +{ + // CLDR 48 cardinal rules for the application's supported languages, for ordinary decimal counts (e = 0). + // https://www.unicode.org/cldr/charts/48/supplemental/language_plural_rules.html + // Decimal scale is significant: English 1 is 'one', while 1.0 is 'other'. + public static PluralCategory Select(string language, decimal count) + { + var n = Math.Abs(count); + var i = decimal.Truncate(n); + var v = (decimal.GetBits(count)[3] >> 16) & 0xff; + var locale = language.Replace('_', '-').ToLowerInvariant(); + var primary = locale.Split('-')[0]; + var million = v == 0 && i != 0 && i % 1_000_000 == 0; + return primary switch + { + "ja" or "ko" => PluralCategory.Other, + "fr" => i is 0 or 1 ? PluralCategory.One + : million ? PluralCategory.Many + : PluralCategory.Other, + "pt" => (locale == "pt-pt" ? i == 1 && v == 0 : i is 0 or 1) ? PluralCategory.One + : million ? PluralCategory.Many + : PluralCategory.Other, + "es" => n == 1 ? PluralCategory.One + : million ? PluralCategory.Many + : PluralCategory.Other, + "it" => i == 1 && v == 0 ? PluralCategory.One + : million ? PluralCategory.Many + : PluralCategory.Other, + "tr" => n == 1 ? PluralCategory.One : PluralCategory.Other, + "cs" => v != 0 ? PluralCategory.Many + : i == 1 ? PluralCategory.One + : i is >= 2 and <= 4 ? PluralCategory.Few + : PluralCategory.Other, + "ru" => v != 0 ? PluralCategory.Other + : i % 10 == 1 && i % 100 != 11 ? PluralCategory.One + : i % 10 is >= 2 and <= 4 && i % 100 is not (>= 12 and <= 14) ? PluralCategory.Few + : PluralCategory.Many, + "pl" => v != 0 ? PluralCategory.Other + : i == 1 ? PluralCategory.One + : i % 10 is >= 2 and <= 4 && i % 100 is not (>= 12 and <= 14) ? PluralCategory.Few + : PluralCategory.Many, + "en" or "nl" or "de" or "fi" => i == 1 && v == 0 ? PluralCategory.One : PluralCategory.Other, + _ => PluralCategory.Other, + }; + } +} diff --git a/WheelWizard/Features/Localization/TranslationFunctions.cs b/WheelWizard/Features/Localization/TranslationFunctions.cs index e000a2caf..4b87418fa 100644 --- a/WheelWizard/Features/Localization/TranslationFunctions.cs +++ b/WheelWizard/Features/Localization/TranslationFunctions.cs @@ -2,8 +2,44 @@ namespace WheelWizard.Localization; public static class TranslationFunctions { -#pragma warning disable IDE1006 // Naming Styles public static string t(string key, params object?[] args) + { + if (args.Length > 0 && TryCount(args[0], out var count)) + return TranslateCount(key, count, args); + var prefixed = TrySplitLanguageKey(key, out var language, out var translationKey); + return Format( + prefixed ? LocalizationProvider.TranslateForLanguage(translationKey, language) : LocalizationProvider.Translate(translationKey), + args + ); + } + + /// Count supplies {$1}; additional arguments supply {$2}, {$3}, etc. + public static string t(string key, decimal count, params object?[] args) => TranslateCount(key, count, [count, .. args]); + + private static string TranslateCount(string key, decimal count, object?[] args) + { + var prefixed = TrySplitLanguageKey(key, out var language, out var translationKey); + return Format(LocalizationProvider.Current.TranslatePlural(translationKey, count, prefixed ? language : null), args); + } + + private static bool TryCount(object? value, out decimal count) + { + count = 0; + if (value is not (sbyte or byte or short or ushort or int or uint or long or ulong or float or double or decimal)) + return false; + try + { + count = Convert.ToDecimal(value, System.Globalization.CultureInfo.InvariantCulture); + return true; + } + catch (OverflowException) + { + return false; + } + } + +#pragma warning disable IDE1006 // Naming Styles + public static string t_legacy(string key, params object?[] args) #pragma warning restore IDE1006 // Naming Styles { var hasLanguagePrefix = TrySplitLanguageKey(key, out var languageCode, out var translationKey); @@ -33,11 +69,11 @@ public static string tTime(TimeSpan timeSpan) { var days = timeSpan.Days; var hours = timeSpan.Hours; - var dayText = t("time.days.n", days); + var dayText = t_legacy("time.days.n", days); if (hours == 0) return dayText; - var hourText = t("time.hours.n", hours); + var hourText = t_legacy("time.hours.n", hours); return $"{dayText} {hourText}"; } @@ -45,11 +81,11 @@ public static string tTime(TimeSpan timeSpan) { var hours = timeSpan.Hours; var minutes = timeSpan.Minutes; - var hourText = t("time.hours.n", hours); + var hourText = t_legacy("time.hours.n", hours); if (minutes == 0) return hourText; - var minuteText = t("time.minutes.n", minutes); + var minuteText = t_legacy("time.minutes.n", minutes); return $"{hourText} {minuteText}"; } @@ -57,15 +93,15 @@ public static string tTime(TimeSpan timeSpan) { var minutes = timeSpan.Minutes; var seconds = timeSpan.Seconds; - var minuteText = t("time.minutes.n", minutes); + var minuteText = t_legacy("time.minutes.n", minutes); if (seconds == 0) return minuteText; - var secondText = t("time.seconds.n", seconds); + var secondText = t_legacy("time.seconds.n", seconds); return $"{minuteText} {secondText}"; } - return t("time.seconds.n", timeSpan.Seconds); + return t_legacy("time.seconds.n", timeSpan.Seconds); } private static string ResolveNumberVariant(string translationKey, string languageCode, object?[] args) diff --git a/WheelWizard/Views/Layout.axaml.cs b/WheelWizard/Views/Layout.axaml.cs index 181a0919b..766148899 100644 --- a/WheelWizard/Views/Layout.axaml.cs +++ b/WheelWizard/Views/Layout.axaml.cs @@ -263,7 +263,7 @@ public void UpdateFriendCount() { var friends = GameLicenseService.ActiveCurrentFriends; FriendsButton.BoxText = $"{friends.Count(friend => friend.IsOnline)}/{friends.Count}"; - FriendsButton.BoxTip = t("hover.friends_online.n", friends.Count(friend => friend.IsOnline)); + FriendsButton.BoxTip = t_legacy("hover.friends_online.n", friends.Count(friend => friend.IsOnline)); } public void UpdateSidebarProfile() @@ -287,7 +287,7 @@ public void UpdatePlayerAndRoomCount(LiveRoomsService sender) { var playerCount = sender.PlayerCount; RoomsButton.BoxText = playerCount.ToString(); - RoomsButton.BoxTip = t("hover.players_online.n", playerCount); + RoomsButton.BoxTip = t_legacy("hover.players_online.n", playerCount); UpdateFriendCount(); } From 1b5564aa78cc5a8f7c7a49984ce0b14a9f7c6f23 Mon Sep 17 00:00:00 2001 From: Dirk Date: Sun, 4 Oct 2026 12:22:45 +0200 Subject: [PATCH 6/6] make it just simple cardinal pluralization right now --- .../Features/Localization/PluralRulesTests.cs | 64 ++++++++++--------- WheelWizard.Test/Resources/Languages/pt.yml | 4 ++ .../EmbeddedYamlLocalizationService.cs | 3 +- .../Features/Localization/PluralRules.cs | 49 +------------- 4 files changed, 41 insertions(+), 79 deletions(-) create mode 100644 WheelWizard.Test/Resources/Languages/pt.yml diff --git a/WheelWizard.Test/Features/Localization/PluralRulesTests.cs b/WheelWizard.Test/Features/Localization/PluralRulesTests.cs index 39056ae3e..f2632dbc8 100644 --- a/WheelWizard.Test/Features/Localization/PluralRulesTests.cs +++ b/WheelWizard.Test/Features/Localization/PluralRulesTests.cs @@ -7,35 +7,15 @@ namespace WheelWizard.Test.Features.Localization; public class PluralRulesTests { [Theory] - [InlineData("en", "1", PluralCategory.One)] - [InlineData("en", "1.0", PluralCategory.Other)] - [InlineData("en", "0", PluralCategory.Other)] - [InlineData("nl", "1", PluralCategory.One)] - [InlineData("de", "2", PluralCategory.Other)] - [InlineData("fi", "1.5", PluralCategory.Other)] - [InlineData("fr", "0", PluralCategory.One)] - [InlineData("fr", "1.5", PluralCategory.One)] - [InlineData("fr", "1000000", PluralCategory.Many)] - [InlineData("pt", "0", PluralCategory.One)] - [InlineData("pt-PT", "0", PluralCategory.Other)] - [InlineData("es", "1.0", PluralCategory.One)] - [InlineData("it", "1.0", PluralCategory.Other)] - [InlineData("it", "1000000", PluralCategory.Many)] - [InlineData("tr", "1.0", PluralCategory.One)] - [InlineData("ja", "1", PluralCategory.Other)] - [InlineData("ko", "2", PluralCategory.Other)] - [InlineData("cs", "3", PluralCategory.Few)] - [InlineData("cs", "1.0", PluralCategory.Many)] - [InlineData("ru", "21", PluralCategory.One)] - [InlineData("ru", "22", PluralCategory.Few)] - [InlineData("ru", "12", PluralCategory.Many)] - [InlineData("ru", "1.5", PluralCategory.Other)] - [InlineData("ru", "-22", PluralCategory.Few)] - [InlineData("pl", "21", PluralCategory.Many)] - [InlineData("pl", "22", PluralCategory.Few)] - [InlineData("pl", "12", PluralCategory.Many)] - public void SelectsCardinalCategory(string language, string count, PluralCategory expected) => - Assert.Equal(expected, PluralRules.Select(language, decimal.Parse(count, CultureInfo.InvariantCulture))); + [InlineData("1", PluralCategory.One)] + [InlineData("1.0", PluralCategory.One)] + [InlineData("0", PluralCategory.Other)] + [InlineData("2", PluralCategory.Other)] + [InlineData("1.5", PluralCategory.Other)] + [InlineData("-1", PluralCategory.Other)] + [InlineData("21", PluralCategory.Other)] + public void SelectsSimpleCategory(string count, PluralCategory expected) => + Assert.Equal(expected, PluralRules.Select(decimal.Parse(count, CultureInfo.InvariantCulture))); [Fact] public void GlobalTranslationSupportsNamedAndPositionalCountsAndLanguageFallback() @@ -49,7 +29,7 @@ public void GlobalTranslationSupportsNamedAndPositionalCountsAndLanguageFallback Assert.Equal("2 items for Alex", TranslationFunctions.t("items", 2, "Alex")); Assert.Equal("Value 3", TranslationFunctions.t("plain", 3)); service.SetLanguage("ru"); - Assert.Equal("ru few", TranslationFunctions.t("items", count: 22)); + Assert.Equal("ru other", TranslationFunctions.t("items", count: 22)); Assert.Equal("English other", TranslationFunctions.t("fallback", count: 22)); Assert.Equal("English other", TranslationFunctions.t("en.fallback", count: 0)); } @@ -59,6 +39,30 @@ public void GlobalTranslationSupportsNamedAndPositionalCountsAndLanguageFallback } } + [Theory] + [InlineData("pt", "pt other")] + [InlineData("pt-BR", "pt other")] + [InlineData("pt-PT", "pt other")] + [InlineData("PT_pt", "pt other")] + public void GlobalTranslationUsesTheSameRuleForEveryRegion(string locale, string expected) + { + var previous = LocalizationProvider.Current; + var service = new EmbeddedYamlLocalizationService(typeof(PluralRulesTests).Assembly); + LocalizationProvider.Use(service); + try + { + Assert.Equal(expected, TranslationFunctions.t($"{locale}.items", count: 0)); + Assert.Equal("English other", TranslationFunctions.t($"{locale}.fallback", count: 0)); + Assert.Equal("pt one", TranslationFunctions.t($"{locale}.items", count: 1)); + service.SetLanguage(locale); + Assert.Equal(expected, TranslationFunctions.t("items", count: 0)); + } + finally + { + LocalizationProvider.Use(previous); + } + } + [Fact] public void LegacyNumericKeysRemainUnchanged() { diff --git a/WheelWizard.Test/Resources/Languages/pt.yml b/WheelWizard.Test/Resources/Languages/pt.yml new file mode 100644 index 000000000..197276ccf --- /dev/null +++ b/WheelWizard.Test/Resources/Languages/pt.yml @@ -0,0 +1,4 @@ +pt: + items: + one: "pt one" + other: "pt other" diff --git a/WheelWizard/Features/Localization/EmbeddedYamlLocalizationService.cs b/WheelWizard/Features/Localization/EmbeddedYamlLocalizationService.cs index 6f8de6f88..92e58b84d 100644 --- a/WheelWizard/Features/Localization/EmbeddedYamlLocalizationService.cs +++ b/WheelWizard/Features/Localization/EmbeddedYamlLocalizationService.cs @@ -52,10 +52,9 @@ public string Translate(string key) public string TranslatePlural(string key, decimal count, string? languageCode = null) { var language = NormalizeLanguage(languageCode ?? CurrentLanguage); - // Select again in English on fallback; a Russian 'few' must not request English 'few'. + var category = PluralRules.Select(count).ToString().ToLowerInvariant(); foreach (var candidate in new[] { language, DefaultLanguage }.Distinct()) { - var category = PluralRules.Select(candidate, count).ToString().ToLowerInvariant(); if ( TryGetValue(candidate, $"{key}.{category}", out var value) || TryGetValue(candidate, $"{key}.other", out value) diff --git a/WheelWizard/Features/Localization/PluralRules.cs b/WheelWizard/Features/Localization/PluralRules.cs index 6b2d077bb..a777eb52f 100644 --- a/WheelWizard/Features/Localization/PluralRules.cs +++ b/WheelWizard/Features/Localization/PluralRules.cs @@ -2,57 +2,12 @@ namespace WheelWizard.Localization; public enum PluralCategory { - Zero, One, - Two, - Few, - Many, Other, } public static class PluralRules { - // CLDR 48 cardinal rules for the application's supported languages, for ordinary decimal counts (e = 0). - // https://www.unicode.org/cldr/charts/48/supplemental/language_plural_rules.html - // Decimal scale is significant: English 1 is 'one', while 1.0 is 'other'. - public static PluralCategory Select(string language, decimal count) - { - var n = Math.Abs(count); - var i = decimal.Truncate(n); - var v = (decimal.GetBits(count)[3] >> 16) & 0xff; - var locale = language.Replace('_', '-').ToLowerInvariant(); - var primary = locale.Split('-')[0]; - var million = v == 0 && i != 0 && i % 1_000_000 == 0; - return primary switch - { - "ja" or "ko" => PluralCategory.Other, - "fr" => i is 0 or 1 ? PluralCategory.One - : million ? PluralCategory.Many - : PluralCategory.Other, - "pt" => (locale == "pt-pt" ? i == 1 && v == 0 : i is 0 or 1) ? PluralCategory.One - : million ? PluralCategory.Many - : PluralCategory.Other, - "es" => n == 1 ? PluralCategory.One - : million ? PluralCategory.Many - : PluralCategory.Other, - "it" => i == 1 && v == 0 ? PluralCategory.One - : million ? PluralCategory.Many - : PluralCategory.Other, - "tr" => n == 1 ? PluralCategory.One : PluralCategory.Other, - "cs" => v != 0 ? PluralCategory.Many - : i == 1 ? PluralCategory.One - : i is >= 2 and <= 4 ? PluralCategory.Few - : PluralCategory.Other, - "ru" => v != 0 ? PluralCategory.Other - : i % 10 == 1 && i % 100 != 11 ? PluralCategory.One - : i % 10 is >= 2 and <= 4 && i % 100 is not (>= 12 and <= 14) ? PluralCategory.Few - : PluralCategory.Many, - "pl" => v != 0 ? PluralCategory.Other - : i == 1 ? PluralCategory.One - : i % 10 is >= 2 and <= 4 && i % 100 is not (>= 12 and <= 14) ? PluralCategory.Few - : PluralCategory.Many, - "en" or "nl" or "de" or "fi" => i == 1 && v == 0 ? PluralCategory.One : PluralCategory.Other, - _ => PluralCategory.Other, - }; - } + // Every language uses the same simple rule, including decimal counts such as 1.0. + public static PluralCategory Select(decimal count) => count == 1 ? PluralCategory.One : PluralCategory.Other; }