From 069b925e18a0a872e34801006bcd529390e8f958 Mon Sep 17 00:00:00 2001 From: Serial <69764315+Serial-ATA@users.noreply.github.com> Date: Mon, 7 Jul 2025 19:25:05 -0400 Subject: [PATCH 1/3] ItemKey: Remove `ItemKey::Unknown` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 🎉 --- CHANGELOG.md | 7 ++ Cargo.toml | 6 +- examples/tag_reader.rs | 2 +- lofty/Cargo.toml | 2 +- lofty/src/ape/tag/item.rs | 9 +- lofty/src/ape/tag/mod.rs | 31 +++-- lofty/src/id3/v1/tag.rs | 26 ++--- lofty/src/id3/v2/frame/conversion.rs | 18 ++- lofty/src/id3/v2/frame/header/mod.rs | 26 ++--- lofty/src/id3/v2/tag.rs | 166 +++++++++++++-------------- lofty/src/id3/v2/tag/tests.rs | 103 +---------------- lofty/src/iff/aiff/tag.rs | 16 +-- lofty/src/iff/wav/tag/mod.rs | 19 +-- lofty/src/mp4/atom_info.rs | 2 +- lofty/src/mp4/ilst/mod.rs | 57 +++++---- lofty/src/ogg/tag.rs | 41 +++---- lofty/src/picture.rs | 16 +-- lofty/src/tag/item.rs | 40 ++----- lofty/src/tag/mod.rs | 74 ++++++------ lofty/src/tag/split_merge_tag.rs | 2 +- lofty/src/tag/tag_ext.rs | 2 +- lofty/src/tag/utils.rs | 20 ++-- lofty/tests/files/aac.rs | 2 +- lofty/tests/files/mpeg.rs | 4 +- lofty/tests/files/util/mod.rs | 2 +- lofty/tests/taglib/util/mod.rs | 2 +- lofty_attr/src/internal.rs | 8 +- 27 files changed, 298 insertions(+), 405 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 368b62f73..7175880d1 100644 --- a/CHANGELOG.md +++ b/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 for the rationale. ## [0.22.4] - 2025-04-29 diff --git a/Cargo.toml b/Cargo.toml index 33cf0b870..a0043bbe7 100644 --- a/Cargo.toml +++ b/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" diff --git a/examples/tag_reader.rs b/examples/tag_reader.rs index dc6be8ad2..66861ed61 100644 --- a/examples/tag_reader.rs +++ b/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(); diff --git a/lofty/Cargo.toml b/lofty/Cargo.toml index 055650510..b243ffe9e 100644 --- a/lofty/Cargo.toml +++ b/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 diff --git a/lofty/src/ape/tag/item.rs b/lofty/src/ape/tag/item.rs index 3cc8118dd..aa324d7c8 100644 --- a/lofty/src/ape/tag/item.rs +++ b/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 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, diff --git a/lofty/src/ape/tag/mod.rs b/lofty/src/ape/tag/mod.rs index d1cdc0388..bf779a0af 100644 --- a/lofty/src/ape/tag/mod.rs +++ b/lofty/src/ape/tag/mod.rs @@ -421,8 +421,10 @@ 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()) { @@ -430,13 +432,15 @@ impl SplitTag for ApeTag { 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 for Tag { impl From 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, @@ -359,11 +359,11 @@ impl From for Id3v1Tag { year, comment, track_number: input - .get_string(&ItemKey::TrackNumber) + .get_string(ItemKey::TrackNumber) .map(|g| g.parse::().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> for &'a Id3v1Tag { impl<'a> Into> 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::().ok()) .and_then(|g| g), genre: self - .get_string(&ItemKey::Genre) + .get_string(ItemKey::Genre) .map(|g| { GENRES .iter() diff --git a/lofty/src/id3/v2/frame/conversion.rs b/lofty/src/id3/v2/frame/conversion.rs index 22d05f9b3..1162d7094 100644 --- a/lofty/src/id3/v2/frame/conversion.rs +++ b/lofty/src/id3/v2/frame/conversion.rs @@ -35,7 +35,7 @@ impl From for Option> { 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(), + ); }, } }, diff --git a/lofty/src/id3/v2/frame/header/mod.rs b/lofty/src/id3/v2/frame/header/mod.rs index 85dfc8433..bc60a642d 100644 --- a/lofty/src/id3/v2/frame/header/mod.rs +++ b/lofty/src/id3/v2/frame/header/mod.rs @@ -134,25 +134,17 @@ impl<'a> Into> for FrameId<'a> { } } -impl<'a> TryFrom<&'a ItemKey> for FrameId<'a> { +impl TryFrom for FrameId<'_> { type Error = LoftyError; - fn try_from(value: &'a ItemKey) -> std::prelude::rust_2015::Result { - 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 { + 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()) } } diff --git a/lofty/src/id3/v2/tag.rs b/lofty/src/id3/v2/tag.rs index b120b2396..b2b453fbf 100644 --- a/lofty/src/id3/v2/tag.rs +++ b/lofty/src/id3/v2/tag.rs @@ -17,7 +17,7 @@ use crate::id3::v2::util::pairs::{ }; use crate::id3::v2::{BinaryFrame, FrameHeader, FrameId, KeyValueFrame, TimestampFrame}; use crate::mp4::AdvisoryRating; -use crate::picture::{Picture, PictureType, TOMBSTONE_PICTURE}; +use crate::picture::{Picture, PictureType}; use crate::tag::companion_tag::CompanionTag; use crate::tag::items::{Lang, Timestamp, UNKNOWN_LANGUAGE}; use crate::tag::{Accessor, ItemKey, ItemValue, MergeTag, SplitTag, Tag, TagExt, TagItem, TagType}; @@ -1079,7 +1079,7 @@ fn handle_tag_split(tag: &mut Tag, frame: &mut Frame<'_>) -> bool { for (item_key, tipl_key) in TIPL_MAPPINGS { if key == *tipl_key { tag.items.push(TagItem::new( - item_key.clone(), + *item_key, ItemValue::Text(core::mem::take(value)), )); return false; // This key-value pair is consumed @@ -1092,20 +1092,19 @@ fn handle_tag_split(tag: &mut Tag, frame: &mut Frame<'_>) -> bool { !key_value_pairs.is_empty() // Frame is consumed if we consumed all items }, - // TODO: HACK!! We are specifically disallowing descriptions with a length of 4. - // This is due to use storing 4 character IDs as Frame::Text on tag merge. - // Maybe ItemKey could use a "TXXX:" prefix eventually, so we would store - // "TXXX:MusicBrainz Album Id" instead of "MusicBrainz Album Id". // Store TXXX/WXXX frames by their descriptions, rather than their IDs &mut Frame::UserText(ExtendedTextFrame { ref description, ref content, .. - }) if !description.is_empty() && description.len() != 4 => { - let item_key = ItemKey::from_key(TagType::Id3v2, description); + }) if !description.is_empty() => { + let Some(item_key) = ItemKey::from_key(TagType::Id3v2, description) else { + return FRAME_RETAINED; + }; + for c in content.split(V4_MULTI_VALUE_SEPARATOR) { tag.items.push(TagItem::new( - item_key.clone(), + item_key, ItemValue::Text(c.to_string()), )); } @@ -1117,10 +1116,13 @@ fn handle_tag_split(tag: &mut Tag, frame: &mut Frame<'_>) -> bool { ref content, .. }) if !description.is_empty() && description.len() != 4 => { - let item_key = ItemKey::from_key(TagType::Id3v2, description); + let Some(item_key) = ItemKey::from_key(TagType::Id3v2, description) else { + return FRAME_RETAINED; + }; + for c in content.split(V4_MULTI_VALUE_SEPARATOR) { tag.items.push(TagItem::new( - item_key.clone(), + item_key, ItemValue::Locator(c.to_string()), )); } @@ -1168,10 +1170,10 @@ fn handle_tag_split(tag: &mut Tag, frame: &mut Frame<'_>) -> bool { language, .. }) => { - let item_key = ItemKey::from_key(TagType::Id3v2, id.as_str()); + let item_key = ItemKey::from_key(TagType::Id3v2, id.as_str()).expect("both of these frames map to valid ItemKeys"); for c in content.split(V4_MULTI_VALUE_SEPARATOR) { - let mut item = TagItem::new(item_key.clone(), ItemValue::Text(c.to_string())); + let mut item = TagItem::new(item_key, ItemValue::Text(c.to_string())); item.set_lang(*language); @@ -1187,15 +1189,14 @@ fn handle_tag_split(tag: &mut Tag, frame: &mut Frame<'_>) -> bool { Frame::Picture(AttachedPictureFrame { picture, .. }) => { - tag.push_picture(std::mem::replace(picture, TOMBSTONE_PICTURE)); + tag.push_picture(std::mem::replace(picture, Picture::EMPTY)); return FRAME_CONSUMED; }, Frame::Timestamp(TimestampFrame { header: FrameHeader {id, ..} , timestamp, .. }) => { - let item_key = ItemKey::from_key(TagType::Id3v2, id.as_str()); - if matches!(item_key, ItemKey::Unknown(_)) { + let Some(item_key) = ItemKey::from_key(TagType::Id3v2, id.as_str()) else { return FRAME_RETAINED; - } + }; if timestamp.verify().is_err() { return FRAME_RETAINED; @@ -1210,21 +1211,27 @@ fn handle_tag_split(tag: &mut Tag, frame: &mut Frame<'_>) -> bool { }, Frame::Text(TextInformationFrame { header: FrameHeader {id, .. }, value: content, .. }) => { - let item_key = ItemKey::from_key(TagType::Id3v2, id.as_str()); + let Some(item_key) = ItemKey::from_key(TagType::Id3v2, id.as_str()) else { + return FRAME_RETAINED; + }; for c in content.split(V4_MULTI_VALUE_SEPARATOR) { tag.items.push(TagItem::new( - item_key.clone(), + item_key, ItemValue::Text(c.to_string()), )); } + return FRAME_CONSUMED; }, Frame::Url(UrlLinkFrame { header: FrameHeader {id, .. }, content, .. }) => { - let item_key = ItemKey::from_key(TagType::Id3v2, id.as_str()); + let Some(item_key) = ItemKey::from_key(TagType::Id3v2, id.as_str()) else { + return FRAME_RETAINED; + }; + tag.items.push(TagItem::new( item_key, ItemValue::Locator(std::mem::take(content)), @@ -1266,7 +1273,7 @@ impl MergeTag for SplitTagRemainder { fn merge_tag(self, mut tag: Tag) -> Id3v2Tag { fn join_text_items<'a>( tag: &mut Tag, - keys: impl IntoIterator, + keys: impl IntoIterator, ) -> Option { let mut concatenated: Option = None; for key in keys { @@ -1305,10 +1312,10 @@ impl MergeTag for SplitTagRemainder { content: String, } - fn join_language_items<'a>( - tag: &'a mut Tag, - key: &ItemKey, - ) -> Option + use<'a>> { + fn join_language_items( + tag: &mut Tag, + key: ItemKey, + ) -> Option + use<'_>> { let mut items = tag.take(key).collect::>(); if items.is_empty() { return None; @@ -1369,36 +1376,34 @@ impl MergeTag for SplitTagRemainder { // Multi-valued text key-to-frame mappings // TODO: Extend this list of item keys as needed or desired for item_key in [ - &ItemKey::TrackArtist, - &ItemKey::AlbumArtist, - &ItemKey::TrackTitle, - &ItemKey::AlbumTitle, - &ItemKey::SetSubtitle, - &ItemKey::TrackSubtitle, - &ItemKey::OriginalAlbumTitle, - &ItemKey::OriginalArtist, - &ItemKey::OriginalLyricist, - &ItemKey::ContentGroup, - &ItemKey::AppleId3v2ContentGroup, - &ItemKey::Genre, - &ItemKey::Mood, - &ItemKey::Composer, - &ItemKey::Conductor, - &ItemKey::Writer, - &ItemKey::Lyricist, - &ItemKey::MusicianCredits, - &ItemKey::InternetRadioStationName, - &ItemKey::InternetRadioStationOwner, - &ItemKey::Remixer, - &ItemKey::Work, - &ItemKey::Movement, - &ItemKey::FileOwner, - &ItemKey::CopyrightMessage, - &ItemKey::Language, + ItemKey::TrackArtist, + ItemKey::AlbumArtist, + ItemKey::TrackTitle, + ItemKey::AlbumTitle, + ItemKey::SetSubtitle, + ItemKey::TrackSubtitle, + ItemKey::OriginalAlbumTitle, + ItemKey::OriginalArtist, + ItemKey::OriginalLyricist, + ItemKey::ContentGroup, + ItemKey::AppleId3v2ContentGroup, + ItemKey::Genre, + ItemKey::Mood, + ItemKey::Composer, + ItemKey::Conductor, + ItemKey::Writer, + ItemKey::Lyricist, + ItemKey::MusicianCredits, + ItemKey::InternetRadioStationName, + ItemKey::InternetRadioStationOwner, + ItemKey::Remixer, + ItemKey::Work, + ItemKey::Movement, + ItemKey::FileOwner, + ItemKey::CopyrightMessage, + ItemKey::Language, ] { - let frame_id = item_key - .map_key(TagType::Id3v2, false) - .expect("valid frame id"); + let frame_id = item_key.map_key(TagType::Id3v2).expect("valid frame id"); if let Some(text) = join_text_items(&mut tag, [item_key]) { let frame = new_text_frame(FrameId::Valid(Cow::Borrowed(frame_id)), text); // Optimization: No duplicate checking according to the preconditions @@ -1409,16 +1414,14 @@ impl MergeTag for SplitTagRemainder { // Multi-valued TXXX key-to-frame mappings for item_key in [ - &ItemKey::TrackArtists, - &ItemKey::Director, - &ItemKey::CatalogNumber, - &ItemKey::MusicBrainzArtistId, - &ItemKey::MusicBrainzReleaseArtistId, - &ItemKey::MusicBrainzWorkId, + ItemKey::TrackArtists, + ItemKey::Director, + ItemKey::CatalogNumber, + ItemKey::MusicBrainzArtistId, + ItemKey::MusicBrainzReleaseArtistId, + ItemKey::MusicBrainzWorkId, ] { - let frame_id = item_key - .map_key(TagType::Id3v2, false) - .expect("valid frame id"); + let frame_id = item_key.map_key(TagType::Id3v2).expect("valid frame id"); if let Some(text) = join_text_items(&mut tag, [item_key]) { let frame = new_user_text_frame(String::from(frame_id), text); // Optimization: No duplicate checking according to the preconditions @@ -1430,13 +1433,10 @@ impl MergeTag for SplitTagRemainder { // Multi-valued Label/Publisher key-to-frame mapping { let frame_id = ItemKey::Label - .map_key(TagType::Id3v2, false) + .map_key(TagType::Id3v2) .expect("valid frame id"); - debug_assert_eq!( - Some(frame_id), - ItemKey::Publisher.map_key(TagType::Id3v2, false) - ); - if let Some(text) = join_text_items(&mut tag, &[ItemKey::Label, ItemKey::Publisher]) { + debug_assert_eq!(Some(frame_id), ItemKey::Publisher.map_key(TagType::Id3v2)); + if let Some(text) = join_text_items(&mut tag, [ItemKey::Label, ItemKey::Publisher]) { let frame = new_text_frame(FrameId::Valid(Cow::Borrowed(frame_id)), text); // Optimization: No duplicate checking according to the preconditions debug_assert!(!merged.frames.contains(&frame)); @@ -1446,7 +1446,7 @@ impl MergeTag for SplitTagRemainder { // Comment/Unsync text for key in [ItemKey::Comment, ItemKey::Lyrics] { - let Some(items) = join_language_items(&mut tag, &key) else { + let Some(items) = join_language_items(&mut tag, key) else { continue; }; @@ -1481,7 +1481,7 @@ impl MergeTag for SplitTagRemainder { 'tipl: { let mut key_value_pairs = Vec::new(); for (item_key, tipl_key) in TIPL_MAPPINGS { - for value in tag.take_strings(item_key) { + for value in tag.take_strings(*item_key) { key_value_pairs.push(((*tipl_key).to_string(), value)); } } @@ -1513,7 +1513,7 @@ impl MergeTag for SplitTagRemainder { } // Flag items - for item_key in [&ItemKey::FlagCompilation, &ItemKey::FlagPodcast] { + for item_key in [ItemKey::FlagCompilation, ItemKey::FlagPodcast] { let Some(text) = tag.take_strings(item_key).next() else { continue; }; @@ -1522,9 +1522,7 @@ impl MergeTag for SplitTagRemainder { continue; }; - let frame_id = item_key - .map_key(TagType::Id3v2, false) - .expect("valid frame id"); + let frame_id = item_key.map_key(TagType::Id3v2).expect("valid frame id"); merged.frames.push(new_text_frame( FrameId::Valid(Cow::Borrowed(frame_id)), @@ -1534,7 +1532,7 @@ impl MergeTag for SplitTagRemainder { // iTunes advisory rating 'rate: { - if let Some(advisory_rating) = tag.take_strings(&ItemKey::ParentalAdvisory).next() { + if let Some(advisory_rating) = tag.take_strings(ItemKey::ParentalAdvisory).next() { let Ok(rating) = advisory_rating.parse::() else { log::warn!( "Parental advisory rating is not a number: {advisory_rating}, discarding" @@ -1555,14 +1553,12 @@ impl MergeTag for SplitTagRemainder { } // Timestamps - for item_key in [&ItemKey::RecordingDate, &ItemKey::OriginalReleaseDate] { + for item_key in [ItemKey::RecordingDate, ItemKey::OriginalReleaseDate] { let Some(text) = tag.take_strings(item_key).next() else { continue; }; - let frame_id = item_key - .map_key(TagType::Id3v2, false) - .expect("valid frame id"); + let frame_id = item_key.map_key(TagType::Id3v2).expect("valid frame id"); let frame; match Timestamp::from_str(&text) { @@ -1677,17 +1673,17 @@ pub(crate) fn tag_frames(tag: &Tag) -> impl Iterator> { let items = tag .items() - .filter(|item| !NUMBER_PAIR_KEYS.contains(item.key())) + .filter(|item| !NUMBER_PAIR_KEYS.contains(&item.key())) .map(TryInto::>::try_into) .filter_map(Result::ok) .chain(create_frameref_for_number_pair( - tag.get_string(&ItemKey::TrackNumber), - tag.get_string(&ItemKey::TrackTotal), + tag.get_string(ItemKey::TrackNumber), + tag.get_string(ItemKey::TrackTotal), "TRCK", )) .chain(create_frameref_for_number_pair( - tag.get_string(&ItemKey::DiscNumber), - tag.get_string(&ItemKey::DiscTotal), + tag.get_string(ItemKey::DiscNumber), + tag.get_string(ItemKey::DiscTotal), "TPOS", )) .chain(create_framerefs_for_companion_tag( diff --git a/lofty/src/id3/v2/tag/tests.rs b/lofty/src/id3/v2/tag/tests.rs index 849bb8acf..97f461e9a 100644 --- a/lofty/src/id3/v2/tag/tests.rs +++ b/lofty/src/id3/v2/tag/tests.rs @@ -368,7 +368,7 @@ fn multi_value_frame_to_tag() { tag.set_artist(String::from("foo\0bar\0baz")); let tag: Tag = tag.into(); - let collected_artists = tag.get_strings(&ItemKey::TrackArtist).collect::>(); + let collected_artists = tag.get_strings(ItemKey::TrackArtist).collect::>(); assert_eq!(&collected_artists, &["foo", "bar", "baz"]) } @@ -527,99 +527,6 @@ fn comments() { ); } -#[test_log::test] -fn txxx_wxxx_tag_conversion() { - let txxx_frame = Frame::UserText(ExtendedTextFrame::new( - TextEncoding::UTF8, - String::from("FOO_TEXT_FRAME"), - String::from("foo content"), - )); - - let wxxx_frame = Frame::UserUrl(ExtendedUrlFrame::new( - TextEncoding::UTF8, - String::from("BAR_URL_FRAME"), - String::from("bar url"), - )); - - let mut tag = Id3v2Tag::default(); - - tag.insert(txxx_frame.clone()); - tag.insert(wxxx_frame.clone()); - - let tag: Tag = tag.into(); - - assert_eq!(tag.item_count(), 2); - let expected_items = [ - TagItem::new( - ItemKey::Unknown(String::from("FOO_TEXT_FRAME")), - ItemValue::Text(String::from("foo content")), - ), - TagItem::new( - ItemKey::Unknown(String::from("BAR_URL_FRAME")), - ItemValue::Locator(String::from("bar url")), - ), - ]; - assert!( - expected_items - .iter() - .zip(tag.items()) - .all(|(expected, actual)| expected == actual) - ); - - let tag: Id3v2Tag = tag.into(); - - assert_eq!(tag.frames.len(), 2); - assert_eq!(&tag.frames, &[txxx_frame, wxxx_frame]) -} - -#[test_log::test] -fn user_defined_frames_conversion() { - let mut id3v2 = Id3v2Tag::default(); - id3v2.insert(Frame::UserText(ExtendedTextFrame::new( - TextEncoding::UTF8, - String::from("FOO_BAR"), - String::from("foo content"), - ))); - - let (split_remainder, split_tag) = id3v2.split_tag(); - assert_eq!(split_remainder.0.len(), 0); - assert_eq!(split_tag.len(), 1); - - let id3v2 = split_remainder.merge_tag(split_tag); - - // Verify we properly convert user defined frames between Tag <-> ID3v2Tag round trips - assert_eq!( - id3v2.frames.first(), - Some(&Frame::UserText(ExtendedTextFrame::new( - TextEncoding::UTF8, // Not considered by PartialEq! - String::from("FOO_BAR"), - String::new(), // Not considered by PartialEq! - ),)) - ); - - // Verify we properly convert user defined frames when writing a Tag, which has to convert - // to the reference types. - let (_remainder, tag) = id3v2.clone().split_tag(); - assert_eq!(tag.len(), 1); - - let mut content = Vec::new(); - tag.dump_to(&mut content, WriteOptions::default()).unwrap(); - assert!(!content.is_empty()); - - // And verify we can reread the tag - let mut reader = std::io::Cursor::new(&content[..]); - - let header = Id3v2Header::parse(&mut reader).unwrap(); - let reparsed = crate::id3::v2::read::parse_id3v2( - &mut reader, - header, - ParseOptions::new().parsing_mode(ParsingMode::Strict), - ) - .unwrap(); - - assert_eq!(id3v2, reparsed); -} - #[test_log::test] fn set_track() { let mut id3v2 = Id3v2Tag::default(); @@ -910,7 +817,7 @@ fn ufid_frame_with_musicbrainz_record_id() { assert_eq!( ItemValue::Text(String::from_utf8(musicbrainz_recording_id.to_vec()).unwrap()), *split_tag - .get_items(&ItemKey::MusicBrainzRecordingId) + .get_items(ItemKey::MusicBrainzRecordingId) .next() .unwrap() .value() @@ -1086,7 +993,7 @@ fn genres_id_multiple() { fn genres_id_multiple_into_tag() { let id3v2 = id3v2_tag_with_genre("(51)(39)"); let tag: Tag = id3v2.into(); - let mut genres = tag.get_strings(&ItemKey::Genre); + let mut genres = tag.get_strings(ItemKey::Genre); assert_eq!(genres.next(), Some("Techno-Industrial")); assert_eq!(genres.next(), Some("Noise")); assert_eq!(genres.next(), None); @@ -1159,7 +1066,7 @@ fn tipl_round_trip() { for (item_key, _) in TIPL_MAPPINGS { assert_eq!( split_tag - .get(item_key) + .get(*item_key) .map(TagItem::value) .and_then(ItemValue::text), Some("Serial-ATA") @@ -1237,7 +1144,7 @@ fn timestamp_roundtrip() { let tag: Tag = tag.into(); assert_eq!(tag.len(), 1); assert_eq!( - tag.get_string(&ItemKey::RecordingDate), + tag.get_string(ItemKey::RecordingDate), Some("2024-06-03T14:08:49") ); diff --git a/lofty/src/iff/aiff/tag.rs b/lofty/src/iff/aiff/tag.rs index 560d24fc7..8b0c17f1f 100644 --- a/lofty/src/iff/aiff/tag.rs +++ b/lofty/src/iff/aiff/tag.rs @@ -286,10 +286,10 @@ impl From for Tag { impl From for AiffTextChunks { fn from(mut input: Tag) -> Self { - let name = input.take_strings(&ItemKey::TrackTitle).next(); - let author = input.take_strings(&ItemKey::TrackArtist).next(); - let copyright = input.take_strings(&ItemKey::CopyrightMessage).next(); - let annotations = input.take_strings(&ItemKey::Comment).collect::>(); + let name = input.take_strings(ItemKey::TrackTitle).next(); + let author = input.take_strings(ItemKey::TrackArtist).next(); + let copyright = input.take_strings(ItemKey::CopyrightMessage).next(); + let annotations = input.take_strings(ItemKey::Comment).collect::>(); Self { name, @@ -582,14 +582,14 @@ mod tests { let tag: Tag = aiff_text.into(); - 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::TrackTitle), Some("Foo title")); + assert_eq!(tag.get_string(ItemKey::TrackArtist), Some("Bar artist")); assert_eq!( - tag.get_string(&ItemKey::CopyrightMessage), + tag.get_string(ItemKey::CopyrightMessage), Some("Baz copyright") ); - let mut comments = tag.get_strings(&ItemKey::Comment); + let mut comments = tag.get_strings(ItemKey::Comment); assert_eq!(comments.next(), Some("Qux annotation")); assert_eq!(comments.next(), Some("Quux annotation")); assert_eq!(comments.next(), Some("Quuz comment")); diff --git a/lofty/src/iff/wav/tag/mod.rs b/lofty/src/iff/wav/tag/mod.rs index 18af89778..ad9ecf537 100644 --- a/lofty/src/iff/wav/tag/mod.rs +++ b/lofty/src/iff/wav/tag/mod.rs @@ -260,7 +260,9 @@ impl From for Tag { let mut tag = Self::new(TagType::RiffInfo); for (k, v) in input.items { - let item_key = ItemKey::from_key(TagType::RiffInfo, &k); + let Some(item_key) = ItemKey::from_key(TagType::RiffInfo, &k) else { + continue; + }; tag.items.push(TagItem::new( item_key, @@ -278,17 +280,8 @@ impl From for RiffInfoList { for item in input.items { if let ItemValue::Text(val) | ItemValue::Locator(val) = item.item_value { - match item.item_key { - ItemKey::Unknown(unknown) => { - if read::verify_key(&unknown) { - riff_info.items.push((unknown, val)) - } - }, - k => { - if let Some(key) = k.map_key(TagType::RiffInfo, false) { - riff_info.items.push((key.to_string(), val)) - } - }, + if let Some(key) = item.item_key.map_key(TagType::RiffInfo) { + riff_info.items.push((key.to_string(), val)) } } } @@ -339,7 +332,7 @@ pub(crate) fn tagitems_into_riff<'a>( items: impl IntoIterator, ) -> impl Iterator { items.into_iter().filter_map(|i| { - let item_key = i.key().map_key(TagType::RiffInfo, true); + let item_key = i.key().map_key(TagType::RiffInfo); match (item_key, i.value()) { (Some(key), ItemValue::Text(val) | ItemValue::Locator(val)) diff --git a/lofty/src/mp4/atom_info.rs b/lofty/src/mp4/atom_info.rs index 899a1d8cb..c7b3a1004 100644 --- a/lofty/src/mp4/atom_info.rs +++ b/lofty/src/mp4/atom_info.rs @@ -67,7 +67,7 @@ impl<'a> TryFrom<&'a ItemKey> for AtomIdent<'a> { type Error = LoftyError; fn try_from(value: &'a ItemKey) -> std::result::Result { - if let Some(mapped_key) = value.map_key(TagType::Mp4Ilst, true) { + if let Some(mapped_key) = value.map_key(TagType::Mp4Ilst) { if mapped_key.starts_with("----") { let mut split = mapped_key.split(':'); diff --git a/lofty/src/mp4/ilst/mod.rs b/lofty/src/mp4/ilst/mod.rs index d4da91665..80ab5d809 100644 --- a/lofty/src/mp4/ilst/mod.rs +++ b/lofty/src/mp4/ilst/mod.rs @@ -10,7 +10,7 @@ use super::AtomIdent; use crate::config::{WriteOptions, global_options}; use crate::error::LoftyError; use crate::mp4::ilst::atom::AtomDataStorage; -use crate::picture::{Picture, PictureType, TOMBSTONE_PICTURE}; +use crate::picture::{Picture, PictureType}; use crate::tag::companion_tag::CompanionTag; use crate::tag::{ Accessor, ItemKey, ItemValue, MergeTag, SplitTag, Tag, TagExt, TagItem, TagType, try_parse_year, @@ -670,19 +670,40 @@ impl SplitTag for Ilst { self.atoms.retain_mut(|atom| { let Atom { ident, data } = atom; - let value = match data.first_mut() { - AtomData::UTF8(text) | AtomData::UTF16(text) => { - ItemValue::Text(std::mem::take(text)) + + let tag_item; + match data.first_mut() { + data @ (AtomData::UTF8(_) | AtomData::UTF16(_) | AtomData::Bool(_)) => { + let Some(key) = ItemKey::from_key( + TagType::Mp4Ilst, + &match ident { + AtomIdent::Fourcc(fourcc) => { + fourcc.iter().map(|b| *b as char).collect::() + }, + AtomIdent::Freeform { mean, name } => { + format!("----:{mean}:{name}") + }, + }, + ) else { + return true; // Keep atom + }; + + match data { + AtomData::UTF8(text) | AtomData::UTF16(text) => { + tag_item = TagItem::new(key, ItemValue::Text(std::mem::take(text))); + }, + AtomData::Bool(b) => { + let text = if *b { "1".to_owned() } else { "0".to_owned() }; + tag_item = TagItem::new(key, ItemValue::Text(text)); + }, + _ => unreachable!(), + } }, AtomData::Picture(picture) => { tag.pictures - .push(std::mem::replace(picture, TOMBSTONE_PICTURE)); + .push(std::mem::replace(picture, Picture::EMPTY)); return false; // Atom consumed }, - AtomData::Bool(b) => { - let text = if *b { "1".to_owned() } else { "0".to_owned() }; - ItemValue::Text(text) - }, // We have to special case track/disc numbers since they are stored together AtomData::Unknown { code: DataType::Reserved, @@ -725,19 +746,7 @@ impl SplitTag for Ilst { }, }; - let key = ItemKey::from_key( - TagType::Mp4Ilst, - &match ident { - AtomIdent::Fourcc(fourcc) => { - fourcc.iter().map(|b| *b as char).collect::() - }, - AtomIdent::Freeform { mean, name } => { - format!("----:{mean}:{name}") - }, - }, - ); - - tag.items.push(TagItem::new(key, value)); + tag.items.push(tag_item); false // Atom consumed }); @@ -1041,8 +1050,8 @@ mod tests { crate::tag::utils::test_utils::verify_tag(&tag, true, true); - assert_eq!(tag.get_string(&ItemKey::DiscNumber), Some("1")); - assert_eq!(tag.get_string(&ItemKey::DiscTotal), Some("2")); + assert_eq!(tag.get_string(ItemKey::DiscNumber), Some("1")); + assert_eq!(tag.get_string(ItemKey::DiscTotal), Some("2")); } #[test_log::test] diff --git a/lofty/src/ogg/tag.rs b/lofty/src/ogg/tag.rs index b5bf23f27..b5660b20a 100644 --- a/lofty/src/ogg/tag.rs +++ b/lofty/src/ogg/tag.rs @@ -529,18 +529,22 @@ impl SplitTag for VorbisComments { fn split_tag(mut self) -> (Self::Remainder, Tag) { let mut tag = Tag::new(TagType::VorbisComments); - for (k, v) in std::mem::take(&mut self.items) { - tag.items.push(TagItem::new( - ItemKey::from_key(TagType::VorbisComments, &k), - ItemValue::Text(v), - )); - } + self.items.retain_mut(|(k, v)| { + let Some(key) = ItemKey::from_key(TagType::VorbisComments, k) else { + return true; + }; + + let v = std::mem::take(v); + tag.items.push(TagItem::new(key, ItemValue::Text(v))); + + false // Item consumed + }); // We need to preserve the vendor string if !tag .items .iter() - .any(|i| i.key() == &ItemKey::EncoderSoftware) + .any(|i| i.key() == ItemKey::EncoderSoftware) { tag.items.push(TagItem::new( ItemKey::EncoderSoftware, @@ -566,7 +570,7 @@ impl MergeTag for SplitTagRemainder { if let Some(TagItem { item_value: ItemValue::Text(val), .. - }) = tag.take(&ItemKey::EncoderSoftware).next() + }) = tag.take(ItemKey::EncoderSoftware).next() { merged.vendor = val; } @@ -590,18 +594,9 @@ impl MergeTag for SplitTagRemainder { } let key; - match item_key { - ItemKey::Unknown(unknown) => { - if !verify_key(&unknown) { - continue; // Bad key, discard the item - } - - key = unknown - }, - _ => match item_key.map_key(TagType::VorbisComments, false) { - Some(mapped_key) => key = mapped_key.to_string(), - None => continue, // No mapping exists, discard the item - }, + match item_key.map_key(TagType::VorbisComments) { + Some(mapped_key) => key = mapped_key.to_string(), + None => continue, // No mapping exists, discard the item } merged.items.push((key, val)); @@ -689,12 +684,12 @@ pub(crate) fn create_vorbis_comments_ref( impl Iterator, impl Iterator, ) { - let vendor = tag.get_string(&ItemKey::EncoderSoftware).unwrap_or(""); + let vendor = tag.get_string(ItemKey::EncoderSoftware).unwrap_or(""); let items = tag.items.iter().filter_map(|i| match i.value() { ItemValue::Text(val) | ItemValue::Locator(val) => i .key() - .map_key(TagType::VorbisComments, true) + .map_key(TagType::VorbisComments) .map(|key| (key, val.as_str())), _ => None, }); @@ -894,7 +889,7 @@ mod tests { ..Default::default() }; let mut tag = Tag::from(vorbis_comments); - assert_eq!(Some("Cmaj"), tag.get_string(&ItemKey::InitialKey)); + assert_eq!(Some("Cmaj"), tag.get_string(ItemKey::InitialKey)); tag.insert_text(ItemKey::InitialKey, "Cmin".to_owned()); vorbis_comments = tag.into(); assert_eq!(Some("Cmin"), vorbis_comments.get("INITIALKEY")); diff --git a/lofty/src/picture.rs b/lofty/src/picture.rs index 231d3b6c5..faf1c7ba4 100644 --- a/lofty/src/picture.rs +++ b/lofty/src/picture.rs @@ -483,6 +483,14 @@ impl Debug for Picture { } impl Picture { + /// Placeholder for conversions + pub(crate) const EMPTY: Self = Picture { + pic_type: PictureType::Other, + mime_type: None, + description: None, + data: Cow::Owned(Vec::new()), + }; + /// Create a [`Picture`] from a reader /// /// NOTES: @@ -822,11 +830,3 @@ impl Picture { } } } - -// A placeholder that is needed during conversions. -pub(crate) const TOMBSTONE_PICTURE: Picture = Picture { - pic_type: PictureType::Other, - mime_type: None, - description: None, - data: Cow::Owned(Vec::new()), -}; diff --git a/lofty/src/tag/item.rs b/lofty/src/tag/item.rs index 84d8ef95f..ad7afd501 100644 --- a/lofty/src/tag/item.rs +++ b/lofty/src/tag/item.rs @@ -48,7 +48,7 @@ macro_rules! gen_map { }).iter().find(|(k, _)| k.eq_ignore_ascii_case(key)).map(|(_, v)| v.clone()) } - pub(crate) fn get_key(&self, item_key: &ItemKey) -> Option<&str> { + pub(crate) fn get_key(&self, item_key: ItemKey) -> Option<&'static str> { match item_key { $( ItemKey::$variant => Some(first_key!($($key)|*)), @@ -432,7 +432,7 @@ macro_rules! gen_item_keys { $(,)? ] ) => { - #[derive(PartialEq, Clone, Debug, Eq, Hash)] + #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] #[allow(missing_docs)] #[non_exhaustive] /// A generic representation of a tag's key @@ -441,31 +441,24 @@ macro_rules! gen_item_keys { $(#[$variant_meta])* $variant_ident, )+ - /// When a key couldn't be mapped to another variant - /// - /// This **will not** allow writing keys that are out of spec (Eg. ID3v2.4 frame IDs **must** be 4 characters) - Unknown(String), } impl ItemKey { - /// Map a format specific key to an `ItemKey` + /// Map a format specific key to an `ItemKey`, if a variant exists /// /// NOTE: If used with ID3v2, this will only check against the ID3v2.4 keys. /// If you wish to use a V2 or V3 key, see [`upgrade_v2`](crate::id3::v2::upgrade_v2) and [`upgrade_v3`](crate::id3::v2::upgrade_v3) - pub fn from_key(tag_type: TagType, key: &str) -> Self { + pub fn from_key(tag_type: TagType, key: &str) -> Option { match tag_type { $( $(#[$feat])? - $tag_type => $MAP.get_item_key(key).unwrap_or_else(|| Self::Unknown(key.to_string())), + $tag_type => $MAP.get_item_key(key), )+ - _ => Self::Unknown(key.to_string()) + _ => None } } /// Maps the variant to a format-specific key - /// - /// Use `allow_unknown` to include [`ItemKey::Unknown`]. It is up to the caller - /// to determine if the unknown key actually fits the format's specifications. - pub fn map_key(&self, tag_type: TagType, allow_unknown: bool) -> Option<&str> { + pub fn map_key(self, tag_type: TagType) -> Option<&'static str> { match tag_type { $( $(#[$feat])? @@ -476,12 +469,6 @@ macro_rules! gen_item_keys { _ => {} } - if let ItemKey::Unknown(unknown) = self { - if allow_unknown { - return Some(unknown) - } - } - None } } @@ -834,7 +821,7 @@ impl TagItem { item_value: ItemValue, ) -> Option { item_key - .map_key(tag_type, false) + .map_key(tag_type) .is_some() .then_some(Self::new(item_key, item_value)) } @@ -882,13 +869,8 @@ impl TagItem { &self.description } - /// Returns a reference to the [`ItemKey`] - pub fn key(&self) -> &ItemKey { - &self.item_key - } - - /// Consumes the `TagItem`, returning its [`ItemKey`] - pub fn into_key(self) -> ItemKey { + /// Returns the [`ItemKey`] + pub fn key(&self) -> ItemKey { self.item_key } @@ -914,6 +896,6 @@ impl TagItem { return VALID_ITEMKEYS.contains(&self.item_key); } - self.item_key.map_key(tag_type, false).is_some() + self.item_key.map_key(tag_type).is_some() } } diff --git a/lofty/src/tag/mod.rs b/lofty/src/tag/mod.rs index b8f25d768..af35e112d 100644 --- a/lofty/src/tag/mod.rs +++ b/lofty/src/tag/mod.rs @@ -32,7 +32,7 @@ macro_rules! impl_accessor { paste::paste! { $( fn $name(&self) -> Option> { - if let Some(ItemValue::Text(txt)) = self.get(&ItemKey::$item_key).map(TagItem::value) { + if let Some(ItemValue::Text(txt)) = self.get(ItemKey::$item_key).map(TagItem::value) { return Some(Cow::Borrowed(txt)) } @@ -91,8 +91,8 @@ macro_rules! impl_accessor { /// // If the type of an item is known, there are getter methods /// // to prevent having to match against the value /// -/// tag.get_string(&ItemKey::TrackTitle); -/// tag.get_binary(&ItemKey::TrackTitle, false); +/// tag.get_string(ItemKey::TrackTitle); +/// tag.get_binary(ItemKey::TrackTitle, false); /// ``` /// /// Converting between formats @@ -139,7 +139,7 @@ impl Accessor for Tag { ); fn track(&self) -> Option { - self.get_u32_from_string(&ItemKey::TrackNumber) + self.get_u32_from_string(ItemKey::TrackNumber) } fn set_track(&mut self, value: u32) { @@ -147,11 +147,11 @@ impl Accessor for Tag { } fn remove_track(&mut self) { - self.remove_key(&ItemKey::TrackNumber); + self.remove_key(ItemKey::TrackNumber); } fn track_total(&self) -> Option { - self.get_u32_from_string(&ItemKey::TrackTotal) + self.get_u32_from_string(ItemKey::TrackTotal) } fn set_track_total(&mut self, value: u32) { @@ -159,11 +159,11 @@ impl Accessor for Tag { } fn remove_track_total(&mut self) { - self.remove_key(&ItemKey::TrackTotal); + self.remove_key(ItemKey::TrackTotal); } fn disk(&self) -> Option { - self.get_u32_from_string(&ItemKey::DiscNumber) + self.get_u32_from_string(ItemKey::DiscNumber) } fn set_disk(&mut self, value: u32) { @@ -171,11 +171,11 @@ impl Accessor for Tag { } fn remove_disk(&mut self) { - self.remove_key(&ItemKey::DiscNumber); + self.remove_key(ItemKey::DiscNumber); } fn disk_total(&self) -> Option { - self.get_u32_from_string(&ItemKey::DiscTotal) + self.get_u32_from_string(ItemKey::DiscTotal) } fn set_disk_total(&mut self, value: u32) { @@ -183,13 +183,13 @@ impl Accessor for Tag { } fn remove_disk_total(&mut self) { - self.remove_key(&ItemKey::DiscTotal); + self.remove_key(ItemKey::DiscTotal); } fn year(&self) -> Option { if let Some(item) = self - .get_string(&ItemKey::Year) - .map_or_else(|| self.get_string(&ItemKey::RecordingDate), Some) + .get_string(ItemKey::Year) + .map_or_else(|| self.get_string(ItemKey::RecordingDate), Some) { return try_parse_year(item); } @@ -198,7 +198,7 @@ impl Accessor for Tag { } fn set_year(&mut self, value: u32) { - if let Some(item) = self.get_string(&ItemKey::RecordingDate) { + if let Some(item) = self.get_string(ItemKey::RecordingDate) { if item.len() >= 4 { let (_, remaining) = item.split_at(4); self.insert_text(ItemKey::RecordingDate, format!("{value}{remaining}")); @@ -208,7 +208,7 @@ impl Accessor for Tag { // Some formats have a dedicated item for `Year`, others just have it as // a part of `RecordingDate` - if ItemKey::Year.map_key(self.tag_type, false).is_some() { + if ItemKey::Year.map_key(self.tag_type).is_some() { self.insert_text(ItemKey::Year, value.to_string()); } else { self.insert_text(ItemKey::RecordingDate, value.to_string()); @@ -216,8 +216,8 @@ impl Accessor for Tag { } fn remove_year(&mut self) { - self.remove_key(&ItemKey::Year); - self.remove_key(&ItemKey::RecordingDate); + self.remove_key(ItemKey::Year); + self.remove_key(ItemKey::RecordingDate); } } @@ -308,12 +308,12 @@ impl Tag { } /// Returns a reference to a [`TagItem`] matching an [`ItemKey`] - pub fn get(&self, item_key: &ItemKey) -> Option<&TagItem> { - self.items.iter().find(|i| &i.item_key == item_key) + pub fn get(&self, item_key: ItemKey) -> Option<&TagItem> { + self.items.iter().find(|i| i.item_key == item_key) } /// Get a string value from an [`ItemKey`] - pub fn get_string(&self, item_key: &ItemKey) -> Option<&str> { + pub fn get_string(&self, item_key: ItemKey) -> Option<&str> { if let Some(ItemValue::Text(ret)) = self.get(item_key).map(TagItem::value) { return Some(ret); } @@ -321,7 +321,7 @@ impl Tag { None } - fn get_u32_from_string(&self, key: &ItemKey) -> Option { + fn get_u32_from_string(&self, key: ItemKey) -> Option { let i = self.get_string(key)?; i.parse::().ok() } @@ -329,7 +329,7 @@ impl Tag { /// Gets a byte slice from an [`ItemKey`] /// /// Use `convert` to convert [`ItemValue::Text`] and [`ItemValue::Locator`] to byte slices - pub fn get_binary(&self, item_key: &ItemKey, convert: bool) -> Option<&[u8]> { + pub fn get_binary(&self, item_key: ItemKey, convert: bool) -> Option<&[u8]> { if let Some(item) = self.get(item_key) { match item.value() { ItemValue::Text(text) | ItemValue::Locator(text) if convert => { @@ -406,7 +406,7 @@ impl Tag { /// Removes all items with the specified [`ItemKey`], and returns them /// /// See also: [take_filter()](Self::take_filter) - pub fn take<'a>(&'a mut self, key: &ItemKey) -> impl Iterator + use<'a> { + pub fn take(&mut self, key: ItemKey) -> impl Iterator + use<'_> { self.take_filter(key, |_| true) } @@ -432,23 +432,23 @@ impl Tag { /// ); /// item.set_description("description".to_owned()); /// tag.push(item); - /// assert_eq!(tag.get_strings(&ItemKey::Comment).count(), 2); + /// assert_eq!(tag.get_strings(ItemKey::Comment).count(), 2); /// /// // Extract all comment items with an empty description. /// let comments = tag - /// .take_filter(&ItemKey::Comment, |item| item.description().is_empty()) + /// .take_filter(ItemKey::Comment, |item| item.description().is_empty()) /// .filter_map(|item| item.into_value().into_string()) /// .collect::>(); /// assert_eq!(comments, vec!["comment without description".to_owned()]); /// /// // The comments that didn't match the filter are still present. - /// assert_eq!(tag.get_strings(&ItemKey::Comment).count(), 1); + /// assert_eq!(tag.get_strings(ItemKey::Comment).count(), 1); /// ``` - pub fn take_filter<'a, F>( - &'a mut self, - key: &ItemKey, + pub fn take_filter( + &mut self, + key: ItemKey, mut filter: F, - ) -> impl Iterator + use<'a, F> + ) -> impl Iterator + use<'_, F> where F: FnMut(&TagItem) -> bool, { @@ -467,17 +467,17 @@ impl Tag { } /// Removes all items with the specified [`ItemKey`], and filters them through [`ItemValue::into_string`] - pub fn take_strings<'a>(&'a mut self, key: &ItemKey) -> impl Iterator + use<'a> { + pub fn take_strings<'a>(&'a mut self, key: ItemKey) -> impl Iterator + use<'a> { self.take(key).filter_map(|i| i.item_value.into_string()) } /// Returns references to all [`TagItem`]s with the specified key - pub fn get_items<'a>(&'a self, key: &'a ItemKey) -> impl Iterator + Clone { + pub fn get_items(&self, key: ItemKey) -> impl Iterator + Clone { self.items.iter().filter(move |i| i.key() == key) } /// Returns references to all texts of [`TagItem`]s with the specified key, and [`ItemValue::Text`] - pub fn get_strings<'a>(&'a self, key: &'a ItemKey) -> impl Iterator + Clone { + pub fn get_strings(&self, key: ItemKey) -> impl Iterator + Clone { self.items.iter().filter_map(move |i| { if i.key() == key { i.value().text() @@ -488,7 +488,7 @@ impl Tag { } /// Returns references to all locators of [`TagItem`]s with the specified key, and [`ItemValue::Locator`] - pub fn get_locators<'a>(&'a self, key: &'a ItemKey) -> impl Iterator + Clone { + pub fn get_locators(&self, key: ItemKey) -> impl Iterator + Clone { self.items.iter().filter_map(move |i| { if i.key() == key { i.value().locator() @@ -499,7 +499,7 @@ impl Tag { } /// Returns references to all bytes of [`TagItem`]s with the specified key, and [`ItemValue::Binary`] - pub fn get_bytes<'a>(&'a self, key: &'a ItemKey) -> impl Iterator + Clone { + pub fn get_bytes(&self, key: ItemKey) -> impl Iterator + Clone { self.items.iter().filter_map(move |i| { if i.key() == key { i.value().binary() @@ -512,7 +512,7 @@ impl Tag { /// Remove an item by its key /// /// This will remove all items with this key. - pub fn remove_key(&mut self, key: &ItemKey) { + pub fn remove_key(&mut self, key: ItemKey) { self.items.retain(|i| i.key() != key) } @@ -641,7 +641,7 @@ impl Tag { impl TagExt for Tag { type Err = LoftyError; - type RefKey<'a> = &'a ItemKey; + type RefKey<'a> = ItemKey; #[inline] fn tag_type(&self) -> TagType { diff --git a/lofty/src/tag/split_merge_tag.rs b/lofty/src/tag/split_merge_tag.rs index 1a76844b7..d5ec1408d 100644 --- a/lofty/src/tag/split_merge_tag.rs +++ b/lofty/src/tag/split_merge_tag.rs @@ -28,7 +28,7 @@ use super::Tag; /// // Modify the metadata in the generic [`lofty::Tag`], independent /// // of the underlying tag and file format. /// tag.insert_text(ItemKey::TrackTitle, "Track Title".to_owned()); -/// tag.remove_key(&ItemKey::Composer); +/// tag.remove_key(ItemKey::Composer); /// /// // ID3v2 <- [`lofty::Tag`] /// let id3v2 = remainder.merge_tag(tag); diff --git a/lofty/src/tag/tag_ext.rs b/lofty/src/tag/tag_ext.rs index d7b91e0ec..53fcce61e 100644 --- a/lofty/src/tag/tag_ext.rs +++ b/lofty/src/tag/tag_ext.rs @@ -52,7 +52,7 @@ pub trait TagExt: Accessor + Into + Sized + private::Sealed { /// assert!(tag.is_empty()); /// /// tag.set_artist(String::from("Foo artist")); - /// assert!(tag.contains(&ItemKey::TrackArtist)); + /// assert!(tag.contains(ItemKey::TrackArtist)); /// ``` fn contains<'a>(&'a self, key: Self::RefKey<'a>) -> bool; diff --git a/lofty/src/tag/utils.rs b/lofty/src/tag/utils.rs index 50288f925..5065aae0d 100644 --- a/lofty/src/tag/utils.rs +++ b/lofty/src/tag/utils.rs @@ -90,10 +90,10 @@ pub(crate) fn dump_tag( use crate::tag::item::ItemKey; AiffTextChunksRef { - name: tag.get_string(&ItemKey::TrackTitle), - author: tag.get_string(&ItemKey::TrackArtist), - copyright: tag.get_string(&ItemKey::CopyrightMessage), - annotations: Some(tag.get_strings(&ItemKey::Comment)), + name: tag.get_string(ItemKey::TrackTitle), + author: tag.get_string(ItemKey::TrackArtist), + copyright: tag.get_string(ItemKey::CopyrightMessage), + annotations: Some(tag.get_strings(ItemKey::Comment)), comments: None, } } @@ -123,17 +123,17 @@ pub(crate) mod test_utils { } pub(crate) 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")); + 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")); + assert_eq!(tag.get_string(ItemKey::TrackNumber), Some("1")); } if genre { - assert_eq!(tag.get_string(&ItemKey::Genre), Some("Classical")); + assert_eq!(tag.get_string(ItemKey::Genre), Some("Classical")); } } diff --git a/lofty/tests/files/aac.rs b/lofty/tests/files/aac.rs index 737ec0519..924bb48da 100644 --- a/lofty/tests/files/aac.rs +++ b/lofty/tests/files/aac.rs @@ -44,7 +44,7 @@ fn read_with_junk_bytes_between_frames() { assert_eq!(id3v2_tag.album().as_deref(), Some("album test")); assert_eq!(id3v2_tag.title().as_deref(), Some("title test")); assert_eq!( - id3v2_tag.get_string(&ItemKey::EncoderSettings), + id3v2_tag.get_string(ItemKey::EncoderSettings), Some("Lavf58.62.100") ); diff --git a/lofty/tests/files/mpeg.rs b/lofty/tests/files/mpeg.rs index bae4ae529..15cb1eaef 100644 --- a/lofty/tests/files/mpeg.rs +++ b/lofty/tests/files/mpeg.rs @@ -47,7 +47,7 @@ fn read_with_junk_bytes_between_frames() { assert_eq!(id3v2_tag.album().as_deref(), Some("album test")); assert_eq!(id3v2_tag.title().as_deref(), Some("title test")); assert_eq!( - id3v2_tag.get_string(&ItemKey::EncoderSettings), + id3v2_tag.get_string(ItemKey::EncoderSettings), Some("Lavf58.62.100") ); @@ -87,7 +87,7 @@ fn issue_87_duplicate_id3v2() { assert_eq!(id3v2_tag.album().as_deref(), Some("album test")); assert_eq!(id3v2_tag.artist().as_deref(), Some("Foo artist")); // Original tag has "artist test" assert_eq!( - id3v2_tag.get_string(&ItemKey::EncoderSettings), + id3v2_tag.get_string(ItemKey::EncoderSettings), Some("Lavf58.62.100") ); assert_eq!(id3v2_tag.title().as_deref(), Some("title test")); diff --git a/lofty/tests/files/util/mod.rs b/lofty/tests/files/util/mod.rs index 4fdd2841c..e740206da 100644 --- a/lofty/tests/files/util/mod.rs +++ b/lofty/tests/files/util/mod.rs @@ -74,7 +74,7 @@ macro_rules! verify_artist { assert_eq!(tag.item_count(), $item_count); assert_eq!( - tag.get(&lofty::prelude::ItemKey::TrackArtist), + tag.get(lofty::prelude::ItemKey::TrackArtist), Some(&lofty::tag::TagItem::new( lofty::prelude::ItemKey::TrackArtist, lofty::tag::ItemValue::Text(String::from($expected_value)) diff --git a/lofty/tests/taglib/util/mod.rs b/lofty/tests/taglib/util/mod.rs index 78201d106..2ae421610 100644 --- a/lofty/tests/taglib/util/mod.rs +++ b/lofty/tests/taglib/util/mod.rs @@ -51,7 +51,7 @@ macro_rules! verify_artist { assert_eq!(tag.item_count(), $item_count); assert_eq!( - tag.get_item_ref(&ItemKey::TrackArtist), + tag.get_item_ref(ItemKey::TrackArtist), Some(&TagItem::new( ItemKey::TrackArtist, ItemValue::Text(String::from($expected_value)) diff --git a/lofty_attr/src/internal.rs b/lofty_attr/src/internal.rs index 5a1978440..5c61aeaa8 100644 --- a/lofty_attr/src/internal.rs +++ b/lofty_attr/src/internal.rs @@ -78,10 +78,10 @@ pub(crate) fn init_write_lookup( insert!(map, AiffText, { lofty::iff::aiff::tag::AiffTextChunksRef { - name: tag.get_string(&lofty::prelude::ItemKey::TrackTitle), - author: tag.get_string(&lofty::prelude::ItemKey::TrackArtist), - copyright: tag.get_string(&lofty::prelude::ItemKey::CopyrightMessage), - annotations: Some(tag.get_strings(&lofty::prelude::ItemKey::Comment)), + name: tag.get_string(lofty::prelude::ItemKey::TrackTitle), + author: tag.get_string(lofty::prelude::ItemKey::TrackArtist), + copyright: tag.get_string(lofty::prelude::ItemKey::CopyrightMessage), + annotations: Some(tag.get_strings(lofty::prelude::ItemKey::Comment)), comments: None, } .write_to(file, write_options) From f8f83f15dd5d3c7765be3653744a04f47555af3b Mon Sep 17 00:00:00 2001 From: Serial <69764315+Serial-ATA@users.noreply.github.com> Date: Mon, 7 Jul 2025 19:40:22 -0400 Subject: [PATCH 2/3] docs: Remove references to `ItemKey::Unknown` --- doc/NEW_TAG.md | 5 +---- lofty/src/id3/v2/tag.rs | 16 ++++++++-------- lofty/src/iff/wav/tag/mod.rs | 7 ++----- lofty/src/ogg/tag.rs | 3 --- lofty/src/tag/mod.rs | 4 +--- 5 files changed, 12 insertions(+), 23 deletions(-) diff --git a/doc/NEW_TAG.md b/doc/NEW_TAG.md index 21446f189..871bc3aeb 100644 --- a/doc/NEW_TAG.md +++ b/doc/NEW_TAG.md @@ -351,12 +351,9 @@ impl SplitTag for FooTag { Now callers can split their `FooTag` into a generic `Tag`, but we'll need a way to merge them back together. This is done with the `MergeTag` trait. -When implementing `MergeTag`, one must take two things into consideration: - -* The distinction between `ItemValue::Text` and `ItemValue::Locator` in certain formats +When implementing `MergeTag`, one may need to take the distinction between `ItemValue::Text` and `ItemValue::Locator` into consideration. * In ID3v2 for example, a locator is only valid for frames starting with W. * In a format such as VorbisComments, there is no need to distinguish between the two. -* The presence of `ItemKey::Unknown` With that in mind, we'll now implement `MergeTag` on `SplitTagRemainder`: diff --git a/lofty/src/id3/v2/tag.rs b/lofty/src/id3/v2/tag.rs index b2b453fbf..01e2cb1ec 100644 --- a/lofty/src/id3/v2/tag.rs +++ b/lofty/src/id3/v2/tag.rs @@ -88,18 +88,18 @@ macro_rules! impl_accessor { /// /// When converting from a [`Tag`] to an `Id3v2Tag`, some frames may need editing. /// -/// * [`ItemKey::Comment`] and [`ItemKey::Lyrics`] - Unlike a normal text frame, these require a language. See [`CommentFrame`] and [`UnsynchronizedTextFrame`] respectively. -/// An attempt is made to create this information, but it may be incorrect. -/// * `language` - Unknown and set to "XXX" -/// * `description` - Left empty, which is invalid if there are more than one of these frames. These frames can only be identified +/// * [`ItemKey::Comment`] and [`ItemKey::Lyrics`] - Unlike a normal text frame, these require a language and description. +/// * If these values aren't specified in the [`TagItem`], it will be filled in with (possibly incorrect) defaults. +/// * `language` - Unknown and set to "XXX" +/// * `description` - Left empty, which is invalid if there are more than one of these frames. These frames can only be identified /// by their descriptions, and as such they are expected to be unique for each. -/// * [`ItemKey::Unknown("WXXX" | "TXXX")`](ItemKey::Unknown) - These frames are also identified by their descriptions. +/// * See [`CommentFrame`] and [`UnsynchronizedTextFrame`] respectively. /// /// ### To `Tag` /// -/// * TXXX/WXXX - These frames will be stored as an [`ItemKey`] by their description. Some variants exist for these descriptions, such as the one for `ReplayGain`, -/// otherwise [`ItemKey::Unknown`] will be used. -/// * Frames that require a language (COMM/USLT) - With ID3v2 being the only format that allows for language-specific items, this information is not retained. +/// * TXXX/WXXX - These frames will be stored as an [`ItemKey`] by their description. Some variants exist +/// for these descriptions, such as [`ItemKey::ReplayGainAlbumGain`]. Anything without a mapping will +/// be discarded. /// * POPM - These frames will be stored as a raw [`ItemValue::Binary`] value under the [`ItemKey::Popularimeter`] key. /// /// ## Special Frames diff --git a/lofty/src/iff/wav/tag/mod.rs b/lofty/src/iff/wav/tag/mod.rs index ad9ecf537..93b9b4cb1 100644 --- a/lofty/src/iff/wav/tag/mod.rs +++ b/lofty/src/iff/wav/tag/mod.rs @@ -37,14 +37,11 @@ macro_rules! impl_accessor { /// /// ### To `Tag` /// -/// All items will be converted to a [`TagItem`], with all unknown keys being stored with [`ItemKey::Unknown`]. +/// All items without an [`ItemKey`] mapping will be discarded. /// /// ### From `Tag` /// -/// When converting a [`TagItem`], two conditions must be met: -/// -/// * The [`TagItem`] has a value other than [`ItemValue::Binary`](crate::ItemValue::Binary) -/// * It has a key that is 4 bytes in length and within the ASCII range +/// When converting a [`TagItem`], it must have a value other than [`ItemValue::Binary`](crate::ItemValue::Binary) #[derive(Default, Debug, PartialEq, Eq, Clone)] #[tag(description = "A RIFF INFO LIST", supported_formats(Wav))] pub struct RiffInfoList { diff --git a/lofty/src/ogg/tag.rs b/lofty/src/ogg/tag.rs index b5660b20a..396dc72ea 100644 --- a/lofty/src/ogg/tag.rs +++ b/lofty/src/ogg/tag.rs @@ -52,9 +52,6 @@ macro_rules! impl_accessor { /// If a [`TagItem`] with the key [`ItemKey::EncoderSoftware`] is available, it will be taken and /// used for the vendor string. /// -/// [`TagItem`]s with [`ItemKey::Unknown`] will have their keys verified for spec compliance. They must fall within -/// ASCII range `0x20` through `0x7D`, excluding `0x3D` ('='). -/// /// When converting [Picture]s, they will first be passed through [`PictureInformation::from_picture`]. /// If the information is available, it will be used. Otherwise, the picture will be stored with zeroed out /// [`PictureInformation`]. diff --git a/lofty/src/tag/mod.rs b/lofty/src/tag/mod.rs index af35e112d..357fcfe9d 100644 --- a/lofty/src/tag/mod.rs +++ b/lofty/src/tag/mod.rs @@ -363,8 +363,6 @@ impl Tag { /// /// * This **will not** verify an [`ItemKey`] mapping exists /// * This **will not** allow writing item keys that are out of spec (keys are verified before writing) - /// - /// This is only necessary if dealing with [`ItemKey::Unknown`]. pub fn insert_unchecked(&mut self, item: TagItem) { self.retain(|i| i.item_key != item.item_key); self.items.push(item); @@ -391,7 +389,7 @@ impl Tag { /// Append a [`TagItem`] to the tag /// - /// Notes: See [`Tag::insert_unchecked`] + /// Notes: See [`Tag::push()`] and the notes of [`Tag::insert_unchecked()`] pub fn push_unchecked(&mut self, item: TagItem) { self.items.push(item); } From 1ec22ca493579f9a06c80667ba44776c2ee06b7a Mon Sep 17 00:00:00 2001 From: Serial <69764315+Serial-ATA@users.noreply.github.com> Date: Mon, 7 Jul 2025 19:41:32 -0400 Subject: [PATCH 3/3] misc: clippy --- lofty/src/id3/v2/tag.rs | 2 +- lofty/src/mp4/ilst/mod.rs | 2 +- lofty/src/tag/mod.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lofty/src/id3/v2/tag.rs b/lofty/src/id3/v2/tag.rs index 01e2cb1ec..ccc52966a 100644 --- a/lofty/src/id3/v2/tag.rs +++ b/lofty/src/id3/v2/tag.rs @@ -1271,7 +1271,7 @@ impl MergeTag for SplitTagRemainder { type Merged = Id3v2Tag; fn merge_tag(self, mut tag: Tag) -> Id3v2Tag { - fn join_text_items<'a>( + fn join_text_items( tag: &mut Tag, keys: impl IntoIterator, ) -> Option { diff --git a/lofty/src/mp4/ilst/mod.rs b/lofty/src/mp4/ilst/mod.rs index 80ab5d809..d7e86af82 100644 --- a/lofty/src/mp4/ilst/mod.rs +++ b/lofty/src/mp4/ilst/mod.rs @@ -744,7 +744,7 @@ impl SplitTag for Ilst { _ => { return true; // Keep atom }, - }; + } tag.items.push(tag_item); false // Atom consumed diff --git a/lofty/src/tag/mod.rs b/lofty/src/tag/mod.rs index 357fcfe9d..4d0201855 100644 --- a/lofty/src/tag/mod.rs +++ b/lofty/src/tag/mod.rs @@ -465,7 +465,7 @@ impl Tag { } /// Removes all items with the specified [`ItemKey`], and filters them through [`ItemValue::into_string`] - pub fn take_strings<'a>(&'a mut self, key: ItemKey) -> impl Iterator + use<'a> { + pub fn take_strings(&mut self, key: ItemKey) -> impl Iterator + use<'_> { self.take(key).filter_map(|i| i.item_value.into_string()) }