diff --git a/WitcherScriptMerger.Core/AppSettings.cs b/WitcherScriptMerger.Core/AppSettings.cs index 97fab67..c0d56cb 100644 --- a/WitcherScriptMerger.Core/AppSettings.cs +++ b/WitcherScriptMerger.Core/AppSettings.cs @@ -1,6 +1,9 @@ using System; using System.Configuration; +using System.IO; +using System.Linq; using System.Reflection; +using System.Xml.Linq; namespace WitcherScriptMerger { @@ -52,8 +55,9 @@ public static string GetEnvironmentOverride(string key) } // Resolves a key's raw string value: an environment-variable override first, then - // falling through to the existing ConfigurationManager-backed lookup. Both Get and - // Get route through this single place so an env-var-sourced value goes through + // the existing ConfigurationManager-backed lookup, then - only when that yields + // nothing usable - the Vortex-managed sidecar (see VortexSidecarFileName). Both Get + // and Get route through this single place so a value from any source goes through // the exact same downstream handling (Get's Parse-based conversion in particular) // as one read from App.config - never a separate ad-hoc parser. string GetRawValue(string key) @@ -63,12 +67,99 @@ string GetRawValue(string key) return envValue; if (CachedConfig.HasFile) - return CachedConfig.AppSettings.Settings[key].Value; + { + // Null-conditional, not a bare .Value: Settings[key] returns null for a key + // that isn't in App.config at all, which used to throw here and get swallowed + // by Get/Get's catch. Returning null instead is observably identical for + // both of those (empty string / default(T)), and it lets a key that exists + // ONLY in the sidecar still be found below rather than dying first. + var value = CachedConfig.AppSettings.Settings[key]?.Value; + if (!string.IsNullOrWhiteSpace(value)) + return value; + + // Blank in our own config. For the path settings that ship blank on purpose + // (GameDirectory/ModsDirectory/VanillaScriptsDirectory - blank means "derive + // from the working directory"), a Vortex-written sidecar value is strictly + // better information than deriving, so prefer it when there is one. + return ReadVortexSidecarSetting(key) ?? value; + } AppState.Notifier.ShowError($"Config file doesn't exist:\n\n{CachedConfig.FilePath}"); return null; } + // Vortex's bundled game-witcher3 extension reads AND writes a script-merger config + // at "\WitcherScriptMerger.exe.config" - the .NET Framework naming + // convention this project used before the .NET 10 modernization. It parses that file + // for MergedModName (scriptmerger.ts::getMergedModName) and writes GameDirectory, + // VanillaScriptsDirectory and ModsDirectory into it (scriptmerger.ts::setMergerConfig) + // when it configures a merger install. A modern .NET app's own configuration is + // ".dll.config" instead, so without this the two never meet: Vortex writes + // a file WSM never reads, and the user "configures WSM through Vortex" with no + // effect at all. + // + // Reading it as a *fallback* rather than an override is deliberate. A non-blank + // value in our own config is an explicit choice (the GUI's own settings screen + // writes there via Set/Save, and Vortex never writes MergedModName), so it must + // win; the sidecar only fills in what we'd otherwise have to guess. Env overrides + // still beat both, unchanged. + public const string VortexSidecarFileName = "WitcherScriptMerger.exe.config"; + + string _sidecarPath; + bool _sidecarChecked; + string _sidecarXml; + + string ReadVortexSidecarSetting(string key) + { + if (!_sidecarChecked) + { + _sidecarChecked = true; + try + { + _sidecarPath = Path.Combine(Path.GetDirectoryName(_assemblyPath) ?? string.Empty, VortexSidecarFileName); + if (File.Exists(_sidecarPath)) + _sidecarXml = File.ReadAllText(_sidecarPath); + } + catch + { + // Unreadable/inaccessible sidecar is not an error - it's an optional + // interop file that usually isn't there at all. Never prompt, never + // throw: this runs inside every settings read, including on scan paths. + _sidecarXml = null; + } + } + + return _sidecarXml == null ? null : ParseAppSettingValue(_sidecarXml, key); + } + + // Split out as a pure string-in/string-out function so the sidecar parsing is + // directly unit-testable without a filesystem, a live AppSettings instance, or + // AppState - see WitcherScriptMerger.Tests/CLAUDE.md's "AppState.Settings-safety + // constraints". Returns null for anything it can't confidently read (malformed XML, + // missing key, blank value), so every caller falls through to its existing + // behavior rather than acting on a half-parsed file. + public static string ParseAppSettingValue(string xml, string key) + { + if (string.IsNullOrWhiteSpace(xml) || string.IsNullOrWhiteSpace(key)) + return null; + + try + { + var value = XDocument.Parse(xml) + .Root?.Elements("appSettings") + .Elements("add") + .Where(e => string.Equals((string)e.Attribute("key"), key, StringComparison.Ordinal)) + .Select(e => (string)e.Attribute("value")) + .FirstOrDefault(); + + return string.IsNullOrWhiteSpace(value) ? null : value; + } + catch + { + return null; + } + } + // Deliberately unaware of GetEnvironmentOverride: this still only ever writes to // CachedConfig/App.config, same as before the env-var override existed. If a // WSM_ override is active for a key this call targets, Get/Get keep diff --git a/WitcherScriptMerger.Core/CLAUDE.md b/WitcherScriptMerger.Core/CLAUDE.md index 619cb2c..a449cc9 100644 --- a/WitcherScriptMerger.Core/CLAUDE.md +++ b/WitcherScriptMerger.Core/CLAUDE.md @@ -602,3 +602,43 @@ the synthetic-edge-cases + real-recorded-hash cross-check pattern described in - Merge history: `MergeInventory.xml`, via `XmlSerializer` (`Inventory/MergeInventory.cs`). - Game load order: `LoadOrder/CustomLoadOrder.cs` reads the game's own `mods.settings` file. + +### Vortex-managed sidecar config (`WitcherScriptMerger.exe.config`) + +`GetRawValue` resolves a key in three steps, first non-blank wins: + +1. `WSM_` environment variable (`GetEnvironmentOverride`). +2. Our own config — `.dll.config`, via `ConfigurationManager`. +3. **The Vortex-managed sidecar**, `AppSettings.VortexSidecarFileName` + (`WitcherScriptMerger.exe.config`) beside the entry assembly. + +Step 3 exists because Vortex's bundled `game-witcher3` extension both **reads and writes** +a script-merger config under the .NET Framework `.exe.config` name this project +stopped using at the .NET 10 modernization. It parses that file for `MergedModName` +(`scriptmerger.ts::getMergedModName`) and writes `GameDirectory`, +`VanillaScriptsDirectory` and `ModsDirectory` into it when configuring a merger install +(`scriptmerger.ts::setMergerConfig`). Without this fallback the two never meet: Vortex +writes a file WSM never reads, so a user who "configures WSM through Vortex" changes +nothing at all — and Vortex logs `failed to ascertain merged mod name - using +"mod0000_MergedFiles"` and silently falls back to a hardcoded guess. + +**A fallback, not an override**, deliberately: a non-blank value in our own config is an +explicit choice (the GUI's settings screen writes there via `Set`/`Save`, and Vortex never +writes `MergedModName`), so it must win. The sidecar only fills in what we would otherwise +have to derive — which is exactly the shape of the three keys Vortex writes, since +`GameDirectory`/`ModsDirectory`/`VanillaScriptsDirectory` all ship blank meaning "derive +from the working directory". Env overrides still beat both. + +`ParseAppSettingValue(xml, key)` is a pure static over the file's text — no filesystem, no +`AppState` — so it's directly unit-testable (`AppSettingsTests`); it returns `null` for +anything it can't confidently read (malformed XML, missing key, blank value) so every +caller falls through to existing behavior instead of acting on a half-parsed file. The +read is cached after the first attempt and never throws or prompts: it runs inside every +settings read, including scan paths where an exception would surface as a merge failure. +`Settings[key]` is dereferenced with `?.` so a key present *only* in the sidecar still +resolves rather than throwing first. + +The WinForms host's csproj emits this file at build and publish (never overwriting an +existing one — Vortex owns it once written); see `WitcherScriptMerger/CLAUDE.md`. +`WitcherScriptMerger.Headless` deliberately does not, since Vortex's extension only ever +looks for a merger named `WitcherScriptMerger.exe`. diff --git a/WitcherScriptMerger.Tests/AppSettingsTests.cs b/WitcherScriptMerger.Tests/AppSettingsTests.cs index 3d67fb2..cc4e971 100644 --- a/WitcherScriptMerger.Tests/AppSettingsTests.cs +++ b/WitcherScriptMerger.Tests/AppSettingsTests.cs @@ -149,5 +149,96 @@ static void WithEnvironmentVariable(string key, string value, Action action) Environment.SetEnvironmentVariable(envVarName, originalValue); } } + + #region Vortex sidecar config (WitcherScriptMerger.exe.config) + + // Coverage for AppSettings.ParseAppSettingValue, the parser behind the + // Vortex-managed sidecar GetRawValue falls back to when our own config leaves a key + // blank - see AppSettings.cs's own comment on VortexSidecarFileName for why that + // file exists (Vortex's bundled game-witcher3 extension reads MergedModName from it + // and writes GameDirectory/VanillaScriptsDirectory/ModsDirectory into it, under the + // .NET Framework ".exe.config" name a modern .NET app doesn't use). + // + // Exercised as a pure static over an XML string: no filesystem, no AppSettings + // instance, no AppState - see WitcherScriptMerger.Tests/CLAUDE.md's + // "AppState.Settings-safety constraints". + const string SidecarXml = @" + + + + + + + +"; + + [Theory] + [InlineData("GameDirectory", @"G:\Games\Witcher3")] + [InlineData("ModsDirectory", @"G:\Games\Witcher3\mods")] + [InlineData("MergedModName", "mod0000_MergedFiles")] + public void ParseAppSettingValue_KeyPresent_ReturnsItsValue(string key, string expected) + { + Assert.Equal(expected, AppSettings.ParseAppSettingValue(SidecarXml, key)); + } + + // Null, never string.Empty, for anything unusable - GetRawValue's `?? value` fallback + // relies on that to fall through to its own (blank) config value rather than + // treating a blank sidecar entry as an answer. + [Theory] + [InlineData("NotInTheFile")] + [InlineData("BlankOne")] + public void ParseAppSettingValue_MissingOrBlankValue_ReturnsNull(string key) + { + Assert.Null(AppSettings.ParseAppSettingValue(SidecarXml, key)); + } + + // Matching is case-sensitive, matching ConfigurationManager's own + // behavior - "gamedirectory" must not resolve "GameDirectory". + [Fact] + public void ParseAppSettingValue_KeyCaseDiffers_ReturnsNull() + { + Assert.Null(AppSettings.ParseAppSettingValue(SidecarXml, "gamedirectory")); + } + + // A malformed/truncated sidecar (Vortex interrupted mid-write, say) must degrade to + // "no answer" rather than throwing: this parser runs inside every settings read, + // including on scan paths where an exception would surface as a merge failure. + [Theory] + [InlineData("")] + [InlineData("not xml at all")] + [InlineData("")] + [InlineData("")] + public void ParseAppSettingValue_MalformedOrEmptyXml_ReturnsNullWithoutThrowing(string xml) + { + Assert.Null(AppSettings.ParseAppSettingValue(xml, "GameDirectory")); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void ParseAppSettingValue_NoXml_ReturnsNull(string xml) + { + Assert.Null(AppSettings.ParseAppSettingValue(xml, "GameDirectory")); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void ParseAppSettingValue_NoKey_ReturnsNull(string key) + { + Assert.Null(AppSettings.ParseAppSettingValue(SidecarXml, key)); + } + + // The file name is the interop contract with Vortex's hardcoded + // scriptmerger.ts::MERGER_CONFIG_FILE - it is not ours to rename. + [Fact] + public void VortexSidecarFileName_MatchesTheNameVortexLooksFor() + { + Assert.Equal("WitcherScriptMerger.exe.config", AppSettings.VortexSidecarFileName); + } + + #endregion } } diff --git a/WitcherScriptMerger.Tests/CLAUDE.md b/WitcherScriptMerger.Tests/CLAUDE.md index bd1bd0f..0c0e6e8 100644 --- a/WitcherScriptMerger.Tests/CLAUDE.md +++ b/WitcherScriptMerger.Tests/CLAUDE.md @@ -64,6 +64,14 @@ does. `DiffPlexMergeEngine` against a real `KDiff3.exe` binary, when a developer happens to have one locally (WSM no longer bundles or requires KDiff3 itself — see `docs/decisions/kdiff3-retirement.md`). +- `AppSettingsTests.cs` — `AppSettings`'s `WSM_` environment-variable override, and + (added alongside the Vortex sidecar interop) `AppSettings.ParseAppSettingValue`: the pure + parser behind the `WitcherScriptMerger.exe.config` fallback Vortex's bundled + `game-witcher3` extension reads and writes. Covers key lookup, case-sensitivity, a blank + value and a missing key both yielding `null` (which is what makes `GetRawValue`'s `??` + fall through correctly), malformed/truncated/empty XML degrading to `null` rather than + throwing, null/blank inputs, and that `VortexSidecarFileName` still matches the name + Vortex hardcodes — see Core's `CLAUDE.md`'s "Vortex-managed sidecar config" section. - `LiveInstall.cs` — see "Live-install cross-check tests" below. ## `AppState.Settings`-safety constraints diff --git a/WitcherScriptMerger/CLAUDE.md b/WitcherScriptMerger/CLAUDE.md index e846f20..8b23156 100644 --- a/WitcherScriptMerger/CLAUDE.md +++ b/WitcherScriptMerger/CLAUDE.md @@ -37,7 +37,17 @@ merges via `InteractiveMergeRunner`, and wires up async callbacks. The publish's `.dll.config` (the `App.config` copy `System.Configuration.ConfigurationManager` actually reads, via Core's `AppSettings.cs`) lands next to the executable — copy it there if deploying the exe on - its own. This resolves correctly even in a single-file publish despite + its own. A second copy, `WitcherScriptMerger.exe.config`, lands beside it via the + `EmitVortexCompatConfigForBuild`/`EmitVortexCompatConfigForPublish` targets in this + project's csproj — that's the .NET Framework name Vortex's bundled `game-witcher3` + extension hardcodes and both reads and writes (see Core's `CLAUDE.md`'s "Vortex-managed + sidecar config"). It is only written when absent: Vortex owns that file once it has + configured a merger install, and clobbering it on every rebuild would throw away the + paths it wrote. Two separate targets rather than one with + `AfterTargets="Build;Publish"` because the SDK defines `$(PublishDir)` + unconditionally (defaulting to `$(OutDir)publish\`), so a single target choosing + "PublishDir if set, else OutDir" silently wrote to the publish folder during an ordinary + build and left the build output without the file. This resolves correctly even in a single-file publish despite `Assembly.GetEntryAssembly().Location` being documented (and confirmed via a real build's `IL3000` warning) to always return `""` for a single-file-bundled assembly — `ConfigurationManager.OpenExeConfiguration("")` still finds and reads the real diff --git a/WitcherScriptMerger/WitcherScriptMerger.csproj b/WitcherScriptMerger/WitcherScriptMerger.csproj index ca170db..0aa5744 100644 --- a/WitcherScriptMerger/WitcherScriptMerger.csproj +++ b/WitcherScriptMerger/WitcherScriptMerger.csproj @@ -28,4 +28,39 @@ + + + + + + + + +