ItemKey: Remove ItemKey::Unknown

`ItemKey::Unknown` has served its purpose, and has had better alternatives for a couple of years now. It's been a source of confusion numerous times, and its existence also requires extra verification logic on conversions.

`ItemKey` is finally `Copy` now too 🎉

Author: Serial-ATA

CHANGELOG.md

@@ -14,6 +14,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
 - **ID3v2**: Check `TXXX:ALBUMARTIST` and `TXXX:ALBUM ARTIST` for `ItemKey::AlbumArtist` conversions
 - **Vorbis Comments**: Check `ALBUM ARTIST` for `ItemKey::AlbumArtist` conversions
+* **ItemKey**: `ItemKey` is now `Copy` ([PR](https://github.com/Serial-ATA/lofty-rs/pull/526))
+
+### Removed
+
+* **ItemKey**: `ItemKey::Unknown` ([PR](https://github.com/Serial-ATA/lofty-rs/pull/526))
+    * `Tag` is now intended for generic metadata editing only, with format-specific items only being available through concrete tag types.
+      See <https://github.com/Serial-ATA/lofty-rs/issues/521> for the rationale.
 
 ## [0.22.4] - 2025-04-29
 

Cargo.toml

@@ -16,9 +16,9 @@ repository = "https://github.com/Serial-ATA/lofty-rs"
 license = "MIT OR Apache-2.0"
 
 [workspace.dependencies]
-lofty = { path = "lofty" }
-lofty_attr = { path = "lofty_attr" }
-ogg_pager = { path = "ogg_pager" }
+lofty = { version = "0.22.4", path = "lofty" }
+lofty_attr = { version = "0.11.1", path = "lofty_attr" }
+ogg_pager = { version = "0.7.0", path = "ogg_pager" }
 
 byteorder = "1.5.0"
 

examples/tag_reader.rs

@@ -33,7 +33,7 @@ fn main() {
 	// import keys from https://docs.rs/lofty/latest/lofty/tag/enum.ItemKey.html
 	println!(
 		"Album Artist: {}",
-		tag.get_string(&ItemKey::AlbumArtist).unwrap_or("None")
+		tag.get_string(ItemKey::AlbumArtist).unwrap_or("None")
 	);
 
 	let properties = tagged_file.properties();

lofty/Cargo.toml

@@ -19,7 +19,7 @@ byteorder     = { workspace = true }
 # ID3 compressed frames
 flate2        = { version = "1.0.30", optional = true }
 # Proc macros
-lofty_attr    = "0.11.1"
+lofty_attr    = { workspace = true }
 # Debug logging
 log           = "0.4.22"
 # OGG Vorbis/Opus

lofty/src/ape/tag/item.rs

@@ -17,6 +17,13 @@ pub struct ApeItem {
 }
 
 impl ApeItem {
+	/// Placeholder for conversions
+	pub(crate) const EMPTY: Self = ApeItem {
+		read_only: false,
+		key: String::new(),
+		value: ItemValue::Text(String::new()),
+	};
+
 	/// Create an [`ApeItem`]
 	///
 	/// # Errors
@@ -71,7 +78,7 @@ impl TryFrom<TagItem> for ApeItem {
 		Self::new(
 			value
 				.item_key
-				.map_key(TagType::Ape, false)
+				.map_key(TagType::Ape)
 				.ok_or_else(|| decode_err!(Ape, "Attempted to convert an unsupported item key"))?
 				.to_string(),
 			value.item_value,

lofty/src/ape/tag/mod.rs

@@ -421,22 +421,26 @@ impl SplitTag for ApeTag {
 
 		let mut tag = Tag::new(TagType::Ape);
 
-		for item in std::mem::take(&mut self.items) {
-			let item_key = ItemKey::from_key(TagType::Ape, item.key());
+		self.items.retain_mut(|item| {
+			let Some(item_key) = ItemKey::from_key(TagType::Ape, item.key()) else {
+				return true;
+			};
 
 			// The text pairs need some special treatment
 			match (item_key, item.value()) {
 				(ItemKey::TrackNumber | ItemKey::TrackTotal, ItemValue::Text(val))
 					if split_pair(val, &mut tag, ItemKey::TrackNumber, ItemKey::TrackTotal)
 						.is_some() =>
 				{
-					continue; // Item consumed
+					*item = ApeItem::EMPTY;
+					false // Item consumed
 				},
 				(ItemKey::DiscNumber | ItemKey::DiscTotal, ItemValue::Text(val))
 					if split_pair(val, &mut tag, ItemKey::DiscNumber, ItemKey::DiscTotal)
 						.is_some() =>
 				{
-					continue; // Item consumed
+					*item = ApeItem::EMPTY;
+					false // Item consumed
 				},
 				(ItemKey::MovementNumber | ItemKey::MovementTotal, ItemValue::Text(val))
 					if split_pair(
@@ -447,13 +451,16 @@ impl SplitTag for ApeTag {
 					)
 					.is_some() =>
 				{
-					continue; // Item consumed
+					*item = ApeItem::EMPTY;
+					false // Item consumed
 				},
 				(k, _) => {
+					let item = std::mem::replace(item, ApeItem::EMPTY);
 					tag.items.push(TagItem::new(k, item.value));
+					false // Item consumed
 				},
 			}
-		}
+		});
 
 		(SplitTagRemainder(self), tag)
 	}
@@ -541,22 +548,22 @@ pub(crate) fn tagitems_into_ape(tag: &Tag) -> impl Iterator<Item = ApeItemRef<'_
 	}
 
 	tag.items()
-		.filter(|item| !NUMBER_PAIR_KEYS.contains(item.key()))
+		.filter(|item| !NUMBER_PAIR_KEYS.contains(&item.key()))
 		.filter_map(|i| {
-			i.key().map_key(TagType::Ape, true).map(|key| ApeItemRef {
+			i.key().map_key(TagType::Ape).map(|key| ApeItemRef {
 				read_only: false,
 				key,
 				value: (&i.item_value).into(),
 			})
 		})
 		.chain(create_apeitemref_for_number_pair(
-			tag.get_string(&ItemKey::TrackNumber),
-			tag.get_string(&ItemKey::TrackTotal),
+			tag.get_string(ItemKey::TrackNumber),
+			tag.get_string(ItemKey::TrackTotal),
 			"Track",
 		))
 		.chain(create_apeitemref_for_number_pair(
-			tag.get_string(&ItemKey::DiscNumber),
-			tag.get_string(&ItemKey::DiscTotal),
+			tag.get_string(ItemKey::DiscNumber),
+			tag.get_string(ItemKey::DiscTotal),
 			"Disk",
 		))
 }

lofty/src/id3/v1/tag.rs

@@ -347,23 +347,23 @@ impl From<Id3v1Tag> for Tag {
 
 impl From<Tag> for Id3v1Tag {
 	fn from(mut input: Tag) -> Self {
-		let title = input.take_strings(&ItemKey::TrackTitle).next();
-		let artist = input.take_strings(&ItemKey::TrackArtist).next();
-		let album = input.take_strings(&ItemKey::AlbumTitle).next();
+		let title = input.take_strings(ItemKey::TrackTitle).next();
+		let artist = input.take_strings(ItemKey::TrackArtist).next();
+		let album = input.take_strings(ItemKey::AlbumTitle).next();
 		let year = input.year().map(|y| y.to_string());
-		let comment = input.take_strings(&ItemKey::Comment).next();
+		let comment = input.take_strings(ItemKey::Comment).next();
 		Self {
 			title,
 			artist,
 			album,
 			year,
 			comment,
 			track_number: input
-				.get_string(&ItemKey::TrackNumber)
+				.get_string(ItemKey::TrackNumber)
 				.map(|g| g.parse::<u8>().ok())
 				.and_then(|g| g),
 			genre: input
-				.get_string(&ItemKey::Genre)
+				.get_string(ItemKey::Genre)
 				.map(|g| {
 					GENRES
 						.iter()
@@ -402,17 +402,17 @@ impl<'a> Into<Id3v1TagRef<'a>> for &'a Id3v1Tag {
 impl<'a> Into<Id3v1TagRef<'a>> for &'a Tag {
 	fn into(self) -> Id3v1TagRef<'a> {
 		Id3v1TagRef {
-			title: self.get_string(&ItemKey::TrackTitle),
-			artist: self.get_string(&ItemKey::TrackArtist),
-			album: self.get_string(&ItemKey::AlbumTitle),
-			year: self.get_string(&ItemKey::Year),
-			comment: self.get_string(&ItemKey::Comment),
+			title: self.get_string(ItemKey::TrackTitle),
+			artist: self.get_string(ItemKey::TrackArtist),
+			album: self.get_string(ItemKey::AlbumTitle),
+			year: self.get_string(ItemKey::Year),
+			comment: self.get_string(ItemKey::Comment),
 			track_number: self
-				.get_string(&ItemKey::TrackNumber)
+				.get_string(ItemKey::TrackNumber)
 				.map(|g| g.parse::<u8>().ok())
 				.and_then(|g| g),
 			genre: self
-				.get_string(&ItemKey::Genre)
+				.get_string(ItemKey::Genre)
 				.map(|g| {
 					GENRES
 						.iter()

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

@@ -35,7 +35,7 @@ impl From<TagItem> for Option<Frame<'static>> {
 			return frame_from_unknown_item(id, input.item_value).ok();
 		}
 
-		match input.item_key.map_key(TagType::Id3v2, true) {
+		match input.item_key.map_key(TagType::Id3v2) {
 			Some(desc) => match input.item_value {
 				ItemValue::Text(text) => {
 					value = Frame::UserText(ExtendedTextFrame::new(
@@ -114,11 +114,10 @@ impl<'a> TryFrom<&'a TagItem> for FrameRef<'a> {
 			},
 			Err(_) => {
 				let item_key = tag_item.key();
-				let Some(desc) = item_key.map_key(TagType::Id3v2, true) else {
-					return Err(Id3v2Error::new(Id3v2ErrorKind::UnsupportedFrameId(
-						item_key.clone(),
-					))
-					.into());
+				let Some(desc) = item_key.map_key(TagType::Id3v2) else {
+					return Err(
+						Id3v2Error::new(Id3v2ErrorKind::UnsupportedFrameId(item_key)).into(),
+					);
 				};
 
 				match tag_item.value() {
@@ -129,10 +128,9 @@ impl<'a> TryFrom<&'a TagItem> for FrameRef<'a> {
 						value = new_user_url_frame(String::from(desc), locator.clone());
 					},
 					_ => {
-						return Err(Id3v2Error::new(Id3v2ErrorKind::UnsupportedFrameId(
-							item_key.clone(),
-						))
-						.into());
+						return Err(
+							Id3v2Error::new(Id3v2ErrorKind::UnsupportedFrameId(item_key)).into(),
+						);
 					},
 				}
 			},

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

@@ -134,25 +134,17 @@ impl<'a> Into<Cow<'a, str>> for FrameId<'a> {
 	}
 }
 
-impl<'a> TryFrom<&'a ItemKey> for FrameId<'a> {
+impl TryFrom<ItemKey> for FrameId<'_> {
 	type Error = LoftyError;
 
-	fn try_from(value: &'a ItemKey) -> std::prelude::rust_2015::Result<Self, Self::Error> {
-		match value {
-			ItemKey::Unknown(unknown) if unknown.len() == 4 => {
-				Self::verify_id(unknown)?;
-				Ok(Self::Valid(Cow::Borrowed(unknown)))
-			},
-			k => {
-				if let Some(mapped) = k.map_key(TagType::Id3v2, false) {
-					if mapped.len() == 4 {
-						Self::verify_id(mapped)?;
-						return Ok(Self::Valid(Cow::Borrowed(mapped)));
-					}
-				}
-
-				Err(Id3v2Error::new(Id3v2ErrorKind::UnsupportedFrameId(k.clone())).into())
-			},
+	fn try_from(value: ItemKey) -> std::prelude::rust_2015::Result<Self, Self::Error> {
+		if let Some(mapped) = value.map_key(TagType::Id3v2) {
+			if mapped.len() == 4 {
+				Self::verify_id(mapped)?;
+				return Ok(Self::Valid(Cow::Borrowed(mapped)));
+			}
 		}
+
+		Err(Id3v2Error::new(Id3v2ErrorKind::UnsupportedFrameId(value)).into())
 	}
 }