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..a148189 --- /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 + +- [x] Fix WebVTT import so cue end times and duration are preserved. +- [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. +- [x] Add or explicitly defer a non-primary settings/command surface for file association. +- [x] Prevent preview from opening as an empty stub when no chapters are loaded. + +## Medium Priority + +- [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. +- [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. +- [x] Clarify whether `FfmpegPath` means ffmpeg or ffprobe directory and validate accordingly. +- [x] Localize native file picker titles and file type labels. +- [x] Add accessible names for icon-only buttons. +- [x] Refactor BDMV parsing so stdout is not passed through diagnostics. + +## Low Priority + +- [x] Move production test-double services out of Infrastructure or mark them test-only. +- [x] Replace fixed `Task.Delay` in `UiCommandTests`. +- [x] Remove sync-over-async from Matroska integration setup. +- [x] Strengthen screenshot tests with layout/content assertions. +- [x] Handle quoted MKVToolNix `DisplayIcon` registry values. +- [x] Hide eac3to export process windows unless visibility is required. diff --git a/src/ChapterTool.Avalonia/Composition/AppCompositionRoot.cs b/src/ChapterTool.Avalonia/Composition/AppCompositionRoot.cs index dfc5568..df60316 100644 --- a/src/ChapterTool.Avalonia/Composition/AppCompositionRoot.cs +++ b/src/ChapterTool.Avalonia/Composition/AppCompositionRoot.cs @@ -106,10 +106,11 @@ public IWindowService CreateWindowService() => themeSettingsStore, themeApplicationService, localizationManager, - owner => new AvaloniaSettingsPickerService(owner), + owner => new AvaloniaSettingsPickerService(owner, localizationManager), CreateExternalToolLocator(), new AvaloniaSettingsCloseConfirmationService(localizationManager), - shellService: CreateShellService()); + shellService: CreateShellService(), + fileAssociationService: CreateFileAssociationService()); public IAppLocalizer CreateLocalizer() => localizationManager; @@ -125,7 +126,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 b055160..5e05a77 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 @@ -611,8 +635,29 @@ External Tools + + File Association + + + Register .mpls playlists to open with ChapterTool. This is a platform integration and is kept out of the primary workflow controls. + + + Register .mpls + + + .mpls is not registered to ChapterTool + + + .mpls is registered to ChapterTool + + + File association is not supported on this platform + + + Unregister + - FFmpeg directory + FFmpeg bin directory (contains ffprobe) ffprobe @@ -656,6 +701,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..4045953 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 プレイリスト + + + チャプター名テンプレートを開く + + + ソースを開く + + + チャプターの保存先 + + + チャプターとメディアファイル + + + テキストファイル + 簡体字中国語 @@ -611,8 +635,29 @@ 外部ツール + + ファイル関連付け + + + .mpls プレイリストを ChapterTool で開くように登録します。これはプラットフォーム連携のため、主要なワークフロー操作には表示しません。 + + + .mpls を登録 + + + .mpls は ChapterTool に登録されていません + + + .mpls は ChapterTool に登録されています + + + このプラットフォームではファイル関連付けはサポートされていません + + + 登録解除 + - FFmpeg ディレクトリ + FFmpeg bin ディレクトリ（ffprobe を含む） ffprobe @@ -656,6 +701,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..4eac91c 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 播放列表 + + + 打开章节名称模板 + + + 打开源文件 + + + 保存章节到 + + + 章节和媒体文件 + + + 文本文件 + 简体中文 @@ -611,8 +635,29 @@ 外部工具 + + 文件关联 + + + 将 .mpls 播放列表注册为使用 ChapterTool 打开。此项属于平台集成，因此不放在主工作流控件中。 + + + 注册 .mpls + + + .mpls 未注册到 ChapterTool + + + .mpls 已注册到 ChapterTool + + + 当前平台不支持文件关联 + + + 取消注册 + - FFmpeg 目录 + FFmpeg bin 目录（包含 ffprobe） ffprobe @@ -656,6 +701,9 @@ 目录中未找到 {name} + + 路径必须是目录 + 当前平台不支持 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/src/ChapterTool.Avalonia/Services/AvaloniaWindowService.cs b/src/ChapterTool.Avalonia/Services/AvaloniaWindowService.cs index 2b6c1fc..8bb059c 100644 --- a/src/ChapterTool.Avalonia/Services/AvaloniaWindowService.cs +++ b/src/ChapterTool.Avalonia/Services/AvaloniaWindowService.cs @@ -6,6 +6,7 @@ using ChapterTool.Avalonia.Views.Tools; using ChapterTool.Core.Services; using ChapterTool.Infrastructure.Configuration; +using ChapterTool.Infrastructure.Platform; namespace ChapterTool.Avalonia.Services; @@ -18,6 +19,7 @@ public sealed class AvaloniaWindowService : IWindowService private readonly Func? settingsPickerFactory; private readonly IExternalToolLocator? externalToolLocator; private readonly IShellService? shellService; + private readonly IFileAssociationService? fileAssociationService; private readonly Dictionary windows = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary parameters = new(StringComparer.OrdinalIgnoreCase); private readonly IAppLocalizer localizer; @@ -30,7 +32,8 @@ public AvaloniaWindowService( Func? settingsPickerFactory = null, IExternalToolLocator? externalToolLocator = null, ISettingsCloseConfirmationService? settingsCloseConfirmationService = null, - IShellService? shellService = null) + IShellService? shellService = null, + IFileAssociationService? fileAssociationService = null) { this.appSettingsStore = appSettingsStore; this.themeSettingsStore = themeSettingsStore; @@ -41,6 +44,7 @@ public AvaloniaWindowService( this.settingsPickerFactory = settingsPickerFactory; this.externalToolLocator = externalToolLocator; this.shellService = shellService; + this.fileAssociationService = fileAssociationService; this.localizer.CultureChanged += (_, _) => { foreach (var (id, window) in windows) @@ -165,7 +169,8 @@ private Control CreateContent(Window window, string id, MainWindowViewModel view settingsPickerFactory?.Invoke(window), externalToolLocator, themeApplicationService, - shellService) + shellService, + fileAssociationService) }, "color-settings" => new ColorSettingsView { DataContext = new ColorSettingsViewModel(themeSettingsStore, themeApplicationService) }, "language" => new LanguageToolView { DataContext = new LanguageToolViewModel(viewModel) }, 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/src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs b/src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs index 04a5a6f..dd6f99d 100644 --- a/src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs +++ b/src/ChapterTool.Avalonia/ViewModels/SettingsToolViewModel.cs @@ -8,6 +8,7 @@ using ChapterTool.Core.Exporting; using ChapterTool.Core.Services; using ChapterTool.Infrastructure.Configuration; +using ChapterTool.Infrastructure.Platform; using ChapterTool.Infrastructure.Tools; namespace ChapterTool.Avalonia.ViewModels; @@ -37,6 +38,7 @@ public sealed class SettingsToolViewModel : ObservableViewModel private readonly IExternalToolLocator? externalToolLocator; private readonly IThemeApplicationService? themeApplicationService; private readonly IShellService? shellService; + private readonly IFileAssociationService? fileAssociationService; private AppSettings savedAppSettings = new(); private ThemeColorSettings savedThemeSettings = ThemeColorSettings.Default; private string selectedLanguage; @@ -58,6 +60,7 @@ public SettingsToolViewModel( IExternalToolLocator? externalToolLocator = null, IThemeApplicationService? themeApplicationService = null, IShellService? shellService = null, + IFileAssociationService? fileAssociationService = null, bool autoLoad = true) { this.owner = owner; @@ -68,6 +71,7 @@ public SettingsToolViewModel( this.externalToolLocator = externalToolLocator; this.themeApplicationService = themeApplicationService; this.shellService = shellService; + this.fileAssociationService = fileAssociationService; selectedLanguage = AppLanguage.Normalize(owner.UiLanguage); defaultSaveFormatIndex = Math.Clamp(owner.SaveFormatIndex, 0, SaveFormats.Length - 1); defaultXmlLanguageIndex = XmlLanguageIndex(owner.XmlLanguage); @@ -107,6 +111,9 @@ public SettingsToolViewModel( ClearFfprobeCommand = ClearCommand(() => FfprobePath = null); ClearFfmpegCommand = ClearCommand(() => FfmpegPath = null); OpenRepositoryCommand = new UiCommand(async (_, token) => await OpenRepositoryAsync(token), _ => shellService is not null); + RegisterFileAssociationCommand = new UiCommand(async (_, token) => await RegisterFileAssociationAsync(token), _ => fileAssociationService is not null); + UnregisterFileAssociationCommand = new UiCommand(async (_, token) => await UnregisterFileAssociationAsync(token), _ => fileAssociationService is not null); + RefreshFileAssociationCommand = new UiCommand(async (_, token) => await RefreshFileAssociationStatusAsync(token), _ => fileAssociationService is not null); this.localizer.CultureChanged += (_, _) => { RefreshLanguages(); @@ -252,7 +259,7 @@ public string? FfmpegPath { if (SetProperty(ref field, CleanPath(value))) { - FfmpegStatus = FormatToolStatus(ValidateTool(value, "ffprobe")); + FfmpegStatus = FormatToolStatus(ValidateToolDirectory(value, "ffprobe")); NotifyUnsavedChanges(); } } @@ -365,6 +372,18 @@ public string FfmpegStatus public UiCommand OpenRepositoryCommand { get; } + public UiCommand RegisterFileAssociationCommand { get; } + + public UiCommand UnregisterFileAssociationCommand { get; } + + public UiCommand RefreshFileAssociationCommand { get; } + + public string FileAssociationStatus + { + get; + private set => SetProperty(ref field, value); + } = string.Empty; + public async ValueTask LoadAsync(CancellationToken cancellationToken) { liveApplyEnabled = false; @@ -386,6 +405,7 @@ public async ValueTask LoadAsync(CancellationToken cancellationToken) liveApplyEnabled = true; ApplyCurrentAppSettingsToOwner(); RefreshToolStatuses(); + await RefreshFileAssociationStatusAsync(cancellationToken); NotifyUnsavedChanges(); StatusText = localizer.GetString("Settings.Status.Ready"); } @@ -418,6 +438,75 @@ private async ValueTask SaveAsync(CancellationToken cancellationToken) StatusText = localizer.GetString("Settings.Status.Saved"); } + private async ValueTask RegisterFileAssociationAsync(CancellationToken cancellationToken) + { + if (fileAssociationService is null) + { + FileAssociationStatus = localizer.GetString("Settings.FileAssociation.Status.Unsupported"); + return; + } + + var result = await fileAssociationService.RegisterAsync( + ".mpls", + "ChapterTool.MPLS", + "ChapterTool MPLS Playlist", + cancellationToken); + await ApplyFileAssociationCommandResultAsync(result, cancellationToken); + } + + private async ValueTask UnregisterFileAssociationAsync(CancellationToken cancellationToken) + { + if (fileAssociationService is null) + { + FileAssociationStatus = localizer.GetString("Settings.FileAssociation.Status.Unsupported"); + return; + } + + var result = await fileAssociationService.UnregisterAsync(".mpls", "ChapterTool.MPLS", cancellationToken); + await ApplyFileAssociationCommandResultAsync(result, cancellationToken); + } + + private async ValueTask ApplyFileAssociationCommandResultAsync(FileAssociationResult result, CancellationToken cancellationToken) + { + if (!result.Success) + { + FileAssociationStatus = FormatFileAssociationStatus(result); + return; + } + + await RefreshFileAssociationStatusAsync(cancellationToken); + } + + private async ValueTask RefreshFileAssociationStatusAsync(CancellationToken cancellationToken) + { + if (fileAssociationService is null) + { + FileAssociationStatus = localizer.GetString("Settings.FileAssociation.Status.Unsupported"); + return; + } + + var result = await fileAssociationService.IsRegisteredAsync(".mpls", "ChapterTool.MPLS", cancellationToken); + FileAssociationStatus = FormatFileAssociationStatus(result); + } + + private string FormatFileAssociationStatus(FileAssociationResult result) + { + var diagnostic = result.Diagnostics.FirstOrDefault(); + if (diagnostic?.Code == "UnsupportedPlatform") + { + return localizer.GetString("Settings.FileAssociation.Status.Unsupported"); + } + + if (diagnostic is not null) + { + return diagnostic.Message; + } + + return result.Success + ? localizer.GetString("Settings.FileAssociation.Status.Registered") + : localizer.GetString("Settings.FileAssociation.Status.NotRegistered"); + } + public void DiscardUnsavedAppearanceChanges() { if (CurrentThemeSettings() == savedThemeSettings) @@ -574,7 +663,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 +679,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 +707,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 +736,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 +907,6 @@ public enum SettingsToolStatusKind Found, Missing, InvalidPath, + NotDirectory, Unsupported } 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 @@ - - @@ -273,6 +273,14 @@ + + + +