From c5c9d84bec89ba79008bb191c423ce88b7cd9e71 Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 22:39:31 +0800 Subject: [PATCH 01/22] docs: add code review fix todo --- docs/code-review-2026-07-06.md | 280 ++++++++++++++++++++++++ docs/code-review-fix-todo-2026-07-06.md | 40 ++++ 2 files changed, 320 insertions(+) create mode 100644 docs/code-review-2026-07-06.md create mode 100644 docs/code-review-fix-todo-2026-07-06.md diff --git a/docs/code-review-2026-07-06.md b/docs/code-review-2026-07-06.md new file mode 100644 index 0000000..eb785e7 --- /dev/null +++ b/docs/code-review-2026-07-06.md @@ -0,0 +1,280 @@ +# Code Review - 2026-07-06 + +## Scope + +This review inspected the current `src/` and `tests/` code for: + +- fake or incomplete stub behavior exposed as real functionality +- low-value or misleading tests +- unusual compatibility or platform handling +- deviations from the repository guidance and common .NET/Avalonia practices + +Four subagents reviewed Core, Infrastructure, Avalonia/UI, and cross-repository suspicious patterns in parallel. The review was read-only; no tests were run. + +Existing working tree note: `scripts/publish.sh` was already modified before this review and was not touched. + +## Summary + +No critical issues were found. The highest-risk findings are: + +- WebVTT import drops cue end times and calculates duration incorrectly. +- Windows terminal fallback in `ShellService` builds a command string from a path. +- File association support exists as a partial service surface but is not complete enough to be trustworthy. +- Preview UI is available when no chapter data exists and opens an empty window. + +## High Severity + +### WebVTT cue end times are parsed but discarded + +- File: `src/ChapterTool.Core/Importing/Text/WebVttChapterImporter.cs:40` +- File: `src/ChapterTool.Core/Importing/Text/WebVttChapterImporter.cs:48` +- File: `src/ChapterTool.Core/Importing/Text/WebVttChapterImporter.cs:62` + +The importer validates the cue end time with `TimeSpan.TryParse(parts[1], out _)`, but the parsed value is discarded. `Chapter.End` is never populated, and `ChapterInfo.Duration` is set to the last chapter start time. + +Impact: imported WebVTT chapters lose end times. Any later export or duration-dependent workflow can produce wrong final segment timing. + +Recommendation: keep the parsed end value, pass it as `End` when creating `Chapter`, and calculate duration from the last valid cue end. + +### Windows terminal fallback is command-injection prone + +- File: `src/ChapterTool.Infrastructure/Platform/ShellService.cs:62` + +The Windows fallback path builds a command string: + +```text +cmd /c start cmd /k "cd /d {directoryPath}" +``` + +Because `directoryPath` is user/path data, characters such as `&` or quotes can change the command. Exceptions around this path are also swallowed. + +Impact: malformed or adversarial paths can execute unintended shell commands; failures are invisible to the user. + +Recommendation: avoid command-string composition. Start `cmd.exe` directly with fixed arguments and set `ProcessStartInfo.WorkingDirectory = directoryPath`, or use a safer platform abstraction that returns a structured failure. + +### File association service is incomplete and not wired to the documented command surface + +- File: `src/ChapterTool.Infrastructure/Platform/WindowsFileAssociationService.cs:27` +- File: `src/ChapterTool.Infrastructure/Platform/WindowsFileAssociationService.cs:60` +- File: `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs:520` +- Related spec: `openspec/specs/avalonia-ui-shell/spec.md:60` + +`WindowsFileAssociationService` writes only the ProgID description and extension default value. It does not write `shell\open\command`, icon metadata, ownership markers, or shell change notifications. `UnregisterAsync` deletes the extension key without confirming it is still owned by ChapterTool. + +The spec says the main window ViewModel shall expose a file association command, but the current command list has no such command. The composition root can create the service, but no user-visible workflow appears to consume it. + +Impact: registration can report success without making files open correctly, and unregister can remove another app's association. The implementation also looks like a feature but lacks an end-to-end user path. + +Recommendation: either complete the service and UI workflow, or remove/hide the feature surface until it is ready. A complete implementation should write an open command, track ownership, guard unregistration, refresh shell associations, and have tests through an injectable registry abstraction. + +### Preview opens an empty window with no loaded chapters + +- File: `src/ChapterTool.Avalonia/Views/MainWindow.axaml:260` +- File: `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs:148` +- File: `src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs:1304` + +The preview command is an unconditional `WindowCommand("preview")`. With no current chapter data, `BuildPreview()` returns empty text, so the user can open a blank preview window. + +Impact: this is a visible UI stub: the control appears usable but has no meaningful behavior in the empty state. Keyboard access such as `F11` can trigger the same result. + +Recommendation: make `PreviewCommand.CanExecute` depend on loaded chapter data, and ensure buttons, menus, and shortcuts share that state. Alternatively, show an explicit empty-state message. + +## Medium Severity + +### `CueChapterImporter` has a fake injectable parser + +- File: `src/ChapterTool.Core/Importing/Cue/CueChapterImporter.cs:3` +- File: `src/ChapterTool.Core/Importing/Cue/CueChapterImporter.cs:29` + +The constructor accepts `CueSheetParser? parser` and stores it, but `ImportAsync` calls the static `CueSheetParser.Parse` method. + +Impact: callers and tests may think parser behavior is replaceable, but the injected object is ignored. + +Recommendation: remove the constructor parameter and field, or introduce a real parser interface/instance method and use it. + +### `IfoChapterImporter` is fake-async and ignores request content + +- File: `src/ChapterTool.Core/Importing/Disc/IfoChapterImporter.cs:16` +- File: `src/ChapterTool.Core/Importing/Disc/IfoChapterImporter.cs:18` +- File: `src/ChapterTool.Core/Importing/Disc/IfoChapterImporter.cs:21` + +`ImportAsync` awaits `Task.CompletedTask`, then reads only from `request.Path`. It ignores `request.Content`. + +Impact: IFO behaves differently from other importers that support in-memory content. The fake await also obscures the synchronous file-only behavior. + +Recommendation: parse from a seekable stream and prefer `request.Content` when present. If file-only sync parsing is intentional for now, remove the fake await and document the limitation. + +### Core tests depend on Infrastructure + +- File: `tests/ChapterTool.Core.Tests/ChapterTool.Core.Tests.csproj:30` +- File: `tests/ChapterTool.Core.Tests/Importing/MediaChapterImporterTests.cs:4` + +`ChapterTool.Core.Tests` references `ChapterTool.Infrastructure` and instantiates `FfprobeMediaChapterReader`. + +Impact: Core tests become mixed integration tests and can fail because of Infrastructure parser behavior rather than Core importer behavior. + +Recommendation: test Core's `MediaChapterImporter` using a fake `IMediaChapterReader` that returns `MediaChapterEntry` values. Keep ffprobe JSON/process parsing tests in `ChapterTool.Infrastructure.Tests`. + +### External tool locator treats any file as an executable + +- File: `src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs:37` +- File: `tests/ChapterTool.Infrastructure.Tests/ExternalToolLocatorTests.cs:15` + +Tool resolution uses `File.Exists` only, and tests use empty text files as successful tool candidates. + +Impact: on Linux/macOS, non-executable files can be reported as found. The settings UI can show success, then runtime execution fails later. + +Recommendation: add executable validation. On Unix-like systems, check execute permissions. On Windows, validate expected executable naming, and consider an optional lightweight `--version` probe for configured tools. + +### Corrupt settings silently reset to defaults + +- File: `src/ChapterTool.Infrastructure/Configuration/AppSettingsStore.cs:43` +- File: `src/ChapterTool.Infrastructure/Configuration/ThemeSettingsStore.cs:29` + +Invalid JSON is caught and replaced with default settings without logging or surfacing a diagnostic. + +Impact: user settings can appear to reset, and a later save can overwrite the corrupt file with defaults, making recovery harder. + +Recommendation: log the parse failure, preserve the corrupt file as `.corrupt`, and expose a user-visible warning before defaults are saved back. + +### Shell failures are silently swallowed + +- File: `src/ChapterTool.Infrastructure/Platform/ShellService.cs:41` +- File: `src/ChapterTool.Infrastructure/Platform/ShellService.cs:74` +- File: `src/ChapterTool.Infrastructure/Platform/ShellService.cs:105` + +Reveal/open-terminal helper paths catch broad exceptions and ignore them. + +Impact: users see an action that does nothing, with no status or log entry. + +Recommendation: catch expected process/platform exceptions and return or log structured failure information. + +### FFmpeg directory setting actually validates ffprobe + +- File: `src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs:248` +- File: `src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs:255` +- File: `src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs:577` +- File: `src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs:593` + +`FfmpegPath` is presented as an FFmpeg directory setting, but validation and discovery check `ffprobe`. + +Impact: users may think they configured FFmpeg, while the app actually validates ffprobe. The setting overlaps semantically with `FfprobePath`. + +Recommendation: rename it to ffprobe directory if that is the intended behavior, or validate/discover `ffmpeg` if it truly represents FFmpeg. + +### Native file picker strings are hard-coded English + +- File: `src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs:11` +- File: `src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs:16` +- File: `src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs:37` +- File: `src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs:54` +- File: `src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs:71` + +File picker titles and file type labels are English string literals. + +Impact: localized UI still opens English system dialogs. + +Recommendation: inject `IAppLocalizer` into `AvaloniaFilePickerService`, or have callers pass localized titles/filter names. + +### Icon buttons lack accessible names + +- File: `src/ChapterTool.Avalonia/Views/MainWindow.axaml:260` +- File: `src/ChapterTool.Avalonia/Views/MainWindow.axaml:267` +- File: `src/ChapterTool.Avalonia/Views/MainWindow.axaml:275` +- File: `src/ChapterTool.Avalonia/Views/MainWindow.axaml:505` +- File: `src/ChapterTool.Avalonia/Views/Tools/SettingsToolView.axaml:101` + +Several icon buttons have tooltip text and automation IDs, but no localized `AutomationProperties.Name`. + +Impact: screen readers may not announce a useful action name. Automation IDs are not a substitute for accessible names. + +Recommendation: add localized `AutomationProperties.Name` and, where useful, `AutomationProperties.HelpText`. Headless tests should verify the accessible name, not only command binding. + +### BDMV parser moves data through diagnostics and narrow regexes + +- File: `src/ChapterTool.Infrastructure/Importing/Bdmv/BdmvChapterImporter.cs:160` +- File: `src/ChapterTool.Infrastructure/Importing/Bdmv/BdmvChapterImporter.cs:344` + +The BDMV importer stores eac3to stdout in a diagnostic message and then parses it later with narrow text regexes. + +Impact: adding another diagnostic can break assumptions, and small eac3to output/version/localization changes can stop detection. + +Recommendation: return an internal structured result that carries stdout separately from diagnostics. Add fixture coverage from real eac3to outputs and handle known format variants. + +## Low Severity + +### Test fakes live in production Infrastructure + +- File: `src/ChapterTool.Infrastructure/Platform/MemoryClipboardService.cs:5` +- File: `src/ChapterTool.Infrastructure/Platform/ScriptedDialogService.cs:5` +- File: `src/ChapterTool.Infrastructure/Platform/RecordingWindowService.cs:5` +- File: `tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs:63` + +These are test-double style services in the production Infrastructure assembly. The test name calls them skeletons. + +Impact: they can be accidentally injected into production paths and silently record operations instead of performing real platform behavior. + +Recommendation: move them to test projects, or mark them internal/test-only and avoid exposing them as production services. + +### `UiCommandTests` waits with a fixed delay + +- File: `tests/ChapterTool.Avalonia.Tests/Commands/UiCommandTests.cs:47` + +The test waits for `async void` command completion with `Task.Delay(50)`. + +Impact: slow CI machines or scheduling delays can make the test flaky. + +Recommendation: use a `TaskCompletionSource`, property changed event, or polling with a bounded timeout for `ExecutionError`. + +### Matroska integration setup blocks on async + +- File: `tests/ChapterTool.Infrastructure.Tests/Importing/MatroskaIntegrationTests.cs:27` + +`IAsyncLifetime.InitializeAsync` uses `.AsTask().Result`. + +Impact: this is a sync-over-async pattern with deadlock and cancellation risks. + +Recommendation: make `InitializeAsync` an `async ValueTask` and `await LocateAsync(...)`. + +### Screenshot tests are weak regression guards + +- File: `tests/ChapterTool.Avalonia.Tests/Headless/MainWindowHeadlessTests.cs:190` +- File: `tests/ChapterTool.Avalonia.Tests/Headless/LocalizationAndLayoutHeadlessTests.cs:117` + +The tests mainly capture screenshots and assert the files exist/non-empty. + +Impact: a badly misaligned or semantically blank UI can still pass. + +Recommendation: keep screenshots as artifacts, but add behavioral/layout assertions such as key controls visible, no bounds overflow, and meaningful non-background pixel regions. + +### MKVToolNix registry DisplayIcon parsing misses quoted paths + +- File: `src/ChapterTool.Infrastructure/Tools/MkvToolNixInstallProbe.cs:78` + +The install probe trims a trailing comma suffix but does not normalize quoted `DisplayIcon` values. + +Impact: common registry values such as `"C:\Program Files\...\mkvtoolnix-gui.exe",0` may fail to resolve to the install directory. + +Recommendation: strip quotes after trimming the icon suffix, then take the directory. Add a quoted DisplayIcon test. + +### eac3to export can show a console window + +- File: `src/ChapterTool.Infrastructure/Importing/Bdmv/BdmvChapterImporter.cs:171` +- File: `tests/ChapterTool.Infrastructure.Tests/BdmvChapterImporterTests.cs:51` + +The eac3to chapter export request explicitly sets `CreateNoWindow: false`, and the test locks in that behavior. + +Impact: GUI import can flash or show a console window. + +Recommendation: default to hidden process windows unless eac3to requires visibility. If visibility is required, document the reason and test that reason rather than the raw flag. + +## Non-Issues / Filtered Results + +The review did not find meaningful hits for: + +- `NotImplementedException` +- `PlatformNotSupportedException` +- `Thread.Sleep` +- global test parallelization disablement in Avalonia tests +- tests reading source/configuration files and asserting incidental source strings + diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md new file mode 100644 index 0000000..c5b77a5 --- /dev/null +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -0,0 +1,40 @@ +# Code Review Fix TODO - 2026-07-06 + +Source report: `docs/code-review-2026-07-06.md` + +## Rules + +- Fix one issue at a time. +- Run focused tests after each fix. +- Commit each completed fix separately. +- Do not include unrelated working tree changes, especially the pre-existing `scripts/publish.sh` modification. + +## High Priority + +- [ ] Fix WebVTT import so cue end times and duration are preserved. +- [ ] Remove command-string path interpolation from `ShellService.OpenTerminalAsync`. +- [ ] Complete or safely hide the file association service/command surface. +- [ ] Prevent preview from opening as an empty stub when no chapters are loaded. + +## Medium Priority + +- [ ] Remove the fake injectable parser from `CueChapterImporter`. +- [ ] Remove fake async behavior from `IfoChapterImporter` and handle `request.Content` consistently. +- [ ] Remove the Infrastructure dependency from Core tests. +- [ ] Validate external tools as executables, not just existing files. +- [ ] Preserve and surface corrupt settings files instead of silently resetting. +- [ ] Return or log shell service failures instead of swallowing them. +- [ ] Clarify whether `FfmpegPath` means ffmpeg or ffprobe directory and validate accordingly. +- [ ] Localize native file picker titles and file type labels. +- [ ] Add accessible names for icon-only buttons. +- [ ] Refactor BDMV parsing so stdout is not passed through diagnostics. + +## Low Priority + +- [ ] Move production test-double services out of Infrastructure or mark them test-only. +- [ ] Replace fixed `Task.Delay` in `UiCommandTests`. +- [ ] Remove sync-over-async from Matroska integration setup. +- [ ] Strengthen screenshot tests with layout/content assertions. +- [ ] Handle quoted MKVToolNix `DisplayIcon` registry values. +- [ ] Hide eac3to export process windows unless visibility is required. + From 83873ae94f25a5f08570fcc8eb2207a910951617 Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 22:48:19 +0800 Subject: [PATCH 02/22] fix: preserve WebVTT cue end times --- docs/code-review-fix-todo-2026-07-06.md | 3 +-- .../Importing/Text/WebVttChapterImporter.cs | 6 +++--- tests/ChapterTool.Core.Tests/Importing/TextImporterTests.cs | 3 +++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index c5b77a5..49b1681 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -11,7 +11,7 @@ Source report: `docs/code-review-2026-07-06.md` ## High Priority -- [ ] Fix WebVTT import so cue end times and duration are preserved. +- [x] Fix WebVTT import so cue end times and duration are preserved. - [ ] Remove command-string path interpolation from `ShellService.OpenTerminalAsync`. - [ ] Complete or safely hide the file association service/command surface. - [ ] Prevent preview from opening as an empty stub when no chapters are loaded. @@ -37,4 +37,3 @@ Source report: `docs/code-review-2026-07-06.md` - [ ] Strengthen screenshot tests with layout/content assertions. - [ ] Handle quoted MKVToolNix `DisplayIcon` registry values. - [ ] Hide eac3to export process windows unless visibility is required. - diff --git a/src/ChapterTool.Core/Importing/Text/WebVttChapterImporter.cs b/src/ChapterTool.Core/Importing/Text/WebVttChapterImporter.cs index 713f61d..85ce0f8 100644 --- a/src/ChapterTool.Core/Importing/Text/WebVttChapterImporter.cs +++ b/src/ChapterTool.Core/Importing/Text/WebVttChapterImporter.cs @@ -37,7 +37,7 @@ public static ChapterImportResult ImportText(string text, string path = "") } var parts = lines[0].Split("-->", StringSplitOptions.TrimEntries); - if (parts.Length != 2 || !TimeSpan.TryParse(parts[0], out var start) || !TimeSpan.TryParse(parts[1], out _)) + if (parts.Length != 2 || !TimeSpan.TryParse(parts[0], out var start) || !TimeSpan.TryParse(parts[1], out var end)) { var code = parts.Length == 2 && parts[1].Contains(' ', StringComparison.Ordinal) ? "WebVttUnsupportedTimingSettings" @@ -45,7 +45,7 @@ public static ChapterImportResult ImportText(string text, string path = "") return ChapterImportResult.Failed(Error(code, $"Unable to parse WebVTT timing line: {lines[0]}")); } - chapters.Add(new Chapter(chapters.Count + 1, start, lines[1])); + chapters.Add(new Chapter(chapters.Count + 1, start, lines[1], End: end)); } if (chapters.Count == 0) @@ -59,7 +59,7 @@ public static ChapterImportResult ImportText(string text, string path = "") 0, "WebVTT", 0, - chapters.Count == 0 ? TimeSpan.Zero : chapters[^1].Time, + chapters[^1].End ?? chapters[^1].Time, chapters); return TextImportUtilities.SingleGroup(path, info); } diff --git a/tests/ChapterTool.Core.Tests/Importing/TextImporterTests.cs b/tests/ChapterTool.Core.Tests/Importing/TextImporterTests.cs index 143eac6..efbb6dd 100644 --- a/tests/ChapterTool.Core.Tests/Importing/TextImporterTests.cs +++ b/tests/ChapterTool.Core.Tests/Importing/TextImporterTests.cs @@ -251,6 +251,9 @@ The Colossus of Rhodes Assert.Equal(7, chapters.Count); Assert.Equal("Introduction", chapters[0].Name); Assert.Equal(TimeSpan.FromMilliseconds(28206), chapters[1].Time); + Assert.Equal(TimeSpan.FromSeconds(26), chapters[0].End); + Assert.Equal(TimeSpan.FromMilliseconds(547500), chapters[^1].End); + Assert.Equal(TimeSpan.FromMilliseconds(547500), result.Groups.Single().Options.Single().ChapterInfo.Duration); } [Fact] From 35d2750f351d5d882bc21d17a8c0db76b5165c1c Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 22:55:25 +0800 Subject: [PATCH 03/22] fix: avoid shell command interpolation --- docs/code-review-fix-todo-2026-07-06.md | 2 +- .../Platform/ShellService.cs | 20 ++++++++++++++++++- .../PlatformServiceTests.cs | 12 +++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index 49b1681..d9ecaeb 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -12,7 +12,7 @@ Source report: `docs/code-review-2026-07-06.md` ## High Priority - [x] Fix WebVTT import so cue end times and duration are preserved. -- [ ] Remove command-string path interpolation from `ShellService.OpenTerminalAsync`. +- [x] Remove command-string path interpolation from `ShellService.OpenTerminalAsync`. - [ ] Complete or safely hide the file association service/command surface. - [ ] Prevent preview from opening as an empty stub when no chapters are loaded. diff --git a/src/ChapterTool.Infrastructure/Platform/ShellService.cs b/src/ChapterTool.Infrastructure/Platform/ShellService.cs index bf01f87..2907448 100644 --- a/src/ChapterTool.Infrastructure/Platform/ShellService.cs +++ b/src/ChapterTool.Infrastructure/Platform/ShellService.cs @@ -59,7 +59,7 @@ public ValueTask OpenTerminalAsync(string directoryPath, CancellationToken cance return ValueTask.CompletedTask; } - Run("cmd", "/c", $"start cmd /k \"cd /d {directoryPath}\""); + Run(CreateWindowsCommandPromptStartInfo(directoryPath)); } else if (OperatingSystem.IsMacOS()) { @@ -92,6 +92,11 @@ private static void Run(string fileName, params string[] arguments) startInfo.ArgumentList.Add(arg); } + Run(startInfo); + } + + private static void Run(ProcessStartInfo startInfo) + { using var process = Process.Start(startInfo); } @@ -107,4 +112,17 @@ private static bool TryRun(string fileName, params string[] arguments) return false; } } + + internal static ProcessStartInfo CreateWindowsCommandPromptStartInfo(string directoryPath) + { + var startInfo = new ProcessStartInfo + { + FileName = "cmd.exe", + WorkingDirectory = directoryPath, + UseShellExecute = false, + CreateNoWindow = false + }; + startInfo.ArgumentList.Add("/k"); + return startInfo; + } } diff --git a/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs b/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs index 116b880..33ceedf 100644 --- a/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs +++ b/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs @@ -78,4 +78,16 @@ public async Task Memory_clipboard_dialog_localization_and_window_services_are_t await windows.HideAsync("preview", TestContext.Current.CancellationToken); Assert.Equal(["show:preview", "hide:preview"], windows.Calls); } + + [Fact] + public void Windows_terminal_fallback_keeps_directory_out_of_command_arguments() + { + const string directory = "C:\\Temp & calc \"quoted\""; + + var startInfo = ShellService.CreateWindowsCommandPromptStartInfo(directory); + + Assert.Equal("cmd.exe", startInfo.FileName); + Assert.Equal(directory, startInfo.WorkingDirectory); + Assert.Equal(["/k"], startInfo.ArgumentList); + } } From 3199634590e109b37d91510a0a0180221a3e84d2 Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 23:10:55 +0800 Subject: [PATCH 04/22] fix: complete Windows file association registration --- docs/code-review-fix-todo-2026-07-06.md | 3 +- .../Platform/WindowsFileAssociationService.cs | 71 ++++++++++++++++--- .../PlatformServiceTests.cs | 9 +++ 3 files changed, 71 insertions(+), 12 deletions(-) diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index d9ecaeb..34b137a 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -13,7 +13,8 @@ Source report: `docs/code-review-2026-07-06.md` - [x] Fix WebVTT import so cue end times and duration are preserved. - [x] Remove command-string path interpolation from `ShellService.OpenTerminalAsync`. -- [ ] Complete or safely hide the file association service/command surface. +- [x] Complete Windows file association registry behavior so registration writes an open command and unregistration protects existing associations. +- [ ] Add or explicitly defer a non-primary settings/command surface for file association. - [ ] Prevent preview from opening as an empty stub when no chapters are loaded. ## Medium Priority diff --git a/src/ChapterTool.Infrastructure/Platform/WindowsFileAssociationService.cs b/src/ChapterTool.Infrastructure/Platform/WindowsFileAssociationService.cs index 4dc44e7..00c6693 100644 --- a/src/ChapterTool.Infrastructure/Platform/WindowsFileAssociationService.cs +++ b/src/ChapterTool.Infrastructure/Platform/WindowsFileAssociationService.cs @@ -13,6 +13,7 @@ namespace ChapterTool.Infrastructure.Platform; public sealed class WindowsFileAssociationService : IFileAssociationService { private const string ClassesRoot = @"Software\Classes"; + private const string OwnerValueName = "ChapterToolOwner"; public ValueTask RegisterAsync( string extension, @@ -23,18 +24,32 @@ public ValueTask RegisterAsync( cancellationToken.ThrowIfCancellationRequested(); try { - // Register ProgId: HKCU\Software\Classes\ + var normalizedExtension = NormalizeExtension(extension); + var applicationPath = CurrentApplicationPath(); + using (var progIdKey = Registry.CurrentUser.CreateSubKey($@"{ClassesRoot}\{progId}")) { progIdKey.SetValue(string.Empty, description); + progIdKey.SetValue(OwnerValueName, progId); + } + + using (var iconKey = Registry.CurrentUser.CreateSubKey($@"{ClassesRoot}\{progId}\DefaultIcon")) + { + iconKey.SetValue(string.Empty, $"{applicationPath},0"); } - // Map extension to ProgId: HKCU\Software\Classes\ - using (var extKey = Registry.CurrentUser.CreateSubKey($@"{ClassesRoot}\{extension}")) + using (var commandKey = Registry.CurrentUser.CreateSubKey($@"{ClassesRoot}\{progId}\shell\open\command")) + { + commandKey.SetValue(string.Empty, BuildOpenCommand(applicationPath)); + } + + using (var extKey = Registry.CurrentUser.CreateSubKey($@"{ClassesRoot}\{normalizedExtension}")) { extKey.SetValue(string.Empty, progId); + extKey.SetValue(OwnerValueName, progId); } + NotifyShellAssociationChanged(); return ValueTask.FromResult(new FileAssociationResult(true, [])); } catch (Exception ex) @@ -44,7 +59,7 @@ public ValueTask RegisterAsync( [new ChapterDiagnostic( DiagnosticSeverity.Error, "FileAssociationRegistrationFailed", - $"Failed to register file association for {extension}: {ex.Message}")])); + $"Failed to register file association for {NormalizeExtension(extension)}: {ex.Message}")])); } } @@ -56,12 +71,22 @@ public ValueTask UnregisterAsync( cancellationToken.ThrowIfCancellationRequested(); try { - // Remove extension mapping - Registry.CurrentUser.DeleteSubKey($@"{ClassesRoot}\{extension}", throwOnMissingSubKey: false); + var normalizedExtension = NormalizeExtension(extension); + using (var extKey = Registry.CurrentUser.OpenSubKey($@"{ClassesRoot}\{normalizedExtension}", writable: true)) + { + if (ExtensionBelongsToProgId(extKey, progId)) + { + Registry.CurrentUser.DeleteSubKeyTree($@"{ClassesRoot}\{normalizedExtension}", throwOnMissingSubKey: false); + } + else if (extKey?.GetValue(OwnerValueName) is not null) + { + extKey.DeleteValue(OwnerValueName, throwOnMissingValue: false); + } + } - // Remove ProgId Registry.CurrentUser.DeleteSubKeyTree($@"{ClassesRoot}\{progId}", throwOnMissingSubKey: false); + NotifyShellAssociationChanged(); return ValueTask.FromResult(new FileAssociationResult(true, [])); } catch (Exception ex) @@ -71,7 +96,7 @@ public ValueTask UnregisterAsync( [new ChapterDiagnostic( DiagnosticSeverity.Error, "FileAssociationUnregistrationFailed", - $"Failed to unregister file association for {extension}: {ex.Message}")])); + $"Failed to unregister file association for {NormalizeExtension(extension)}: {ex.Message}")])); } } @@ -83,9 +108,13 @@ public ValueTask IsRegisteredAsync( cancellationToken.ThrowIfCancellationRequested(); try { - using var extKey = Registry.CurrentUser.OpenSubKey($@"{ClassesRoot}\{extension}"); + var normalizedExtension = NormalizeExtension(extension); + using var extKey = Registry.CurrentUser.OpenSubKey($@"{ClassesRoot}\{normalizedExtension}"); + using var commandKey = Registry.CurrentUser.OpenSubKey($@"{ClassesRoot}\{progId}\shell\open\command"); if (extKey?.GetValue(string.Empty) is string registeredProgId - && string.Equals(registeredProgId, progId, StringComparison.OrdinalIgnoreCase)) + && string.Equals(registeredProgId, progId, StringComparison.OrdinalIgnoreCase) + && commandKey?.GetValue(string.Empty) is string command + && !string.IsNullOrWhiteSpace(command)) { return ValueTask.FromResult(new FileAssociationResult(true, [])); } @@ -99,7 +128,27 @@ public ValueTask IsRegisteredAsync( [new ChapterDiagnostic( DiagnosticSeverity.Warning, "FileAssociationCheckFailed", - $"Failed to check file association for {extension}: {ex.Message}")])); + $"Failed to check file association for {NormalizeExtension(extension)}: {ex.Message}")])); } } + + internal static string BuildOpenCommand(string applicationPath) => $"\"{applicationPath}\" \"%1\""; + + internal static bool ExtensionBelongsToProgId(RegistryKey? extensionKey, string progId) => + extensionKey?.GetValue(string.Empty) is string registeredProgId + && string.Equals(registeredProgId, progId, StringComparison.OrdinalIgnoreCase); + + private static string NormalizeExtension(string extension) => + extension.StartsWith(".", StringComparison.Ordinal) ? extension : $".{extension}"; + + private static string CurrentApplicationPath() => + Environment.ProcessPath + ?? System.Reflection.Assembly.GetEntryAssembly()?.Location + ?? AppContext.BaseDirectory; + + private static void NotifyShellAssociationChanged() => + SHChangeNotify(0x08000000, 0x0000, IntPtr.Zero, IntPtr.Zero); + + [System.Runtime.InteropServices.DllImport("shell32.dll")] + private static extern void SHChangeNotify(uint eventId, uint flags, IntPtr item1, IntPtr item2); } diff --git a/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs b/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs index 33ceedf..85492a9 100644 --- a/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs +++ b/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs @@ -90,4 +90,13 @@ public void Windows_terminal_fallback_keeps_directory_out_of_command_arguments() Assert.Equal(directory, startInfo.WorkingDirectory); Assert.Equal(["/k"], startInfo.ArgumentList); } + + [Fact] + [System.Runtime.Versioning.SupportedOSPlatform("windows")] + public void Windows_file_association_builds_open_command_with_file_placeholder() + { + var command = WindowsFileAssociationService.BuildOpenCommand(@"C:\Program Files\ChapterTool\ChapterTool.exe"); + + Assert.Equal("\"C:\\Program Files\\ChapterTool\\ChapterTool.exe\" \"%1\"", command); + } } From f8227f91d85529fb9e55d911e177055521775ae9 Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 23:18:08 +0800 Subject: [PATCH 05/22] fix: disable empty preview command --- docs/code-review-fix-todo-2026-07-06.md | 2 +- .../ViewModels/MainWindowViewModel.cs | 6 +++--- .../ViewModels/MainWindowViewModelTests.cs | 18 ++++++++++++++++++ 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index 34b137a..182226b 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -15,7 +15,7 @@ Source report: `docs/code-review-2026-07-06.md` - [x] Remove command-string path interpolation from `ShellService.OpenTerminalAsync`. - [x] Complete Windows file association registry behavior so registration writes an open command and unregistration protects existing associations. - [ ] Add or explicitly defer a non-primary settings/command surface for file association. -- [ ] Prevent preview from opening as an empty stub when no chapters are loaded. +- [x] Prevent preview from opening as an empty stub when no chapters are loaded. ## Medium Priority diff --git a/src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs b/src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs index 5034b8a..7991120 100644 --- a/src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs +++ b/src/ChapterTool.Avalonia/ViewModels/MainWindowViewModel.cs @@ -145,7 +145,7 @@ public MainWindowViewModel( return ValueTask.CompletedTask; }, _ => currentInfo is not null); - PreviewCommand = WindowCommand("preview"); + PreviewCommand = WindowCommand("preview", () => currentInfo is not null); LogCommand = WindowCommand("log"); SettingsCommand = WindowCommand("settings"); ColorSettingsCommand = WindowCommand("color-settings"); @@ -1301,8 +1301,8 @@ private static int ComboIndexFor(FrameRateOption option) return frameRateService.Options.FirstOrDefault(option => option.LegacyMplsCode == legacyCode); } - private UiCommand WindowCommand(string id) => - new(async (_, token) => await windowService.ShowAsync(id, this, token)); + private UiCommand WindowCommand(string id, Func? canExecute = null) => + new(async (_, token) => await windowService.ShowAsync(id, this, token), _ => canExecute?.Invoke() ?? true); private async ValueTask OpenRelatedMediaAsync(object? parameter, CancellationToken cancellationToken) { diff --git a/tests/ChapterTool.Avalonia.Tests/ViewModels/MainWindowViewModelTests.cs b/tests/ChapterTool.Avalonia.Tests/ViewModels/MainWindowViewModelTests.cs index 8c77f23..a6948d2 100644 --- a/tests/ChapterTool.Avalonia.Tests/ViewModels/MainWindowViewModelTests.cs +++ b/tests/ChapterTool.Avalonia.Tests/ViewModels/MainWindowViewModelTests.cs @@ -723,6 +723,7 @@ public async Task AuxiliaryWindowCommandsUseWindowService() var windows = new FakeWindowService(); var vm = CreateViewModel(windowService: windows); + await vm.LoadCommand.ExecuteAsync("movie.txt"); await vm.PreviewCommand.ExecuteAsync(); await vm.LogCommand.ExecuteAsync(); @@ -748,6 +749,23 @@ public async Task PreviewAndLogUseCurrentChapterState() Assert.Equal(string.Empty, vm.LogText()); } + [Fact] + public async Task PreviewCommandRequiresLoadedChapterState() + { + var windows = new FakeWindowService(); + var vm = CreateViewModel(windowService: windows); + + Assert.False(vm.PreviewCommand.CanExecute()); + await vm.PreviewCommand.ExecuteAsync(); + Assert.Empty(windows.Opened); + + await vm.LoadCommand.ExecuteAsync("movie.txt"); + + Assert.True(vm.PreviewCommand.CanExecute()); + await vm.PreviewCommand.ExecuteAsync(); + Assert.Equal(["preview"], windows.Opened); + } + [Fact] public async Task OpenRelatedMediaUsesShellServiceWhenReferenceExists() { From 31a8936ffa61f78f9da35915e17ab003914db18e Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 23:19:48 +0800 Subject: [PATCH 06/22] refactor: remove unused cue parser injection --- docs/code-review-fix-todo-2026-07-06.md | 2 +- src/ChapterTool.Core/Importing/Cue/CueChapterImporter.cs | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index 182226b..e1ab31a 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -19,7 +19,7 @@ Source report: `docs/code-review-2026-07-06.md` ## Medium Priority -- [ ] Remove the fake injectable parser from `CueChapterImporter`. +- [x] Remove the fake injectable parser from `CueChapterImporter`. - [ ] Remove fake async behavior from `IfoChapterImporter` and handle `request.Content` consistently. - [ ] Remove the Infrastructure dependency from Core tests. - [ ] Validate external tools as executables, not just existing files. diff --git a/src/ChapterTool.Core/Importing/Cue/CueChapterImporter.cs b/src/ChapterTool.Core/Importing/Cue/CueChapterImporter.cs index c61b84d..c05d048 100644 --- a/src/ChapterTool.Core/Importing/Cue/CueChapterImporter.cs +++ b/src/ChapterTool.Core/Importing/Cue/CueChapterImporter.cs @@ -1,9 +1,7 @@ namespace ChapterTool.Core.Importing.Cue; -public sealed class CueChapterImporter(CueSheetParser? parser = null) : IChapterImporter +public sealed class CueChapterImporter : IChapterImporter { - private readonly CueSheetParser parser = parser ?? new CueSheetParser(); - public string Id => "cue"; public IReadOnlySet SupportedExtensions { get; } = new HashSet(StringComparer.OrdinalIgnoreCase) From 8710119d725932619fbdd300ceac3386d5b64905 Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 23:23:11 +0800 Subject: [PATCH 07/22] fix: import IFO chapters from request content --- docs/code-review-fix-todo-2026-07-06.md | 2 +- .../Importing/Disc/IfoChapterImporter.cs | 49 +++++++++++++++---- .../Importing/IfoImporterTests.cs | 17 +++++++ 3 files changed, 57 insertions(+), 11 deletions(-) diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index e1ab31a..2f4811e 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -20,7 +20,7 @@ Source report: `docs/code-review-2026-07-06.md` ## Medium Priority - [x] Remove the fake injectable parser from `CueChapterImporter`. -- [ ] Remove fake async behavior from `IfoChapterImporter` and handle `request.Content` consistently. +- [x] Remove fake async behavior from `IfoChapterImporter` and handle `request.Content` consistently. - [ ] Remove the Infrastructure dependency from Core tests. - [ ] Validate external tools as executables, not just existing files. - [ ] Preserve and surface corrupt settings files instead of silently resetting. diff --git a/src/ChapterTool.Core/Importing/Disc/IfoChapterImporter.cs b/src/ChapterTool.Core/Importing/Disc/IfoChapterImporter.cs index 93e25d3..17c9216 100644 --- a/src/ChapterTool.Core/Importing/Disc/IfoChapterImporter.cs +++ b/src/ChapterTool.Core/Importing/Disc/IfoChapterImporter.cs @@ -15,10 +15,12 @@ public sealed partial class IfoChapterImporter : IChapterImporter public async ValueTask ImportAsync(ChapterImportRequest request, CancellationToken cancellationToken) { - await Task.CompletedTask; + Stream? ownedStream = null; try { - var options = GetStreams(request.Path) + var stream = await OpenImportStreamAsync(request, cancellationToken); + ownedStream = ReferenceEquals(stream, request.Content) ? null : stream; + var options = GetStreams(request.Path, stream) .Select((info, index) => new ChapterSourceOption( $"pgc-{index}", $"{info.SourceName}__{info.Chapters.Count}", @@ -37,15 +39,25 @@ public async ValueTask ImportAsync(ChapterImportRequest req { return ChapterImportResult.Failed(Error("InvalidIfo", exception.Message)); } + finally + { + ownedStream?.Dispose(); + } } public static IReadOnlyList GetStreams(string path) { - var count = GetPgcCount(path); + using var stream = File.OpenRead(path); + return GetStreams(path, stream); + } + + private static IReadOnlyList GetStreams(string path, Stream stream) + { + var count = GetPgcCount(stream); var streams = new List(); for (var i = 1; i <= count; i++) { - var info = GetChapterInfo(path, i); + var info = GetChapterInfo(path, stream, i); if (info is not null) { streams.Add(info); @@ -68,9 +80,9 @@ public static TimeSpan ConvertDvdPlaybackTime(byte hour, byte minute, byte secon return TimeSpan.FromSeconds(totalFrames / rate); } - private static ChapterInfo? GetChapterInfo(string path, int programChain) + private static ChapterInfo? GetChapterInfo(string path, Stream stream, int programChain) { - var chapters = GetChapters(path, programChain, out var duration, out var isNtsc); + var chapters = GetChapters(stream, programChain, out var duration, out var isNtsc); if (duration.TotalSeconds < 10) { return null; @@ -93,11 +105,10 @@ public static TimeSpan ConvertDvdPlaybackTime(byte hour, byte minute, byte secon chapters); } - private static List GetChapters(string path, int programChain, out TimeSpan duration, out bool isNtsc) + private static List GetChapters(Stream stream, int programChain, out TimeSpan duration, out bool isNtsc) { duration = TimeSpan.Zero; isNtsc = true; - using var stream = File.OpenRead(path); var pcgit = GetPcgitPosition(stream); var chainOffset = GetChainOffset(stream, pcgit, programChain); var programCount = GetNumberOfPrograms(stream, pcgit, chainOffset); @@ -137,14 +148,32 @@ private static List GetChapters(string path, int programChain, out Time return chapters; } - private static int GetPgcCount(string path) + private static int GetPgcCount(Stream stream) { - using var stream = File.OpenRead(path); var offset = ToInt32(ReadBlock(stream, 0xCC, 4)); stream.Position = 2048 * offset + 0x01; return stream.ReadByte(); } + private static async ValueTask OpenImportStreamAsync(ChapterImportRequest request, CancellationToken cancellationToken) + { + if (request.Content is null) + { + return File.OpenRead(request.Path); + } + + if (request.Content.CanSeek) + { + request.Content.Position = 0; + return request.Content; + } + + var memory = new MemoryStream(); + await request.Content.CopyToAsync(memory, cancellationToken); + memory.Position = 0; + return memory; + } + private static long GetPcgitPosition(Stream stream) => ToInt32(ReadBlock(stream, 0xCC, 4)) * 0x800L; private static uint GetChainOffset(Stream stream, long pcgit, int programChain) => diff --git a/tests/ChapterTool.Core.Tests/Importing/IfoImporterTests.cs b/tests/ChapterTool.Core.Tests/Importing/IfoImporterTests.cs index e744fb6..eafee7d 100644 --- a/tests/ChapterTool.Core.Tests/Importing/IfoImporterTests.cs +++ b/tests/ChapterTool.Core.Tests/Importing/IfoImporterTests.cs @@ -58,6 +58,23 @@ public async Task Vts33SampleImportsHighTitleNumber() Assert.Equal(["VTS_33_1", "VTS_33_2", "VTS_33_3"], infos.Select(static info => info.SourceName)); } + [Fact] + public async Task ImportAsyncReadsRequestContentStream() + { + var importer = new IfoChapterImporter(); + var path = FixtureResolver.Fixture("Importing", "Disc", "Ifo", "VTS_05_0.IFO"); + await using var content = File.OpenRead(path); + + var result = await importer.ImportAsync( + new ChapterImportRequest(path, content), + TestContext.Current.CancellationToken); + + Assert.True(result.Success, Diagnostics(result)); + var info = result.Groups.Single().Options.Select(static option => option.ChapterInfo).First(); + Assert.Equal("VTS_05_1", info.SourceName); + Assert.Equal(7, info.Chapters.Count); + } + [Theory] [InlineData("NULL.IFO", "NoChaptersFound")] [InlineData("OUT_OF_RANGE.IFO", "InvalidIfo")] From 409b5101b951125f517db1bbb9a1972964577897 Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 23:26:17 +0800 Subject: [PATCH 08/22] test: decouple core media tests from infrastructure --- docs/code-review-fix-todo-2026-07-06.md | 2 +- .../ChapterTool.Core.Tests.csproj | 1 - .../Importing/MediaChapterImporterTests.cs | 81 +++++++++---------- 3 files changed, 40 insertions(+), 44 deletions(-) diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index 2f4811e..f46e1b9 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -21,7 +21,7 @@ Source report: `docs/code-review-2026-07-06.md` - [x] Remove the fake injectable parser from `CueChapterImporter`. - [x] Remove fake async behavior from `IfoChapterImporter` and handle `request.Content` consistently. -- [ ] Remove the Infrastructure dependency from Core tests. +- [x] Remove the Infrastructure dependency from Core tests. - [ ] Validate external tools as executables, not just existing files. - [ ] Preserve and surface corrupt settings files instead of silently resetting. - [ ] Return or log shell service failures instead of swallowing them. diff --git a/tests/ChapterTool.Core.Tests/ChapterTool.Core.Tests.csproj b/tests/ChapterTool.Core.Tests/ChapterTool.Core.Tests.csproj index bf733b2..542eefe 100644 --- a/tests/ChapterTool.Core.Tests/ChapterTool.Core.Tests.csproj +++ b/tests/ChapterTool.Core.Tests/ChapterTool.Core.Tests.csproj @@ -27,7 +27,6 @@ - diff --git a/tests/ChapterTool.Core.Tests/Importing/MediaChapterImporterTests.cs b/tests/ChapterTool.Core.Tests/Importing/MediaChapterImporterTests.cs index c9efa95..a8b1469 100644 --- a/tests/ChapterTool.Core.Tests/Importing/MediaChapterImporterTests.cs +++ b/tests/ChapterTool.Core.Tests/Importing/MediaChapterImporterTests.cs @@ -1,7 +1,5 @@ using ChapterTool.Core.Importing; using ChapterTool.Core.Importing.Media; -using ChapterTool.Core.Services; -using ChapterTool.Infrastructure.Importing.Media; namespace ChapterTool.Core.Tests.Importing; @@ -10,7 +8,11 @@ public sealed class MediaChapterImporterTests [Fact] public async Task ImportAsyncMapsOrderedStartsEndsDurationAndUnicodeTitles() { - var importer = CreateImporter("ffprobe_chapters_single_edition.json"); + var importer = CreateImporter( + Entry(0, 0, "1/1000", 0, 5000, "0.000000", "5.000000", ("title", "Chapter 01")), + Entry(1, 1, "1/1000", 5000, 12000, "5.000000", "12.000000", ("title", "Chapter 02")), + Entry(2, 2, "1/1000", 12000, 20000, "12.000000", "20.000000", ("title", "章节 03")), + Entry(3, 3, "1/1000", 20000, 30000, "20.000000", "30.000000", ("title", "Chapter 04"))); var result = await importer.ImportAsync(new ChapterImportRequest("movie.mp4"), TestContext.Current.CancellationToken); @@ -27,7 +29,11 @@ public async Task ImportAsyncMapsOrderedStartsEndsDurationAndUnicodeTitles() [Fact] public async Task ImportAsyncFallsBackToTimeBaseAndTitleFallbacks() { - var importer = CreateImporter("ffprobe_chapters_time_base_fallback.json"); + var importer = CreateImporter( + Entry(0, 0, "1/44100", 0, 220500, "0.000000", "5.000000", ("title", "Start with decimals, end with decimals")), + Entry(1, 1, "1/44100", 441000, 661500, null, null, ("title", "Time base fallback only")), + Entry(2, 2, "1/1000", 20000, 35000, "20.000000", "35.000000", ("TITLE", "Uppercase TITLE tag")), + Entry(3, 3, "1/1000", 35000, 40000, "35.000000", "40.000000")); var result = await importer.ImportAsync(new ChapterImportRequest("audio.ogg"), TestContext.Current.CancellationToken); @@ -40,7 +46,11 @@ public async Task ImportAsyncFallsBackToTimeBaseAndTitleFallbacks() [Fact] public async Task ImportAsyncPreservesMissingAndNonContiguousEnds() { - var importer = CreateImporter("ffprobe_chapters_non_contiguous.json"); + var importer = CreateImporter( + Entry(0, 0, "1/1000", 0, 8000, "0.000000", "8.000000", ("title", "Overlaps next")), + Entry(1, 1, "1/1000", 5000, 15000, "5.000000", "15.000000", ("title", "Starts before previous ends")), + Entry(2, 2, "1/1000", 20000, null, "20.000000", null, ("title", "Missing end")), + Entry(3, 3, "1/1000", 35000, 35000, "35.000000", "35.000000", ("title", "End equals start"))); var result = await importer.ImportAsync(new ChapterImportRequest("movie.nut"), TestContext.Current.CancellationToken); @@ -56,7 +66,7 @@ public async Task ImportAsyncPreservesMissingAndNonContiguousEnds() [Fact] public async Task ImportAsyncFailsEmptyChapterOutput() { - var importer = CreateImporter("ffprobe_chapters_empty.json"); + var importer = CreateImporter(); var result = await importer.ImportAsync(new ChapterImportRequest("empty.wav"), TestContext.Current.CancellationToken); @@ -67,7 +77,9 @@ public async Task ImportAsyncFailsEmptyChapterOutput() [Fact] public async Task ImportAsyncSkipsInvalidStartsAndFailsWhenNoneRemain() { - var importer = CreateImporter("ffprobe_chapters_invalid_timestamps.json"); + var importer = CreateImporter( + Entry(0, 0, "1/1000", -1000, 1000, "-1.000000", "1.000000", ("title", "Negative")), + Entry(1, 1, "bad", 10, 20, null, null, ("title", "Bad time base"))); var result = await importer.ImportAsync(new ChapterImportRequest("bad.mp3"), TestContext.Current.CancellationToken); @@ -78,7 +90,11 @@ public async Task ImportAsyncSkipsInvalidStartsAndFailsWhenNoneRemain() [Fact] public async Task ImportAsyncGroupsEditionsByEditionUidWithUntaggedLast() { - var importer = CreateImporter("ffprobe_chapters_mixed_edition.json"); + var importer = CreateImporter( + Entry(0, 0, "1/1000", 0, 10000, "0.000000", "10.000000", ("title", "Tagged Chapter 1"), ("EDITION_UID", "100")), + Entry(1, 1, "1/1000", 10000, 20000, "10.000000", "20.000000", ("title", "Tagged Chapter 2"), ("EDITION_UID", "100")), + Entry(2, 2, "1/1000", 0, 5000, "0.000000", "5.000000", ("title", "Untagged Chapter 1")), + Entry(3, 3, "1/1000", 5000, 15000, "5.000000", "15.000000", ("title", "Untagged Chapter 2"))); var result = await importer.ImportAsync(new ChapterImportRequest("movie.mkv"), TestContext.Current.CancellationToken); @@ -91,45 +107,26 @@ public async Task ImportAsyncGroupsEditionsByEditionUidWithUntaggedLast() Assert.Equal(["Untagged Chapter 1", "Untagged Chapter 2"], options[1].ChapterInfo.Chapters.Select(static chapter => chapter.Name)); } - private static MediaChapterImporter CreateImporter(string jsonFixtureFileName) - { - var fixtureDirectory = Path.Combine(RepositoryRoot(), "tests", "ChapterTool.Core.Tests", "Fixtures", "Importing", "Media", "FfprobeJson"); - var jsonPath = Path.Combine(fixtureDirectory, jsonFixtureFileName); - var json = File.ReadAllText(jsonPath); - var reader = new FfprobeMediaChapterReader( - new FakeToolLocator(new ExternalToolLocation(true, "ffprobe")), - new FakeProcessRunner(new ProcessRunResult(0, json, "", false, false, "ffprobe", [], null))); - return new MediaChapterImporter(reader); - } - - private static string RepositoryRoot() - { - var directory = new DirectoryInfo(AppContext.BaseDirectory); - while (directory is not null) - { - if (File.Exists(Path.Combine(directory.FullName, "ChapterTool.Avalonia.slnx"))) - { - return directory.FullName; - } - - directory = directory.Parent; - } - - throw new DirectoryNotFoundException("Could not locate repository root from test output directory."); - } + private static MediaChapterImporter CreateImporter(params MediaChapterEntry[] entries) => + new(new FakeMediaChapterReader(MediaChapterReadResult.Succeeded(entries))); private static string Diagnostics(ChapterImportResult result) => string.Join(Environment.NewLine, result.Diagnostics.Select(static diagnostic => $"{diagnostic.Severity} {diagnostic.Code}: {diagnostic.Message}")); - private sealed class FakeToolLocator(ExternalToolLocation location) : IExternalToolLocator - { - public ValueTask LocateAsync(string toolId, CancellationToken cancellationToken) => - ValueTask.FromResult(location); - } - - private sealed class FakeProcessRunner(ProcessRunResult result) : IProcessRunner + private static MediaChapterEntry Entry( + int sourceOrder, + int? id, + string? timeBase, + long? start, + long? end, + string? startTime, + string? endTime, + params (string Key, string Value)[] tags) => + new(id, timeBase, start, end, startTime, endTime, tags.ToDictionary(static tag => tag.Key, static tag => tag.Value, StringComparer.Ordinal), sourceOrder); + + private sealed class FakeMediaChapterReader(MediaChapterReadResult result) : IMediaChapterReader { - public ValueTask RunAsync(ProcessRunRequest request, CancellationToken cancellationToken) => + public ValueTask ReadAsync(string path, CancellationToken cancellationToken) => ValueTask.FromResult(result); } } From 6e43b091248cb1714c998613687aafdcc667e7e3 Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 23:33:25 +0800 Subject: [PATCH 09/22] fix: require executable external tool candidates --- docs/code-review-fix-todo-2026-07-06.md | 2 +- .../Tools/ExternalToolLocator.cs | 44 +++++++-- .../ExternalToolLocatorTests.cs | 89 ++++++++++++++----- 3 files changed, 106 insertions(+), 29 deletions(-) diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index f46e1b9..d17f6d8 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -22,7 +22,7 @@ Source report: `docs/code-review-2026-07-06.md` - [x] Remove the fake injectable parser from `CueChapterImporter`. - [x] Remove fake async behavior from `IfoChapterImporter` and handle `request.Content` consistently. - [x] Remove the Infrastructure dependency from Core tests. -- [ ] Validate external tools as executables, not just existing files. +- [x] Validate external tools as executables, not just existing files. - [ ] Preserve and surface corrupt settings files instead of silently resetting. - [ ] Return or log shell service failures instead of swallowing them. - [ ] Clarify whether `FfmpegPath` means ffmpeg or ffprobe directory and validate accordingly. diff --git a/src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs b/src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs index 26b4a1c..5428f75 100644 --- a/src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs +++ b/src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs @@ -34,7 +34,7 @@ public async ValueTask LocateAsync(string toolId, Cancella foreach (var candidate in ExternalToolPathResolver.ExpandConfiguredCandidates(configuredPath, executableName)) { - if (File.Exists(candidate)) + if (IsExecutableCandidate(candidate, executableName)) { return Cache(cacheKey, new ExternalToolLocation(true, candidate)); } @@ -43,7 +43,7 @@ public async ValueTask LocateAsync(string toolId, Cancella foreach (var directory in searchDirectories ?? []) { var candidate = Path.Combine(directory, executableName); - if (File.Exists(candidate)) + if (IsExecutableCandidate(candidate, executableName)) { return Cache(cacheKey, new ExternalToolLocation(true, candidate)); } @@ -52,7 +52,7 @@ public async ValueTask LocateAsync(string toolId, Cancella foreach (var candidate in defaultCandidateProvider.FindCandidates(toolId, executableName)) { cancellationToken.ThrowIfCancellationRequested(); - if (File.Exists(candidate)) + if (IsExecutableCandidate(candidate, executableName)) { return Cache(cacheKey, new ExternalToolLocation(true, candidate)); } @@ -63,7 +63,7 @@ public async ValueTask LocateAsync(string toolId, Cancella foreach (var candidate in mkvToolNixInstallProbe.FindMkvExtractCandidates(executableName)) { cancellationToken.ThrowIfCancellationRequested(); - if (File.Exists(candidate)) + if (IsExecutableCandidate(candidate, executableName)) { return Cache(cacheKey, new ExternalToolLocation(true, candidate)); } @@ -86,6 +86,39 @@ public async ValueTask LocateAsync(string toolId, Cancella _ => null }; + private static bool IsExecutableCandidate(string path, string executableName) + { + if (!File.Exists(path)) + { + return false; + } + + if (OperatingSystem.IsWindows()) + { + return Path.GetFileName(path).Equals(executableName, StringComparison.OrdinalIgnoreCase) + || Path.GetExtension(path).Equals(".exe", StringComparison.OrdinalIgnoreCase); + } + + try + { + var mode = File.GetUnixFileMode(path); + const UnixFileMode executeBits = UnixFileMode.UserExecute | UnixFileMode.GroupExecute | UnixFileMode.OtherExecute; + return (mode & executeBits) != 0; + } + catch (IOException) + { + return false; + } + catch (UnauthorizedAccessException) + { + return false; + } + catch (PlatformNotSupportedException) + { + return true; + } + } + private ExternalToolLocation? TryGetCachedLocation(ToolCacheKey key) { lock (cacheSyncRoot) @@ -97,7 +130,8 @@ public async ValueTask LocateAsync(string toolId, Cancella if (cached.Location.Found) { - if (!string.IsNullOrWhiteSpace(cached.Location.Path) && File.Exists(cached.Location.Path)) + var executableName = ExternalToolPathResolver.ExecutableName(key.ToolId); + if (!string.IsNullOrWhiteSpace(cached.Location.Path) && IsExecutableCandidate(cached.Location.Path, executableName)) { return cached.Location; } diff --git a/tests/ChapterTool.Infrastructure.Tests/ExternalToolLocatorTests.cs b/tests/ChapterTool.Infrastructure.Tests/ExternalToolLocatorTests.cs index 9e8a35e..0040c0a 100644 --- a/tests/ChapterTool.Infrastructure.Tests/ExternalToolLocatorTests.cs +++ b/tests/ChapterTool.Infrastructure.Tests/ExternalToolLocatorTests.cs @@ -12,11 +12,11 @@ public async Task LocateAsync_uses_configured_mkvtoolnix_path_before_search_path var configuredDirectory = Path.Combine(root, "configured"); Directory.CreateDirectory(configuredDirectory); var expectedToolPath = Path.Combine(configuredDirectory, ToolExecutable("mkvextract")); - await File.WriteAllTextAsync(expectedToolPath, ""); + await CreateToolFileAsync(expectedToolPath); var searchDirectory = Path.Combine(root, "search"); Directory.CreateDirectory(searchDirectory); - await File.WriteAllTextAsync(Path.Combine(searchDirectory, ToolExecutable("mkvextract")), ""); + await CreateToolFileAsync(Path.Combine(searchDirectory, ToolExecutable("mkvextract"))); var settingsStore = new AppSettingsStore(root, [root]); await settingsStore.SaveAsync( @@ -35,11 +35,11 @@ public async Task LocateAsync_uses_configured_mkvextract_executable_before_searc { var root = CreateTempDirectory(); var configuredExecutable = Path.Combine(root, ToolExecutable("custom-mkvextract")); - await File.WriteAllTextAsync(configuredExecutable, ""); + await CreateToolFileAsync(configuredExecutable); var searchDirectory = Path.Combine(root, "search"); Directory.CreateDirectory(searchDirectory); - await File.WriteAllTextAsync(Path.Combine(searchDirectory, ToolExecutable("mkvextract")), ""); + await CreateToolFileAsync(Path.Combine(searchDirectory, ToolExecutable("mkvextract"))); var settingsStore = new AppSettingsStore(root, [root]); await settingsStore.SaveAsync( @@ -60,10 +60,10 @@ public async Task LocateAsync_uses_search_path_before_platform_install_discovery var searchDirectory = Path.Combine(root, "search"); Directory.CreateDirectory(searchDirectory); var expectedToolPath = Path.Combine(searchDirectory, ToolExecutable("mkvextract")); - await File.WriteAllTextAsync(expectedToolPath, ""); + await CreateToolFileAsync(expectedToolPath); var platformToolPath = Path.Combine(root, "platform", ToolExecutable("mkvextract")); Directory.CreateDirectory(Path.GetDirectoryName(platformToolPath)!); - await File.WriteAllTextAsync(platformToolPath, ""); + await CreateToolFileAsync(platformToolPath); var locator = CreateLocatorWithoutDefaultCandidates( new AppSettingsStore(root, [root]), @@ -82,7 +82,7 @@ public async Task LocateAsync_uses_platform_mkvtoolnix_discovery_for_mkvextract( var root = CreateTempDirectory(); var platformToolPath = Path.Combine(root, "platform", ToolExecutable("mkvextract")); Directory.CreateDirectory(Path.GetDirectoryName(platformToolPath)!); - await File.WriteAllTextAsync(platformToolPath, ""); + await CreateToolFileAsync(platformToolPath); var locator = CreateLocatorWithoutDefaultCandidates( new AppSettingsStore(root, [root]), @@ -101,7 +101,7 @@ public async Task LocateAsync_does_not_use_platform_discovery_for_eac3to() var root = CreateTempDirectory(); var platformToolPath = Path.Combine(root, "platform", ToolExecutable("mkvextract")); Directory.CreateDirectory(Path.GetDirectoryName(platformToolPath)!); - await File.WriteAllTextAsync(platformToolPath, ""); + await CreateToolFileAsync(platformToolPath); var locator = CreateLocatorWithoutDefaultCandidates( new AppSettingsStore(root, [root]), @@ -129,18 +129,35 @@ public async Task LocateAsync_returns_missing_dependency_when_tool_is_absent() Assert.Equal("MissingDependency", location.DiagnosticCode); } + [Fact] + public async Task LocateAsync_ignores_configured_file_that_is_not_executable() + { + var root = CreateTempDirectory(); + var configuredPath = Path.Combine(root, OperatingSystem.IsWindows() ? "ffprobe.txt" : "ffprobe"); + await File.WriteAllTextAsync(configuredPath, ""); + var settingsStore = new AppSettingsStore(root, [root]); + await settingsStore.SaveAsync(new AppSettings(FfprobePath: configuredPath), TestContext.Current.CancellationToken); + var locator = CreateLocatorWithoutDefaultCandidates(settingsStore, [], new FakeMkvToolNixInstallProbe()); + + var location = await locator.LocateAsync("ffprobe", TestContext.Current.CancellationToken); + + Assert.False(location.Found); + Assert.Null(location.Path); + Assert.Equal("MissingDependency", location.DiagnosticCode); + } + [Fact] public async Task LocateAsync_uses_configured_ffprobe_executable_before_ffmpeg_directory_and_search_paths() { var root = CreateTempDirectory(); var configuredExecutable = Path.Combine(root, ToolExecutable("custom-ffprobe")); - await File.WriteAllTextAsync(configuredExecutable, ""); + await CreateToolFileAsync(configuredExecutable); var ffmpegDirectory = Path.Combine(root, "ffmpeg"); Directory.CreateDirectory(ffmpegDirectory); - await File.WriteAllTextAsync(Path.Combine(ffmpegDirectory, ToolExecutable("ffprobe")), ""); + await CreateToolFileAsync(Path.Combine(ffmpegDirectory, ToolExecutable("ffprobe"))); var searchDirectory = Path.Combine(root, "search"); Directory.CreateDirectory(searchDirectory); - await File.WriteAllTextAsync(Path.Combine(searchDirectory, ToolExecutable("ffprobe")), ""); + await CreateToolFileAsync(Path.Combine(searchDirectory, ToolExecutable("ffprobe"))); var settingsStore = new AppSettingsStore(root, [root]); await settingsStore.SaveAsync( @@ -160,11 +177,11 @@ public async Task LocateAsync_uses_search_path_after_configured_path_is_cleared( var root = CreateTempDirectory(); var configuredDirectory = Path.Combine(root, "configured"); Directory.CreateDirectory(configuredDirectory); - await File.WriteAllTextAsync(Path.Combine(configuredDirectory, ToolExecutable("ffprobe")), ""); + await CreateToolFileAsync(Path.Combine(configuredDirectory, ToolExecutable("ffprobe"))); var searchDirectory = Path.Combine(root, "search"); Directory.CreateDirectory(searchDirectory); var expectedToolPath = Path.Combine(searchDirectory, ToolExecutable("ffprobe")); - await File.WriteAllTextAsync(expectedToolPath, ""); + await CreateToolFileAsync(expectedToolPath); var settingsStore = new AppSettingsStore(root, [root]); await settingsStore.SaveAsync(new AppSettings(FfprobePath: configuredDirectory), TestContext.Current.CancellationToken); await settingsStore.SaveAsync(new AppSettings(FfprobePath: null), TestContext.Current.CancellationToken); @@ -183,11 +200,11 @@ public async Task LocateAsync_uses_updated_settings_after_configured_path_is_cle var configuredDirectory = Path.Combine(root, "configured"); Directory.CreateDirectory(configuredDirectory); var configuredToolPath = Path.Combine(configuredDirectory, ToolExecutable("ffprobe")); - await File.WriteAllTextAsync(configuredToolPath, ""); + await CreateToolFileAsync(configuredToolPath); var searchDirectory = Path.Combine(root, "search"); Directory.CreateDirectory(searchDirectory); var searchToolPath = Path.Combine(searchDirectory, ToolExecutable("ffprobe")); - await File.WriteAllTextAsync(searchToolPath, ""); + await CreateToolFileAsync(searchToolPath); var settingsStore = new AppSettingsStore(root, [root]); await settingsStore.SaveAsync(new AppSettings(FfprobePath: configuredDirectory), TestContext.Current.CancellationToken); var locator = CreateLocatorWithoutDefaultCandidates(settingsStore, [searchDirectory], new FakeMkvToolNixInstallProbe()); @@ -205,7 +222,7 @@ public async Task LocateAsync_reuses_cached_found_location() { var root = CreateTempDirectory(); var toolPath = Path.Combine(root, ToolExecutable("ffprobe")); - await File.WriteAllTextAsync(toolPath, ""); + await CreateToolFileAsync(toolPath); var provider = new CountingDefaultCandidateProvider(toolPath); var locator = new ExternalToolLocator( new AppSettingsStore(root, [root]), @@ -231,8 +248,8 @@ public async Task LocateAsync_rescans_when_cached_found_location_disappears() Directory.CreateDirectory(secondDirectory); var firstToolPath = Path.Combine(firstDirectory, ToolExecutable("ffprobe")); var secondToolPath = Path.Combine(secondDirectory, ToolExecutable("ffprobe")); - await File.WriteAllTextAsync(firstToolPath, ""); - await File.WriteAllTextAsync(secondToolPath, ""); + await CreateToolFileAsync(firstToolPath); + await CreateToolFileAsync(secondToolPath); var locator = CreateLocatorWithoutDefaultCandidates( new AppSettingsStore(root, [root]), [firstDirectory, secondDirectory], @@ -253,7 +270,7 @@ public async Task LocateAsync_uses_configured_ffprobe_directory() var configuredDirectory = Path.Combine(root, "configured"); Directory.CreateDirectory(configuredDirectory); var expectedToolPath = Path.Combine(configuredDirectory, ToolExecutable("ffprobe")); - await File.WriteAllTextAsync(expectedToolPath, ""); + await CreateToolFileAsync(expectedToolPath); var settingsStore = new AppSettingsStore(root, [root]); await settingsStore.SaveAsync( @@ -274,7 +291,7 @@ public async Task LocateAsync_uses_configured_ffmpeg_directory_for_ffprobe() var ffmpegDirectory = Path.Combine(root, "ffmpeg"); Directory.CreateDirectory(ffmpegDirectory); var expectedToolPath = Path.Combine(ffmpegDirectory, ToolExecutable("ffprobe")); - await File.WriteAllTextAsync(expectedToolPath, ""); + await CreateToolFileAsync(expectedToolPath); var settingsStore = new AppSettingsStore(root, [root]); await settingsStore.SaveAsync( @@ -295,7 +312,7 @@ public async Task LocateAsync_uses_search_directory_for_ffprobe_when_unconfigure var searchDirectory = Path.Combine(root, "search"); Directory.CreateDirectory(searchDirectory); var expectedToolPath = Path.Combine(searchDirectory, ToolExecutable("ffprobe")); - await File.WriteAllTextAsync(expectedToolPath, ""); + await CreateToolFileAsync(expectedToolPath); var locator = CreateLocatorWithoutDefaultCandidates(new AppSettingsStore(root, [root]), [searchDirectory], new FakeMkvToolNixInstallProbe()); @@ -312,7 +329,7 @@ public void WindowsProbe_expands_registry_install_directories_and_display_icons( var installDirectory = Path.Combine(root, "MKVToolNix"); Directory.CreateDirectory(installDirectory); var expectedToolPath = Path.Combine(installDirectory, ToolExecutable("mkvextract")); - File.WriteAllText(expectedToolPath, ""); + CreateToolFile(expectedToolPath); var displayIcon = Path.Combine(installDirectory, OperatingSystem.IsWindows() ? "mkvtoolnix-gui.exe,0" : "mkvtoolnix-gui,0"); var probe = new WindowsMkvToolNixInstallProbe( new FakeWindowsRegistryInstallProbe([installDirectory, displayIcon]), @@ -329,7 +346,7 @@ public void MacProbe_finds_mkvextract_inside_versioned_app_bundle() var root = CreateTempDirectory(); var expectedToolPath = Path.Combine(root, "MKVToolNix-96.0.app", "Contents", "MacOS", "mkvextract"); Directory.CreateDirectory(Path.GetDirectoryName(expectedToolPath)!); - File.WriteAllText(expectedToolPath, ""); + CreateToolFile(expectedToolPath); var probe = new MacMkvToolNixInstallProbe([root], enabled: true); var candidates = probe.FindMkvExtractCandidates("mkvextract").ToArray(); @@ -347,6 +364,32 @@ public void UnixProbe_returns_no_platform_install_candidates() private static string ToolExecutable(string name) => OperatingSystem.IsWindows() ? $"{name}.exe" : name; + private static async Task CreateToolFileAsync(string path) + { + await File.WriteAllTextAsync(path, ""); + MarkExecutable(path); + } + + private static void CreateToolFile(string path) + { + File.WriteAllText(path, ""); + MarkExecutable(path); + } + + private static void MarkExecutable(string path) + { + if (OperatingSystem.IsWindows()) + { + return; + } + + File.SetUnixFileMode( + path, + UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute | + UnixFileMode.GroupRead | UnixFileMode.GroupExecute | + UnixFileMode.OtherRead | UnixFileMode.OtherExecute); + } + private static ExternalToolLocator CreateLocatorWithoutDefaultCandidates( AppSettingsStore settingsStore, IReadOnlyList searchDirectories, From 6687f24a6355c139cb180118116bbcb781beb9d1 Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 23:37:30 +0800 Subject: [PATCH 10/22] fix: preserve corrupt settings files --- docs/code-review-fix-todo-2026-07-06.md | 2 +- .../Configuration/AppSettingsStore.cs | 6 ++-- .../Configuration/CorruptSettingsFile.cs | 31 +++++++++++++++++ .../CorruptSettingsFileException.cs | 15 ++++++++ .../Configuration/ThemeSettingsStore.cs | 4 +-- .../SettingsMigrationTests.cs | 34 +++++++++++++++++++ 6 files changed, 85 insertions(+), 7 deletions(-) create mode 100644 src/ChapterTool.Infrastructure/Configuration/CorruptSettingsFile.cs create mode 100644 src/ChapterTool.Infrastructure/Configuration/CorruptSettingsFileException.cs diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index d17f6d8..d3c24df 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -23,7 +23,7 @@ Source report: `docs/code-review-2026-07-06.md` - [x] Remove fake async behavior from `IfoChapterImporter` and handle `request.Content` consistently. - [x] Remove the Infrastructure dependency from Core tests. - [x] Validate external tools as executables, not just existing files. -- [ ] Preserve and surface corrupt settings files instead of silently resetting. +- [x] Preserve and surface corrupt settings files instead of silently resetting. - [ ] Return or log shell service failures instead of swallowing them. - [ ] Clarify whether `FfmpegPath` means ffmpeg or ffprobe directory and validate accordingly. - [ ] Localize native file picker titles and file type labels. diff --git a/src/ChapterTool.Infrastructure/Configuration/AppSettingsStore.cs b/src/ChapterTool.Infrastructure/Configuration/AppSettingsStore.cs index 9ef6901..0d629d8 100644 --- a/src/ChapterTool.Infrastructure/Configuration/AppSettingsStore.cs +++ b/src/ChapterTool.Infrastructure/Configuration/AppSettingsStore.cs @@ -40,11 +40,9 @@ public async ValueTask LoadAsync(CancellationToken cancellationToke Cache(settings, fileState); return settings; } - catch (JsonException) + catch (JsonException exception) { - settings = new AppSettings(); - Cache(settings, fileState); - return settings; + throw CorruptSettingsFile.Preserve(currentPath, exception); } } diff --git a/src/ChapterTool.Infrastructure/Configuration/CorruptSettingsFile.cs b/src/ChapterTool.Infrastructure/Configuration/CorruptSettingsFile.cs new file mode 100644 index 0000000..fd9c355 --- /dev/null +++ b/src/ChapterTool.Infrastructure/Configuration/CorruptSettingsFile.cs @@ -0,0 +1,31 @@ +namespace ChapterTool.Infrastructure.Configuration; + +internal static class CorruptSettingsFile +{ + public static CorruptSettingsFileException Preserve(string path, Exception exception) + { + var backupPath = NextBackupPath(path); + File.Move(path, backupPath); + return new CorruptSettingsFileException(path, backupPath, exception); + } + + private static string NextBackupPath(string path) + { + var backupPath = path + ".corrupt"; + if (!File.Exists(backupPath)) + { + return backupPath; + } + + for (var index = 1; index < int.MaxValue; index++) + { + var candidate = $"{backupPath}.{index}"; + if (!File.Exists(candidate)) + { + return candidate; + } + } + + throw new IOException($"Unable to allocate a corrupt settings backup path for '{path}'."); + } +} diff --git a/src/ChapterTool.Infrastructure/Configuration/CorruptSettingsFileException.cs b/src/ChapterTool.Infrastructure/Configuration/CorruptSettingsFileException.cs new file mode 100644 index 0000000..5bbd40a --- /dev/null +++ b/src/ChapterTool.Infrastructure/Configuration/CorruptSettingsFileException.cs @@ -0,0 +1,15 @@ +namespace ChapterTool.Infrastructure.Configuration; + +public sealed class CorruptSettingsFileException : Exception +{ + public CorruptSettingsFileException(string settingsPath, string backupPath, Exception innerException) + : base($"Settings file '{settingsPath}' contains invalid JSON. The corrupt file was preserved at '{backupPath}'.", innerException) + { + SettingsPath = settingsPath; + BackupPath = backupPath; + } + + public string SettingsPath { get; } + + public string BackupPath { get; } +} diff --git a/src/ChapterTool.Infrastructure/Configuration/ThemeSettingsStore.cs b/src/ChapterTool.Infrastructure/Configuration/ThemeSettingsStore.cs index 920f9f1..acf1186 100644 --- a/src/ChapterTool.Infrastructure/Configuration/ThemeSettingsStore.cs +++ b/src/ChapterTool.Infrastructure/Configuration/ThemeSettingsStore.cs @@ -26,9 +26,9 @@ public async ValueTask LoadAsync(CancellationToken cancellat return await JsonSerializer.DeserializeAsync(stream, JsonOptions, cancellationToken) ?? ThemeColorSettings.Default; } - catch (JsonException) + catch (JsonException exception) { - return ThemeColorSettings.Default; + throw CorruptSettingsFile.Preserve(currentPath, exception); } } diff --git a/tests/ChapterTool.Infrastructure.Tests/SettingsMigrationTests.cs b/tests/ChapterTool.Infrastructure.Tests/SettingsMigrationTests.cs index cd1008f..ba44324 100644 --- a/tests/ChapterTool.Infrastructure.Tests/SettingsMigrationTests.cs +++ b/tests/ChapterTool.Infrastructure.Tests/SettingsMigrationTests.cs @@ -70,6 +70,23 @@ public async Task App_settings_cache_returns_saved_values_without_stale_reads() Assert.Equal(@"C:\Tools\ffprobe.exe", saved.FfprobePath); } + [Fact] + public async Task App_settings_preserves_corrupt_current_file_and_surfaces_error() + { + var root = CreateTempDirectory(); + var settingsPath = Path.Combine(root, "appsettings.json"); + await File.WriteAllTextAsync(settingsPath, "{"); + var store = new AppSettingsStore(root, [root]); + + var exception = await Assert.ThrowsAsync( + async () => await store.LoadAsync(TestContext.Current.CancellationToken)); + + Assert.Equal(settingsPath, exception.SettingsPath); + Assert.Equal(settingsPath + ".corrupt", exception.BackupPath); + Assert.False(File.Exists(settingsPath)); + Assert.Equal("{", await File.ReadAllTextAsync(exception.BackupPath)); + } + [Fact] public async Task Theme_settings_loads_legacy_color_config_in_six_slot_order() { @@ -109,6 +126,23 @@ await File.WriteAllTextAsync( Assert.Equal("#212223", theme.MouseOverColor); } + [Fact] + public async Task Theme_settings_preserves_corrupt_current_file_and_surfaces_error() + { + var root = CreateTempDirectory(); + var settingsPath = Path.Combine(root, "theme-colors.json"); + await File.WriteAllTextAsync(settingsPath, "{"); + var store = new ThemeSettingsStore(root, [root]); + + var exception = await Assert.ThrowsAsync( + async () => await store.LoadAsync(TestContext.Current.CancellationToken)); + + Assert.Equal(settingsPath, exception.SettingsPath); + Assert.Equal(settingsPath + ".corrupt", exception.BackupPath); + Assert.False(File.Exists(settingsPath)); + Assert.Equal("{", await File.ReadAllTextAsync(exception.BackupPath)); + } + private static string CreateTempDirectory() { var path = Path.Combine(Path.GetTempPath(), "ChapterTool.Tests", Guid.NewGuid().ToString("N")); From bb3125b03e0e304a8b6b3363bc8ee801299576dd Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 23:40:24 +0800 Subject: [PATCH 11/22] fix: log shell service failures --- docs/code-review-fix-todo-2026-07-06.md | 2 +- .../Platform/ShellService.cs | 48 ++++++++++++++----- .../PlatformServiceTests.cs | 38 +++++++++++++++ 3 files changed, 75 insertions(+), 13 deletions(-) diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index d3c24df..3d6cf04 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -24,7 +24,7 @@ Source report: `docs/code-review-2026-07-06.md` - [x] Remove the Infrastructure dependency from Core tests. - [x] Validate external tools as executables, not just existing files. - [x] Preserve and surface corrupt settings files instead of silently resetting. -- [ ] Return or log shell service failures instead of swallowing them. +- [x] Return or log shell service failures instead of swallowing them. - [ ] Clarify whether `FfmpegPath` means ffmpeg or ffprobe directory and validate accordingly. - [ ] Localize native file picker titles and file type labels. - [ ] Add accessible names for icon-only buttons. diff --git a/src/ChapterTool.Infrastructure/Platform/ShellService.cs b/src/ChapterTool.Infrastructure/Platform/ShellService.cs index 2907448..e7c665b 100644 --- a/src/ChapterTool.Infrastructure/Platform/ShellService.cs +++ b/src/ChapterTool.Infrastructure/Platform/ShellService.cs @@ -1,14 +1,30 @@ using System.Diagnostics; using ChapterTool.Core.Services; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; namespace ChapterTool.Infrastructure.Platform; public sealed class ShellService : IShellService { + private readonly ILogger logger; + private readonly Func startProcess; + + public ShellService(ILogger? logger = null) + : this(logger, Process.Start) + { + } + + internal ShellService(ILogger? logger, Func startProcess) + { + this.logger = logger ?? NullLogger.Instance; + this.startProcess = startProcess; + } + public ValueTask OpenAsync(string target, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); - using var _ = Process.Start(new ProcessStartInfo + using var _ = Start(new ProcessStartInfo { FileName = target, UseShellExecute = true @@ -38,9 +54,9 @@ public ValueTask RevealInFolderAsync(string filePath, CancellationToken cancella Run("xdg-open", dir); } } - catch + catch (Exception exception) { - // Best-effort: silently ignore failures when platform tool is unavailable. + logger.LogWarning(exception, "Unable to reveal '{FilePath}' in the platform file manager.", filePath); } return ValueTask.CompletedTask; @@ -54,7 +70,7 @@ public ValueTask OpenTerminalAsync(string directoryPath, CancellationToken cance if (OperatingSystem.IsWindows()) { // Prefer Windows Terminal, fall back to cmd - if (TryRun("wt", "-d", directoryPath)) + if (TryRun("wt", out _, "-d", directoryPath)) { return ValueTask.CompletedTask; } @@ -68,18 +84,21 @@ public ValueTask OpenTerminalAsync(string directoryPath, CancellationToken cance else { // Try common terminal emulators - TryRun("x-terminal-emulator", "--working-directory", directoryPath); + if (!TryRun("x-terminal-emulator", out var exception, "--working-directory", directoryPath)) + { + logger.LogWarning(exception, "Unable to open a terminal in '{DirectoryPath}'.", directoryPath); + } } } - catch + catch (Exception exception) { - // Best-effort: silently ignore failures when platform tool is unavailable. + logger.LogWarning(exception, "Unable to open a terminal in '{DirectoryPath}'.", directoryPath); } return ValueTask.CompletedTask; } - private static void Run(string fileName, params string[] arguments) + private void Run(string fileName, params string[] arguments) { var startInfo = new ProcessStartInfo { @@ -95,24 +114,29 @@ private static void Run(string fileName, params string[] arguments) Run(startInfo); } - private static void Run(ProcessStartInfo startInfo) + private void Run(ProcessStartInfo startInfo) { - using var process = Process.Start(startInfo); + using var process = Start(startInfo); } - private static bool TryRun(string fileName, params string[] arguments) + private bool TryRun(string fileName, out Exception? exception, params string[] arguments) { try { Run(fileName, arguments); + exception = null; return true; } - catch + catch (Exception caught) { + exception = caught; return false; } } + private Process Start(ProcessStartInfo startInfo) => + startProcess(startInfo) ?? throw new InvalidOperationException($"Unable to start process '{startInfo.FileName}'."); + internal static ProcessStartInfo CreateWindowsCommandPromptStartInfo(string directoryPath) { var startInfo = new ProcessStartInfo diff --git a/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs b/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs index 85492a9..1a8997b 100644 --- a/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs +++ b/tests/ChapterTool.Infrastructure.Tests/PlatformServiceTests.cs @@ -1,5 +1,6 @@ using ChapterTool.Core.Services; using ChapterTool.Infrastructure.Platform; +using Microsoft.Extensions.Logging; namespace ChapterTool.Infrastructure.Tests; @@ -91,6 +92,20 @@ public void Windows_terminal_fallback_keeps_directory_out_of_command_arguments() Assert.Equal(["/k"], startInfo.ArgumentList); } + [Fact] + public async Task Shell_service_logs_reveal_failures() + { + var logger = new RecordingLogger(); + var service = new ShellService(logger, _ => throw new InvalidOperationException("launcher unavailable")); + + await service.RevealInFolderAsync("missing.mkv", TestContext.Current.CancellationToken); + + var entry = Assert.Single(logger.Entries); + Assert.Equal(LogLevel.Warning, entry.Level); + Assert.IsType(entry.Exception); + Assert.Contains("missing.mkv", entry.Message, StringComparison.Ordinal); + } + [Fact] [System.Runtime.Versioning.SupportedOSPlatform("windows")] public void Windows_file_association_builds_open_command_with_file_placeholder() @@ -99,4 +114,27 @@ public void Windows_file_association_builds_open_command_with_file_placeholder() Assert.Equal("\"C:\\Program Files\\ChapterTool\\ChapterTool.exe\" \"%1\"", command); } + + private sealed class RecordingLogger : ILogger + { + public List Entries { get; } = []; + + public IDisposable? BeginScope(TState state) + where TState : notnull => + null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + Entries.Add(new LogEntry(logLevel, formatter(state, exception), exception)); + } + } + + private sealed record LogEntry(LogLevel Level, string Message, Exception? Exception); } From 9db47b75186ba44f0d46d9ed80c88e479abd1cb0 Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 23:48:24 +0800 Subject: [PATCH 12/22] fix: clarify ffmpeg directory setting --- docs/code-review-fix-todo-2026-07-06.md | 2 +- .../Localization/Resources/Strings.en-US.resx | 5 ++- .../Localization/Resources/Strings.ja-JP.resx | 5 ++- .../Localization/Resources/Strings.zh-CN.resx | 5 ++- .../ViewModels/SettingsToolViewModel.cs | 33 +++++++++++++++++-- .../Tools/ExternalToolLocator.cs | 4 ++- .../ViewModels/SettingsToolViewModelTests.cs | 23 +++++++++++++ .../ExternalToolLocatorTests.cs | 20 +++++++++++ 8 files changed, 89 insertions(+), 8 deletions(-) diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index 3d6cf04..90d9b11 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -25,7 +25,7 @@ Source report: `docs/code-review-2026-07-06.md` - [x] Validate external tools as executables, not just existing files. - [x] Preserve and surface corrupt settings files instead of silently resetting. - [x] Return or log shell service failures instead of swallowing them. -- [ ] Clarify whether `FfmpegPath` means ffmpeg or ffprobe directory and validate accordingly. +- [x] Clarify whether `FfmpegPath` means ffmpeg or ffprobe directory and validate accordingly. - [ ] Localize native file picker titles and file type labels. - [ ] Add accessible names for icon-only buttons. - [ ] Refactor BDMV parsing so stdout is not passed through diagnostics. diff --git a/src/ChapterTool.Avalonia/Localization/Resources/Strings.en-US.resx b/src/ChapterTool.Avalonia/Localization/Resources/Strings.en-US.resx index b055160..8486086 100644 --- a/src/ChapterTool.Avalonia/Localization/Resources/Strings.en-US.resx +++ b/src/ChapterTool.Avalonia/Localization/Resources/Strings.en-US.resx @@ -612,7 +612,7 @@ External Tools - FFmpeg directory + FFmpeg bin directory (contains ffprobe) ffprobe @@ -656,6 +656,9 @@ {name} was not found in this directory + + Path must be a directory + Unsupported on this platform diff --git a/src/ChapterTool.Avalonia/Localization/Resources/Strings.ja-JP.resx b/src/ChapterTool.Avalonia/Localization/Resources/Strings.ja-JP.resx index e2d9efe..7c979bd 100644 --- a/src/ChapterTool.Avalonia/Localization/Resources/Strings.ja-JP.resx +++ b/src/ChapterTool.Avalonia/Localization/Resources/Strings.ja-JP.resx @@ -612,7 +612,7 @@ 外部ツール - FFmpeg ディレクトリ + FFmpeg bin ディレクトリ（ffprobe を含む） ffprobe @@ -656,6 +656,9 @@ このディレクトリに {name} が見つかりません + + パスはディレクトリである必要があります + このプラットフォームでは未対応です diff --git a/src/ChapterTool.Avalonia/Localization/Resources/Strings.zh-CN.resx b/src/ChapterTool.Avalonia/Localization/Resources/Strings.zh-CN.resx index 24e4c6a..5f54281 100644 --- a/src/ChapterTool.Avalonia/Localization/Resources/Strings.zh-CN.resx +++ b/src/ChapterTool.Avalonia/Localization/Resources/Strings.zh-CN.resx @@ -612,7 +612,7 @@ 外部工具 - FFmpeg 目录 + FFmpeg bin 目录（包含 ffprobe） ffprobe @@ -656,6 +656,9 @@ 目录中未找到 {name} + + 路径必须是目录 + 当前平台不支持 diff --git a/src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs b/src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs index 04a5a6f..2e9362f 100644 --- a/src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs +++ b/src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs @@ -252,7 +252,7 @@ public string? FfmpegPath { if (SetProperty(ref field, CleanPath(value))) { - FfmpegStatus = FormatToolStatus(ValidateTool(value, "ffprobe")); + FfmpegStatus = FormatToolStatus(ValidateToolDirectory(value, "ffprobe")); NotifyUnsavedChanges(); } } @@ -574,7 +574,7 @@ private void RefreshToolStatuses() MkvToolnixStatus = FormatToolStatus(ValidateTool(MkvToolnixPath, "mkvextract")); Eac3toStatus = FormatToolStatus(ValidateTool(Eac3toPath, "eac3to")); FfprobeStatus = FormatToolStatus(ValidateTool(FfprobePath, "ffprobe")); - FfmpegStatus = FormatToolStatus(ValidateTool(FfmpegPath, "ffprobe")); + FfmpegStatus = FormatToolStatus(ValidateToolDirectory(FfmpegPath, "ffprobe")); } private async ValueTask DiscoverAndFillToolPathsAsync(CancellationToken cancellationToken) @@ -590,7 +590,7 @@ private async ValueTask DiscoverAndFillToolPathsAsync(CancellationToken cancella var ffprobe = await DiscoverExecutableAsync("ffprobe", FfprobePath, cancellationToken); FfprobePath = ffprobe; - if (string.IsNullOrWhiteSpace(FfmpegPath) || ValidateTool(FfmpegPath, "ffprobe").Kind != SettingsToolStatusKind.Found) + if (string.IsNullOrWhiteSpace(FfmpegPath) || ValidateToolDirectory(FfmpegPath, "ffprobe").Kind != SettingsToolStatusKind.Found) { var ffprobeDirectory = string.IsNullOrWhiteSpace(ffprobe) ? null : Path.GetDirectoryName(ffprobe); FfmpegPath = string.IsNullOrWhiteSpace(ffprobeDirectory) ? FfmpegPath : ffprobeDirectory; @@ -618,6 +618,7 @@ private string FormatToolStatus(SettingsToolStatus status) => SettingsToolStatusKind.Found => localizer.Format("Settings.ToolStatus.Found", new Dictionary { ["path"] = status.ResolvedPath }), SettingsToolStatusKind.Missing => localizer.Format("Settings.ToolStatus.Missing", new Dictionary { ["name"] = status.ExpectedExecutable }), SettingsToolStatusKind.InvalidPath => localizer.GetString("Settings.ToolStatus.InvalidPath"), + SettingsToolStatusKind.NotDirectory => localizer.GetString("Settings.ToolStatus.NotDirectory"), _ => localizer.GetString("Settings.ToolStatus.Unsupported") }; @@ -646,6 +647,31 @@ private static SettingsToolStatus ValidateTool(string? configuredPath, string to : new SettingsToolStatus(SettingsToolStatusKind.InvalidPath, text, executableName); } + private static SettingsToolStatus ValidateToolDirectory(string? configuredPath, string toolId) + { + if (string.IsNullOrWhiteSpace(configuredPath)) + { + return new SettingsToolStatus( + SettingsToolStatusKind.Discovery, + null, + ExternalToolPathResolver.ExecutableName(toolId)); + } + + var text = configuredPath.Trim(); + var executableName = ExternalToolPathResolver.ExecutableName(toolId); + if (!Directory.Exists(text)) + { + return File.Exists(text) + ? new SettingsToolStatus(SettingsToolStatusKind.NotDirectory, text, executableName) + : new SettingsToolStatus(SettingsToolStatusKind.InvalidPath, text, executableName); + } + + var candidate = Path.Combine(text, executableName); + return File.Exists(candidate) + ? new SettingsToolStatus(SettingsToolStatusKind.Found, candidate, executableName) + : new SettingsToolStatus(SettingsToolStatusKind.Missing, candidate, executableName); + } + private ThemeColorSettings CurrentThemeSettings() { var defaults = ThemeColorSettings.Default.OrderedSlots.ToList(); @@ -792,5 +818,6 @@ public enum SettingsToolStatusKind Found, Missing, InvalidPath, + NotDirectory, Unsupported } diff --git a/src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs b/src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs index 5428f75..c9568c3 100644 --- a/src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs +++ b/src/ChapterTool.Infrastructure/Tools/ExternalToolLocator.cs @@ -82,7 +82,9 @@ public async ValueTask LocateAsync(string toolId, Cancella { "mkvextract" => settings.MkvToolnixPath, "eac3to" => settings.Eac3toPath, - "ffprobe" => !string.IsNullOrWhiteSpace(settings.FfprobePath) ? settings.FfprobePath : settings.FfmpegPath, + "ffprobe" => !string.IsNullOrWhiteSpace(settings.FfprobePath) + ? settings.FfprobePath + : Directory.Exists(settings.FfmpegPath) ? settings.FfmpegPath : null, _ => null }; diff --git a/tests/ChapterTool.Avalonia.Tests/ViewModels/SettingsToolViewModelTests.cs b/tests/ChapterTool.Avalonia.Tests/ViewModels/SettingsToolViewModelTests.cs index b33b063..4151d39 100644 --- a/tests/ChapterTool.Avalonia.Tests/ViewModels/SettingsToolViewModelTests.cs +++ b/tests/ChapterTool.Avalonia.Tests/ViewModels/SettingsToolViewModelTests.cs @@ -406,6 +406,29 @@ public async Task ToolValidationUsesDirectoryExecutableExpansion() } } + [Fact] + public async Task FfmpegPathRequiresDirectoryContainingFfprobe() + { + var root = Path.Combine(Path.GetTempPath(), "ChapterTool.Tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(root); + var ffprobe = Path.Combine(root, ToolExecutable("ffprobe")); + await File.WriteAllTextAsync(ffprobe, ""); + var appStore = new FakeAppSettingsStore(new AppSettings(FfmpegPath: ffprobe)); + var owner = CreateOwner(appStore); + var viewModel = CreateViewModel(owner, appStore, new FakeThemeSettingsStore(ThemeColorSettings.Default), new AppLocalizationManager("en-US")); + + try + { + await viewModel.LoadAsync(TestContext.Current.CancellationToken); + + Assert.Equal("Path must be a directory", viewModel.FfmpegStatus); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + [Fact] public async Task ValidateToolsDiscoversAndFillsExternalToolPaths() { diff --git a/tests/ChapterTool.Infrastructure.Tests/ExternalToolLocatorTests.cs b/tests/ChapterTool.Infrastructure.Tests/ExternalToolLocatorTests.cs index 0040c0a..c12c2ac 100644 --- a/tests/ChapterTool.Infrastructure.Tests/ExternalToolLocatorTests.cs +++ b/tests/ChapterTool.Infrastructure.Tests/ExternalToolLocatorTests.cs @@ -305,6 +305,26 @@ await settingsStore.SaveAsync( Assert.Equal(expectedToolPath, location.Path); } + [Fact] + public async Task LocateAsync_does_not_treat_ffmpeg_path_as_ffprobe_executable() + { + var root = CreateTempDirectory(); + var ffprobePath = Path.Combine(root, ToolExecutable("ffprobe")); + await CreateToolFileAsync(ffprobePath); + + var settingsStore = new AppSettingsStore(root, [root]); + await settingsStore.SaveAsync( + new AppSettings(FfmpegPath: ffprobePath), + TestContext.Current.CancellationToken); + var locator = CreateLocatorWithoutDefaultCandidates(settingsStore, [], new FakeMkvToolNixInstallProbe()); + + var location = await locator.LocateAsync("ffprobe", TestContext.Current.CancellationToken); + + Assert.False(location.Found); + Assert.Null(location.Path); + Assert.Equal("MissingDependency", location.DiagnosticCode); + } + [Fact] public async Task LocateAsync_uses_search_directory_for_ffprobe_when_unconfigured() { From b75a27ed9d1ed9bf1ff6f6a763e48efeec9bbce3 Mon Sep 17 00:00:00 2001 From: TautCony Date: Mon, 6 Jul 2026 23:55:54 +0800 Subject: [PATCH 13/22] fix: localize native picker options --- docs/code-review-fix-todo-2026-07-06.md | 2 +- .../Composition/AppCompositionRoot.cs | 4 +- .../Localization/Resources/Strings.en-US.resx | 24 +++++ .../Localization/Resources/Strings.ja-JP.resx | 24 +++++ .../Localization/Resources/Strings.zh-CN.resx | 24 +++++ .../Services/AvaloniaFilePickerService.cs | 91 +++++++++++-------- .../Services/AvaloniaSettingsPickerService.cs | 20 ++-- .../Services/AvaloniaPickerServiceTests.cs | 29 ++++++ 8 files changed, 168 insertions(+), 50 deletions(-) create mode 100644 tests/ChapterTool.Avalonia.Tests/Services/AvaloniaPickerServiceTests.cs diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index 90d9b11..7abd8f0 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -26,7 +26,7 @@ Source report: `docs/code-review-2026-07-06.md` - [x] Preserve and surface corrupt settings files instead of silently resetting. - [x] Return or log shell service failures instead of swallowing them. - [x] Clarify whether `FfmpegPath` means ffmpeg or ffprobe directory and validate accordingly. -- [ ] Localize native file picker titles and file type labels. +- [x] Localize native file picker titles and file type labels. - [ ] Add accessible names for icon-only buttons. - [ ] Refactor BDMV parsing so stdout is not passed through diagnostics. diff --git a/src/ChapterTool.Avalonia/Composition/AppCompositionRoot.cs b/src/ChapterTool.Avalonia/Composition/AppCompositionRoot.cs index dfc5568..4efd10a 100644 --- a/src/ChapterTool.Avalonia/Composition/AppCompositionRoot.cs +++ b/src/ChapterTool.Avalonia/Composition/AppCompositionRoot.cs @@ -106,7 +106,7 @@ public IWindowService CreateWindowService() => themeSettingsStore, themeApplicationService, localizationManager, - owner => new AvaloniaSettingsPickerService(owner), + owner => new AvaloniaSettingsPickerService(owner, localizationManager), CreateExternalToolLocator(), new AvaloniaSettingsCloseConfirmationService(localizationManager), shellService: CreateShellService()); @@ -125,7 +125,7 @@ public static IFileAssociationService CreateFileAssociationService() return new UnsupportedFileAssociationService(); } - public static IFilePickerService CreateFilePickerService(Window owner) => new AvaloniaFilePickerService(owner); + public IFilePickerService CreateFilePickerService(Window owner) => new AvaloniaFilePickerService(owner, localizationManager); public IExternalToolLocator CreateExternalToolLocator() => new ExternalToolLocator(appSettingsStore, PathSearchDirectories().ToList()); diff --git a/src/ChapterTool.Avalonia/Localization/Resources/Strings.en-US.resx b/src/ChapterTool.Avalonia/Localization/Resources/Strings.en-US.resx index 8486086..a374b31 100644 --- a/src/ChapterTool.Avalonia/Localization/Resources/Strings.en-US.resx +++ b/src/ChapterTool.Avalonia/Localization/Resources/Strings.en-US.resx @@ -341,6 +341,30 @@ time + + Append MPLS + + + Executable files + + + MPLS playlist + + + Open Chapter Name Template + + + Open Source + + + Save Chapters To + + + Chapter and media files + + + Text files + Simplified Chinese diff --git a/src/ChapterTool.Avalonia/Localization/Resources/Strings.ja-JP.resx b/src/ChapterTool.Avalonia/Localization/Resources/Strings.ja-JP.resx index 7c979bd..945ab26 100644 --- a/src/ChapterTool.Avalonia/Localization/Resources/Strings.ja-JP.resx +++ b/src/ChapterTool.Avalonia/Localization/Resources/Strings.ja-JP.resx @@ -341,6 +341,30 @@ 時間 + + MPLS を追加 + + + 実行ファイル + + + MPLS プレイリスト + + + チャプター名テンプレートを開く + + + ソースを開く + + + チャプターの保存先 + + + チャプターとメディアファイル + + + テキストファイル + 簡体字中国語 diff --git a/src/ChapterTool.Avalonia/Localization/Resources/Strings.zh-CN.resx b/src/ChapterTool.Avalonia/Localization/Resources/Strings.zh-CN.resx index 5f54281..3c5d82b 100644 --- a/src/ChapterTool.Avalonia/Localization/Resources/Strings.zh-CN.resx +++ b/src/ChapterTool.Avalonia/Localization/Resources/Strings.zh-CN.resx @@ -341,6 +341,30 @@ 时间 + + 追加 MPLS + + + 可执行文件 + + + MPLS 播放列表 + + + 打开章节名称模板 + + + 打开源文件 + + + 保存章节到 + + + 章节和媒体文件 + + + 文本文件 + 简体中文 diff --git a/src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs b/src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs index 2539342..235f54f 100644 --- a/src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs +++ b/src/ChapterTool.Avalonia/Services/AvaloniaFilePickerService.cs @@ -1,25 +1,14 @@ using Avalonia.Controls; using Avalonia.Platform.Storage; +using ChapterTool.Avalonia.Localization; namespace ChapterTool.Avalonia.Services; -public sealed class AvaloniaFilePickerService(Window owner) : IFilePickerService +public sealed class AvaloniaFilePickerService(Window owner, IAppLocalizer localizer) : IFilePickerService { public async ValueTask PickSourceAsync(CancellationToken cancellationToken) { - var files = await owner.StorageProvider.OpenFilePickerAsync(new FilePickerOpenOptions - { - Title = "Open Source", - AllowMultiple = false, - FileTypeFilter = - [ - new FilePickerFileType("Chapter and media files") - { - Patterns = ["*.txt", "*.xml", "*.vtt", "*.cue", "*.flac", "*.tak", "*.mpls", "*.ifo", "*.xpl", "*.mkv", "*.mka", "*.mp4", "*.m4a", "*.m4v"] - }, - FilePickerFileTypes.All - ] - }); + var files = await owner.StorageProvider.OpenFilePickerAsync(CreateSourceOptions(localizer)); if (files.Count > 0) { cancellationToken.ThrowIfCancellationRequested(); @@ -32,16 +21,7 @@ public sealed class AvaloniaFilePickerService(Window owner) : IFilePickerService public async ValueTask PickMplsAsync(CancellationToken cancellationToken) { - var files = await owner.StorageProvider.OpenFilePickerAsync(new FilePickerOpenOptions - { - Title = "Append MPLS", - AllowMultiple = false, - FileTypeFilter = - [ - new FilePickerFileType("MPLS playlist") { Patterns = ["*.mpls"] }, - FilePickerFileTypes.All - ] - }); + var files = await owner.StorageProvider.OpenFilePickerAsync(CreateMplsOptions(localizer)); cancellationToken.ThrowIfCancellationRequested(); return files.Count > 0 ? files[0].Path.LocalPath : null; @@ -49,16 +29,7 @@ public sealed class AvaloniaFilePickerService(Window owner) : IFilePickerService public async ValueTask PickChapterNameTemplateAsync(CancellationToken cancellationToken) { - var files = await owner.StorageProvider.OpenFilePickerAsync(new FilePickerOpenOptions - { - Title = "Open Chapter Name Template", - AllowMultiple = false, - FileTypeFilter = - [ - new FilePickerFileType("Text files") { Patterns = ["*.txt"] }, - FilePickerFileTypes.All - ] - }); + var files = await owner.StorageProvider.OpenFilePickerAsync(CreateChapterNameTemplateOptions(localizer)); cancellationToken.ThrowIfCancellationRequested(); return files.Count > 0 ? files[0].Path.LocalPath : null; @@ -66,13 +37,55 @@ public sealed class AvaloniaFilePickerService(Window owner) : IFilePickerService public async ValueTask PickSaveDirectoryAsync(CancellationToken cancellationToken) { - var folders = await owner.StorageProvider.OpenFolderPickerAsync(new FolderPickerOpenOptions - { - Title = "Save Chapters To", - AllowMultiple = false - }); + var folders = await owner.StorageProvider.OpenFolderPickerAsync(CreateSaveDirectoryOptions(localizer)); cancellationToken.ThrowIfCancellationRequested(); return folders.Count > 0 ? folders[0].Path.LocalPath : null; } + + internal static FilePickerOpenOptions CreateSourceOptions(IAppLocalizer localizer) => + new() + { + Title = localizer.GetString("FilePicker.OpenSource.Title"), + AllowMultiple = false, + FileTypeFilter = + [ + new FilePickerFileType(localizer.GetString("FilePicker.SourceFiles")) + { + Patterns = ["*.txt", "*.xml", "*.vtt", "*.cue", "*.flac", "*.tak", "*.mpls", "*.ifo", "*.xpl", "*.mkv", "*.mka", "*.mp4", "*.m4a", "*.m4v"] + }, + FilePickerFileTypes.All + ] + }; + + internal static FilePickerOpenOptions CreateMplsOptions(IAppLocalizer localizer) => + new() + { + Title = localizer.GetString("FilePicker.AppendMpls.Title"), + AllowMultiple = false, + FileTypeFilter = + [ + new FilePickerFileType(localizer.GetString("FilePicker.MplsPlaylist")) { Patterns = ["*.mpls"] }, + FilePickerFileTypes.All + ] + }; + + internal static FilePickerOpenOptions CreateChapterNameTemplateOptions(IAppLocalizer localizer) => + new() + { + Title = localizer.GetString("FilePicker.OpenChapterNameTemplate.Title"), + AllowMultiple = false, + FileTypeFilter = + [ + new FilePickerFileType(localizer.GetString("FilePicker.TextFiles")) { Patterns = ["*.txt"] }, + FilePickerFileTypes.All + ] + }; + + internal static FolderPickerOpenOptions CreateSaveDirectoryOptions(IAppLocalizer localizer) => + new() + { + Title = localizer.GetString("FilePicker.SaveChaptersTo.Title"), + AllowMultiple = false + }; } diff --git a/src/ChapterTool.Avalonia/Services/AvaloniaSettingsPickerService.cs b/src/ChapterTool.Avalonia/Services/AvaloniaSettingsPickerService.cs index 1e34be6..2cad8b3 100644 --- a/src/ChapterTool.Avalonia/Services/AvaloniaSettingsPickerService.cs +++ b/src/ChapterTool.Avalonia/Services/AvaloniaSettingsPickerService.cs @@ -1,9 +1,10 @@ using Avalonia.Controls; using Avalonia.Platform.Storage; +using ChapterTool.Avalonia.Localization; namespace ChapterTool.Avalonia.Services; -public sealed class AvaloniaSettingsPickerService(Window owner) : ISettingsPickerService +public sealed class AvaloniaSettingsPickerService(Window owner, IAppLocalizer localizer) : ISettingsPickerService { public async ValueTask PickDirectoryAsync(string title, CancellationToken cancellationToken) { @@ -19,21 +20,24 @@ public sealed class AvaloniaSettingsPickerService(Window owner) : ISettingsPicke public async ValueTask PickExecutableAsync(string title, CancellationToken cancellationToken) { - var files = await owner.StorageProvider.OpenFilePickerAsync(new FilePickerOpenOptions + var files = await owner.StorageProvider.OpenFilePickerAsync(CreateExecutableOptions(title, localizer)); + + cancellationToken.ThrowIfCancellationRequested(); + return files.Count > 0 ? files[0].Path.LocalPath : null; + } + + internal static FilePickerOpenOptions CreateExecutableOptions(string title, IAppLocalizer localizer) => + new() { Title = title, AllowMultiple = false, FileTypeFilter = [ - new FilePickerFileType("Executable files") + new FilePickerFileType(localizer.GetString("FilePicker.ExecutableFiles")) { Patterns = OperatingSystem.IsWindows() ? ["*.exe"] : ["*"] }, FilePickerFileTypes.All ] - }); - - cancellationToken.ThrowIfCancellationRequested(); - return files.Count > 0 ? files[0].Path.LocalPath : null; - } + }; } diff --git a/tests/ChapterTool.Avalonia.Tests/Services/AvaloniaPickerServiceTests.cs b/tests/ChapterTool.Avalonia.Tests/Services/AvaloniaPickerServiceTests.cs new file mode 100644 index 0000000..8077cc4 --- /dev/null +++ b/tests/ChapterTool.Avalonia.Tests/Services/AvaloniaPickerServiceTests.cs @@ -0,0 +1,29 @@ +using ChapterTool.Avalonia.Localization; +using ChapterTool.Avalonia.Services; + +namespace ChapterTool.Avalonia.Tests.Services; + +public sealed class AvaloniaPickerServiceTests +{ + [Fact] + public void File_picker_options_use_localized_titles_and_file_type_names() + { + var localizer = new AppLocalizationManager("zh-CN"); + + var source = AvaloniaFilePickerService.CreateSourceOptions(localizer); + var mpls = AvaloniaFilePickerService.CreateMplsOptions(localizer); + var template = AvaloniaFilePickerService.CreateChapterNameTemplateOptions(localizer); + var saveDirectory = AvaloniaFilePickerService.CreateSaveDirectoryOptions(localizer); + var executable = AvaloniaSettingsPickerService.CreateExecutableOptions("选择工具", localizer); + + Assert.Equal("打开源文件", source.Title); + Assert.Equal("章节和媒体文件", source.FileTypeFilter?.First().Name); + Assert.Equal("追加 MPLS", mpls.Title); + Assert.Equal("MPLS 播放列表", mpls.FileTypeFilter?.First().Name); + Assert.Equal("打开章节名称模板", template.Title); + Assert.Equal("文本文件", template.FileTypeFilter?.First().Name); + Assert.Equal("保存章节到", saveDirectory.Title); + Assert.Equal("选择工具", executable.Title); + Assert.Equal("可执行文件", executable.FileTypeFilter?.First().Name); + } +} From 466a3e8b3ec62f6b6878ccad48c7b50e7423cf05 Mon Sep 17 00:00:00 2001 From: TautCony Date: Tue, 7 Jul 2026 00:02:55 +0800 Subject: [PATCH 14/22] fix: name icon-only buttons for accessibility --- docs/code-review-fix-todo-2026-07-06.md | 2 +- .../Views/MainWindow.axaml | 4 ++ .../Views/Tools/SettingsToolView.axaml | 20 ++++----- .../MainWindowInteractionHeadlessTests.cs | 13 ++++++ .../Headless/SettingsToolHeadlessTests.cs | 43 +++++++++++++++++++ 5 files changed, 71 insertions(+), 11 deletions(-) diff --git a/docs/code-review-fix-todo-2026-07-06.md b/docs/code-review-fix-todo-2026-07-06.md index 7abd8f0..c868d5f 100644 --- a/docs/code-review-fix-todo-2026-07-06.md +++ b/docs/code-review-fix-todo-2026-07-06.md @@ -27,7 +27,7 @@ Source report: `docs/code-review-2026-07-06.md` - [x] Return or log shell service failures instead of swallowing them. - [x] Clarify whether `FfmpegPath` means ffmpeg or ffprobe directory and validate accordingly. - [x] Localize native file picker titles and file type labels. -- [ ] Add accessible names for icon-only buttons. +- [x] Add accessible names for icon-only buttons. - [ ] Refactor BDMV parsing so stdout is not passed through diagnostics. ## Low Priority diff --git a/src/ChapterTool.Avalonia/Views/MainWindow.axaml b/src/ChapterTool.Avalonia/Views/MainWindow.axaml index ff990cf..3e57a26 100644 --- a/src/ChapterTool.Avalonia/Views/MainWindow.axaml +++ b/src/ChapterTool.Avalonia/Views/MainWindow.axaml @@ -260,6 +260,7 @@ - @@ -122,10 +122,10 @@ - - @@ -136,10 +136,10 @@ - - @@ -150,10 +150,10 @@ - - @@ -164,10 +164,10 @@ - - diff --git a/tests/ChapterTool.Avalonia.Tests/Headless/MainWindowInteractionHeadlessTests.cs b/tests/ChapterTool.Avalonia.Tests/Headless/MainWindowInteractionHeadlessTests.cs index fa9076c..9ab93d7 100644 --- a/tests/ChapterTool.Avalonia.Tests/Headless/MainWindowInteractionHeadlessTests.cs +++ b/tests/ChapterTool.Avalonia.Tests/Headless/MainWindowInteractionHeadlessTests.cs @@ -1,3 +1,4 @@ +using Avalonia.Automation; using Avalonia.Controls; using Avalonia.Headless.XUnit; using Avalonia.Input; @@ -142,4 +143,16 @@ public async Task Auxiliary_command_surfaces_are_visible_and_bound() Assert.Equal(["preview"], host.WindowService.Opened); Assert.Same(host.ViewModel, host.WindowService.Parameters.Single()); } + + [AvaloniaFact] + public async Task Icon_only_main_window_buttons_have_accessible_names() + { + using var host = new MainWindowHeadlessTestHost(); + await host.LayoutAsync(); + + Assert.Equal("Preview", AutomationProperties.GetName(host.RequiredControl