From ea8da897a7ef465fd1a346c582ae1e33537cccd6 Mon Sep 17 00:00:00 2001
From: Uwe Klotz <uwe.klotz@gmail.com>
Date: Mon, 30 Jan 2023 10:07:57 +0100
Subject: [PATCH] Remove special case handling of empty string values

---
 CHANGELOG.md       |  3 +++
 src/ape/tag/mod.rs |  5 -----
 src/id3/v1/tag.rs  |  5 -----
 src/id3/v2/tag.rs  | 41 +++++++++++++++++++----------------------
 src/tag/mod.rs     | 10 ++++------
 5 files changed, 26 insertions(+), 38 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 49ba5acda..7687efd62 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+- **APE**/**ID3v1**/**ID3v2**/**Tag**
+  - Allow empty strings as values instead of removing the corresponding item when empty ([PR](https://github.com/Serial-ATA/lofty-rs/pull/134))
+
 ## [0.11.0] - 2022-1-29
 
 ### Added
diff --git a/src/ape/tag/mod.rs b/src/ape/tag/mod.rs
index 71661bac6..1b9ce37e0 100644
--- a/src/ape/tag/mod.rs
+++ b/src/ape/tag/mod.rs
@@ -33,11 +33,6 @@ macro_rules! impl_accessor {
 				}
 
 				fn [<set_ $name>](&mut self, value: String) {
-					if value.is_empty() {
-						self.[<remove_ $name>]();
-						return;
-					}
-
 					self.insert(ApeItem {
 						read_only: false,
 						key: String::from(crate::tag::item::first_key!($($key)|*)),
diff --git a/src/id3/v1/tag.rs b/src/id3/v1/tag.rs
index 6991ae0f4..86587d9b8 100644
--- a/src/id3/v1/tag.rs
+++ b/src/id3/v1/tag.rs
@@ -24,11 +24,6 @@ macro_rules! impl_accessor {
 				}
 
 				fn [<set_ $name>](&mut self, value: String) {
-					if value.is_empty() {
-						self.[<remove_ $name>]();
-						return;
-					}
-
 					self.$name = Some(value)
 				}
 
diff --git a/src/id3/v2/tag.rs b/src/id3/v2/tag.rs
index 57e32229c..2aa5fcf65 100644
--- a/src/id3/v2/tag.rs
+++ b/src/id3/v2/tag.rs
@@ -34,11 +34,6 @@ macro_rules! impl_accessor {
 				}
 
 				fn [<set_ $name>](&mut self, value: String) {
-					if value.is_empty() {
-						self.remove($id);
-						return;
-					}
-
 					self.insert(Frame {
 						id: FrameID::Valid(String::from($id).into()),
 						value: FrameValue::Text {
@@ -694,28 +689,30 @@ impl SplitAndMergeTag for ID3v2Tag {
 	}
 
 	fn merge_tag(&mut self, mut tag: Tag) {
-		fn join_items(tag: &mut Tag, key: &ItemKey) -> String {
+		fn join_items(tag: &mut Tag, key: &ItemKey) -> Option<String> {
 			let mut iter = tag.take_strings(key);
-
-			match iter.next() {
-				None => String::new(),
-				Some(first) => {
-					let mut s = String::with_capacity(iter.size_hint().0);
-					s.push_str(&first);
-					iter.for_each(|i| {
-						s.push(V4_MULTI_VALUE_SEPARATOR);
-						s.push_str(&i);
-					});
-
-					s
-				},
-			}
+			iter.next().map(|first| {
+				// Use the length of the first string for estimating the capacity
+				// of the concatenated string.
+				let estimated_len_per_item = first.len();
+				let min_remaining_items = iter.size_hint().0;
+				let mut concatenated = first;
+				concatenated.reserve((1 + estimated_len_per_item) * min_remaining_items);
+				iter.for_each(|i| {
+					concatenated.push(V4_MULTI_VALUE_SEPARATOR);
+					concatenated.push_str(&i);
+				});
+				concatenated
+			})
 		}
 
 		self.frames.reserve(tag.item_count() as usize);
 
-		let artists = join_items(&mut tag, &ItemKey::TrackArtist);
-		self.set_artist(artists);
+		if let Some(artists) = join_items(&mut tag, &ItemKey::TrackArtist) {
+			self.set_artist(artists);
+		} else {
+			self.remove_artist()
+		}
 
 		for item in tag.items {
 			let frame: Frame<'_> = match item.into() {
diff --git a/src/tag/mod.rs b/src/tag/mod.rs
index 7b923d2df..e4a57c018 100644
--- a/src/tag/mod.rs
+++ b/src/tag/mod.rs
@@ -27,11 +27,6 @@ macro_rules! impl_accessor {
 				}
 
 				fn [<set_ $name>](&mut self, value: String) {
-					if value.is_empty() {
-						self.[<remove_ $name>]();
-						return;
-					}
-
 					self.insert_item(TagItem::new(ItemKey::$item_key, ItemValue::Text(value)));
 				}
 
@@ -719,13 +714,16 @@ mod tests {
 	}
 
 	#[test]
-	fn insert_empty() {
+	fn should_preserve_empty_title() {
 		let mut tag = Tag::new(TagType::ID3v2);
 		tag.set_title(String::from("Foo title"));
 
 		assert_eq!(tag.title().as_deref(), Some("Foo title"));
 
 		tag.set_title(String::new());
+		assert_eq!(tag.title().as_deref(), Some(""));
+
+		tag.remove_title();
 		assert_eq!(tag.title(), None);
 	}
 }