Add tag conversion tests

src/logic/ape/tag/item.rs

@@ -32,6 +32,14 @@ impl ApeItem {
 	pub fn set_read_only(&mut self) {
 		self.read_only = true
 	}
+
+	pub fn key(&self) -> &str {
+		&self.key
+	}
+
+	pub fn value(&self) -> &ItemValue {
+		&self.value
+	}
 }
 
 impl TryFrom<TagItem> for ApeItem {
@@ -53,6 +61,7 @@ impl TryFrom<TagItem> for ApeItem {
 
 pub(in crate::logic) struct ApeItemRef<'a> {
 	pub read_only: bool,
+	pub key: &'a str,
 	pub value: ItemValueRef<'a>,
 }
 
@@ -60,6 +69,7 @@ impl<'a> Into<ApeItemRef<'a>> for &'a ApeItem {
 	fn into(self) -> ApeItemRef<'a> {
 		ApeItemRef {
 			read_only: self.read_only,
+			key: self.key(),
 			value: (&self.value).into(),
 		}
 	}

src/logic/ape/tag/mod.rs

@@ -7,7 +7,6 @@ use crate::logic::ape::tag::item::{ApeItem, ApeItemRef};
 use crate::types::item::{ItemKey, ItemValue, TagItem};
 use crate::types::tag::{Tag, TagType};
 
-use std::collections::HashMap;
 use std::convert::TryInto;
 use std::fs::File;
 use std::io::{Read, Seek};
@@ -16,20 +15,30 @@ use std::io::{Read, Seek};
 /// An APE tag
 pub struct ApeTag {
 	pub read_only: bool,
-	pub(super) items: HashMap<String, ApeItem>,
+	pub(super) items: Vec<ApeItem>,
 }
 
 impl ApeTag {
 	pub fn get_key(&self, key: &str) -> Option<&ApeItem> {
-		self.items.get(key)
+		self.items
+			.iter()
+			.find(|i| i.key().eq_ignore_ascii_case(key))
 	}
 
-	pub fn push_item(&mut self, value: ApeItem) {
-		let _ = self.items.insert(value.key.clone(), value);
+	pub fn insert(&mut self, value: ApeItem) {
+		self.remove_key(value.key());
+		self.items.push(value);
 	}
 
 	pub fn remove_key(&mut self, key: &str) {
-		let _ = self.items.remove(key);
+		self.items
+			.iter()
+			.position(|i| i.key() == key)
+			.map(|p| self.items.remove(p));
+	}
+
+	pub fn items(&self) -> &[ApeItem] {
+		&self.items
 	}
 }
 
@@ -50,7 +59,7 @@ impl From<ApeTag> for Tag {
 	fn from(input: ApeTag) -> Self {
 		let mut tag = Tag::new(TagType::Ape);
 
-		for (_, item) in input.items {
+		for item in input.items {
 			let item = TagItem::new(ItemKey::from_key(&TagType::Ape, &*item.key), item.value);
 
 			tag.insert_item_unchecked(item)
@@ -66,7 +75,7 @@ impl From<Tag> for ApeTag {
 
 		for item in input.items {
 			if let Ok(ape_item) = item.try_into() {
-				ape_tag.push_item(ape_item)
+				ape_tag.insert(ape_item)
 			}
 		}
 
@@ -75,7 +84,7 @@ impl From<Tag> for ApeTag {
 				if let Ok(item) =
 					ApeItem::new(key.to_string(), ItemValue::Binary(pic.as_ape_bytes()))
 				{
-					ape_tag.push_item(item)
+					ape_tag.insert(item)
 				}
 			}
 		}
@@ -86,34 +95,28 @@ impl From<Tag> for ApeTag {
 
 pub(in crate::logic) struct ApeTagRef<'a> {
 	read_only: bool,
-	pub(super) items: HashMap<&'a str, ApeItemRef<'a>>,
+	pub(super) items: Box<dyn Iterator<Item = ApeItemRef<'a>> + 'a>,
 }
 
 impl<'a> ApeTagRef<'a> {
-	pub(crate) fn write_to(&self, file: &mut File) -> Result<()> {
+	pub(crate) fn write_to(&mut self, file: &mut File) -> Result<()> {
 		write::write_to(file, self)
 	}
 }
 
 impl<'a> Into<ApeTagRef<'a>> for &'a Tag {
 	fn into(self) -> ApeTagRef<'a> {
-		let mut items = HashMap::<&'a str, ApeItemRef<'a>>::new();
-
-		for item in &self.items {
-			let key = item.key().map_key(&TagType::Ape, true).unwrap();
-
-			items.insert(
-				key,
-				ApeItemRef {
-					read_only: false,
-					value: (&item.item_value).into(),
-				},
-			);
-		}
-
 		ApeTagRef {
 			read_only: false,
-			items,
+			items: Box::new(self.items.iter().filter_map(|i| {
+				i.key().map_key(&TagType::Ape, true).map_or(None, |key| {
+					Some(ApeItemRef {
+						read_only: false,
+						key,
+						value: (&i.item_value).into(),
+					})
+				})
+			})),
 		}
 	}
 }
@@ -122,15 +125,7 @@ impl<'a> Into<ApeTagRef<'a>> for &'a ApeTag {
 	fn into(self) -> ApeTagRef<'a> {
 		ApeTagRef {
 			read_only: self.read_only,
-			items: {
-				let mut items = HashMap::<&str, ApeItemRef<'a>>::new();
-
-				for (k, v) in &self.items {
-					items.insert(k.as_str(), v.into());
-				}
-
-				items
-			},
+			items: Box::new(self.items.iter().map(|i| i.into())),
 		}
 	}
 }

src/logic/ape/tag/read.rs

@@ -88,7 +88,7 @@ where
 			item.set_read_only()
 		}
 
-		tag.push_item(item);
+		tag.insert(item);
 	}
 
 	// Version 1 doesn't include a header

src/logic/ape/tag/write.rs

@@ -13,7 +13,7 @@ use std::io::{Cursor, Read, Seek, SeekFrom, Write};
 
 use byteorder::{LittleEndian, WriteBytesExt};
 
-pub(in crate::logic) fn write_to(data: &mut File, tag: &ApeTagRef) -> Result<()> {
+pub(in crate::logic) fn write_to(data: &mut File, tag: &mut ApeTagRef) -> Result<()> {
 	match Probe::new().file_type(data) {
 		Some(ft) if ft == FileType::APE || ft == FileType::MP3 => {},
 		_ => return Err(LoftyError::UnsupportedTag),
@@ -39,7 +39,7 @@ pub(in crate::logic) fn write_to(data: &mut File, tag: &ApeTagRef) -> Result<()>
 		let (mut existing, size) = read_ape_tag(data, false)?;
 
 		// Only keep metadata around that's marked read only
-		existing.items.retain(|_i, v| v.read_only);
+		existing.items.retain(|i| i.read_only);
 
 		if !existing.items.is_empty() {
 			read_only = Some(existing)
@@ -70,7 +70,7 @@ pub(in crate::logic) fn write_to(data: &mut File, tag: &ApeTagRef) -> Result<()>
 
 		let (mut existing, size) = read_ape_tag(data, true)?;
 
-		existing.items.retain(|_, v| v.read_only);
+		existing.items.retain(|i| i.read_only);
 
 		if !existing.items.is_empty() {
 			read_only = Some(existing)
@@ -86,7 +86,7 @@ pub(in crate::logic) fn write_to(data: &mut File, tag: &ApeTagRef) -> Result<()>
 
 	// Preserve any metadata marked as read only
 	let tag = if let Some(read_only) = read_only {
-		create_ape_tag(&Into::<ApeTagRef>::into(&read_only))?
+		create_ape_tag(&mut Into::<ApeTagRef>::into(&read_only))?
 	} else {
 		create_ape_tag(tag)?
 	};
@@ -115,17 +115,20 @@ pub(in crate::logic) fn write_to(data: &mut File, tag: &ApeTagRef) -> Result<()>
 	Ok(())
 }
 
-fn create_ape_tag(tag: &ApeTagRef) -> Result<Vec<u8>> {
+fn create_ape_tag(tag: &mut ApeTagRef) -> Result<Vec<u8>> {
+	let items = &mut tag.items;
+	let mut peek = items.peekable();
+
 	// Unnecessary to write anything if there's no metadata
-	if tag.items.is_empty() {
+	if peek.peek().is_none() {
 		Ok(Vec::<u8>::new())
 	} else {
 		let mut tag_write = Cursor::new(Vec::<u8>::new());
 
-		let item_count = tag.items.len() as u32;
+		let mut item_count = 0_u32;
 
-		for (k, v) in &tag.items {
-			let (mut flags, value) = match v.value {
+		for item in peek {
+			let (mut flags, value) = match item.value {
 				ItemValueRef::Binary(value) => {
 					tag_write.write_u32::<LittleEndian>(value.len() as u32)?;
 
@@ -143,14 +146,16 @@ fn create_ape_tag(tag: &ApeTagRef) -> Result<Vec<u8>> {
 				},
 			};
 
-			if v.read_only {
+			if item.read_only {
 				flags |= 1_u32
 			}
 
 			tag_write.write_u32::<LittleEndian>(flags)?;
-			tag_write.write_all(k.as_bytes())?;
+			tag_write.write_all(item.key.as_bytes())?;
 			tag_write.write_u8(0)?;
 			tag_write.write_all(value)?;
+
+			item_count += 1;
 		}
 
 		let size = tag_write.get_ref().len();

src/logic/id3/v1/constants.rs

@@ -196,10 +196,12 @@ pub const GENRES: [&str; 192] = [
 	"Psybient",
 ];
 
-pub const VALID_ITEMKEYS: [ItemKey; 5] = [
+pub const VALID_ITEMKEYS: [ItemKey; 7] = [
 	ItemKey::TrackTitle,
 	ItemKey::TrackArtist,
 	ItemKey::AlbumTitle,
 	ItemKey::Year,
 	ItemKey::Comment,
+	ItemKey::TrackNumber,
+	ItemKey::Genre,
 ];

src/logic/id3/v1/tag.rs

@@ -106,7 +106,7 @@ impl From<Tag> for Id3v1Tag {
 			year: input.get_string(&ItemKey::Year).map(str::to_owned),
 			comment: input.get_string(&ItemKey::Comment).map(str::to_owned),
 			track_number: input
-				.get_string(&ItemKey::Genre)
+				.get_string(&ItemKey::TrackNumber)
 				.map(|g| g.parse::<u8>().ok())
 				.and_then(|g| g),
 			genre: input
@@ -155,7 +155,7 @@ impl<'a> Into<Id3v1TagRef<'a>> for &'a Tag {
 			year: self.get_string(&ItemKey::Year),
 			comment: self.get_string(&ItemKey::Comment),
 			track_number: self
-				.get_string(&ItemKey::Genre)
+				.get_string(&ItemKey::TrackNumber)
 				.map(|g| g.parse::<u8>().ok())
 				.and_then(|g| g),
 			genre: self

src/logic/iff/wav/tag/mod.rs

@@ -16,6 +16,13 @@ pub struct RiffInfoList {
 }
 
 impl RiffInfoList {
+	pub fn get(&self, key: &str) -> Option<&str> {
+		self.items
+			.iter()
+			.find(|(k, _)| k == key)
+			.map(|(_, v)| v.as_str())
+	}
+
 	pub fn insert(&mut self, key: String, value: String) {
 		if valid_key(key.as_str()) {
 			self.items

src/logic/mp4/ilst/mod.rs

@@ -119,6 +119,14 @@ impl Atom {
 	pub fn new(ident: AtomIdent, data: AtomData) -> Self {
 		Self { ident, data }
 	}
+
+	pub fn ident(&self) -> &AtomIdent {
+		&self.ident
+	}
+
+	pub fn data(&self) -> &AtomData {
+		&self.data
+	}
 }
 
 #[derive(Eq, PartialEq, Debug)]

src/types/item.rs

@@ -342,7 +342,8 @@ item_keys!(
 	// Style
 	Genre => [
 		TagType::Id3v2 => "TCON", TagType::Mp4Atom => "\u{a9}gen",
-		TagType::VorbisComments => "GENRE", TagType::RiffInfo => "IGNR"
+		TagType::VorbisComments => "GENRE", TagType::RiffInfo => "IGNR",
+		TagType::Ape => "Genre"
 	],
 	InitialKey => [
 		TagType::Id3v2 => "TKEY"

src/types/tag.rs

@@ -156,11 +156,6 @@ impl Tag {
 	/// Insert a [`TagItem`], replacing any existing one of the same type
 	///
 	/// NOTE: This **will** verify an [`ItemKey`] mapping exists for the target [`TagType`]
-	///
-	/// # Warning
-	///
-	/// When dealing with ID3v2, it may be necessary to use [`insert_item_unchecked`](Tag::insert_item_unchecked).
-	/// See [`id3`](crate::id3::v2) for an explanation.
 	pub fn insert_item(&mut self, item: TagItem) -> bool {
 		if item.re_map(&self.tag_type).is_some() {
 			self.insert_item_unchecked(item);

tests/tags/conversions.rs

@@ -0,0 +1,219 @@
+use crate::{APE, ID3V1, ID3V2, ILST, RIFF_INFO, VORBIS_COMMENTS};
+
+use lofty::ape::ApeTag;
+use lofty::id3::v1::Id3v1Tag;
+use lofty::id3::v2::{FrameValue, Id3v2Tag, LanguageFrame, TextEncoding};
+use lofty::iff::RiffInfoList;
+use lofty::mp4::{AtomData, AtomIdent, Ilst};
+use lofty::ogg::VorbisComments;
+use lofty::{ItemKey, ItemValue, Tag, TagType};
+
+fn create_tag(tag_type: TagType) -> Tag {
+	let mut tag = Tag::new(tag_type);
+
+	tag.insert_text(ItemKey::TrackTitle, String::from("Foo title"));
+	tag.insert_text(ItemKey::TrackArtist, String::from("Bar artist"));
+	tag.insert_text(ItemKey::AlbumTitle, String::from("Baz album"));
+	tag.insert_text(ItemKey::Comment, String::from("Qux comment"));
+	tag.insert_text(ItemKey::TrackNumber, String::from("1"));
+	tag.insert_text(ItemKey::Genre, String::from("Classical"));
+
+	tag
+}
+
+fn verify_tag(tag: &Tag, track_number: bool, genre: bool) {
+	assert_eq!(tag.get_string(&ItemKey::TrackTitle), Some("Foo title"));
+	assert_eq!(tag.get_string(&ItemKey::TrackArtist), Some("Bar artist"));
+	assert_eq!(tag.get_string(&ItemKey::AlbumTitle), Some("Baz album"));
+	assert_eq!(tag.get_string(&ItemKey::Comment), Some("Qux comment"));
+
+	if track_number {
+		assert_eq!(tag.get_string(&ItemKey::TrackNumber), Some("1"));
+	}
+
+	if genre {
+		assert_eq!(tag.get_string(&ItemKey::Genre), Some("Classical"));
+	}
+}
+
+#[test]
+fn ape_to_tag() {
+	let ape = ApeTag::read_from(&mut std::io::Cursor::new(&APE[..])).unwrap();
+
+	let tag: Tag = ape.into();
+
+	verify_tag(&tag, true, true);
+}
+
+#[test]
+fn tag_to_ape() {
+	fn verify_key(tag: &ApeTag, key: &str, expected_val: &str) {
+		assert_eq!(
+			tag.get_key(key).map(|i| i.value()),
+			Some(&ItemValue::Text(String::from(expected_val)))
+		);
+	}
+
+	let tag = create_tag(TagType::Ape);
+
+	let ape_tag: ApeTag = tag.into();
+
+	verify_key(&ape_tag, "Title", "Foo title");
+	verify_key(&ape_tag, "Artist", "Bar artist");
+	verify_key(&ape_tag, "Album", "Baz album");
+	verify_key(&ape_tag, "Comment", "Qux comment");
+	verify_key(&ape_tag, "Track", "1");
+	verify_key(&ape_tag, "Genre", "Classical");
+}
+
+#[test]
+fn id3v1_to_tag() {
+	let id3v1 = Id3v1Tag::read_from(ID3V1);
+
+	let tag: Tag = id3v1.into();
+
+	verify_tag(&tag, true, true);
+}
+
+#[test]
+fn tag_to_id3v1() {
+	let tag = create_tag(TagType::Id3v1);
+
+	let id3v1_tag: Id3v1Tag = tag.into();
+
+	assert_eq!(id3v1_tag.title.as_deref(), Some("Foo title"));
+	assert_eq!(id3v1_tag.artist.as_deref(), Some("Bar artist"));
+	assert_eq!(id3v1_tag.album.as_deref(), Some("Baz album"));
+	assert_eq!(id3v1_tag.comment.as_deref(), Some("Qux comment"));
+	assert_eq!(id3v1_tag.track_number, Some(1));
+	assert_eq!(id3v1_tag.genre, Some(32));
+}
+
+#[test]
+fn id3v2_to_tag() {
+	let id3v2 = Id3v2Tag::read_from(&mut &ID3V2[..]).unwrap();
+
+	let tag: Tag = id3v2.into();
+
+	verify_tag(&tag, true, true);
+}
+
+#[test]
+fn tag_to_id3v2() {
+	fn verify_frame(tag: &Id3v2Tag, id: &str, value: &str) {
+		let frame = tag.get(id);
+
+		assert!(frame.is_some());
+
+		let frame = frame.unwrap();
+
+		assert_eq!(
+			frame.content(),
+			&FrameValue::Text {
+				encoding: TextEncoding::UTF8,
+				value: String::from(value)
+			}
+		);
+	}
+
+	let tag = create_tag(TagType::Id3v2);
+
+	let id3v2_tag: Id3v2Tag = tag.into();
+
+	verify_frame(&id3v2_tag, "TIT2", "Foo title");
+	verify_frame(&id3v2_tag, "TPE1", "Bar artist");
+	verify_frame(&id3v2_tag, "TALB", "Baz album");
+
+	let frame = id3v2_tag.get("COMM").unwrap();
+	assert_eq!(
+		frame.content(),
+		&FrameValue::Comment(LanguageFrame {
+			encoding: TextEncoding::Latin1,
+			language: String::from("eng"),
+			description: String::new(),
+			content: String::from("Qux comment")
+		})
+	);
+
+	verify_frame(&id3v2_tag, "TRCK", "1");
+	verify_frame(&id3v2_tag, "TCON", "Classical");
+}
+
+#[test]
+fn ilst_to_tag() {
+	let ilst = Ilst::read_from(&mut &ILST[..], (ILST.len() - 1) as u64).unwrap();
+
+	let tag: Tag = ilst.into();
+
+	verify_tag(&tag, false, true);
+}
+
+#[test]
+fn tag_to_ilst() {
+	fn verify_atom(ilst: &Ilst, ident: [u8; 4], data: &str) {
+		let atom = ilst.atom(&AtomIdent::Fourcc(ident)).unwrap();
+
+		let data = AtomData::UTF8(String::from(data));
+
+		assert_eq!(atom.data(), &data);
+	}
+
+	let tag = create_tag(TagType::Mp4Atom);
+
+	let ilst: Ilst = tag.into();
+
+	verify_atom(&ilst, *b"\xa9nam", "Foo title");
+	verify_atom(&ilst, *b"\xa9ART", "Bar artist");
+	verify_atom(&ilst, *b"\xa9alb", "Baz album");
+	verify_atom(&ilst, *b"\xa9cmt", "Qux comment");
+	verify_atom(&ilst, *b"\xa9gen", "Classical");
+}
+
+#[test]
+fn riff_info_to_tag() {
+	let riff_info = RiffInfoList::read_from(
+		&mut std::io::Cursor::new(&RIFF_INFO),
+		(RIFF_INFO.len() - 1) as u64,
+	)
+	.unwrap();
+
+	let tag: Tag = riff_info.into();
+
+	verify_tag(&tag, true, false);
+}
+
+#[test]
+fn tag_to_riff_info() {
+	let tag = create_tag(TagType::RiffInfo);
+
+	let riff_info: RiffInfoList = tag.into();
+
+	assert_eq!(riff_info.get("INAM"), Some("Foo title"));
+	assert_eq!(riff_info.get("IART"), Some("Bar artist"));
+	assert_eq!(riff_info.get("IPRD"), Some("Baz album"));
+	assert_eq!(riff_info.get("ICMT"), Some("Qux comment"));
+	assert_eq!(riff_info.get("IPRT"), Some("1"));
+}
+
+#[test]
+fn vorbis_comments_to_tag() {
+	let vorbis_comments = VorbisComments::read_from(&mut &VORBIS_COMMENTS[..]).unwrap();
+
+	let tag: Tag = vorbis_comments.into();
+
+	verify_tag(&tag, true, true);
+}
+
+#[test]
+fn tag_to_vorbis_comments() {
+	let tag = create_tag(TagType::VorbisComments);
+
+	let vorbis_comments: VorbisComments = tag.into();
+
+	assert_eq!(vorbis_comments.get_item("TITLE"), Some("Foo title"));
+	assert_eq!(vorbis_comments.get_item("ARTIST"), Some("Bar artist"));
+	assert_eq!(vorbis_comments.get_item("ALBUM"), Some("Baz album"));
+	assert_eq!(vorbis_comments.get_item("COMMENT"), Some("Qux comment"));
+	assert_eq!(vorbis_comments.get_item("TRACKNUMBER"), Some("1"));
+	assert_eq!(vorbis_comments.get_item("GENRE"), Some("Classical"));
+}

tests/tags/main.rs

@@ -1 +1,9 @@
+mod conversions;
 mod read;
+
+const APE: [u8; 209] = *include_bytes!("assets/test.apev2");
+const ID3V1: [u8; 128] = *include_bytes!("assets/test.id3v1");
+const ID3V2: [u8; 1168] = *include_bytes!("assets/test.id3v2");
+const ILST: [u8; 1024] = *include_bytes!("assets/test.ilst");
+const RIFF_INFO: [u8; 100] = *include_bytes!("assets/test.riff");
+const VORBIS_COMMENTS: [u8; 152] = *include_bytes!("assets/test.vorbis");

tests/tags/read.rs

@@ -6,12 +6,7 @@ use lofty::mp4::{Atom, AtomData, AtomIdent, Ilst};
 use lofty::ogg::VorbisComments;
 use lofty::ItemValue;
 
-const APE: [u8; 209] = *include_bytes!("assets/test.apev2");
-const ID3V1: [u8; 128] = *include_bytes!("assets/test.id3v1");
-const ID3V2: [u8; 1168] = *include_bytes!("assets/test.id3v2");
-const ILST: [u8; 1024] = *include_bytes!("assets/test.ilst");
-const RIFF_INFO: [u8; 100] = *include_bytes!("assets/test.riff");
-const VORBIS_COMMENTS: [u8; 152] = *include_bytes!("assets/test.vorbis");
+use crate::{APE, ID3V1, ID3V2, ILST, RIFF_INFO, VORBIS_COMMENTS};
 
 #[test]
 fn read_ape() {
@@ -53,17 +48,21 @@ fn read_ape() {
 	)
 	.unwrap();
 
-	expected_tag.push_item(title_item);
-	expected_tag.push_item(artist_item);
-	expected_tag.push_item(album_item);
-	expected_tag.push_item(comment_item);
-	expected_tag.push_item(year_item);
-	expected_tag.push_item(track_number_item);
-	expected_tag.push_item(genre_item);
+	expected_tag.insert(title_item);
+	expected_tag.insert(artist_item);
+	expected_tag.insert(album_item);
+	expected_tag.insert(comment_item);
+	expected_tag.insert(year_item);
+	expected_tag.insert(track_number_item);
+	expected_tag.insert(genre_item);
 
 	let parsed_tag = ApeTag::read_from(&mut std::io::Cursor::new(APE)).unwrap();
 
-	assert_eq!(expected_tag, parsed_tag);
+	assert_eq!(expected_tag.items().len(), parsed_tag.items().len());
+
+	for item in expected_tag.items() {
+		assert!(parsed_tag.items().contains(item))
+	}
 }
 
 #[test]