Split and rejoin tags for read/modify/write round trips

src/ape/tag/mod.rs

@@ -6,7 +6,7 @@ use crate::ape::tag::item::{ApeItem, ApeItemRef};
 use crate::error::{LoftyError, Result};
 use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
-use crate::traits::{Accessor, TagExt};
+use crate::traits::{Accessor, SplitAndMergeTag, TagExt};
 
 use std::borrow::Cow;
 use std::convert::TryInto;
@@ -289,8 +289,8 @@ impl TagExt for ApeTag {
 	}
 }
 
-impl From<ApeTag> for Tag {
-	fn from(input: ApeTag) -> Self {
+impl SplitAndMergeTag for ApeTag {
+	fn split_tag(&mut self) -> Tag {
 		fn split_pair(
 			content: &str,
 			tag: &mut Tag,
@@ -312,7 +312,7 @@ impl From<ApeTag> for Tag {
 
 		let mut tag = Tag::new(TagType::APE);
 
-		for item in input.items {
+		for item in std::mem::take(&mut self.items) {
 			let item_key = ItemKey::from_key(TagType::APE, item.key());
 
 			// The text pairs need some special treatment
@@ -321,13 +321,13 @@ impl From<ApeTag> for Tag {
 					if split_pair(val, &mut tag, ItemKey::TrackNumber, ItemKey::TrackTotal)
 						.is_some() =>
 				{
-					continue
+					continue; // Item consumed
 				},
 				(ItemKey::DiscNumber | ItemKey::DiscTotal, ItemValue::Text(val))
 					if split_pair(val, &mut tag, ItemKey::DiscNumber, ItemKey::DiscTotal)
 						.is_some() =>
 				{
-					continue
+					continue; // Item consumed
 				},
 				(ItemKey::MovementNumber | ItemKey::MovementTotal, ItemValue::Text(val))
 					if split_pair(
@@ -338,36 +338,46 @@ impl From<ApeTag> for Tag {
 					)
 					.is_some() =>
 				{
-					continue
+					continue; // Item consumed
+				},
+				(k, _) => {
+					tag.items.push(TagItem::new(k, item.value));
 				},
-				(k, _) => tag.items.push(TagItem::new(k, item.value)),
 			}
 		}
 
 		tag
 	}
-}
-
-impl From<Tag> for ApeTag {
-	fn from(input: Tag) -> Self {
-		let mut ape_tag = Self::default();
 
-		for item in input.items {
+	fn merge_tag(&mut self, tag: Tag) {
+		for item in tag.items {
 			if let Ok(ape_item) = item.try_into() {
-				ape_tag.insert(ape_item)
+				self.insert(ape_item)
 			}
 		}
 
-		for pic in input.pictures {
+		for pic in tag.pictures {
 			if let Some(key) = pic.pic_type.as_ape_key() {
 				if let Ok(item) =
 					ApeItem::new(key.to_string(), ItemValue::Binary(pic.as_ape_bytes()))
 				{
-					ape_tag.insert(item)
+					self.insert(item)
 				}
 			}
 		}
+	}
+}
+
+impl From<ApeTag> for Tag {
+	fn from(mut input: ApeTag) -> Self {
+		input.split_tag()
+	}
+}
 
+impl From<Tag> for ApeTag {
+	fn from(input: Tag) -> Self {
+		let mut ape_tag = Self::default();
+		ape_tag.merge_tag(input);
 		ape_tag
 	}
 }

src/id3/v1/tag.rs

@@ -2,7 +2,7 @@ use crate::error::{LoftyError, Result};
 use crate::id3::v1::constants::GENRES;
 use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
-use crate::traits::{Accessor, TagExt};
+use crate::traits::{Accessor, SplitAndMergeTag, TagExt};
 
 use std::borrow::Cow;
 use std::fs::{File, OpenOptions};
@@ -238,33 +238,49 @@ impl TagExt for ID3v1Tag {
 	}
 }
 
-impl From<ID3v1Tag> for Tag {
-	fn from(input: ID3v1Tag) -> Self {
-		let mut tag = Self::new(TagType::ID3v1);
+impl SplitAndMergeTag for ID3v1Tag {
+	fn split_tag(&mut self) -> Tag {
+		let mut tag = Tag::new(TagType::ID3v1);
 
-		input.title.map(|t| tag.insert_text(ItemKey::TrackTitle, t));
-		input
-			.artist
+		self.title
+			.take()
+			.map(|t| tag.insert_text(ItemKey::TrackTitle, t));
+		self.artist
+			.take()
 			.map(|a| tag.insert_text(ItemKey::TrackArtist, a));
-		input.album.map(|a| tag.insert_text(ItemKey::AlbumTitle, a));
-		input.year.map(|y| tag.insert_text(ItemKey::Year, y));
-		input.comment.map(|c| tag.insert_text(ItemKey::Comment, c));
-
-		if let Some(t) = input.track_number {
+		self.album
+			.take()
+			.map(|a| tag.insert_text(ItemKey::AlbumTitle, a));
+		self.year.take().map(|y| tag.insert_text(ItemKey::Year, y));
+		self.comment
+			.take()
+			.map(|c| tag.insert_text(ItemKey::Comment, c));
+
+		if let Some(t) = self.track_number.take() {
 			tag.items.push(TagItem::new(
 				ItemKey::TrackNumber,
 				ItemValue::Text(t.to_string()),
 			))
 		}
 
-		if let Some(genre_index) = input.genre {
+		if let Some(genre_index) = self.genre.take() {
 			if let Some(genre) = GENRES.get(genre_index as usize) {
 				tag.insert_text(ItemKey::Genre, (*genre).to_string());
 			}
 		}
 
 		tag
 	}
+
+	fn merge_tag(&mut self, tag: Tag) {
+		*self = tag.into();
+	}
+}
+
+impl From<ID3v1Tag> for Tag {
+	fn from(mut input: ID3v1Tag) -> Self {
+		input.split_tag()
+	}
 }
 
 impl From<Tag> for ID3v1Tag {

src/id3/v2/tag.rs

@@ -6,10 +6,10 @@ use crate::error::{LoftyError, Result};
 use crate::id3::v2::frame::FrameRef;
 use crate::id3::v2::items::encoded_text_frame::EncodedTextFrame;
 use crate::id3::v2::items::language_frame::LanguageFrame;
-use crate::picture::{Picture, PictureType};
+use crate::picture::{Picture, PictureType, TOMBSTONE_PICTURE};
 use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
-use crate::traits::{Accessor, TagExt};
+use crate::traits::{Accessor, SplitAndMergeTag, TagExt};
 use crate::util::text::TextEncoding;
 
 use std::borrow::Cow;
@@ -533,8 +533,8 @@ impl TagExt for ID3v2Tag {
 	}
 }
 
-impl From<ID3v2Tag> for Tag {
-	fn from(input: ID3v2Tag) -> Self {
+impl SplitAndMergeTag for ID3v2Tag {
+	fn split_tag(&mut self) -> Tag {
 		fn split_pair(
 			content: &str,
 			tag: &mut Tag,
@@ -555,13 +555,13 @@ impl From<ID3v2Tag> for Tag {
 			Some(())
 		}
 
-		let mut tag = Self::new(TagType::ID3v2);
+		let mut tag = Tag::new(TagType::ID3v2);
 
-		for frame in input.frames {
-			let id = frame.id;
+		self.frames.retain_mut(|frame| {
+			let id = &frame.id;
 
 			// The text pairs need some special treatment
-			match (id.as_str(), frame.value) {
+			match (id.as_str(), &mut frame.value) {
 				("TRCK", FrameValue::Text { value: content, .. })
 					if split_pair(
 						&content,
@@ -571,13 +571,13 @@ impl From<ID3v2Tag> for Tag {
 					)
 					.is_some() =>
 				{
-					continue
+					false // Frame consumed
 				},
 				("TPOS", FrameValue::Text { value: content, .. })
 					if split_pair(&content, &mut tag, ItemKey::DiscNumber, ItemKey::DiscTotal)
 						.is_some() =>
 				{
-					continue
+					false // Frame consumed
 				},
 				("MVIN", FrameValue::Text { value: content, .. })
 					if split_pair(
@@ -588,7 +588,7 @@ impl From<ID3v2Tag> for Tag {
 					)
 					.is_some() =>
 				{
-					continue
+					false // Frame consumed
 				},
 				// Store TXXX/WXXX frames by their descriptions, rather than their IDs
 				(
@@ -606,6 +606,7 @@ impl From<ID3v2Tag> for Tag {
 							ItemValue::Text(c.to_string()),
 						));
 					}
+					false // Frame consumed
 				},
 				(
 					"WXXX",
@@ -622,6 +623,7 @@ impl From<ID3v2Tag> for Tag {
 							ItemValue::Locator(c.to_string()),
 						));
 					}
+					false // Frame consumed
 				},
 				(id, value) => {
 					let item_key = ItemKey::from_key(TagType::ID3v2, id);
@@ -642,22 +644,22 @@ impl From<ID3v2Tag> for Tag {
 							description,
 							..
 						}) => {
-							if description == EMPTY_CONTENT_DESCRIPTOR {
+							if *description == EMPTY_CONTENT_DESCRIPTOR {
 								for c in content.split(V4_MULTI_VALUE_SEPARATOR) {
 									tag.items.push(TagItem::new(
 										item_key.clone(),
 										ItemValue::Text(c.to_string()),
 									));
 								}
+								return false; // Frame consumed
 							}
 							// ...else do not convert text frames with a non-empty content
 							// descriptor that would otherwise unintentionally be modified
 							// and corrupted by the incomplete conversion into a [`TagItem`].
 							// TODO: How to convert these frames consistently and safely
 							// such that the content descriptor is preserved during read/write
 							// round trips?
-
-							continue;
+							return true; // Keep frame
 						},
 						FrameValue::Text { value: content, .. } => {
 							for c in content.split(V4_MULTI_VALUE_SEPARATOR) {
@@ -666,34 +668,34 @@ impl From<ID3v2Tag> for Tag {
 									ItemValue::Text(c.to_string()),
 								));
 							}
-
-							continue;
+							return false; // Frame consumed
 						},
 						FrameValue::URL(content)
-						| FrameValue::UserURL(EncodedTextFrame { content, .. }) => ItemValue::Locator(content),
+						| FrameValue::UserURL(EncodedTextFrame { content, .. }) => {
+							ItemValue::Locator(std::mem::take(content))
+						},
 						FrameValue::Picture { picture, .. } => {
-							tag.push_picture(picture);
-							continue;
+							tag.push_picture(std::mem::replace(picture, TOMBSTONE_PICTURE));
+							return false; // Frame consumed
 						},
 						FrameValue::Popularimeter(popularimeter) => {
 							ItemValue::Binary(popularimeter.as_bytes())
 						},
-						FrameValue::Binary(binary) => ItemValue::Binary(binary),
+						FrameValue::Binary(binary) => ItemValue::Binary(std::mem::take(binary)),
 					};
 
 					tag.items.push(TagItem::new(item_key, item_value));
+					false // Frame consumed
 				},
 			}
-		}
+		});
 
 		tag
 	}
-}
 
-impl From<Tag> for ID3v2Tag {
-	fn from(mut input: Tag) -> Self {
-		fn join_items(input: &mut Tag, key: &ItemKey) -> String {
-			let mut iter = input.take_strings(key);
+	fn merge_tag(&mut self, mut tag: Tag) {
+		fn join_items(tag: &mut Tag, key: &ItemKey) -> String {
+			let mut iter = tag.take_strings(key);
 
 			match iter.next() {
 				None => String::new(),
@@ -710,25 +712,22 @@ impl From<Tag> for ID3v2Tag {
 			}
 		}
 
-		let mut id3v2_tag = ID3v2Tag {
-			frames: Vec::with_capacity(input.item_count() as usize),
-			..ID3v2Tag::default()
-		};
+		self.frames.reserve(tag.item_count() as usize);
 
-		let artists = join_items(&mut input, &ItemKey::TrackArtist);
-		id3v2_tag.set_artist(artists);
+		let artists = join_items(&mut tag, &ItemKey::TrackArtist);
+		self.set_artist(artists);
 
-		for item in input.items {
+		for item in tag.items {
 			let frame: Frame<'_> = match item.into() {
 				Some(frame) => frame,
 				None => continue,
 			};
 
-			id3v2_tag.insert(frame);
+			self.insert(frame);
 		}
 
-		for picture in input.pictures {
-			id3v2_tag.frames.push(Frame {
+		for picture in tag.pictures {
+			self.frames.push(Frame {
 				id: FrameID::Valid(Cow::Borrowed("APIC")),
 				value: FrameValue::Picture {
 					encoding: TextEncoding::UTF8,
@@ -737,7 +736,19 @@ impl From<Tag> for ID3v2Tag {
 				flags: FrameFlags::default(),
 			})
 		}
+	}
+}
+
+impl From<ID3v2Tag> for Tag {
+	fn from(mut input: ID3v2Tag) -> Self {
+		input.split_tag()
+	}
+}
 
+impl From<Tag> for ID3v2Tag {
+	fn from(input: Tag) -> Self {
+		let mut id3v2_tag = ID3v2Tag::default();
+		id3v2_tag.merge_tag(input);
 		id3v2_tag
 	}
 }