ID3v2: Unify generic conversions

`Tag::save_to()` and `Into::<Id3v2Tag>::into(tag)` would take different paths, causing frames to not get merged (and possibly other weird behavior). Now, both take the same path.

All frames have been updated to use `Cow`, to avoid allocations in the `Tag::save_to()` path (unless absolutely necessary, like for track numbers).

This should be easier to maintain now that everything's unified. And possibly more efficient, since it's no longer cloning everything.

Author: Serial-ATA

CHANGELOG.md

@@ -24,6 +24,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Support can be enabled with the new `serde` feature (not enabled by default)
 - **Probe**: `Probe::read_bound()` ([PR](https://github.com/Serial-ATA/lofty-rs/pull/557))
   - Same as `Probe::read()`, but returns a [`BoundTaggedFile`](https://docs.rs/lofty/latest/lofty/file/struct.BoundTaggedFile.html)
+- **ID3v2** ([PR](https://github.com/Serial-ATA/lofty-rs/pull/576)):
+  - Unified the two generic conversion paths
+    - The background conversion used in `Tag::save_to()`, and the direct conversion done via `Into::<Id3v2Tag>::into(tag)` used to
+      take different paths, causing certain conversions and frame merging to not occur in the former case ([issue](https://github.com/Serial-ATA/lofty-rs/issues/349)).
+      They now use the same logic, which has also been rewritten to reuse data whenever possible, instead of cloning like before.
+  - The following frames now use `Cow` internally: `CommentFrame`, `UnsynchronizedTextFrame`, `TextInformationFrame`, `ExtendedTextFrame`, `UrlLinkFrame`, `ExtendedUrlFrame`,
+    `AttachedPictureFrame`, `PopularimeterFrame`, `KeyValueFrame`, `RelativeVolumeAdjustmentFrame`, `UniqueFileIdentifierFrame`, `OwnershipFrame`, `EventTimingCodesFrame`,
+    `PrivateFrame`, `BinaryFrame`
+  - `FrameId::is_valid()` and `FrameId::is_outdated()`
 - **Other**: `EXTENSIONS` list containing common file extensions for all supported audio file types ([issue](https://github.com/Serial-ATA/lofty-rs/issues/509)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/558))
   - This is useful for filtering files when scanning directories. If your app uses extension filtering, **please consider switching to this**, as to not
     miss any supported files.

lofty/src/flac/write.rs

@@ -36,7 +36,9 @@ where
 			write_to_inner(file, &mut comments_ref, write_options)
 		},
 		// This tag can *only* be removed in this format
-		TagType::Id3v2 => crate::id3::v2::tag::Id3v2TagRef::empty().write_to(file, write_options),
+		TagType::Id3v2 => {
+			crate::id3::v2::tag::conversion::Id3v2TagRef::empty().write_to(file, write_options)
+		},
 		_ => err!(UnsupportedTag),
 	}
 }

lofty/src/id3/v2/frame/conversion.rs

@@ -34,6 +34,15 @@ impl<'a> FrameHeader<'a> {
 	}
 }
 
+impl FrameHeader<'static> {
+	pub(crate) fn downgrade(&self) -> FrameHeader<'_> {
+		FrameHeader {
+			id: self.id.downgrade(),
+			flags: self.flags,
+		}
+	}
+}
+
 /// An `ID3v2` frame ID
 ///
 /// ⚠ WARNING ⚠: Be very careful when constructing this by hand. It is recommended to use [`FrameId::new`].
@@ -78,6 +87,47 @@ impl<'a> FrameId<'a> {
 		}
 	}
 
+	/// Whether this frame ID represents an outdated (ID3v2.2) ID
+	///
+	/// Note that frames with ID3v2.2 IDs *must* be upgraded to a 4-character ID3v2.3/4 ID in order to be
+	/// written, otherwise they will be discarded.
+	///
+	/// # Examples
+	///
+	/// ```rust
+	/// use lofty::id3::v2::FrameId;
+	///
+	/// # fn main() -> lofty::error::Result<()> {
+	/// let id_valid = FrameId::new("TPE1")?;
+	/// assert!(!id_valid.is_outdated());
+	///
+	/// let id_outdated = FrameId::new("TP1")?;
+	/// assert!(id_outdated.is_outdated());
+	/// # Ok(()) }
+	/// ```
+	pub fn is_outdated(&self) -> bool {
+		matches!(self, FrameId::Outdated(_))
+	}
+
+	/// Whether this frame ID represents a valid (ID3v2.3 or ID3v2.4) ID
+	///
+	/// # Examples
+	///
+	/// ```rust
+	/// use lofty::id3::v2::FrameId;
+	///
+	/// # fn main() -> lofty::error::Result<()> {
+	/// let id_valid = FrameId::new("TPE1")?;
+	/// assert!(id_valid.is_valid());
+	///
+	/// let id_outdated = FrameId::new("TP1")?;
+	/// assert!(!id_outdated.is_valid());
+	/// # Ok(()) }
+	/// ```
+	pub fn is_valid(&self) -> bool {
+		matches!(self, FrameId::Valid(_))
+	}
+
 	/// Extracts the string from the ID
 	pub fn as_str(&self) -> &str {
 		match self {
@@ -122,6 +172,15 @@ impl<'a> FrameId<'a> {
 	}
 }
 
+impl FrameId<'static> {
+	pub(crate) fn downgrade(&self) -> FrameId<'_> {
+		match self {
+			FrameId::Valid(id) => FrameId::Valid(Cow::Borrowed(&**id)),
+			FrameId::Outdated(id) => FrameId::Outdated(Cow::Borrowed(&**id)),
+		}
+	}
+}
+
 impl Display for FrameId<'_> {
 	fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
 		f.write_str(self.as_str())

lofty/src/id3/v2/frame/header/mod.rs

@@ -1,11 +1,11 @@
 use super::FrameFlags;
+use crate::config::ParseOptions;
 use crate::error::{Id3v2Error, Id3v2ErrorKind, Result};
 use crate::id3::v2::FrameId;
 use crate::id3::v2::util::synchsafe::SynchsafeInteger;
 use crate::id3::v2::util::upgrade::{upgrade_v2, upgrade_v3};
 use crate::util::text::utf8_decode_str;
 
-use crate::config::ParseOptions;
 use std::borrow::Cow;
 use std::io::Read;
 

lofty/src/id3/v2/frame/header/parse.rs

@@ -1,5 +1,4 @@
 pub(super) mod content;
-mod conversion;
 pub(super) mod header;
 pub(super) mod read;
 
@@ -17,7 +16,6 @@ use header::FrameId;
 
 use std::borrow::Cow;
 use std::hash::Hash;
-use std::ops::Deref;
 
 pub(super) const MUSICBRAINZ_UFID_OWNER: &str = "http://musicbrainz.org";
 
@@ -27,7 +25,7 @@ pub(super) const MUSICBRAINZ_UFID_OWNER: &str = "http://musicbrainz.org";
 /// are supposed to have an empty content descriptor. Only those
 /// are currently supported as [`TagItem`]s to avoid ambiguities
 /// and to prevent inconsistencies when writing them.
-pub(super) const EMPTY_CONTENT_DESCRIPTOR: String = String::new();
+pub(super) const EMPTY_CONTENT_DESCRIPTOR: Cow<'static, str> = Cow::Borrowed("");
 
 // TODO: Messy module, rough conversions
 
@@ -159,11 +157,34 @@ impl<'a> Frame<'a> {
 		Frame::Text(TextInformationFrame {
 			header: FrameHeader::new(FrameId::Valid(id), FrameFlags::default()),
 			encoding: TextEncoding::UTF8,
-			value: content,
+			value: Cow::Owned(content),
 		})
 	}
 }
 
+impl Frame<'static> {
+	pub(super) fn downgrade(&self) -> Frame<'_> {
+		match self {
+			Frame::Comment(f) => Frame::Comment(f.downgrade()),
+			Frame::UnsynchronizedText(f) => Frame::UnsynchronizedText(f.downgrade()),
+			Frame::Text(f) => Frame::Text(f.downgrade()),
+			Frame::UserText(f) => Frame::UserText(f.downgrade()),
+			Frame::Url(f) => Frame::Url(f.downgrade()),
+			Frame::UserUrl(f) => Frame::UserUrl(f.downgrade()),
+			Frame::Picture(f) => Frame::Picture(f.downgrade()),
+			Frame::Popularimeter(f) => Frame::Popularimeter(f.downgrade()),
+			Frame::KeyValue(f) => Frame::KeyValue(f.downgrade()),
+			Frame::RelativeVolumeAdjustment(f) => Frame::RelativeVolumeAdjustment(f.downgrade()),
+			Frame::UniqueFileIdentifier(f) => Frame::UniqueFileIdentifier(f.downgrade()),
+			Frame::Ownership(f) => Frame::Ownership(f.downgrade()),
+			Frame::EventTimingCodes(f) => Frame::EventTimingCodes(f.downgrade()),
+			Frame::Private(f) => Frame::Private(f.downgrade()),
+			Frame::Timestamp(f) => Frame::Timestamp(f.downgrade()),
+			Frame::Binary(f) => Frame::Binary(f.downgrade()),
+		}
+	}
+}
+
 impl Frame<'_> {
 	/// Check for empty content
 	///
@@ -405,24 +426,3 @@ impl FrameFlags {
 		flags
 	}
 }
-
-#[derive(Clone)]
-pub(crate) struct FrameRef<'a>(pub(crate) Cow<'a, Frame<'a>>);
-
-impl<'a> Deref for FrameRef<'a> {
-	type Target = Frame<'a>;
-
-	fn deref(&self) -> &Self::Target {
-		self.0.as_ref()
-	}
-}
-
-impl<'a> Frame<'a> {
-	pub(crate) fn as_opt_ref(&'a self) -> Option<FrameRef<'a>> {
-		if let FrameId::Valid(_) = self.id() {
-			Some(FrameRef(Cow::Borrowed(self)))
-		} else {
-			None
-		}
-	}
-}

lofty/src/id3/v2/frame/mod.rs

@@ -9,6 +9,7 @@ use crate::id3::v2::util::synchsafe::{SynchsafeInteger, UnsynchronizedStream};
 use crate::id3::v2::{BinaryFrame, FrameFlags, FrameHeader, FrameId};
 use crate::macros::try_vec;
 
+use std::borrow::Cow;
 use std::io::Read;
 
 use byteorder::{BigEndian, ReadBytesExt};
@@ -240,7 +241,7 @@ fn handle_encryption<R: Read>(
 
 	let encrypted_frame = Frame::Binary(BinaryFrame {
 		header: FrameHeader::new(id, flags),
-		data: content,
+		data: Cow::Owned(content),
 	});
 
 	// Nothing further we can do with encrypted frames