task: Rewrite Settings loading and writing in to the DI Feature pattern#231
Merged
task: Rewrite Settings loading and writing in to the DI Feature pattern#231
Conversation
…not the goal of this branch but i had to clean up settings so it had to be moved away
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
WheelWizard/Features/WiiManagement/MiiManagement/MiiExtensions.cs (1)
68-87:⚠️ Potential issue | 🟡 MinorAdd defensive validation for MAC address parsing.
Settings.Get<string>(Settings.MACADDRESS)could return an empty, null, or malformed string. The subsequentSplit(':')andbyte.Parsecalls would throw unhandled exceptions (NullReferenceException,IndexOutOfRangeException, orFormatException).Consider returning a safe default (e.g.,
truefor global) when the MAC address is unavailable or invalid.🛡️ Proposed defensive handling
public static bool IsGlobal(this Mii self) { // If it has blue pants, then its definitely global if ((self.MiiId1 >> 5) == 0b110) return true; // But it can also be global if the mac address is not the same as your own address var macAddressString = Settings.Get<string>(Settings.MACADDRESS); + if (string.IsNullOrWhiteSpace(macAddressString)) + return true; // Assume global if MAC address is unavailable + var macParts = macAddressString.Split(':'); + if (macParts.Length != 6) + return true; // Assume global if MAC address format is invalid + var macBytes = new byte[6]; for (var i = 0; i < 6; i++) - macBytes[i] = byte.Parse(macParts[i], System.Globalization.NumberStyles.HexNumber); + { + if (!byte.TryParse(macParts[i], System.Globalization.NumberStyles.HexNumber, null, out macBytes[i])) + return true; // Assume global if MAC address byte is invalid + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@WheelWizard/Features/WiiManagement/MiiManagement/MiiExtensions.cs` around lines 68 - 87, The IsGlobal extension method currently assumes Settings.Get<string>(Settings.MACADDRESS) yields a valid MAC string and directly calls Split and byte.Parse; add defensive validation around the MAC retrieval and parsing (check for null/empty, ensure macParts.Length == 6, and catch FormatException/Overflow) and if validation fails return a safe default (e.g., true for global) to avoid NullReferenceException/IndexOutOfRangeException/FormatException; keep the existing logic using macBytes, systemId0, and comparisons against self.SystemId0..3 when parsing succeeds.WheelWizard/Views/Pages/Settings/OtherSettings.axaml.cs (1)
44-47: 🧹 Nitpick | 🔵 TrivialEmpty method body - consider removing or adding a TODO.
ForceLoadSettings()has an empty body with just a comment. If no settings need to be "force-loaded", consider removing this method entirely to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@WheelWizard/Views/Pages/Settings/OtherSettings.axaml.cs` around lines 44 - 47, ForceLoadSettings() is an empty method that confuses readers; either remove the method or implement/annotate it. Locate the ForceLoadSettings() method in OtherSettings.axaml.cs and if no forced-loading behavior is required delete the method and any callers; if you intend to implement it later add a clear TODO comment or a minimal implementation that documents expected behavior (e.g., call the existing settings load routine) so the purpose is explicit.WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs (1)
72-78:⚠️ Potential issue | 🟡 MinorAdd null/parse safety for
radioButton.Tag.
int.Parse(radioButton.Tag.ToString()!)assumes the Tag is always a valid integer string. If a Tag is misconfigured in XAML, this will throwFormatExceptionorNullReferenceException.🛡️ Proposed fix with safe parsing
private void UpdateResolution(object? sender, RoutedEventArgs e) { - if (sender is RadioButton radioButton && radioButton.IsChecked == true) + if (sender is RadioButton { IsChecked: true, Tag: { } tag } radioButton + && int.TryParse(tag.ToString(), out var resolution)) { - SettingsService.Set(SettingsService.INTERNAL_RESOLUTION, int.Parse(radioButton.Tag.ToString()!)); + SettingsService.Set(SettingsService.INTERNAL_RESOLUTION, resolution); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs` around lines 72 - 78, In UpdateResolution, avoid directly calling int.Parse on radioButton.Tag: first null-check radioButton.Tag and its ToString(), then use int.TryParse on the string and only call SettingsService.Set(SettingsService.INTERNAL_RESOLUTION, parsedValue) when TryParse succeeds; if parsing fails, bail out (or log) instead of throwing—this targets the UpdateResolution method and the use of SettingsService.INTERNAL_RESOLUTION and radioButton.Tag.WheelWizard/Features/Settings/DolphinSettingManager.cs (1)
137-168:⚠️ Potential issue | 🟠 MajorHandle missing INI files so settings can be persisted.
If the INI file doesn’t exist,
ChangeIniSettingsreturns early. That makesSaveSettingsa no-op for first-time files and prevents defaults from being created inLoadSettings. Consider creating an empty file (and its directory) when missing, while still bailing out on unreadable existing files.🛠️ Suggested fix
private void ChangeIniSettings(string fileName, string section, string settingToChange, string value) { - var lines = ReadIniFile(fileName)?.ToList(); - if (lines == null) - return; + var filePath = ConfigFolderPath(fileName); + var lines = ReadIniFile(fileName)?.ToList(); + if (lines == null) + { + if (fileSystem.File.Exists(filePath)) + return; // unreadable existing file; avoid clobbering + lines = []; + var directory = fileSystem.Path.GetDirectoryName(filePath); + if (!string.IsNullOrWhiteSpace(directory)) + fileSystem.Directory.CreateDirectory(directory); + } var sectionIndex = lines.IndexOf($"[{section}]"); if (sectionIndex == -1) { lines.Add($"[{section}]"); lines.Add($"{settingToChange} = {value}"); - fileSystem.File.WriteAllLines(ConfigFolderPath(fileName), lines); + fileSystem.File.WriteAllLines(filePath, lines); return; } @@ - fileSystem.File.WriteAllLines(ConfigFolderPath(fileName), lines); + fileSystem.File.WriteAllLines(filePath, lines); return; } @@ - fileSystem.File.WriteAllLines(ConfigFolderPath(fileName), lines); + fileSystem.File.WriteAllLines(filePath, lines); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@WheelWizard/Features/Settings/DolphinSettingManager.cs` around lines 137 - 168, ChangeIniSettings currently returns if ReadIniFile(fileName) is null, which prevents creating new INI files; update ChangeIniSettings to detect a missing file (ReadIniFile returns null because the file doesn't exist) and create the directory (using ConfigFolderPath(fileName) parent) and an empty file before proceeding so settings can be added and persisted, while still preserving the early return for unreadable/corrupt existing files; reference ReadIniFile, ChangeIniSettings, ConfigFolderPath(fileName), and fileSystem.File.WriteAllLines when implementing the creation logic so SaveSettings and LoadSettings can create first-time files but still bail on unreadable files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@WheelWizard.Test/Features/LinuxDolphinInstallerTests.cs`:
- Around line 19-27: Add a negative unit test to cover the non-happy path for
IsDolphinInstalledInFlatpak: create a test similar to the existing
IsDolphinInstalledInFlatpak_ReturnsTrue_WhenFlatpakInfoExitCodeIsZero but
arrange _processService.Run("flatpak", "info
org.DolphinEmu.dolphin-emu").Returns(Ok(1)), call
_installer.IsDolphinInstalledInFlatpak(), and Assert.False on the result so the
method’s false branch is exercised; reference the same test class and helpers
(IsDolphinInstalledInFlatpak, _processService.Run, Ok, _installer) when adding
the new test.
In `@WheelWizard.Test/Features/Settings/DolphinSettingsTests.cs`:
- Around line 61-78: The test LoadSettings_ReadsExistingValue_FromIniFile leaves
shared SettingsRuntime state set by SettingsTestUtils.InitializeSettingsRuntime,
so add deterministic cleanup to avoid cross-test pollution: modify the test
class or test to implement IDisposable or use a test fixture that calls a
SettingsTestUtils.ResetSettingsRuntime (or similar) after each test, ensuring
any static/runtime state initialized by InitializeSettingsRuntime is restored;
update the test class to either implement IDisposable and call the reset in
Dispose, or add a [TearDown]/[Fact cleanup] equivalent to invoke the reset so
parallel runs remain isolated.
In `@WheelWizard/Features/DolphinInstaller/DolphinInstallerExtensions.cs`:
- Around line 13-15: Register the Linux-specific services only when running on
Linux by wrapping the three AddSingleton calls (ILinuxCommandEnvironment ->
LinuxCommandEnvironment, ILinuxProcessService -> LinuxProcessService,
ILinuxDolphinInstaller -> LinuxDolphinInstaller) in a runtime platform check
using RuntimeInformation.IsOSPlatform(OSPlatform.Linux); update
DolphinInstallerExtensions.cs so the DI container only registers these
implementations when the check passes, making the platform dependency explicit
and avoiding unconditional registrations.
In `@WheelWizard/Features/DolphinInstaller/LinuxProcessService.cs`:
- Around line 15-38: The Run method currently sets RedirectStandardOutput and
RedirectStandardError to true but never reads them, risking a deadlock when
WaitForExit() is called; modify the ProcessStartInfo in Run (the
ProcessStartInfo object creation) to disable redirection by removing or setting
RedirectStandardOutput = false and RedirectStandardError = false (or simply omit
those properties) so the process won't block, leaving UseShellExecute and
CreateNoWindow as-is; alternatively, if capturing output is required later,
replace the redirection with async reads on process.StandardOutput/StandardError
before calling WaitForExit.
In `@WheelWizard/Features/Settings/ADDING_SETTINGS.md`:
- Around line 4-57: Fix typos and grammar in ADDING_SETTINGS.md: correct
misspellings (e.g., "settigns" → "settings", "writeing" → "writing"), fix casing
and punctuation (capitalize "Dolphin" sentences, remove stray "="), correct
pluralization and article usage, and improve phrasing for clarity in sections
that reference ISettingsServices.cs, ISettingsProperties, SettingsManager.cs,
RegisterWhWz, RegisterDolphin, VirtualSetting, SetDependencies,
SettingsManager.Get and SettingsManager.Set so examples and descriptions read
cleanly and consistently.
- Around line 1-53: Add missing blank lines around Markdown headings and fenced
code blocks to satisfy MD022/MD031: ensure there is an empty line before and
after each top-level and subsection heading (e.g., "## Setting Types", "##
Adding settings", "### Wheel Wizard", "### Dolphin", "### Virtual", "##
Reading/Writing settings") and around every fenced C# code block shown (the
snippets that reference ISettingsServices.cs / ISettingsProperties, the
SettingsManager.cs property, the RegisterWhWz/RegisterDolphin examples, the
VirtualSetting example, and the final reading/writing snippet). Update the
document so each triple-backtick fence and each heading has a blank line above
and below it, and fix small typos in adjacent sentences (e.g., "settigns" ->
"settings", "than" -> "then") while preserving the existing examples and
identifiers like ISettingsProperties, MY_NEW_SETTING, SettingsManager,
RegisterWhWz, RegisterDolphin, and VirtualSetting.
In `@WheelWizard/Features/Settings/DolphinSettingManager.cs`:
- Around line 60-73: The LoadSettings method currently sets _loaded inside the
lock before checking fileSystem.Directory.Exists(PathManager.ConfigFolderPath),
causing LoadSettings to be a no-op if the config folder is missing; move the
assignment of _loaded so it happens only after the directory existence check
(and after any successful loading of _settings or copying into
settingsSnapshot), i.e. keep the lock around reading _settings/_syncRoot but
defer setting _loaded to after the Directory.Exists check (or after actual load
completes); ensure ReloadSettings still clears _loaded as before so future
attempts can re-run LoadSettings.
In `@WheelWizard/Features/Settings/SettingsLocalizationService.cs`:
- Around line 31-36: ApplyCulture currently constructs a CultureInfo from
settingsManager.Get<string>(settingsManager.WW_LANGUAGE) which can throw
CultureNotFoundException for invalid/malformed codes; wrap the construction and
assignment in a try/catch that catches CultureNotFoundException (and optionally
FormatException) and falls back to a safe default culture (e.g.,
CultureInfo.InvariantCulture or CultureInfo.CurrentCulture) while logging or
handling the error via settingsManager or an injected logger; ensure
CultureInfo.CurrentCulture and CultureInfo.CurrentUICulture are only set on
success or set to the chosen fallback in the catch block to avoid startup
crashes.
- Around line 5-11: SettingsLocalizationService holds a subscription in the
_subscription field but doesn't implement IDisposable, causing a leak; implement
IDisposable on the SettingsLocalizationService class and add a Dispose (or
DisposeAsync if preferred) that locks on _syncRoot, checks
_initialized/_subscription, and calls _subscription.Dispose() (then sets
_subscription = null and _initialized = false) to unsubscribe from
settingsSignalBus; ensure the constructor/activation path that assigns
_subscription (via settingsSignalBus) remains unchanged but is cleaned up in the
new Dispose implementation.
In `@WheelWizard/Features/Settings/SettingsManager.cs`:
- Around line 65-97: The portable "user" directory check inside the
USER_FOLDER_PATH validator uses Directory.Exists("user") which checks the
process CWD instead of the Dolphin exe directory; update the validator (in the
RegisterWhWz lambda that sets USER_FOLDER_PATH) to compute the Dolphin
executable dir once (via PathManager.GetDolphinExeDirectory()) and check
_fileSystem.Directory.Exists(Path.Combine(dolphinExeDir, "user")) instead of
Directory.Exists("user") so the check targets the Dolphin installation directory
consistent with the portable.txt check.
In `@WheelWizard/Features/Settings/SettingsRuntime.cs`:
- Around line 27-33: Add a null check in SettingsRuntime.Initialize to mirror
SettingsSignalRuntime.Initialize: call
ArgumentNullException.ThrowIfNull(settingsManager) at the start of
SettingsRuntime.Initialize before acquiring the lock on SyncRoot, then proceed
to assign _current = settingsManager inside the lock so you never silently set
_current to null (matches the validation pattern used in
SettingsSignalRuntime.Initialize).
In `@WheelWizard/Features/Settings/SettingsSignalBus.cs`:
- Around line 33-46: The Publish method currently invokes subscriber handlers
directly, so a thrown exception in any handler stops delivery to later
subscribers; modify Publish in SettingsSignalBus to iterate the captured
handlers array and invoke each handler inside a try-catch that catches
Exception, logs or records the error (using your existing logger or an
appropriate mechanism) and continues to the next handler; keep the lock on
_syncRoot only for copying _subscribers.Values to handlers and create the
SettingChangedSignal(setting) once as shown so failed handlers do not prevent
others from receiving the signal.
In `@WheelWizard/Features/Settings/SettingsStartupInitializer.cs`:
- Around line 12-24: The Initialize method in SettingsStartupInitializer should
guard against exceptions from SettingsSignalRuntime.Initialize,
SettingsRuntime.Initialize, settingsManager.LoadSettings, and
localizationService.Initialize so startup doesn't crash; wrap the initialization
sequence (those calls) in a try/catch around
SettingsStartupInitializer.Initialize, catch Exception ex, call
logger.LogError(ex, "Startup initialization failed: {Message}", ex.Message) and
return early on error, then proceed to the existing ValidateCorePathSettings
logic; keep the existing Result-based failure handling unchanged.
In `@WheelWizard/Features/Settings/Types/DolphinSetting.cs`:
- Around line 10-18: The constructor DolphinSetting(Type type, (string, string,
string) location, object defaultValue, Action<DolphinSetting> saveAction) should
defensively guard against a null saveAction before assigning to the field
_saveAction; validate saveAction (the parameter) and if null replace it with a
no-op Action<DolphinSetting> (e.g., _ => { }) or throw an ArgumentNullException
depending on desired behavior, then assign FileName and Section as before so
later invocations of _saveAction won't NRE.
In `@WheelWizard/Features/Settings/Types/WhWzSetting.cs`:
- Around line 12-16: Constructor WhWzSetting currently assigns the saveAction
parameter to _saveAction without validating it, which can cause a
NullReferenceException when SetInternal calls _saveAction(this); modify the
WhWzSetting(Type, string, object, Action<WhWzSetting>) constructor to validate
saveAction and throw an ArgumentNullException(nameof(saveAction)) if it's null,
then assign it to _saveAction so SetInternal can safely invoke
_saveAction(this).
---
Outside diff comments:
In `@WheelWizard/Features/Settings/DolphinSettingManager.cs`:
- Around line 137-168: ChangeIniSettings currently returns if
ReadIniFile(fileName) is null, which prevents creating new INI files; update
ChangeIniSettings to detect a missing file (ReadIniFile returns null because the
file doesn't exist) and create the directory (using ConfigFolderPath(fileName)
parent) and an empty file before proceeding so settings can be added and
persisted, while still preserving the early return for unreadable/corrupt
existing files; reference ReadIniFile, ChangeIniSettings,
ConfigFolderPath(fileName), and fileSystem.File.WriteAllLines when implementing
the creation logic so SaveSettings and LoadSettings can create first-time files
but still bail on unreadable files.
In `@WheelWizard/Features/WiiManagement/MiiManagement/MiiExtensions.cs`:
- Around line 68-87: The IsGlobal extension method currently assumes
Settings.Get<string>(Settings.MACADDRESS) yields a valid MAC string and directly
calls Split and byte.Parse; add defensive validation around the MAC retrieval
and parsing (check for null/empty, ensure macParts.Length == 6, and catch
FormatException/Overflow) and if validation fails return a safe default (e.g.,
true for global) to avoid
NullReferenceException/IndexOutOfRangeException/FormatException; keep the
existing logic using macBytes, systemId0, and comparisons against
self.SystemId0..3 when parsing succeeds.
In `@WheelWizard/Views/Pages/Settings/OtherSettings.axaml.cs`:
- Around line 44-47: ForceLoadSettings() is an empty method that confuses
readers; either remove the method or implement/annotate it. Locate the
ForceLoadSettings() method in OtherSettings.axaml.cs and if no forced-loading
behavior is required delete the method and any callers; if you intend to
implement it later add a clear TODO comment or a minimal implementation that
documents expected behavior (e.g., call the existing settings load routine) so
the purpose is explicit.
In `@WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs`:
- Around line 72-78: In UpdateResolution, avoid directly calling int.Parse on
radioButton.Tag: first null-check radioButton.Tag and its ToString(), then use
int.TryParse on the string and only call
SettingsService.Set(SettingsService.INTERNAL_RESOLUTION, parsedValue) when
TryParse succeeds; if parsing fails, bail out (or log) instead of throwing—this
targets the UpdateResolution method and the use of
SettingsService.INTERNAL_RESOLUTION and radioButton.Tag.
patchzyy
approved these changes
Feb 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
rewote Settings to a Dependency Injection Feature.
ReadingWritingSubscribingandUnsubscribingbits. We don't have threads, however, we do use Tasks and other stuff, therefore you still can get race conditions. so this is just an prevention for that just in caserewrote DolphinFlatpackInstraller in to a feature
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores