Fix file processing issue for newly added tracks#90
Merged
Conversation
…ress flag reset - Extract FileTrackValues as a dedicated per-file track data model - Replace TrackConfiguration reuse in per-file collections with FileTrackValues - Add ILanguageProvider.Resolve() for centralized language code resolution - Fix _suppressBatchConfigUpdate not being reset on early return in ApplyTrackProperties (try/finally) - Only log SelectedLanguage null warning when not in internal sync path - Update MkvPropeditArgumentsGenerator remark to reflect that per-file values are updated by the user - Add MkvPropeditArgumentsGenerator logging partial class - Correct 'iso639_3' to 'ISO 639-3' in test comment - Update tests for new FileTrackValues model and refactored APIs Refs: #85
- Rename LogSelectedLanguageNullFallback to LogSelectedLanguageReceivedNull - Simplify message to state the fact without assuming the source
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #85 by separating modification intent (global TrackConfiguration.ShouldModify*) from per-file writable values (FileTrackValues), aiming to ensure files added after an initial processing run are still processed correctly on subsequent runs.
Changes:
- Introduces
FileTrackValuesand switchesFileTrackConfiguration/IBatchConfiguration.GetTrackListForFileto use per-file track values instead of per-fileTrackConfiguration. - Updates
MkvPropeditArgumentsGeneratorto readShouldModify*from global tracks while reading write-values from per-file tracks, with added defensive logging. - Adds
ILanguageProvider.Resolve(...)and updates track config creation to use it; updates ViewModels to acceptILoggerand improves suppress-flag reset behavior.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MatroskaBatchFlow.Uno.UnitTests/Presentation/VideoViewModelTests.cs | Updates ctor calls for logger-injected VideoViewModel. |
| tests/MatroskaBatchFlow.Uno.UnitTests/Presentation/TrackViewModelBaseTests.cs | Updates tests to use FileTrackValues and logger-injected base ctor. |
| tests/MatroskaBatchFlow.Uno.UnitTests/Presentation/SubtitleViewModelTests.cs | Updates ctor calls for logger-injected SubtitleViewModel. |
| tests/MatroskaBatchFlow.Uno.UnitTests/Presentation/AudioViewModelTests.cs | Updates ctor calls for logger-injected AudioViewModel. |
| tests/MatroskaBatchFlow.Uno.IntegrationTests/TrackModificationIntegrationTests.cs | Updates initializer/generator/viewmodel construction to include new deps/loggers. |
| tests/MatroskaBatchFlow.Core.UnitTests/Services/TrackConfigurationFactoryTests.cs | Refactors tests to validate delegation to ILanguageProvider.Resolve. |
| tests/MatroskaBatchFlow.Core.UnitTests/Services/LanguageProviderTests.cs | Adds coverage for new Resolve behavior and match paths. |
| tests/MatroskaBatchFlow.Core.UnitTests/Services/BatchTrackConfigurationInitializerTests.cs | Updates initializer ctor and adds regression tests around new per-file model. |
| tests/MatroskaBatchFlow.Core.UnitTests/Services/BatchConfigurationTests.cs | Updates migration tests to use FileTrackValues. |
| src/MatroskaBatchFlow.Uno/Presentation/VideoViewModel.cs | Adds ILogger<VideoViewModel> ctor param and forwards to base. |
| src/MatroskaBatchFlow.Uno/Presentation/TrackViewModelBase.cs | Adds logging + splits global-flag updates vs per-file value updates; fixes suppress reset via try/finally. |
| src/MatroskaBatchFlow.Uno/Presentation/TrackViewModelBase.Logging.cs | Adds LoggerMessage for null SelectedLanguage fallback warning. |
| src/MatroskaBatchFlow.Uno/Presentation/SubtitleViewModel.cs | Adds ILogger<SubtitleViewModel> ctor param and forwards to base. |
| src/MatroskaBatchFlow.Uno/Presentation/AudioViewModel.cs | Adds ILogger<AudioViewModel> ctor param and forwards to base. |
| src/MatroskaBatchFlow.Core/Services/TrackConfigurationFactory.cs | Uses ILanguageProvider.Resolve instead of local parsing logic. |
| src/MatroskaBatchFlow.Core/Services/MkvPropeditArgumentsGenerator.cs | Uses global ShouldModify* + per-file FileTrackValues with defensive logging. |
| src/MatroskaBatchFlow.Core/Services/MkvPropeditArgumentsGenerator.Logging.cs | Adds warning log for per-file track index exceeding global list. |
| src/MatroskaBatchFlow.Core/Services/LanguageProvider.cs | Implements new Resolve API. |
| src/MatroskaBatchFlow.Core/Services/ILanguageProvider.cs | Adds Resolve(string?) contract. |
| src/MatroskaBatchFlow.Core/Services/IBatchConfiguration.cs | Updates docs + changes per-file track accessor to return FileTrackValues. |
| src/MatroskaBatchFlow.Core/Services/BatchTrackConfigurationInitializer.cs | Creates per-file FileTrackValues and expands global tracks via factory. |
| src/MatroskaBatchFlow.Core/Services/BatchConfiguration.cs | Updates GetTrackListForFile return type; tightens OnPropertyChanged visibility. |
| src/MatroskaBatchFlow.Core/Models/FileTrackValues.cs | New model representing per-file writable track values plus scanned source data. |
| src/MatroskaBatchFlow.Core/Models/FileTrackConfiguration.cs | Switches per-file track collections to ObservableCollection<FileTrackValues>. |
tests/MatroskaBatchFlow.Core.UnitTests/Services/BatchTrackConfigurationInitializerTests.cs
Show resolved
Hide resolved
src/MatroskaBatchFlow.Core/Services/BatchTrackConfigurationInitializer.cs
Show resolved
Hide resolved
…file - Change AddTracksForFile to read Name, Language, Default, Forced, and Enabled values from the global TrackConfiguration instead of per-file FileTrackValues - Fixes newly added files being processed with stale scanned values instead of user-configured values - Fixes toggle-off-then-on scenario leaving new files inconsistent
tests/MatroskaBatchFlow.Uno.IntegrationTests/TrackModificationIntegrationTests.cs
Show resolved
Hide resolved
tests/MatroskaBatchFlow.Core.UnitTests/Services/BatchTrackConfigurationInitializerTests.cs
Show resolved
Hide resolved
- Update FileTrackValues XML docs to clarify global TrackConfiguration is the source of truth for write-values during command generation - Update TrackViewModelBase comment to clarify per-file sync is for consistency, not correctness - Configure ILanguageProvider.Resolve default in test mocks to prevent null Language values
Owner
Author
|
As I worked on the pull request I realized I need to do a overhaul of the property value tracking system as things start to get a bit messy. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce a per-file track values model to ensure that newly added files in a batch are processed correctly during subsequent runs. This update includes a centralized language code resolution method and improves logging for track processing. The changes also ensure that the suppress flag is reset appropriately, preventing files from being skipped.
Previously, per-file track data reused the full TrackConfiguration model, which includes ShouldModify* intent flags. Newly added files received copies with default (false) flags, causing MkvPropeditArgumentsGenerator to skip them entirely even though the user had enabled modifications on the global track. This PR introduces a dedicated FileTrackValues model that carries only data (Name, Language, flags), separating it from the global modification intent. It also includes improved logging for track processing, and a fix for _suppressBatchConfigUpdate not being reset on early return.
Known limitation
The per-file propagation in UpdateBatchConfigTrackProperty copies the global value identically to every file's FileTrackValues. This is correct for the current use case (uniform batch edits), but I expect it won't scale (properly) to scenarios where per-file values need to differ from the global setting, for example templated track names like "Episode {id} - English" or "Track {n}".
However, such features are not in the works right now, but it is worth considering the impact on them. Furthermore, supporting that would likely require a value expression or template model on the global TrackConfiguration, per-file resolution against file/track metadata, and possibly a third state distinguishing "use global template" from "per-file override". The current two-tier split (TrackConfiguration for intent + FileTrackValues for data) is a reasonable foundation for that future work.
Fixes #85