APEv2 multivalued-tag doesn't parse correctly

[Linxuanhua1]

Reproducer

I tried this code:

    let source_path = path::PathBuf::from("./tests/encode_test_audio/ape_source.ape");


    let source_file = read_from_path(source_path).unwrap();

    let source_tag = source_file.first_tag().ok_or("源文件没有标签").unwrap();;

    for tag in source_tag.items(){
        println!("{:?}, {:?}", tag.key(), tag.value());
    }
<code>

this is the output：

TrackArtist, Text("むにょっ\0秋山裕和")

According to the Apev2 specification, multi-valued tags use \x00 as the spiltter
I think we should pass tags like this:

TrackArtist, Text("むにょっ")
TrackArtist, Text("秋山裕和")

Summary

As Above

Expected behavior

No response

Assets

No response

[Linxuanhua1]

I looked at part of the source code. I think it may be better.

Change the type of all input values for standard tags to vec like set_title.

Remove the insert and push methods, add a new set method to all metadata types. It takes two args, key and value. The type of value use vec to better support mutlivalued-tag.

The set method will delete original value and write the input value.

Modify the internal storage way for all metadata types to merge multiple keys as one and multiple value into a single vec.

With the current solution, using insert does not allow multiple values to be written to a tag, but using push does not indicate whether duplicate values have been written. This solution resolves the aforementioned issues, as the user has full control over the values being written.

What's you opinion? We could have a discussion and I could help you change the code.

[Serial-ATA]

Interesting, didn't know about the multi-value convention.

Remove the insert and push methods, add a new set method to all metadata types. It takes two args, key and value. The type of value use vec to better support mutlivalued-tag.

No need to do anything that extreme, push can append a value to an existing item and insert can replace any existing item then insert. That's the same behavior as other formats like VorbisComments.

[Serial-ATA]

Change the type of all input values for standard tags to vec like set_title.

Also, check out the docs for Accessor: https://docs.rs/lofty/latest/lofty/tag/trait.Accessor.html
It intentionally doesn't support multiple values. You should be using Tag::push() instead.

[Linxuanhua1]

Change the type of all input values for standard tags to vec like set_title.

Also, check out the docs for Accessor: https://docs.rs/lofty/latest/lofty/tag/trait.Accessor.html It intentionally doesn't support multiple values. You should be using Tag::push() instead.

Let me make it more clearly. The idea I suggested is unrelated to this bug. It's an inconvenience I encountered while writing the code.Maybe later I will oepn an enhancement issue.

We now have a multivalued artist tag as shown below that needs to be written.
["小倉朝陽", 遥うみ", 八日堂朔莉", 坂リリ", 大蔵瑠美音", 遥そら", 桜小路亜十礼", "川崎美鳥"]
Under the current model, this is the only way we can write it.

let artists = vec!["小倉朝陽", "遥うみ", "八日堂朔莉", "坂リリ", "大蔵瑠美音", "遥そら", "桜小路亜十礼", "川崎美鳥"];

for artist in artists {
    source_tag.push("TrackArtist".to_string(), artist.to_string());
}

If we want to know if the original tag and the written tag are duplicates, we need to read them first and then make the judgment.

let artists = vec!["小倉朝陽", "遥うみ", "八日堂朔莉", "坂リリ", "大蔵瑠美音", "遥そら", "桜小路亜十礼", "川崎美鳥"];
let original_artists = source_tag.get_all("TrackArtist");
    
for original_artist in original_artists {
    if artists.contains(&original_artist) {
        ...
    }
}

If we change the input value into vec, we could parse the value like this. And the set mode is to overwrite, you will not care about whether the value is in original tag.

let artists = vec!["小倉朝陽", "遥うみ", "八日堂朔莉", "坂リリ", "大蔵瑠美音", "遥そら", "桜小路亜十礼", "川崎美鳥"];
source_tag.set_trackartist(&artists);

or

source_tag.set("TrackArtist".to_string(), &artists);

Moreover, there's the need to distinguish between single and multiple values, because even a single value can be passed into the vec, as is shown below.

let artists = vec!["小倉朝陽"];
source_tag.set_trackartist(&artists);

This change also allows for a unified external interface.

So I think that changing the storage way of tag in crate and the method is meaningful.

What do you think?

[Serial-ATA]

If we want to know if the original tag and the written tag are duplicates, we need to read them first and then make the judgment.

If you're just setting a value and don't care about the existing ones, you can do:

let artists = vec!["小倉朝陽", "遥うみ", "八日堂朔莉", "坂リリ", "大蔵瑠美音", "遥そら", "桜小路亜十礼", "川崎美鳥"];

source_tag.remove_key(ItemKey::TrackArtist);
for artist in artists {
    source_tag.push(TagItem::new(ItemKey::TrackArtist, ItemValue::Text(artist.to_string())));
}

Or if you use ApeTag directly:

let artists = vec!["小倉朝陽", "遥うみ", "八日堂朔莉", "坂リリ", "大蔵瑠美音", "遥そら", "桜小路亜十礼", "川崎美鳥"];

source_tag.remove("Artist");
for artist in artists {
    source_tag.push(ApeItem::new("Artist".to_string(), ItemValue::Text(artist.to_string()))?);
}

So I think that changing the storage way of tag in crate and the method is meaningful.

Accessor is intended to be very simple so that it maps well to every supported tag format. That means no multi-value support and very limited field support. If you need anything extra, you'll have to start working with ItemKeys.

[Linxuanhua1]

If we want to know if the original tag and the written tag are duplicates, we need to read them first and then make the judgment.

If you're just setting a value and don't care about the existing ones, you can do:

let artists = vec!["小倉朝陽", "遥うみ", "八日堂朔莉", "坂リリ", "大蔵瑠美音", "遥そら", "桜小路亜十礼", "川崎美鳥"];

source_tag.remove_key(ItemKey::TrackArtist);
for artist in artists {
source_tag.push(TagItem::new(ItemKey::TrackArtist, ItemValue::Text(artist.to_string())));
}
Or if you use ApeTag directly:

let artists = vec!["小倉朝陽", "遥うみ", "八日堂朔莉", "坂リリ", "大蔵瑠美音", "遥そら", "桜小路亜十礼", "川崎美鳥"];

source_tag.remove("Artist");
for artist in artists {
source_tag.push(ApeItem::new("Artist".to_string(), ItemValue::Text(artist.to_string()))?);
}

So I think that changing the storage way of tag in crate and the method is meaningful.

Accessor is intended to be very simple so that it maps well to every supported tag format. That means no multi-value support and very limited field support. If you need anything extra, you'll have to start working with ItemKeys.

No problem, I will have a try. Once I finished the work, I will make a pr.