-
Notifications
You must be signed in to change notification settings - Fork 4
fix: Surface unified Tag view over all metadata sources (#6) #30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
decriptor
wants to merge
3
commits into
main
Choose a base branch
from
fix-flac-tag-pictures
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,171 @@ | ||
| // Copyright (c) 2025-2026 Stephen Shaw and contributors | ||
| // Licensed under the MIT License. See LICENSE file in the project root for full license information. | ||
|
|
||
| namespace TagLibSharp2.Core; | ||
|
|
||
| /// <summary> | ||
| /// A tag facade that exposes a unified view over multiple underlying <see cref="Tag"/> | ||
| /// instances in priority order. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// Getters return the first non-null/non-empty value from the ordered list of tags. | ||
| /// Pictures are unioned across members and deduplicated on (PictureType, MimeType, | ||
| /// PictureData). | ||
| /// </para> | ||
| /// <para> | ||
| /// Setters write through to <b>every</b> non-null underlying tag so that content | ||
| /// stays consistent across formats. This matches user expectations on files that | ||
| /// carry redundant metadata (e.g. an MP3 with both ID3v2 and ID3v1) and prevents | ||
| /// post-save drift where one tag has the new value and the other has the old one. | ||
| /// Format-specific limits still apply per tag (ID3v1 truncates to 30 bytes, etc.). | ||
| /// </para> | ||
| /// <para> | ||
| /// <see cref="Render"/> throws because a <see cref="CombinedTag"/> is a view, not | ||
| /// a serializable block: render the owning file or a specific underlying tag instead. | ||
| /// <see cref="Clear"/> clears every non-null underlying tag. | ||
| /// </para> | ||
| /// <para> | ||
| /// The motivating case is an MP3 file that carries both an ID3v2 tag | ||
| /// (https://id3.org/id3v2.4.0-structure) and an ID3v1 tag (https://id3.org/ID3v1). | ||
| /// ID3v2 is authoritative when both are present, but ID3v1-only fields must still | ||
| /// surface through the unified view so legacy data is not silently lost. | ||
| /// </para> | ||
| /// </remarks> | ||
| public class CombinedTag : Tag | ||
| { | ||
| readonly Tag?[] _tags; | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new <see cref="CombinedTag"/> with the supplied tags in | ||
| /// priority order. The first non-null tag's value wins for each field. | ||
| /// </summary> | ||
| /// <param name="tags">Tags in priority order. Null entries are allowed and ignored.</param> | ||
| public CombinedTag (params Tag?[] tags) | ||
| { | ||
| _tags = tags ?? []; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the underlying tags in priority order. Null entries are preserved. | ||
| /// </summary> | ||
| protected IReadOnlyList<Tag?> Tags => _tags; | ||
|
|
||
| T? FirstNonDefault<T> (Func<Tag, T?> selector, Func<T?, bool> isDefault) | ||
| { | ||
| foreach (var tag in _tags) { | ||
| if (tag is null) | ||
| continue; | ||
| var value = selector (tag); | ||
| if (!isDefault (value)) | ||
| return value; | ||
| } | ||
| return default; | ||
| } | ||
|
|
||
| string? FirstNonEmptyString (Func<Tag, string?> selector) => | ||
| FirstNonDefault (selector, string.IsNullOrEmpty); | ||
|
|
||
| uint? FirstNonNullUInt (Func<Tag, uint?> selector) => | ||
| FirstNonDefault<uint?> (selector, v => !v.HasValue); | ||
|
|
||
| void WriteToAll (Action<Tag> setter) | ||
| { | ||
| foreach (var tag in _tags) { | ||
| if (tag is not null) | ||
| setter (tag); | ||
| } | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override TagTypes TagType { | ||
| get { | ||
| var types = TagTypes.None; | ||
| foreach (var tag in _tags) { | ||
| if (tag is not null) | ||
| types |= tag.TagType; | ||
| } | ||
| return types; | ||
| } | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Title { | ||
| get => FirstNonEmptyString (t => t.Title); | ||
| set => WriteToAll (t => t.Title = value); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Artist { | ||
| get => FirstNonEmptyString (t => t.Artist); | ||
| set => WriteToAll (t => t.Artist = value); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Album { | ||
| get => FirstNonEmptyString (t => t.Album); | ||
| set => WriteToAll (t => t.Album = value); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Year { | ||
| get => FirstNonEmptyString (t => t.Year); | ||
| set => WriteToAll (t => t.Year = value); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Comment { | ||
| get => FirstNonEmptyString (t => t.Comment); | ||
| set => WriteToAll (t => t.Comment = value); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Genre { | ||
| get => FirstNonEmptyString (t => t.Genre); | ||
| set => WriteToAll (t => t.Genre = value); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override uint? Track { | ||
| get => FirstNonNullUInt (t => t.Track); | ||
| set => WriteToAll (t => t.Track = value); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| #pragma warning disable CA1819 // Properties should not return arrays - Tag API contract | ||
| public override IPicture[] Pictures { | ||
| get { | ||
| var merged = new List<IPicture> (); | ||
| var seen = new HashSet<(PictureType, string, BinaryData)> (); | ||
| foreach (var tag in _tags) { | ||
| if (tag is null) | ||
| continue; | ||
| foreach (var p in tag.Pictures) { | ||
| if (seen.Add ((p.PictureType, p.MimeType, p.PictureData))) | ||
| merged.Add (p); | ||
| } | ||
| } | ||
| return [.. merged]; | ||
| } | ||
| set => WriteToAll (t => t.Pictures = value ?? []); | ||
| } | ||
| #pragma warning restore CA1819 | ||
|
|
||
| /// <inheritdoc/> | ||
| /// <exception cref="NotSupportedException"> | ||
| /// <see cref="CombinedTag"/> is a view over multiple tags and does not produce its | ||
| /// own serialized representation. Render the owning file or a specific underlying | ||
| /// tag instead. | ||
| /// </exception> | ||
| public override BinaryData Render () => | ||
| throw new NotSupportedException ( | ||
| "CombinedTag is a view over multiple tags; it has no standalone binary representation. " | ||
| + "Render the owning file (e.g. Mp3File.Render) or a specific underlying tag instead."); | ||
|
|
||
| /// <inheritdoc/> | ||
| /// <remarks> | ||
| /// Clears every non-null underlying tag, leaving each instance in place but empty. | ||
| /// </remarks> | ||
| public override void Clear () => | ||
| WriteToAll (t => t.Clear ()); | ||
| } | ||
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| // Copyright (c) 2025-2026 Stephen Shaw and contributors | ||
| // Licensed under the MIT License. See LICENSE file in the project root for full license information. | ||
|
|
||
| using TagLibSharp2.Core; | ||
|
|
||
| namespace TagLibSharp2.Xiph; | ||
|
|
||
| /// <summary> | ||
| /// Unified tag view over a <see cref="FlacFile"/>'s metadata. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// FLAC can store pictures in two spec-defined locations: | ||
| /// </para> | ||
| /// <list type="bullet"> | ||
| /// <item>Native PICTURE metadata blocks (block type 6), per RFC 9639 §8.8.</item> | ||
| /// <item>METADATA_BLOCK_PICTURE fields inside the VORBIS_COMMENT block (base64-encoded), | ||
| /// per https://wiki.xiph.org/VorbisComment#METADATA_BLOCK_PICTURE.</item> | ||
| /// </list> | ||
| /// <para> | ||
| /// This class provides a single Tag view that surfaces both so callers do not need | ||
| /// to know FLAC's internal storage layout. | ||
| /// </para> | ||
| /// </remarks> | ||
| public sealed class FlacTag : Tag | ||
| { | ||
| readonly FlacFile _file; | ||
|
|
||
| internal FlacTag (FlacFile file) | ||
| { | ||
| _file = file; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override TagTypes TagType => TagTypes.Xiph | TagTypes.FlacMetadata; | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Title { | ||
| get => _file.VorbisComment?.Title; | ||
| set => EnsureVorbisComment ().Title = value; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Artist { | ||
| get => _file.VorbisComment?.Artist; | ||
| set => EnsureVorbisComment ().Artist = value; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Album { | ||
| get => _file.VorbisComment?.Album; | ||
| set => EnsureVorbisComment ().Album = value; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Year { | ||
| get => _file.VorbisComment?.Year; | ||
| set => EnsureVorbisComment ().Year = value; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Comment { | ||
| get => _file.VorbisComment?.Comment; | ||
| set => EnsureVorbisComment ().Comment = value; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string? Genre { | ||
| get => _file.VorbisComment?.Genre; | ||
| set => EnsureVorbisComment ().Genre = value; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override uint? Track { | ||
| get => _file.VorbisComment?.Track; | ||
| set => EnsureVorbisComment ().Track = value; | ||
| } | ||
|
|
||
| VorbisComment EnsureVorbisComment () => | ||
| _file.VorbisComment ??= new VorbisComment ("TagLibSharp2"); | ||
|
|
||
| /// <inheritdoc/> | ||
| #pragma warning disable CA1819 // Properties should not return arrays - Tag API contract | ||
| public override IPicture[] Pictures { | ||
| get { | ||
| var blockPictures = _file.Pictures; | ||
| var embedded = _file.VorbisComment?.Pictures ?? []; | ||
| var merged = new List<IPicture> (blockPictures.Count + embedded.Length); | ||
| var seen = new HashSet<(PictureType, string, BinaryData)> (); | ||
| foreach (var p in blockPictures) { | ||
| if (seen.Add ((p.PictureType, p.MimeType, p.PictureData))) | ||
| merged.Add (p); | ||
| } | ||
| foreach (var p in embedded) { | ||
| if (seen.Add ((p.PictureType, p.MimeType, p.PictureData))) | ||
| merged.Add (p); | ||
| } | ||
| return [.. merged]; | ||
| } | ||
| set { | ||
| _file.RemoveAllPictures (); | ||
| _file.VorbisComment?.RemoveAllPictures (); | ||
| if (value is null) | ||
| return; | ||
| foreach (var p in value) { | ||
| var flacPic = p as FlacPicture ?? new FlacPicture ( | ||
| p.MimeType, p.PictureType, p.Description, p.PictureData, 0, 0, 0, 0); | ||
| _file.AddPicture (flacPic); | ||
| } | ||
| } | ||
| } | ||
| #pragma warning restore CA1819 | ||
|
|
||
| /// <inheritdoc/> | ||
| /// <exception cref="NotSupportedException"> | ||
| /// A FLAC file's tag state lives in multiple metadata blocks (VORBIS_COMMENT plus | ||
| /// zero or more PICTURE blocks) and is rendered as part of the full file layout. | ||
| /// It has no standalone binary representation. Render the owning | ||
| /// <see cref="FlacFile"/> instead. | ||
| /// </exception> | ||
| public override BinaryData Render () => | ||
| throw new NotSupportedException ( | ||
| "FlacTag is a view over multiple FLAC metadata blocks and has no standalone binary " | ||
| + "representation. Render the owning FlacFile instead (FlacFile.Render)."); | ||
|
|
||
| /// <inheritdoc/> | ||
| /// <remarks> | ||
| /// Clears every metadata source this view surfaces: the VorbisComment fields | ||
| /// (including any METADATA_BLOCK_PICTURE entries) and the native PICTURE blocks | ||
| /// held on the <see cref="FlacFile"/>. | ||
| /// </remarks> | ||
| public override void Clear () | ||
| { | ||
| _file.VorbisComment?.Clear (); | ||
| _file.RemoveAllPictures (); | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CombinedTag property setters are empty (no-op). That means writing via an IMediaFile.Tag facade (e.g., Mp3File.Tag or MediaFileResult.Tag) will silently drop changes, and Tag.CopyTo(target) will also be ineffective when the target is a CombinedTag. Consider either (a) forwarding setters to a canonical/primary underlying tag (and documenting which one), or (b) throwing NotSupportedException from setters to avoid silent failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adopted option (a) in 18bbe22. Setters now forward to every non-null underlying tag (not just one canonical member). Rationale: for MP3, both ID3v2 and ID3v1 persist on the file, so a user-facing
file.Tag.Title = "x"expects both to save with the same value rather than silently diverging. Format-specific limits still apply when each member stores the value (ID3v1 truncates to 30 bytes per https://id3.org/ID3v1).CombinedTagTests.Setters_WriteThroughToEveryMemberandMp3FileTests.Tag_Setter_WritesThroughToBothIdTagscover this.