review: Address Copilot feedback on unified Tag views

Five review comments on #30 identified that the CombinedTag and FlacTag
facades had semantics inconsistent with the Tag abstraction and with
the rest of the IMediaFile implementations. All five are addressed:

1. CombinedTag.Render was a silent no-op returning empty bytes. A
   CombinedTag is a view, not a serializable block -- rendering it
   should surface misuse. Now throws NotSupportedException with a
   message pointing callers to the owning file or a specific underlying
   tag.

2. CombinedTag.Clear was a silent no-op. "Clear the tag" must actually
   zero out the metadata, not succeed while leaving the values intact.
   Now clears every non-null underlying tag.

3. CombinedTag setters were silent no-ops. Code in this repo and
   documented examples assume `file.Tag.Title = "x"` mutates the file,
   so silently dropping writes broke a load-bearing contract. Setters
   now write through to every non-null underlying tag, which also keeps
   ID3v2 + ID3v1 consistent on save.

4. FlacFile.Tag and Mp3File.Tag always returned a non-null facade even
   on files with no metadata. Other IMediaFile implementations (Dsf,
   Dff, Musepack, WavPack, MonkeysAudio, Ogg*, Asf, Mp4, Aiff) return
   null when the tag block is absent. Both now match that convention:
   null when the file has neither an underlying tag nor pictures.

5. FlacTag.Render/Clear had the same issue as #1-2. Render now throws
   NotSupportedException (a FlacTag spans multiple metadata blocks --
   render the FlacFile); Clear now wipes the VorbisComment fields
   (including embedded METADATA_BLOCK_PICTURE entries via
   VorbisComment.Clear) and removes native PICTURE blocks.

Test updates:

* CombinedTagTests: replaced the three no-op characterization tests with
  their mirror-image positive tests (setters write through, Render
  throws, Clear clears every member).
* FlacTagTests: replaced the combined "Render+Clear are no-ops" test
  with separate positive tests for Render throwing and Clear clearing.
  Added Tag_AbsentMetadata_ReturnsNull to lock in the nullability
  contract. Updated tests that started from a CreateMinimal() FLAC
  (Tag == null now) to seed with a VorbisComment first.
* Mp3FileTests: added Tag_NoTagsPresent_ReturnsNull for the nullability
  contract and Tag_Setter_WritesThroughToBothIdTags to verify the
  write-through behavior keeps v2 and v1 in sync.

All 3807 functional tests pass on net8.0 and net10.0.

Author: decriptor

src/TagLibSharp2/Core/CombinedTag.cs

@@ -14,9 +14,16 @@ namespace TagLibSharp2.Core;
 /// PictureData).
 /// </para>
 /// <para>
-/// Writes are no-ops on this base facade; callers wanting mutation should operate
-/// on the underlying tags directly. Format-specific subclasses (e.g. the FLAC tag
-/// view) override individual members to forward writes to the appropriate storage.
+/// 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
@@ -62,6 +69,14 @@ public CombinedTag (params Tag?[] tags)
 	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 {
@@ -77,43 +92,43 @@ public override TagTypes TagType {
 	/// <inheritdoc/>
 	public override string? Title {
 		get => FirstNonEmptyString (t => t.Title);
-		set { }
+		set => WriteToAll (t => t.Title = value);
 	}
 
 	/// <inheritdoc/>
 	public override string? Artist {
 		get => FirstNonEmptyString (t => t.Artist);
-		set { }
+		set => WriteToAll (t => t.Artist = value);
 	}
 
 	/// <inheritdoc/>
 	public override string? Album {
 		get => FirstNonEmptyString (t => t.Album);
-		set { }
+		set => WriteToAll (t => t.Album = value);
 	}
 
 	/// <inheritdoc/>
 	public override string? Year {
 		get => FirstNonEmptyString (t => t.Year);
-		set { }
+		set => WriteToAll (t => t.Year = value);
 	}
 
 	/// <inheritdoc/>
 	public override string? Comment {
 		get => FirstNonEmptyString (t => t.Comment);
-		set { }
+		set => WriteToAll (t => t.Comment = value);
 	}
 
 	/// <inheritdoc/>
 	public override string? Genre {
 		get => FirstNonEmptyString (t => t.Genre);
-		set { }
+		set => WriteToAll (t => t.Genre = value);
 	}
 
 	/// <inheritdoc/>
 	public override uint? Track {
 		get => FirstNonNullUInt (t => t.Track);
-		set { }
+		set => WriteToAll (t => t.Track = value);
 	}
 
 	/// <inheritdoc/>
@@ -132,13 +147,25 @@ public override IPicture[] Pictures {
 			}
 			return [.. merged];
 		}
-		set { }
+		set => WriteToAll (t => t.Pictures = value ?? []);
 	}
 #pragma warning restore CA1819
 
 	/// <inheritdoc/>
-	public override BinaryData Render () => BinaryData.Empty;
+	/// <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/>
-	public override void Clear () { }
+	/// <remarks>
+	/// Clears every non-null underlying tag, leaving each instance in place but empty.
+	/// </remarks>
+	public override void Clear () =>
+		WriteToAll (t => t.Clear ());
 }

src/TagLibSharp2/Mpeg/Mp3File.cs

@@ -88,7 +88,15 @@ public sealed class Mp3File : IMediaFile
 	public bool HasId3v2Tag => Id3v2Tag is not null;
 
 	/// <inheritdoc />
-	public Tag? Tag => new CombinedTag (Id3v2Tag, Id3v1Tag);
+	/// <inheritdoc />
+	/// <remarks>
+	/// Returns a <see cref="CombinedTag"/> composing the ID3v2 and ID3v1 tags when at
+	/// least one is present. ID3v2 takes priority; ID3v1 supplies fallback values for
+	/// fields not set in v2 (and receives mirrored writes so both tags stay in sync
+	/// on save). Returns null when neither tag is present.
+	/// </remarks>
+	public Tag? Tag =>
+		(Id3v2Tag is null && Id3v1Tag is null) ? null : new CombinedTag (Id3v2Tag, Id3v1Tag);
 
 	/// <inheritdoc />
 	IMediaProperties? IMediaFile.AudioProperties => Properties;

src/TagLibSharp2/Xiph/FlacFile.cs

@@ -274,7 +274,22 @@ public string? Comment {
 	FlacTag? _tag;
 
 	/// <inheritdoc />
-	public Tag? Tag => _tag ??= new FlacTag (this);
+	/// <remarks>
+	/// Returns null when the file has neither a VorbisComment nor any PICTURE blocks,
+	/// matching the "tag is absent" semantics used by other <see cref="IMediaFile"/>
+	/// implementations. Once instantiated, the <see cref="FlacTag"/> is cached so a
+	/// later setter call (e.g. <c>file.Tag.Title = "..."</c> after creating a
+	/// VorbisComment) sees the same view.
+	/// </remarks>
+	public Tag? Tag {
+		get {
+			if (_tag is not null)
+				return _tag;
+			if (VorbisComment is null && _pictures.Count == 0)
+				return null;
+			return _tag = new FlacTag (this);
+		}
+	}
 
 	/// <inheritdoc />
 	IMediaProperties? IMediaFile.AudioProperties => Properties;

src/TagLibSharp2/Xiph/FlacTag.cs

@@ -112,8 +112,26 @@ public override IPicture[] Pictures {
 #pragma warning restore CA1819
 
 	/// <inheritdoc/>
-	public override BinaryData Render () => BinaryData.Empty;
+	/// <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/>
-	public override void Clear () { }
+	/// <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 ();
+	}
 }

tests/TagLibSharp2.Tests/Core/CombinedTagTests.cs

@@ -150,45 +150,58 @@ public void Pictures_UnionsAcrossTagsWithDedup ()
 	}
 
 	/// <summary>
-	/// The base <see cref="CombinedTag"/> exposes writes as no-ops so that it
-	/// cannot accidentally mutate underlying tags without a subclass making an
-	/// explicit decision. Format-specific subclasses override individual
-	/// setters when write-through is the correct behavior.
+	/// Setters write through to every non-null underlying tag so content stays
+	/// consistent across formats. For MP3, that means a single
+	/// <c>file.Tag.Title = "x"</c> updates both ID3v2 and ID3v1, matching the
+	/// round-trip expectation that saving a file emits matching values in both.
+	/// Per-format length/encoding limits (e.g. ID3v1's 30-byte cap,
+	/// https://id3.org/ID3v1) still apply when each member stores the value.
 	/// </summary>
 	[TestMethod]
-	public void Setters_AreNoOpsOnBaseFacade ()
+	public void Setters_WriteThroughToEveryMember ()
 	{
-		var primary = new Id3v2Tag { Title = "Original" };
-		var combined = new CombinedTag (primary);
+		var primary = new Id3v2Tag { Title = "Original 2", Track = 1 };
+		var secondary = new Id3v1Tag { Title = "Original 1", Track = 2 };
+		var combined = new CombinedTag (primary, secondary);
 
 		combined.Title = "Changed";
 		combined.Track = 42;
-		combined.Pictures = [];
 
-		Assert.AreEqual ("Original", primary.Title, "Base CombinedTag.Title setter does not mutate members");
-		Assert.IsNull (primary.Track, "Base CombinedTag.Track setter does not mutate members");
+		Assert.AreEqual ("Changed", primary.Title);
+		Assert.AreEqual ("Changed", secondary.Title);
+		Assert.AreEqual ((uint)42, primary.Track);
+		Assert.AreEqual ((uint)42, secondary.Track);
 	}
 
+	/// <summary>
+	/// A <see cref="CombinedTag"/> is a view — it has no standalone binary
+	/// representation. Callers should render the owning file or a specific
+	/// underlying tag; calling <see cref="Tag.Render"/> on the facade throws
+	/// to surface the misuse instead of silently returning empty bytes.
+	/// </summary>
 	[TestMethod]
-	public void Render_ReturnsEmptyBinaryData ()
+	public void Render_ThrowsNotSupported ()
 	{
 		var combined = new CombinedTag (new Id3v2Tag { Title = "x" });
 
-		var rendered = combined.Render ();
-
-		Assert.IsTrue (rendered.IsEmpty,
-			"CombinedTag is a view; it does not own bytes and Render returns empty");
+		Assert.ThrowsExactly<NotSupportedException> (() => combined.Render ());
 	}
 
+	/// <summary>
+	/// <see cref="Tag.Clear"/> on the facade clears every non-null underlying
+	/// tag. Callers expect "clear the tag" to actually zero out the metadata,
+	/// not silently succeed with the old values intact.
+	/// </summary>
 	[TestMethod]
-	public void Clear_IsNoOp ()
+	public void Clear_ClearsEveryUnderlyingMember ()
 	{
-		var primary = new Id3v2Tag { Title = "Keep Me" };
-		var combined = new CombinedTag (primary);
+		var primary = new Id3v2Tag { Title = "Wipe Me" };
+		var secondary = new Id3v1Tag { Title = "Also Wipe Me" };
+		var combined = new CombinedTag (primary, secondary);
 
 		combined.Clear ();
 
-		Assert.AreEqual ("Keep Me", primary.Title,
-			"Base CombinedTag.Clear does not mutate members");
+		Assert.IsTrue (string.IsNullOrEmpty (primary.Title));
+		Assert.IsTrue (string.IsNullOrEmpty (secondary.Title));
 	}
 }

tests/TagLibSharp2.Tests/Mpeg/Mp3FileTests.cs

@@ -124,6 +124,51 @@ public void Tag_BothIdTags_PrefersId3v2ButFallsBackToId3v1 ()
 			"Genre should fall back to ID3v1 when ID3v2 has no Genre");
 	}
 
+	/// <summary>
+	/// An MP3 file with no ID3v2 and no ID3v1 must expose <c>Tag == null</c>,
+	/// matching the "tag is absent" convention used by other
+	/// <see cref="IMediaFile"/> implementations in this repo. A Mp3File that
+	/// always materialized a facade would lie about the absence of metadata.
+	/// </summary>
+	[TestMethod]
+	public void Tag_NoTagsPresent_ReturnsNull ()
+	{
+		var audioOnly = new byte[256];
+
+		var result = Mp3File.Read (audioOnly);
+
+		Assert.IsTrue (result.IsSuccess);
+		Assert.IsNull (result.File!.Id3v2Tag);
+		Assert.IsNull (result.File.Id3v1Tag);
+		Assert.IsNull (result.File.Tag);
+	}
+
+	/// <summary>
+	/// Writing through the combined Tag view updates both ID3v2 and ID3v1 so the
+	/// file saves with consistent metadata in both tags. ID3v1's format-specific
+	/// limits still apply (30-byte cap per https://id3.org/ID3v1), but the
+	/// values round-trip as far as each format allows.
+	/// </summary>
+	[TestMethod]
+	public void Tag_Setter_WritesThroughToBothIdTags ()
+	{
+		var id3v2 = new Id3v2Tag { Title = "Before" };
+		var id3v1 = new Id3v1Tag { Title = "Before" };
+		var v2Data = id3v2.Render ();
+		var v1Data = id3v1.Render ();
+
+		var audioData = new byte[256];
+		var fullData = new byte[v2Data.Length + audioData.Length + v1Data.Length];
+		v2Data.Span.CopyTo (fullData);
+		v1Data.Span.CopyTo (fullData.AsSpan (v2Data.Length + audioData.Length));
+
+		var file = Mp3File.Read (fullData).File!;
+		file.Tag!.Title = "After";
+
+		Assert.AreEqual ("After", file.Id3v2Tag!.Title, "ID3v2 updated");
+		Assert.AreEqual ("After", file.Id3v1Tag!.Title, "ID3v1 updated");
+	}
+
 	[TestMethod]
 	public void Read_BothTags_ParsesBoth ()
 	{