From 6d19452aae311af2da33f0b94e3cbf2e3c3913fb Mon Sep 17 00:00:00 2001 From: emeric Date: Sun, 15 Sep 2024 13:45:43 +0200 Subject: [PATCH] Centralized artist display name reconstruction (fixed some albumartist isues), ref #491 --- src/libs/metadata/impl/Parser.cpp | 42 ++++++++++++--------- src/libs/metadata/test/Parser.cpp | 63 ++++++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 20 deletions(-) diff --git a/src/libs/metadata/impl/Parser.cpp b/src/libs/metadata/impl/Parser.cpp index 381ab592..a6657c57 100644 --- a/src/libs/metadata/impl/Parser.cpp +++ b/src/libs/metadata/impl/Parser.cpp @@ -197,6 +197,27 @@ namespace lms::metadata { return std::any_of(std::cbegin(subStrs), std::cend(subStrs), [&str](const std::string& subStr) { return str.find(subStr) != std::string_view::npos; }); } + + std::string computeArtistDisplayName(std::span artists, const std::optional artistTag, std::span artistTagDelimiters) + { + if (artists.size() == 1) + return artists.front().name; + else if (artists.size() > 1) + { + std::vector artistNames; + std::transform(std::cbegin(artists), std::cend(artists), std::back_inserter(artistNames), [](const Artist& artist) -> std::string_view { return artist.name; }); + + // Picard use case: if we manage to match all artists in the "artist" tag (considered single-valued), and if it does not contain any custom artist delimiter, we use it as the display name + // Otherwise, we reconstruct the string using a standard, hardcoded, join + if (artistTag && !strIsContainingAny(*artistTag, artistTagDelimiters) && strIsMatchingArtistNames(*artistTag, artistNames)) + return *artistTag; + else + return core::stringUtils::joinStrings(artistNames, ", "); + } + + return ""; + } + } // namespace std::unique_ptr createParser(ParserBackend parserBackend, ParserReadStyle parserReadStyle) @@ -318,22 +339,7 @@ namespace lms::metadata track.medium = getMedium(tagReader); track.artists = getArtists(tagReader, { TagType::Artists, TagType::Artist }, { TagType::ArtistSortOrder }, { TagType::MusicBrainzArtistID }, _artistTagDelimiters, _defaultTagDelimiters); - - if (track.artists.size() == 1) - track.artistDisplayName = track.artists.front().name; - else if (track.artists.size() > 1) - { - std::vector artistNames; - std::transform(std::cbegin(track.artists), std::cend(track.artists), std::back_inserter(artistNames), [](const Artist& artist) -> std::string_view { return artist.name; }); - - // Picard use case: if we manage to match all artists in the "artist" tag (considered single-valued), and if it does not contain any custom artist delimiter, we use it as the display name - // Otherwise, we reconstruct the string using a standard, hardcoded, join - const std::optional artistTag{ getTagValueAs(tagReader, TagType::Artist) }; - if (artistTag && !strIsContainingAny(*artistTag, _artistTagDelimiters) && strIsMatchingArtistNames(*artistTag, artistNames)) - track.artistDisplayName = *artistTag; - else - track.artistDisplayName = core::stringUtils::joinStrings(artistNames, ", "); - } + track.artistDisplayName = computeArtistDisplayName(track.artists, getTagValueAs(tagReader, TagType::Artist), _artistTagDelimiters); track.conductorArtists = getArtists(tagReader, { TagType::Conductors, TagType::Conductor }, { TagType::ConductorsSortOrder, TagType::ConductorSortOrder }, {}, _artistTagDelimiters, _defaultTagDelimiters); track.composerArtists = getArtists(tagReader, { TagType::Composers, TagType::Composer }, { TagType::ComposersSortOrder, TagType::ComposerSortOrder }, {}, _artistTagDelimiters, _defaultTagDelimiters); @@ -393,10 +399,10 @@ namespace lms::metadata release.emplace(); release->name = std::move(*releaseName); release->sortName = getTagValueAs(tagReader, TagType::AlbumSortOrder).value_or(""); - release->artistDisplayName = getTagValueAs(tagReader, TagType::AlbumArtist).value_or(""); // TODO try to join albumartists if present + release->artists = getArtists(tagReader, { TagType::AlbumArtists, TagType::AlbumArtist }, { TagType::AlbumArtistsSortOrder, TagType::AlbumArtistSortOrder }, { TagType::MusicBrainzReleaseArtistID }, _artistTagDelimiters, _defaultTagDelimiters); + release->artistDisplayName = computeArtistDisplayName(release->artists, getTagValueAs(tagReader, TagType::AlbumArtist), _artistTagDelimiters); release->mbid = getTagValueAs(tagReader, TagType::MusicBrainzReleaseID); release->groupMBID = getTagValueAs(tagReader, TagType::MusicBrainzReleaseGroupID); - release->artists = getArtists(tagReader, { TagType::AlbumArtists, TagType::AlbumArtist }, { TagType::AlbumArtistsSortOrder, TagType::AlbumArtistSortOrder }, { TagType::MusicBrainzReleaseArtistID }, _artistTagDelimiters, _defaultTagDelimiters); release->mediumCount = getTagValueAs(tagReader, TagType::TotalDiscs); release->isCompilation = getTagValueAs(tagReader, TagType::Compilation).value_or(false); release->labels = getTagValuesAs(tagReader, TagType::RecordLabel, _defaultTagDelimiters); diff --git a/src/libs/metadata/test/Parser.cpp b/src/libs/metadata/test/Parser.cpp index 4b294002..fd8ff770 100644 --- a/src/libs/metadata/test/Parser.cpp +++ b/src/libs/metadata/test/Parser.cpp @@ -270,7 +270,7 @@ namespace lms::metadata EXPECT_EQ(track->medium->release->name, "MyAlbum"); EXPECT_EQ(track->medium->release->artists[0].name, "AlbumArtist1"); EXPECT_EQ(track->medium->release->artists[1].name, "AlbumArtist2"); - EXPECT_EQ(track->medium->release->artistDisplayName, "AlbumArtist1 / AlbumArtist2"); // TODO: reconstruct artist display name since a custom delimiter is hit + EXPECT_EQ(track->medium->release->artistDisplayName, "AlbumArtist1, AlbumArtist2"); } TEST(Parser, noArtistInArtist) @@ -287,7 +287,7 @@ namespace lms::metadata EXPECT_EQ(track->artistDisplayName, ""); } - TEST(Parser, singleArtistInArtist) + TEST(Parser, singleArtistInArtists) { const TestTagReader testTags{ { @@ -337,6 +337,65 @@ namespace lms::metadata EXPECT_EQ(track->artistDisplayName, "Artist1, Artist2"); // reconstruct artist display name since multiple entries are found and nothing is set in artist } + TEST(Parser, singleArtistInAlbumArtists) + { + const TestTagReader testTags{ + { + // nothing in AlbumArtist! + { TagType::Album, { "MyAlbum" } }, + { TagType::AlbumArtists, { "Artist1" } }, + } + }; + + std::unique_ptr track{ Parser{}.parse(testTags) }; + + ASSERT_TRUE(track->medium); + ASSERT_TRUE(track->medium->release); + ASSERT_EQ(track->medium->release->artists.size(), 1); + EXPECT_EQ(track->medium->release->artists[0].name, "Artist1"); + EXPECT_EQ(track->medium->release->artistDisplayName, "Artist1"); + } + + TEST(Parser, multipleArtistsInAlbumArtist) + { + const TestTagReader testTags{ + { + // nothing in AlbumArtists! + { TagType::Album, { "MyAlbum" } }, + { TagType::AlbumArtist, { "Artist1", "Artist2" } }, + } + }; + + std::unique_ptr track{ Parser{}.parse(testTags) }; + + ASSERT_TRUE(track->medium); + ASSERT_TRUE(track->medium->release); + ASSERT_EQ(track->medium->release->artists.size(), 2); + EXPECT_EQ(track->medium->release->artists[0].name, "Artist1"); + EXPECT_EQ(track->medium->release->artists[1].name, "Artist2"); + EXPECT_EQ(track->medium->release->artistDisplayName, "Artist1, Artist2"); // reconstruct artist display name since multiple entries are found + } + + TEST(Parser, multipleArtistsInAlbumArtists) + { + const TestTagReader testTags{ + { + // nothing in AlbumArtist! + { TagType::Album, { "MyAlbum" } }, + { TagType::AlbumArtists, { "Artist1", "Artist2" } }, + } + }; + + std::unique_ptr track{ Parser{}.parse(testTags) }; + + ASSERT_TRUE(track->medium); + ASSERT_TRUE(track->medium->release); + ASSERT_EQ(track->medium->release->artists.size(), 2); + EXPECT_EQ(track->medium->release->artists[0].name, "Artist1"); + EXPECT_EQ(track->medium->release->artists[1].name, "Artist2"); + EXPECT_EQ(track->medium->release->artistDisplayName, "Artist1, Artist2"); // reconstruct artist display name since multiple entries are found and nothing is set in artist + } + TEST(Parser, multipleArtistsInArtistsButNotAllMBIDs) { const TestTagReader testTags{