misc: Introduce WriteOptions

CHANGELOG.md

@@ -6,6 +6,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+- `WriteOptions` ([issue](https://github.com/Serial-ATA/lofty-rs/issues/228)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/361)):
+	- ⚠️ Important ⚠️: This update introduces `WriteOptions` to allow for finer grained control over how
+	  Lofty writes tags. These are best used as global user-configurable options, as most options will
+	  not apply to all files. The defaults are set to be as safe as possible,
+	  see [here](https://docs.rs/lofty/latest/lofty/struct.WriteOptions.html#impl-Default-for-WriteOptions).
+
 ### Fixed
 - **Vorbis**: Fix panic when reading properties of zero-length files ([issue](https://github.com/Serial-ATA/lofty-rs/issues/342)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/365))
 - **ID3v2**: Fix panic when reading an RVA2 frame with a peak larger than 248 bits ([issue](https://github.com/Serial-ATA/lofty-rs/issues/295)) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/364))

Cargo.toml

@@ -18,7 +18,7 @@ byteorder     = "1.5.0"
 # ID3 compressed frames
 flate2        = { version = "1.0.28", optional = true }
 # Proc macros
-lofty_attr    = "0.9.0"
+lofty_attr = { path = "lofty_attr" }
 # Debug logging
 log           = "0.4.20"
 # OGG Vorbis/Opus

examples/tag_writer.rs

@@ -1,4 +1,4 @@
-use lofty::{Accessor, Probe, Tag, TagExt, TaggedFileExt};
+use lofty::{Accessor, Probe, Tag, TagExt, TaggedFileExt, WriteOptions};
 
 use structopt::StructOpt;
 
@@ -75,7 +75,7 @@ fn main() {
 		tag.set_genre(genre)
 	}
 
-	tag.save_to_path(&opt.path)
+	tag.save_to_path(&opt.path, WriteOptions::default())
 		.expect("ERROR: Failed to write the tag!");
 
 	println!("INFO: Tag successfully updated!");

lofty_attr/src/internal.rs

@@ -48,32 +48,32 @@ pub(crate) fn init_write_lookup(
 			read_only: false,
 			items: lofty::ape::tag::tagitems_into_ape(tag),
 		}
-		.write_to(data)
+		.write_to(data, write_options)
 	});
 
 	insert!(map, Id3v1, {
-		Into::<lofty::id3::v1::tag::Id3v1TagRef<'_>>::into(tag).write_to(data)
+		Into::<lofty::id3::v1::tag::Id3v1TagRef<'_>>::into(tag).write_to(data, write_options)
 	});
 
 	if id3v2_strippable {
 		insert!(map, Id3v2, {
-			lofty::id3::v2::tag::Id3v2TagRef::empty().write_to(data)
+			lofty::id3::v2::tag::Id3v2TagRef::empty().write_to(data, write_options)
 		});
 	} else {
 		insert!(map, Id3v2, {
 			lofty::id3::v2::tag::Id3v2TagRef {
 				flags: lofty::id3::v2::Id3v2TagFlags::default(),
 				frames: lofty::id3::v2::tag::tag_frames(tag),
 			}
-			.write_to(data)
+			.write_to(data, write_options)
 		});
 	}
 
 	insert!(map, RiffInfo, {
 		lofty::iff::wav::tag::RIFFInfoListRef::new(lofty::iff::wav::tag::tagitems_into_riff(
 			tag.items(),
 		))
-		.write_to(data)
+		.write_to(data, write_options)
 	});
 
 	insert!(map, AiffText, {
@@ -84,7 +84,7 @@ pub(crate) fn init_write_lookup(
 			annotations: Some(tag.get_strings(&lofty::tag::item::ItemKey::Comment)),
 			comments: None,
 		}
-		.write_to(data)
+		.write_to(data, write_options)
 	});
 
 	map
@@ -111,7 +111,7 @@ pub(crate) fn write_module(
 	quote! {
 		pub(crate) mod write {
 			#[allow(unused_variables)]
-			pub(crate) fn write_to(data: &mut ::std::fs::File, tag: &::lofty::Tag) -> ::lofty::error::Result<()> {
+			pub(crate) fn write_to(data: &mut ::std::fs::File, tag: &::lofty::Tag, write_options: ::lofty::WriteOptions) -> ::lofty::error::Result<()> {
 				match tag.tag_type() {
 					#( #applicable_formats )*
 					_ => crate::macros::err!(UnsupportedTag),

lofty_attr/src/lofty_file.rs

@@ -445,7 +445,7 @@ fn generate_audiofile_impl(file: &LoftyFile) -> syn::Result<proc_macro2::TokenSt
 				#read_fn(reader, parse_options)
 			}
 
-			fn save_to(&self, file: &mut ::std::fs::File) -> ::lofty::error::Result<()> {
+			fn save_to(&self, file: &mut ::std::fs::File, write_options: ::lofty::WriteOptions) -> ::lofty::error::Result<()> {
 				use ::lofty::TagExt as _;
 				use ::std::io::Seek as _;
 				#save_to_body
@@ -480,7 +480,7 @@ fn get_save_to_body(
 	// Custom write fn
 	if let Some(write_fn) = write_fn {
 		return quote! {
-			#write_fn(&self, file)
+			#write_fn(&self, file, write_options)
 		};
 	}
 
@@ -490,13 +490,13 @@ fn get_save_to_body(
 			quote! {
 				if let Some(ref tag) = self.#name {
 					file.rewind()?;
-					tag.save_to(file)?;
+					tag.save_to(file, write_options)?;
 				}
 			}
 		} else {
 			quote! {
 				file.rewind()?;
-				self.#name.save_to(file)?;
+				self.#name.save_to(file, write_options)?;
 			}
 		}
 	});

src/ape/tag/mod.rs

@@ -8,6 +8,7 @@ use crate::id3::v2::util::pairs::{format_number_pair, set_number, NUMBER_PAIR_KE
 use crate::tag::item::{ItemKey, ItemValue, ItemValueRef, TagItem};
 use crate::tag::{try_parse_year, Tag, TagType};
 use crate::traits::{Accessor, MergeTag, SplitTag, TagExt};
+use crate::write_options::WriteOptions;
 
 use std::borrow::Cow;
 use std::fs::File;
@@ -320,25 +321,33 @@ impl TagExt for ApeTag {
 	///
 	/// * Attempting to write the tag to a format that does not support it
 	/// * An existing tag has an invalid size
-	fn save_to(&self, file: &mut File) -> std::result::Result<(), Self::Err> {
+	fn save_to(
+		&self,
+		file: &mut File,
+		write_options: WriteOptions,
+	) -> std::result::Result<(), Self::Err> {
 		ApeTagRef {
 			read_only: self.read_only,
 			items: self.items.iter().map(Into::into),
 		}
-		.write_to(file)
+		.write_to(file, write_options)
 	}
 
 	/// Dumps the tag to a writer
 	///
 	/// # Errors
 	///
 	/// * [`std::io::Error`]
-	fn dump_to<W: Write>(&self, writer: &mut W) -> std::result::Result<(), Self::Err> {
+	fn dump_to<W: Write>(
+		&self,
+		writer: &mut W,
+		write_options: WriteOptions,
+	) -> std::result::Result<(), Self::Err> {
 		ApeTagRef {
 			read_only: self.read_only,
 			items: self.items.iter().map(Into::into),
 		}
-		.dump_to(writer)
+		.dump_to(writer, write_options)
 	}
 
 	fn remove_from_path<P: AsRef<Path>>(&self, path: P) -> std::result::Result<(), Self::Err> {
@@ -482,12 +491,16 @@ impl<'a, I> ApeTagRef<'a, I>
 where
 	I: Iterator<Item = ApeItemRef<'a>>,
 {
-	pub(crate) fn write_to(&mut self, file: &mut File) -> Result<()> {
-		write::write_to(file, self)
+	pub(crate) fn write_to(&mut self, file: &mut File, write_options: WriteOptions) -> Result<()> {
+		write::write_to(file, self, write_options)
 	}
 
-	pub(crate) fn dump_to<W: Write>(&mut self, writer: &mut W) -> Result<()> {
-		let temp = write::create_ape_tag(self)?;
+	pub(crate) fn dump_to<W: Write>(
+		&mut self,
+		writer: &mut W,
+		write_options: WriteOptions,
+	) -> Result<()> {
+		let temp = write::create_ape_tag(self, std::iter::empty(), write_options)?;
 		writer.write_all(&temp)?;
 
 		Ok(())
@@ -531,7 +544,7 @@ pub(crate) fn tagitems_into_ape(tag: &Tag) -> impl Iterator<Item = ApeItemRef<'_
 #[cfg(test)]
 mod tests {
 	use crate::ape::{ApeItem, ApeTag};
-	use crate::{Accessor, ItemKey, ItemValue, Tag, TagExt, TagItem, TagType};
+	use crate::{Accessor, ItemKey, ItemValue, Tag, TagExt, TagItem, TagType, WriteOptions};
 
 	use crate::id3::v2::util::pairs::DEFAULT_NUMBER_IN_PAIR;
 	use std::io::Cursor;
@@ -608,7 +621,9 @@ mod tests {
 			.unwrap();
 
 		let mut writer = Vec::new();
-		parsed_tag.dump_to(&mut writer).unwrap();
+		parsed_tag
+			.dump_to(&mut writer, WriteOptions::default())
+			.unwrap();
 
 		let mut temp_reader = Cursor::new(writer);
 

src/ape/tag/write.rs

@@ -7,14 +7,19 @@ use crate::id3::{find_id3v1, find_id3v2, find_lyrics3v2, FindId3v2Config};
 use crate::macros::{decode_err, err};
 use crate::probe::Probe;
 use crate::tag::item::ItemValueRef;
+use crate::write_options::WriteOptions;
 
 use std::fs::File;
 use std::io::{Cursor, Read, Seek, SeekFrom, Write};
 
 use byteorder::{LittleEndian, WriteBytesExt};
 
 #[allow(clippy::shadow_unrelated)]
-pub(crate) fn write_to<'a, I>(data: &mut File, tag: &mut ApeTagRef<'a, I>) -> Result<()>
+pub(crate) fn write_to<'a, I>(
+	data: &mut File,
+	tag_ref: &mut ApeTagRef<'a, I>,
+	write_options: WriteOptions,
+) -> Result<()>
 where
 	I: Iterator<Item = ApeItemRef<'a>>,
 {
@@ -43,11 +48,13 @@ where
 	let start = data.stream_position()?;
 	match read::read_ape_tag(data, false)? {
 		Some((mut existing_tag, header)) => {
-			// Only keep metadata around that's marked read only
-			existing_tag.items.retain(|i| i.read_only);
+			if write_options.respect_read_only {
+				// Only keep metadata around that's marked read only
+				existing_tag.items.retain(|i| i.read_only);
 
-			if !existing_tag.items.is_empty() {
-				read_only = Some(existing_tag)
+				if !existing_tag.items.is_empty() {
+					read_only = Some(existing_tag)
+				}
 			}
 
 			header_ape_tag = (true, (start, start + u64::from(header.size)))
@@ -72,15 +79,22 @@ where
 	// Also check this tag for any read only items
 	let start = data.stream_position()? as usize + 32;
 	if let Some((mut existing_tag, header)) = read::read_ape_tag(data, true)? {
-		let size = header.size;
-
-		existing_tag.items.retain(|i| i.read_only);
+		if write_options.respect_read_only {
+			existing_tag.items.retain(|i| i.read_only);
 
-		if !existing_tag.items.is_empty() {
-			read_only = Some(existing_tag)
+			if !existing_tag.items.is_empty() {
+				read_only = match read_only {
+					Some(mut read_only) => {
+						read_only.items.extend(existing_tag.items);
+						Some(read_only)
+					},
+					None => Some(existing_tag),
+				}
+			}
 		}
 
 		// Since the "start" was really at the end of the tag, this sanity check seems necessary
+		let size = header.size;
 		if let Some(start) = start.checked_sub(size as usize) {
 			ape_tag_location = Some(start..start + size as usize);
 		} else {
@@ -89,13 +103,15 @@ where
 	}
 
 	// Preserve any metadata marked as read only
-	let tag = if let Some(read_only) = read_only {
-		create_ape_tag(&mut ApeTagRef {
-			read_only: read_only.read_only,
-			items: read_only.items.iter().map(Into::into),
-		})?
+	let tag;
+	if let Some(read_only) = read_only {
+		tag = create_ape_tag(
+			tag_ref,
+			read_only.items.iter().map(Into::into),
+			write_options,
+		)?;
 	} else {
-		create_ape_tag(tag)?
+		tag = create_ape_tag(tag_ref, std::iter::empty(), write_options)?;
 	};
 
 	data.rewind()?;
@@ -122,9 +138,14 @@ where
 	Ok(())
 }
 
-pub(super) fn create_ape_tag<'a, I>(tag: &mut ApeTagRef<'a, I>) -> Result<Vec<u8>>
+pub(super) fn create_ape_tag<'a, 'b, I, R>(
+	tag: &mut ApeTagRef<'a, I>,
+	mut read_only: R,
+	write_options: WriteOptions,
+) -> Result<Vec<u8>>
 where
 	I: Iterator<Item = ApeItemRef<'a>>,
+	R: Iterator<Item = ApeItemRef<'b>>,
 {
 	let items = &mut tag.items;
 	let mut peek = items.peekable();
@@ -134,6 +155,12 @@ where
 		return Ok(Vec::<u8>::new());
 	}
 
+	if read_only.next().is_some() && write_options.respect_read_only {
+		// TODO: Implement retaining read only items
+		log::warn!("Retaining read only items is not supported yet");
+		drop(read_only);
+	}
+
 	let mut tag_write = Cursor::new(Vec::<u8>::new());
 
 	let mut item_count = 0_u32;