Conversation
- Create ProjectSettings and WindowSettings models - Implement ProjectSettingsService with async support - Support saving/loading project settings to JSON - Track recent files, open tabs, and window state - Add comprehensive unit tests - Register service in DI container Closes #8
There was a problem hiding this comment.
Pull request overview
Adds a JSON-backed project configuration system with async load/save support and wires it into the UI DI container, along with unit tests.
Changes:
- Introduces
ProjectSettings/WindowSettingsmodels andIProjectSettingsService+ implementation. - Adds JSON persistence (sync + async) for project settings (recent files, tabs, window state).
- Registers new common services in the UI DI container and adds unit tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| BannerlordModEditor.UI/App.axaml.cs | Registers new services (scanner/detector/settings) in DI. |
| BannerlordModEditor.Common/Services/ProjectSettingsService.cs | Implements models + JSON load/save service (sync/async). |
| BannerlordModEditor.Common.Tests/Services/ProjectSettingsServiceTests.cs | Adds xUnit coverage for default/path/load/save scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public async Task<ProjectSettings> LoadAsync(string projectPath) | ||
| { | ||
| return await Task.Run(() => Load(projectPath)); | ||
| } |
There was a problem hiding this comment.
LoadAsync/SaveAsync are implemented via Task.Run, which offloads synchronous disk I/O to the thread pool rather than using true async file I/O. This can reduce scalability and responsiveness under load. Prefer File.ReadAllTextAsync/File.WriteAllTextAsync (and async JSON serialize/deserialize where applicable) so the async API is genuinely non-blocking.
| public async Task SaveAsync(ProjectSettings settings, string projectPath) | ||
| { | ||
| await Task.Run(() => Save(settings, projectPath)); | ||
| } |
There was a problem hiding this comment.
LoadAsync/SaveAsync are implemented via Task.Run, which offloads synchronous disk I/O to the thread pool rather than using true async file I/O. This can reduce scalability and responsiveness under load. Prefer File.ReadAllTextAsync/File.WriteAllTextAsync (and async JSON serialize/deserialize where applicable) so the async API is genuinely non-blocking.
| catch | ||
| { | ||
| return GetDefault(projectPath); | ||
| } |
There was a problem hiding this comment.
The blanket catch { return GetDefault(...) } will silently hide parsing errors and filesystem failures, making issues hard to diagnose (and potentially masking real misconfigurations). Consider narrowing to expected exceptions (e.g., JsonException, IOException) and at minimum emitting a log/trace entry; for unexpected exceptions, consider rethrowing so failures aren’t silently ignored.
| catch | |
| { | |
| return GetDefault(projectPath); | |
| } | |
| catch (JsonException ex) | |
| { | |
| Console.Error.WriteLine($"Failed to parse project settings file '{settingsPath}': {ex}"); | |
| return GetDefault(projectPath); | |
| } | |
| catch (IOException ex) | |
| { | |
| Console.Error.WriteLine($"I/O error while reading project settings file '{settingsPath}': {ex}"); | |
| return GetDefault(projectPath); | |
| } |
| settings.ProjectPath = projectPath; | ||
| settings.LastOpened = DateTime.Now; |
There was a problem hiding this comment.
DateTime.Now is local-time dependent and makes values harder to compare across machines/timezones; it also tends to complicate deterministic tests. Prefer DateTime.UtcNow or DateTimeOffset.UtcNow for persisted timestamps. Also, mutating settings.ProjectPath and settings.LastOpened inside Save is a side effect that may surprise callers; if this is required behavior, consider documenting it on the interface/method.
| settings.LastOpened = DateTime.Now; | ||
|
|
||
| var json = JsonSerializer.Serialize(settings, JsonOptions); | ||
| File.WriteAllText(settingsPath, json); |
There was a problem hiding this comment.
Writing directly to the final settings file risks leaving a partially written/corrupted JSON file if the process crashes mid-write or the disk fills up. Prefer an atomic write pattern (write to a temp file in the same directory, flush, then replace/move over the target) to improve reliability.
| File.WriteAllText(settingsPath, json); | |
| var tempSettingsPath = settingsPath + ".tmp"; | |
| File.WriteAllText(tempSettingsPath, json); | |
| File.Move(tempSettingsPath, settingsPath, true); |
| { | ||
| var result = _service.GetSettingsFilePath("/test/project"); | ||
|
|
||
| Assert.Equal("/test/project/.bannerlordmodeditor.json", result); |
There was a problem hiding this comment.
This assertion hard-codes / path separators and will fail on Windows where Path.Combine uses \. Build the expected value using Path.Combine("/test/project", ".bannerlordmodeditor.json") (or construct the test input using Path.Combine too) so the test is cross-platform.
| Assert.Equal("/test/project/.bannerlordmodeditor.json", result); | |
| var expectedPath = Path.Combine("/test/project", ".bannerlordmodeditor.json"); | |
| Assert.Equal(expectedPath, result); |
| public class ProjectSettings | ||
| { | ||
| public string ProjectName { get; set; } = string.Empty; | ||
| public string ProjectPath { get; set; } = string.Empty; | ||
| public string GameVersion { get; set; } = "Latest"; | ||
| public string LastOpenedFile { get; set; } = string.Empty; | ||
| public DateTime LastOpened { get; set; } | ||
| public Dictionary<string, string> CustomSettings { get; set; } = new(); | ||
| public List<string> RecentFiles { get; set; } = new(); | ||
| public List<string> OpenTabs { get; set; } = new(); | ||
| public WindowSettings Window { get; set; } = new(); | ||
| } | ||
|
|
||
| public class WindowSettings | ||
| { | ||
| public double Width { get; set; } = 1200; | ||
| public double Height { get; set; } = 800; | ||
| public double X { get; set; } | ||
| public double Y { get; set; } | ||
| public bool IsMaximized { get; set; } | ||
| } |
There was a problem hiding this comment.
ProjectSettings/WindowSettings models and the service/interface are all declared in the same file. Since these are likely to be consumed independently (models by UI, service by DI/tests), splitting them into separate files (e.g., Models/ProjectSettings.cs, Models/WindowSettings.cs, Services/IProjectSettingsService.cs) will make the codebase easier to navigate and evolve.
Summary
Changes
BannerlordModEditor.Common/Services/ProjectSettingsService.cs- ImplementationBannerlordModEditor.Common.Tests/Services/ProjectSettingsServiceTests.cs- Unit testsBannerlordModEditor.UI/App.axaml.cs- DI registrationCloses #8