ID3v2: Fix about number pair of track and disk #149

mikanbako · 2023-02-20T01:48:44Z

This PR fixes #145.

TODOs

Rebase to the main branch for conflict.
Fix id3::v2::tag::tag_frames() that called from tag::Tag::save_to().
Add tests about frame values.
Improve parsing number pair.

uklotzde · 2023-02-23T09:55:38Z

src/id3/v2/tag.rs

@@ -25,6 +26,8 @@ const COMMENT_FRAME_ID: &str = "COMM";
 const V4_MULTI_VALUE_SEPARATOR: char = '\0';
 const NUMBER_PAIR_SEPARATOR: char = '/';

+const DEFAULT_NUMBER: u32 = 1;


I suggest to use 0 as the default, which should never appear as a regular value.

DEFAULT_NUMBER_IN_PAIR for disambiguation of the very generic name

Please add comments to both constants that these apply to both track and disk numbers

I fixed this part.

uklotzde · 2023-02-23T10:01:05Z

src/id3/v2/tag.rs

@@ -285,6 +288,30 @@ impl ID3v2Tag {

 		(None, None)
 	}
+
+	fn insert_frame(&mut self, item: TagItem) {


The purpose of this function is to insert an item by mapping it to the underlying representation, but it does not (always) insert a frame -> insert_item()

I fixed this part.

uklotzde · 2023-02-23T10:04:06Z

src/id3/v2/tag.rs

+		fn set_number<F: FnMut(u32)>(item: TagItem, mut setter: F) {
+			let number = item.into_value().text().map(str::parse::<u32>);
+
+			if let Some(Ok(number)) = number {


Log a warning instead of silently ignoring values that couldn't be parsed successfully?

I fixed this part.

uklotzde · 2023-02-23T10:04:42Z

src/id3/v2/tag.rs

@@ -349,6 +376,14 @@ fn new_picture_frame(picture: Picture, flags: FrameFlags) -> Frame<'static> {
 	}
 }

+fn merge_num_pair<N, T>(number: N, total: T) -> String


format_number_pair()? No need for abbreviations here.

Using optional arguments would eliminate redundant code in setters and make this function more versatile and testable in isolation.

I fixed this part.

uklotzde · 2023-02-23T10:13:29Z

src/id3/v2/tag.rs

+		}
+
+		if let Some(total) = total {
+			let number = value.unwrap_or_else(|| Cow::Owned(DEFAULT_NUMBER.to_string()));


A const &str representation for DEFAULT_NUMBER would avoid an unecessary allocation -> value.as_deref().unwrap_or(DEFAULT_NUMBER_STR).

With the proposed changes and extension of merge_num_pair this might even become obsolete.

I did not fixed it. Because merge_num_pair was changed.

uklotzde · 2023-02-23T10:19:23Z

src/id3/v2/tag.rs

+		})
+	}
+
+	let number_pair_keys = &[


This should be extracted as a constant of the module. Probably near the other number constants. Serves as additional documentation.

I fixed this part.

uklotzde · 2023-02-23T10:21:51Z

src/id3/v2/tag.rs

@@ -1634,4 +1723,103 @@ mod tests {

 		assert_eq!(id3v2, reparsed);
 	}
+
+	#[test]


The tests only cover a single case. Missing are test for only number and only total.

I am also curious to know what happens if the frame value

is empty (contains only whitespace)

"/"

"/1"

"1/"

This should be handled by lightweight unit tests, no need for heavy-weight file tests.

I added tests about the missing tests at first.

I will add tests about the frame values.

mikanbako · 2023-02-23T12:10:58Z

@uklotzde Thank you for review. I will fix this PR.

uklotzde · 2023-02-23T14:19:19Z

@uklotzde Thank you for review. I will fix this PR.

Take my proposals with a grain of salt. All my remarks in PRs are intended as additional input from an outside viewer who has a different perspective on the topic. They should support you to validate and rethink your design decisions or to reveal blank spaces and edge cases.

A seemingly dumb question is often the most helpful feedback you could get for improving the code.

mikanbako · 2023-02-24T00:12:33Z

I fixed my existing code at first.

I will add tests about the frame values.

@uklotzde Thank you for comment. You have helped me to improve my PR more.

mikanbako · 2023-02-24T04:32:29Z

I fixed a log message only.

I will add tests about the frame values.

uklotzde · 2023-02-24T12:32:42Z

src/id3/v2/tag.rs

+	fn insert_number_pair(&mut self, id: &'static str, number: Option<u32>, total: Option<u32>) {
+		let content = format_number_pair(number, total);
+
+		match content {


Using if let Some() = would be more idiomatic. I remember that clippy warns when matching on Option.

Thank you for comment. I fixed the code.

mikanbako · 2023-02-25T03:31:15Z

I added tests about the frame values and fixed the code to except invalid values.

uklotzde · 2023-02-25T14:00:51Z

src/id3/v2/tag.rs

+		assert_invalid("/1");
+		assert_invalid("1/");
+		assert_invalid("a/b");
+		assert_invalid("1 / 2");


I would expect that leading/trailing whitespaces around the numbers are trimmed.

https://en.wikipedia.org/wiki/Robustness_principle

uklotzde · 2023-02-25T14:01:24Z

src/id3/v2/tag.rs

+		assert_invalid("1 / 2");
+		assert_invalid("1/2/3");
+		assert_invalid("1//2");
+		assert_invalid(" 1/2 ");


This input should also be considered as valid.

uklotzde · 2023-02-25T14:05:54Z

src/id3/v2/tag.rs

+
+			let is_valid = if let Some(total) = total {
+				content.len() == number.len() + 1 + total.len()
+					&& str::parse::<u32>(number).is_ok()


By extracting a parse_number() function you could handle the whitespace trimming consistently. I consider "total" as a special kind of number, so the naming should be reasonable and we do not need to name it parse_number_or_total() .

mikanbako · 2023-02-25T15:51:29Z

@uklotzde Thank you for comments. I will modify my code.

Before I do that, please allow to confirm the details about valid or invalid frames.

I understood the following table, is it right?

frame value	valid or invalid	number	total
/1	invalid	-	-
1/	invalid	-	-
1 / 2	valid	1	2
1/2/3	valid	1	2
1//2	valid	1	2
1/2	valid	1	2
a/b	invalid	-	-
1/b	invalid	-	-
a/1	invalid	-	-

uklotzde · 2023-02-25T17:43:49Z

Before I do that, please allow to confirm the details about valid or invalid frames.

I suggest to only ignore whitespace, because the overarching objective is to preserve all (meaningful) information. Consequently both 1/2/3 and 1//2 should not be parsed as a pair of numbers, because we cannot reason about the intention of the user. Play it safe.

I am unsure about 1/ and /1 which could both be interpreted as a missing numbers (defaulting to 0) when using a sloppy formatter that unconditionally inserts the separator.

mikanbako · 2023-02-26T01:01:44Z

@uklotzde Thank you for the comment.

I will modify my code to ignore whitespaces.

For the present, 1/ and /1 will be treated as invalid.

In other words, I plan to modify my code for the following table.

frame value	valid or invalid	number	total
/1	invalid	-	-
1/	invalid	-	-
1 / 2	valid	1	2
1/2/3	invalid	-	-
1//2	invalid	-	-
1/2	valid	1	2
a/b	invalid	-	-
1/b	invalid	-	-
a/1	invalid	-	-

mikanbako · 2023-02-26T03:52:50Z

I modified my code to ignore whitespaces while parsing a number pair.

uklotzde · 2023-02-26T10:36:28Z

src/id3/v2/tag.rs

 			total_key: ItemKey,
 		) -> Option<()> {
+			fn parse_number(source: Option<&str>) -> Option<&str> {


This function should accept source: &str. Otherwise you are implicitly re-implementing Option::and_then.

uklotzde · 2023-02-26T10:37:50Z

src/id3/v2/tag.rs

+			let number_source = split.next()?;
+			let number = parse_number(Some(number_source))?;
+			let total_source = split.next();
+			let total = parse_number(total_source);


let total = total_source.and_then(parse_number)

uklotzde · 2023-02-26T10:39:07Z

src/id3/v2/tag.rs

+				if str::parse::<u32>(number).is_ok() {
+					Some(number)
+				} else {
+					log::warn!("{number:?} could not be parsed as a number.");


Logging a warning if the input is empty is not desired.

mikanbako · 2023-02-26T12:20:38Z

@uklotzde Thank you for the advice. I fixed my code.

uklotzde · 2023-02-26T13:17:31Z

src/id3/v2/tag.rs

@@ -547,21 +607,54 @@ impl SplitAndMergeTag for ID3v2Tag {
 		fn split_pair(
 			content: &str,
 			tag: &mut Tag,
-			current_key: ItemKey,
+			number_key: ItemKey,
 			total_key: ItemKey,
 		) -> Option<()> {


Using Option<()> for early returns is sometimes handy but also confusing. Better return nothing and use if-let-else statements for early returns.

Please allow me to confirm the details.

split_pair() is called in the match statement as the match guard. if let guards are experimental now.

rust-lang/rust#51114

Sorry, I wanted to referr to let-else statements introduced with Rust 1.65. These should be convenient here and allow to avoid the final return of a meaningless result.

Thank you for details. I will consider to use let-else.

uklotzde · 2023-02-26T13:24:22Z

src/id3/v2/tag.rs

+			let total_source = split.next();
+			let total = total_source.and_then(parse_number);
+
+			let is_valid = if let Some(total_source) = total_source {


This check doesn't work. You will notice if you add tests for leading/trailing whitespace.

Implicitly assuming that the length of NUMBER_PAIR_SEPARATOR equals 1 byte is brittle.

Please allow me to confirm the details.

You will notice if you add tests for leading/trailing whitespace.

What test cases do you think? The followings are passed as a valid content when I added the unit test:

" 1 "

"  1  " (The spaces have 2 characters)

"1  /  2" (The spaces have 2 characters)

"  1  /  2  " (The spaces have 2 characters)

Because content is checked the following:

A not trimmed string has a number only.

One separator character and two not trimmed strings that have a number only.

Implicitly assuming that the length of NUMBER_PAIR_SEPARATOR equals 1 byte is brittle.

May do not NUMBER_PAIR_SEPARATOR equals 1 bytes? I would be glad if you could provide an example.

Otherwise, do you mean that the constant should be extracted?

The magic number number 1 appears out of the context of NUMBER_PAIR_SEPARATOR. If the constant was of type str you could use NUMBER_PAIR_SEPARATOR.len(). But using the size of the separator is probably no longer needed after fixing the parsing. It should not be relevant for the decision, only number and total with their parse results need to be considered.

Returning an Option from parse_number() is probably no sufficient, because you need to distinguish between empty and not a number.

Using the low-level input representation again for making the final decision is conceptually unsound, different levels of abstraction.

Indeed, I overlooked some tests. But I am still unsure that this code covers all cases correctly, especially the expression content.len() == number_source.len() + 1 + total_source.len(). I already mentioned the various reasons why it is not suitable here.

Thank you for details. I understand that I should improve parsing about content. I will consider it.

uklotzde · 2023-02-27T00:16:47Z

src/id3/v2/tag.rs

-					.push(TagItem::new(total_key, ItemValue::Text(total.to_string())))
-			}
+			let number_source = split.next()?;
+			let number = parse_number(number_source)?;


let Some(number) = parse_number(number_source) else { return; };

mikanbako · 2023-02-27T00:17:01Z

@uklotzde Thank you for comments. I replied for them.

uklotzde · 2023-02-27T00:45:26Z

src/id3/v2/tag.rs

+				let number = source.trim();
+
+				if number.is_empty() {
+					return None;


Returning an empty str (= Some(number)) would be an option to distinguish between empty and invalid numbers. There are many possibilities for encoding the 3 variants empty, number, and invalid.

uklotzde · 2023-02-27T11:51:13Z

While trying to get rid of the input string comparisons I came up with a simple and effective solution: mikanbako#1

mikanbako · 2023-02-27T15:00:58Z

@uklotzde Thank you for PR. It is merged.

uklotzde

Thank you for this nice cooperation! The code is very solid now after we have considered many possible use cases. The tests will ensure that everything works as expected and are a good starting point if the behavior ever needs to be modified.

@Serial-ATA Please take a final look and then merge. The recent clippy failures seem to be unrelated and should be fixed separately, not by this PR.

Serial-ATA · 2023-02-28T13:43:44Z

Thanks for fixing this! And thanks @uklotzde for the review. This looks good, and has a lot of tests to back it up 😄.

This makes me realize that the same issue will occur in APE tags as well, which use the same format for track/disk information. I'll have to make an issue for that.

mikanbako force-pushed the fix-id3v2-track-disk-total branch from 2221d2c to e7e5f36 Compare February 23, 2023 05:26

mikanbako changed the title ~~[WIP] ID3v2: Fix about track and disk number / total~~ [WIP] ID3v2: Fix about number pair of track and disk Feb 23, 2023

mikanbako force-pushed the fix-id3v2-track-disk-total branch 2 times, most recently from 14b1404 to f29200d Compare February 23, 2023 06:57

mikanbako marked this pull request as ready for review February 23, 2023 07:02

mikanbako changed the title ~~[WIP] ID3v2: Fix about number pair of track and disk~~ ID3v2: Fix about number pair of track and disk Feb 23, 2023

uklotzde suggested changes Feb 23, 2023

View reviewed changes

mikanbako added 3 commits February 24, 2023 08:40

ID3v2: Merge number pair of track in Accessor

d1f710f

ID3v2: Merge number pair of disk in Accessor

528201c

ID3v2: Merge number pair of track and disk from Tag

2766d20

mikanbako force-pushed the fix-id3v2-track-disk-total branch from f29200d to 438f032 Compare February 24, 2023 00:00

mikanbako force-pushed the fix-id3v2-track-disk-total branch from 438f032 to b623dad Compare February 24, 2023 04:30

mikanbako marked this pull request as draft February 24, 2023 04:34

uklotzde reviewed Feb 24, 2023

View reviewed changes

mikanbako added 2 commits February 25, 2023 00:22

ID3v2: Merge number pair of track and disk while saving tag

1e78325

ID3v2: Change default number from 1 to 0 for track and disk

5a36550

mikanbako force-pushed the fix-id3v2-track-disk-total branch from b623dad to 8dc3166 Compare February 25, 2023 03:24

mikanbako marked this pull request as ready for review February 25, 2023 04:00

uklotzde reviewed Feb 25, 2023

View reviewed changes

mikanbako marked this pull request as draft February 26, 2023 01:04

mikanbako force-pushed the fix-id3v2-track-disk-total branch from 8dc3166 to f85ce9f Compare February 26, 2023 03:47

mikanbako marked this pull request as ready for review February 26, 2023 03:50

uklotzde reviewed Feb 26, 2023

View reviewed changes

ID3v2: Except invalid number pair while parsing ID3v2Tag

a437026

mikanbako force-pushed the fix-id3v2-track-disk-total branch from f85ce9f to a437026 Compare February 26, 2023 12:14

uklotzde reviewed Feb 26, 2023

View reviewed changes

uklotzde reviewed Feb 27, 2023

View reviewed changes

mikanbako marked this pull request as draft February 27, 2023 00:59

ID3v2: Simplify parsing of number/total pairs

6be9131

mikanbako marked this pull request as ready for review February 27, 2023 15:01

uklotzde approved these changes Feb 27, 2023

View reviewed changes

Serial-ATA merged commit b8c972f into Serial-ATA:main Feb 28, 2023

Serial-ATA mentioned this pull request Feb 28, 2023

APE track/disk numbers are not properly converted when writing #159

Closed

ID3v2: Fix about number pair of track and disk #149

ID3v2: Fix about number pair of track and disk #149

Conversation

mikanbako commented Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikanbako commented Feb 23, 2023

uklotzde commented Feb 23, 2023

mikanbako commented Feb 24, 2023

mikanbako commented Feb 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikanbako commented Feb 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikanbako commented Feb 25, 2023

uklotzde commented Feb 25, 2023 • edited Loading

mikanbako commented Feb 26, 2023

mikanbako commented Feb 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikanbako commented Feb 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Feb 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

mikanbako commented Feb 27, 2023

Choose a reason for hiding this comment

uklotzde commented Feb 27, 2023

mikanbako commented Feb 27, 2023

uklotzde left a comment

Choose a reason for hiding this comment

Serial-ATA commented Feb 28, 2023

mikanbako commented Feb 20, 2023 •

edited

Loading

uklotzde commented Feb 25, 2023 •

edited

Loading

uklotzde Feb 26, 2023 •

edited

Loading

uklotzde Feb 27, 2023 •

edited

Loading