Fixed some bad artist display name computations

src/libs/database/impl/Release.cpp

@@ -229,6 +229,23 @@ namespace lms::db
 
             return query;
         }
+
+        template<typename ResultType>
+        Wt::Dbo::Query<ResultType> createArtistQuery(Wt::Dbo::Session& session, std::string_view itemToSelect, ReleaseId releaseId, TrackArtistLinkType linkType)
+        {
+            auto query{ session.query<ResultType>("SELECT " + std::string{ itemToSelect } + " from artist a")
+                            .join("track_artist_link t_a_l ON t_a_l.artist_id = a.id")
+                            .join("track t ON t.id = t_a_l.track_id")
+                            .where("t.release_id = ?")
+                            .bind(releaseId)
+                            .where("+t_a_l.type = ?")
+                            .bind(linkType) // adding + since the query planner does not a good job when analyze is not performed
+                            .groupBy("a.id")
+                            .orderBy("t_a_l.id") };
+
+            return query;
+        };
+
     } // namespace
 
     Label::Label(std::string_view name)
@@ -530,19 +547,18 @@ namespace lms::db
     {
         assert(session());
 
-        const auto query{ session()->query<Wt::Dbo::ptr<Artist>>(
-                                       "SELECT a FROM artist a"
-                                       " INNER JOIN track_artist_link t_a_l ON t_a_l.artist_id = a.id"
-                                       " INNER JOIN track t ON t.id = t_a_l.track_id")
-                              .where("t.release_id = ?")
-                              .bind(getId())
-                              .where("+t_a_l.type = ?")
-                              .bind(linkType) // adding + since the query planner does not a good job when analyze is not performed
-                              .groupBy("a.id") };
-
+        const auto query{ createArtistQuery<Wt::Dbo::ptr<Artist>>(*session(), "a", getId(), linkType) };
         return utils::fetchQueryResults<Artist::pointer>(query);
     }
 
+    std::vector<ArtistId> Release::getArtistIds(TrackArtistLinkType linkType) const
+    {
+        assert(session());
+
+        const auto query{ createArtistQuery<ArtistId>(*session(), "a.id", getId(), linkType) };
+        return utils::fetchQueryResults(query);
+    }
+
     std::vector<Release::pointer> Release::getSimilarReleases(std::optional<std::size_t> offset, std::optional<std::size_t> count) const
     {
         assert(session());

src/libs/database/include/database/Release.hpp

@@ -270,6 +270,7 @@ namespace lms::db
 
         // Get the artists of this release
         std::vector<ObjectPtr<Artist>> getArtists(TrackArtistLinkType type = TrackArtistLinkType::Artist) const;
+        std::vector<ArtistId> getArtistIds(TrackArtistLinkType type = TrackArtistLinkType::Artist) const;
         std::vector<ObjectPtr<Artist>> getReleaseArtists() const { return getArtists(TrackArtistLinkType::ReleaseArtist); }
         bool hasVariousArtists() const;
         std::vector<pointer> getSimilarReleases(std::optional<std::size_t> offset = {}, std::optional<std::size_t> count = {}) const;

src/libs/database/test/Release.cpp

@@ -670,6 +670,58 @@ namespace lms::db::tests
         }
     }
 
+    TEST_F(DatabaseFixture, Release_releaseArtist)
+    {
+        ScopedRelease release{ session, "MyRelease" };
+        ScopedTrack track{ session };
+        ScopedArtist artist{ session, "MyArtist" };
+
+        {
+            auto transaction{ session.createReadTransaction() };
+
+            const auto releases{ Release::findIds(session, Release::FindParameters{}.setArtist(artist.getId(), { TrackArtistLinkType::ReleaseArtist })) };
+            EXPECT_EQ(releases.results.size(), 0);
+            EXPECT_EQ(Release::getCount(session, Release::FindParameters{}.setArtist(artist.getId(), { TrackArtistLinkType::ReleaseArtist })), 0);
+            EXPECT_EQ(Release::getCount(session, Release::FindParameters{}.setArtist(artist.getId())), 0);
+            EXPECT_EQ(release->getArtists(TrackArtistLinkType::ReleaseArtist).size(), 0);
+            EXPECT_EQ(release->getArtistIds(TrackArtistLinkType::ReleaseArtist).size(), 0);
+        }
+
+        {
+            auto transaction{ session.createWriteTransaction() };
+            track.get().modify()->setRelease(release.get());
+            TrackArtistLink::create(session, track.get(), artist.get(), TrackArtistLinkType::ReleaseArtist);
+        }
+
+        {
+            auto transaction{ session.createReadTransaction() };
+
+            auto artists{ release->getArtists(TrackArtistLinkType::ReleaseArtist) };
+            ASSERT_EQ(artists.size(), 1);
+            EXPECT_EQ(artists.front()->getId(), artist.getId());
+        }
+
+        {
+            auto transaction{ session.createReadTransaction() };
+
+            auto artists{ release->getArtistIds(TrackArtistLinkType::ReleaseArtist) };
+            ASSERT_EQ(artists.size(), 1);
+            EXPECT_EQ(artists.front(), artist.getId());
+        }
+
+        {
+            auto transaction{ session.createReadTransaction() };
+
+            EXPECT_EQ(Release::getCount(session, Release::FindParameters{}), 1);
+
+            const auto releases{ Release::findIds(session, Release::FindParameters{}.setArtist(artist.getId(), { TrackArtistLinkType::ReleaseArtist })) };
+            ASSERT_EQ(releases.results.size(), 1);
+            EXPECT_EQ(releases.results.front(), release.getId());
+            EXPECT_EQ(Release::getCount(session, Release::FindParameters{}.setArtist(artist.getId(), { TrackArtistLinkType::ReleaseArtist })), 1);
+            EXPECT_EQ(Release::getCount(session, Release::FindParameters{}.setArtist(artist.getId())), 1);
+        }
+    }
+
     TEST_F(DatabaseFixture, Release_getDiscCount)
     {
         ScopedRelease release{ session, "MyRelease" };

src/libs/metadata/impl/Parser.cpp

@@ -198,7 +198,7 @@ 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<const Artist> artists, const std::optional<std::string> artistTag, std::span<const std::string> artistsTag, std::span<const std::string> artistTagDelimiters)
+        std::string computeArtistDisplayName(std::span<const Artist> artists, const std::optional<std::string> artistTag, std::span<const std::string> artistTagDelimiters)
         {
             std::string artistDisplayName;
 
@@ -209,22 +209,12 @@ namespace lms::metadata
                 std::vector<std::string_view> 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 no custom delimiter was used, we use it as the display name
+                // Picard use case: if we manage to match all artists in the "artist" tag (considered single-valued), and if no custom delimiter is hit, we use it as the display name
                 // Otherwise, we reconstruct the string using a standard, hardcoded, join
                 if (artistTag && strIsMatchingArtistNames(*artistTag, artistNames))
                 {
-                    if (artistsTag.size() == artists.size())
-                    {
-                        // artists was used
-                        if (std::none_of(std::begin(artistsTag), std::cend(artistsTag), [&](std::string_view tag) { return strIsContainingAny(tag, artistTagDelimiters); }))
-                            artistDisplayName = *artistTag;
-                    }
-                    else
-                    {
-                        // artist was used
-                        if (!strIsContainingAny(*artistTag, artistTagDelimiters))
-                            artistDisplayName = *artistTag;
-                    }
+                    if (!strIsContainingAny(*artistTag, artistTagDelimiters))
+                        artistDisplayName = *artistTag;
                 }
 
                 if (artistDisplayName.empty())
@@ -354,7 +344,7 @@ namespace lms::metadata
 
         track.medium = getMedium(tagReader);
         track.artists = getArtists(tagReader, { TagType::Artists, TagType::Artist }, { TagType::ArtistSortOrder }, { TagType::MusicBrainzArtistID }, _artistTagDelimiters, _defaultTagDelimiters);
-        track.artistDisplayName = computeArtistDisplayName(track.artists, getTagValueAs<std::string>(tagReader, TagType::Artist), getTagValuesAs<std::string>(tagReader, TagType::Artists, {}), _artistTagDelimiters);
+        track.artistDisplayName = computeArtistDisplayName(track.artists, getTagValueAs<std::string>(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);
@@ -415,7 +405,7 @@ namespace lms::metadata
         release->name = std::move(*releaseName);
         release->sortName = getTagValueAs<std::string>(tagReader, TagType::AlbumSortOrder).value_or("");
         release->artists = getArtists(tagReader, { TagType::AlbumArtists, TagType::AlbumArtist }, { TagType::AlbumArtistsSortOrder, TagType::AlbumArtistSortOrder }, { TagType::MusicBrainzReleaseArtistID }, _artistTagDelimiters, _defaultTagDelimiters);
-        release->artistDisplayName = computeArtistDisplayName(release->artists, getTagValueAs<std::string>(tagReader, TagType::AlbumArtist), getTagValuesAs<std::string>(tagReader, TagType::AlbumArtists, {}), _artistTagDelimiters);
+        release->artistDisplayName = computeArtistDisplayName(release->artists, getTagValueAs<std::string>(tagReader, TagType::AlbumArtist), _artistTagDelimiters);
         release->mbid = getTagValueAs<core::UUID>(tagReader, TagType::MusicBrainzReleaseID);
         release->groupMBID = getTagValueAs<core::UUID>(tagReader, TagType::MusicBrainzReleaseGroupID);
         release->mediumCount = getTagValueAs<std::size_t>(tagReader, TagType::TotalDiscs);

src/libs/metadata/test/Parser.cpp

@@ -273,7 +273,47 @@ namespace lms::metadata
         EXPECT_EQ(track->medium->release->artistDisplayName, "AlbumArtist1, AlbumArtist2");
     }
 
-    TEST(Parser, customDelimitersNotForDisplayString)
+    TEST(Parser, customDelimiters_foundInArtist)
+    {
+        const TestTagReader testTags{
+            {
+                { TagType::Artist, { "Artist1; Artist2" } },
+                { TagType::Artists, { "Artist1", "Artist2" } },
+            }
+        };
+
+        Parser parser;
+        static_cast<IParser&>(parser).setArtistTagDelimiters(std::vector<std::string>{ "; " });
+
+        std::unique_ptr<Track> track{ parser.parse(testTags) };
+
+        ASSERT_EQ(track->artists.size(), 2);
+        EXPECT_EQ(track->artists[0].name, "Artist1");
+        EXPECT_EQ(track->artists[1].name, "Artist2");
+        EXPECT_EQ(track->artistDisplayName, "Artist1, Artist2"); // reconstruct the display name since we hit a custom delimiter in Artist
+    }
+
+    TEST(Parser, customDelimiters_foundInArtists)
+    {
+        const TestTagReader testTags{
+            {
+                { TagType::Artist, { "Artist1 feat. Artist2" } },
+                { TagType::Artists, { "Artist1; Artist2" } },
+            }
+        };
+
+        Parser parser;
+        static_cast<IParser&>(parser).setArtistTagDelimiters(std::vector<std::string>{ "; " });
+
+        std::unique_ptr<Track> track{ parser.parse(testTags) };
+
+        ASSERT_EQ(track->artists.size(), 2);
+        EXPECT_EQ(track->artists[0].name, "Artist1");
+        EXPECT_EQ(track->artists[1].name, "Artist2");
+        EXPECT_EQ(track->artistDisplayName, "Artist1 feat. Artist2");
+    }
+
+    TEST(Parser, customDelimiters_notUsed)
     {
         const TestTagReader testTags{
             {
@@ -283,7 +323,7 @@ namespace lms::metadata
         };
 
         Parser parser;
-        static_cast<IParser&>(parser).setArtistTagDelimiters(std::vector<std::string>{ " & " });
+        static_cast<IParser&>(parser).setArtistTagDelimiters(std::vector<std::string>{ "; " });
 
         std::unique_ptr<Track> track{ parser.parse(testTags) };
 
@@ -293,7 +333,7 @@ namespace lms::metadata
         EXPECT_EQ(track->artistDisplayName, "Artist1 & Artist2");
     }
 
-    TEST(Parser, customDelimitersUsedForArtist)
+    TEST(Parser, customDelimiters_onlyInArtist)
     {
         const TestTagReader testTags{
             {

src/lms/ui/Utils.cpp

@@ -239,29 +239,24 @@ namespace lms::ui::utils
     {
         using namespace db;
 
-        Artist::FindParameters params;
-        params.setRelease(release->getId());
-        params.setLinkType(TrackArtistLinkType::ReleaseArtist);
-
-        if (const auto releaseArtists{ Artist::findIds(LmsApp->getDbSession(), params) }; !releaseArtists.results.empty())
+        if (const std::vector<ArtistId> releaseArtists{ release->getArtistIds(TrackArtistLinkType::ReleaseArtist) }; !releaseArtists.empty())
         {
-            if (releaseArtists.results.size() == 1 && releaseArtists.results.front() == omitIfMatchThisArtist)
+            if (releaseArtists.size() == 1 && releaseArtists.front() == omitIfMatchThisArtist)
                 return {};
 
-            return createArtistDisplayNameWithAnchors(release->getArtistDisplayName(), releaseArtists.results, cssAnchorClass);
+            return createArtistDisplayNameWithAnchors(release->getArtistDisplayName(), releaseArtists, cssAnchorClass);
         }
 
-        params.setLinkType(TrackArtistLinkType::Artist);
-        const auto artists{ Artist::findIds(LmsApp->getDbSession(), params) };
-        if (artists.results.size() == 1)
+        const auto artists{ release->getArtistIds(TrackArtistLinkType::Artist) };
+        if (artists.size() == 1)
         {
-            if (artists.results.front() == omitIfMatchThisArtist)
+            if (artists.front() == omitIfMatchThisArtist)
                 return {};
 
-            return createArtistAnchorList({ artists.results.front() }, cssAnchorClass);
+            return createArtistAnchorList({ artists.front() }, cssAnchorClass);
         }
 
-        if (artists.results.size() > 1)
+        if (artists.size() > 1)
         {
             auto res{ std::make_unique<Wt::WContainerWidget>() };
             res->addNew<Wt::WText>(Wt::WString::tr("Lms.Explore.various-artists"));