diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 9aa6ce4..3e8a92f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -32,7 +32,9 @@ jobs: - name: clippy check run: | cargo clippy --no-deps --all-targets -- -D warnings + cargo clippy --no-deps --all-targets --features=preserve_order -- -D warnings cargo clippy --no-deps --all-targets --features=serde -- -D warnings + cargo clippy --no-deps --all-targets --features=preserve_order,serde -- -D warnings docs: name: docs @@ -45,7 +47,9 @@ jobs: - name: docs check run: | cargo doc --no-deps + cargo doc --no-deps --features=preserve_order cargo doc --no-deps --features=serde + cargo doc --no-deps --features=preserve_order,serde test: name: test @@ -58,4 +62,6 @@ jobs: - name: run tests run: | cargo test + cargo test --features=preserve_order cargo test --features=serde + cargo test --features=preserve_order,serde diff --git a/Cargo.toml b/Cargo.toml index 7ace8fa..14f3cff 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,10 +12,12 @@ edition = "2018" byteorder = "1.3.4" hex = "0.4.2" +indexmap = { version = "2.13.0", optional = true } serde = { version = "1.0.217", features = ["derive"], optional = true } [features] -serde = ["dep:serde"] +preserve_order = ["dep:indexmap"] +serde = ["dep:serde", "indexmap?/serde"] [dev-dependencies] -serde_json = "1.0.134" +serde_json = { version = "1.0.134", features = ["preserve_order"] } diff --git a/src/block.rs b/src/block.rs index 80fcd9c..83b8c71 100644 --- a/src/block.rs +++ b/src/block.rs @@ -1,8 +1,11 @@ use crate::error::{Error, ErrorKind, Result}; use byteorder::{ReadBytesExt, WriteBytesExt, BE}; +#[cfg(feature = "preserve_order")] +use indexmap::{map::Entry, IndexMap as Map}; -use std::collections::HashMap; +#[cfg(not(feature = "preserve_order"))] +use std::collections::{hash_map::Entry, HashMap as Map}; use std::convert::TryInto; use std::io::{Read, Write}; use std::iter::repeat; @@ -896,7 +899,7 @@ pub struct VorbisComment { /// The vendor string. pub vendor_string: String, /// A map of keys to a list of their values. - pub comments: HashMap>, + pub comments: Map>, } impl VorbisComment { @@ -904,7 +907,7 @@ impl VorbisComment { pub fn new() -> VorbisComment { VorbisComment { vendor_string: String::new(), - comments: HashMap::new(), + comments: Map::new(), } } @@ -982,29 +985,39 @@ impl VorbisComment { /// Sets the comments for the specified key. Any previous values under the key will be removed. pub fn set, V: Into>(&mut self, key: K, values: Vec) { - let key_owned = key.into(); - self.remove(&key_owned[..]); - self.comments - .insert(key_owned, values.into_iter().map(|s| s.into()).collect()); + let key = key.into(); + let values = values.into_iter().map(V::into).collect(); + self.comments.insert(key, values); } /// Removes the comments for the specified key. pub fn remove(&mut self, key: &str) { + #[cfg(feature = "preserve_order")] + self.comments.shift_remove(key); + #[cfg(not(feature = "preserve_order"))] self.comments.remove(key); } - /// Removes any matching key/value pairs. - pub fn remove_pair(&mut self, key: &str, value: &str) { - if let Some(list) = self.comments.get_mut(key) { - list.retain(|s| &s[..] != value); + fn remove_keys(&mut self, keys: &[&str]) { + #[cfg(feature = "preserve_order")] + self.comments.retain(|k, _| !keys.contains(&k.as_str())); + #[cfg(not(feature = "preserve_order"))] + for key in keys { + self.remove(key); } + } - let mut num_values = 0; - if let Some(values) = self.get(key) { - num_values = values.len(); - } - if num_values == 0 { - self.remove(key) + /// Removes any matching key/value pairs. + pub fn remove_pair(&mut self, key: &str, value: &str) { + if let Entry::Occupied(mut entry) = self.comments.entry(key.to_string()) { + let values = entry.get_mut(); + values.retain(|s| &s[..] != value); + if values.is_empty() { + #[cfg(feature = "preserve_order")] + entry.shift_remove(); + #[cfg(not(feature = "preserve_order"))] + entry.remove(); + } } } @@ -1024,8 +1037,7 @@ impl VorbisComment { /// Removes all values with the ARTIST key. This will result in any ARTISTSORT comments being /// removed as well. pub fn remove_artist(&mut self) { - self.remove("ARTISTSORT"); - self.remove("ARTIST"); + self.remove_keys(&["ARTISTSORT", "ARTIST"]); } /// Returns a reference to the vector of values with the ALBUM key. @@ -1043,8 +1055,7 @@ impl VorbisComment { /// Removes all values with the ALBUM key. This will result in any ALBUMSORT comments being /// removed as well. pub fn remove_album(&mut self) { - self.remove("ALBUMSORT"); - self.remove("ALBUM"); + self.remove_keys(&["ALBUMSORT", "ALBUM"]); } /// Returns a reference to the vector of values with the GENRE key. @@ -1077,8 +1088,7 @@ impl VorbisComment { /// Removes all values with the TITLE key. This will result in any TITLESORT comments being /// removed as well. pub fn remove_title(&mut self) { - self.remove("TITLESORT"); - self.remove("TITLE"); + self.remove_keys(&["TITLESORT", "TITLE"]); } /// Attempts to convert the first TRACKNUMBER comment to a `u32`. @@ -1138,8 +1148,7 @@ impl VorbisComment { /// Removes all values with the ALBUMARTIST key. This will result in any ALBUMARTISTSORT /// comments being removed as well. pub fn remove_album_artist(&mut self) { - self.remove("ALBUMARTISTSORT"); - self.remove("ALBUMARTIST"); + self.remove_keys(&["ALBUMARTISTSORT", "ALBUMARTIST"]); } /// Returns a reference to the vector of values with the LYRICS key. @@ -1262,3 +1271,171 @@ pub(crate) fn read_ident(mut reader: R) -> Result<()> { )) } } + +#[cfg(test)] +mod tests { + use super::*; + #[cfg(all(feature = "preserve_order", feature = "serde"))] + use serde_json::{json, Value}; + + fn example_vorbis_comment() -> VorbisComment { + let mut vc = VorbisComment::new(); + vc.set_title(vec!["Track Title"]); + vc.set_artist(vec!["The Artist"]); + vc.set("ARTISTSORT", vec!["Artist, The"]); + vc.set_album(vec!["Test Album"]); + vc.set_album_artist(vec!["Album Artist"]); + vc.set_track(2); + vc.set_total_tracks(10); + vc.set_genre(vec!["Rock", "Pop"]); + vc.set_lyrics(vec!["Lyrics"]); + vc + } + + #[test] + fn vorbis_comment_encoding_round_trip() { + let vc = example_vorbis_comment(); + let encoded = vc.to_bytes(); + let decoded = VorbisComment::from_bytes(&encoded).unwrap(); + assert_eq!(vc, decoded); + } + + #[cfg(feature = "preserve_order")] + #[test] + fn vorbis_comment_preserve_order() { + let mut vorbis = VorbisComment::new(); + + // Iteration order should match insertion order; overwrites retain the prior position. + vorbis.set("D_KEY", vec!["first"]); + vorbis.set("C_KEY", vec!["will_be_updated"]); + vorbis.set("B_KEY", vec!["third", "fourth"]); + vorbis.set("A_KEY", vec!["last"]); + vorbis.set("C_KEY", vec!["second"]); + + let encoded = vorbis.to_bytes(); + let decoded = VorbisComment::from_bytes(&encoded).unwrap(); + + assert_eq!(vorbis, decoded); + let decoded_keys: Vec<&str> = decoded.comments.keys().map(String::as_str).collect(); + assert_eq!(vec!["D_KEY", "C_KEY", "B_KEY", "A_KEY"], decoded_keys); + } + + #[test] + fn vorbis_comment_remove_keys() { + let mut vc = example_vorbis_comment(); + assert!(vc.comments.contains_key("ARTIST")); + assert!(vc.comments.contains_key("ARTISTSORT")); + assert!(vc.comments.contains_key("ALBUM")); + assert!(!vc.comments.contains_key("ALBUMSORT")); + + vc.remove_keys(&["ARTISTSORT", "ARTIST"]); + vc.remove_keys(&["ALBUMSORT", "ALBUM"]); + + assert!(!vc.comments.contains_key("ARTIST")); + assert!(!vc.comments.contains_key("ARTISTSORT")); + assert!(!vc.comments.contains_key("ALBUM")); + } + + #[cfg(feature = "preserve_order")] + #[test] + fn vorbis_comment_remove_keys_preserve_order() { + let mut vc = VorbisComment::new(); + vc.set("A", vec!["1"]); + vc.set("B", vec!["2"]); + vc.set("C", vec!["3"]); + vc.set("D", vec!["4"]); + + vc.remove_keys(&["B", "D"]); + + let remaining_keys: Vec<&str> = vc.comments.keys().map(String::as_str).collect(); + assert_eq!(remaining_keys, vec!["A", "C"]); + } + + #[test] + fn vorbis_comment_remove_pair() { + let mut vc = example_vorbis_comment(); + assert_eq!(vc.track(), Some(2)); + assert_eq!(vc.total_tracks(), Some(10)); + assert_eq!( + vc.genre().unwrap(), + &vec!["Rock".to_string(), "Pop".to_string()] + ); + + vc.remove_pair("TRACKNUMBER", "5"); // No effect. + vc.remove_pair("TOTALTRACKS", "10"); // No more tag at all. + vc.remove_pair("GENRE", "Rock"); // Remove one (but not all) of the values. + + assert_eq!(vc.track(), Some(2)); + assert_eq!(vc.total_tracks(), None); + assert!(!vc.comments.contains_key("TOTALTRACKS")); + assert_eq!(vc.genre().unwrap(), &vec!["Pop".to_string()]); + } + + #[cfg(feature = "preserve_order")] + #[test] + fn vorbis_comment_remove_pair_preserve_order() { + let mut vc = VorbisComment::new(); + vc.set("A", vec!["1"]); + vc.set("B", vec!["2"]); + vc.set("C", vec!["3", "4"]); + vc.set("D", vec!["5"]); + + vc.remove_pair("A", "no_effect"); + vc.remove_pair("B", "2"); + vc.remove_pair("C", "4"); + + let remaining_keys: Vec<&str> = vc.comments.keys().map(String::as_str).collect(); + assert_eq!(remaining_keys, vec!["A", "C", "D"]); + assert_eq!(vc.get("C"), Some(&vec!["3".to_string()])); + } + + #[cfg(feature = "serde")] + #[test] + fn vorbis_comment_serde_round_trip() { + let vc = example_vorbis_comment(); + let encoded = serde_json::to_string(&vc).unwrap(); + let decoded: VorbisComment = serde_json::from_str(&encoded).unwrap(); + assert_eq!(vc, decoded); + } + + #[cfg(all(feature = "preserve_order", feature = "serde"))] + #[test] + fn vorbis_comment_serde_preserve_order() { + let expected_json = json!({ + "vendor_string": "sprocketeer", + "comments": { + "KEY_4": ["first"], + "KEY_3": ["second"], + "KEY_2": ["third", "fourth"], + "KEY_1": ["fifth"], + "KEY_0": ["last"], + }, + }); + let mut vc = VorbisComment::new(); + vc.vendor_string = "sprocketeer".to_string(); + // Deliberately not sorted lexigraphically, and with a replacement. + vc.set("KEY_4", vec!["first"]); + vc.set("KEY_3", vec!["replaced"]); + vc.set("KEY_2", vec!["third", "fourth"]); + vc.set("KEY_1", vec!["fifth"]); + vc.set("KEY_0", vec!["last"]); + vc.set("KEY_3", vec!["second"]); + + let encoded = serde_json::to_string(&vc).unwrap(); + + // Check ordering is preserved when directly decoding. + let decoded_vc: VorbisComment = serde_json::from_str(&encoded).unwrap(); + assert_eq!(vc, decoded_vc); + + // Check the ordering is preserved when decoding as JSON. + let decoded_json: Value = serde_json::from_str(&encoded).unwrap(); + assert_eq!(decoded_json, expected_json); + let tag_keys: Vec<&str> = decoded_json["comments"] + .as_object() + .unwrap() + .keys() + .map(String::as_str) + .collect(); + assert_eq!(tag_keys, vec!["KEY_4", "KEY_3", "KEY_2", "KEY_1", "KEY_0"]); + } +} diff --git a/src/tag.rs b/src/tag.rs index 7e3fe50..545923f 100644 --- a/src/tag.rs +++ b/src/tag.rs @@ -188,9 +188,7 @@ impl<'a> Tag { /// assert!(tag.get_vorbis(&key).is_none()); /// ``` pub fn remove_vorbis(&mut self, key: &str) { - self.vorbis_comments_mut() - .comments - .remove(&key.to_ascii_uppercase()); + self.vorbis_comments_mut().remove(&key.to_ascii_uppercase()); } /// Removes the vorbis comments with the specified key and value.