diff --git a/src/libs/database/impl/Release.cpp b/src/libs/database/impl/Release.cpp index 9e4acb47..4ab4437a 100644 --- a/src/libs/database/impl/Release.cpp +++ b/src/libs/database/impl/Release.cpp @@ -229,6 +229,23 @@ namespace lms::db return query; } + + template + Wt::Dbo::Query createArtistQuery(Wt::Dbo::Session& session, std::string_view itemToSelect, ReleaseId releaseId, TrackArtistLinkType linkType) + { + auto query{ session.query("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>( - "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>(*session(), "a", getId(), linkType) }; return utils::fetchQueryResults(query); } + std::vector Release::getArtistIds(TrackArtistLinkType linkType) const + { + assert(session()); + + const auto query{ createArtistQuery(*session(), "a.id", getId(), linkType) }; + return utils::fetchQueryResults(query); + } + std::vector Release::getSimilarReleases(std::optional offset, std::optional count) const { assert(session()); diff --git a/src/libs/database/include/database/Release.hpp b/src/libs/database/include/database/Release.hpp index 728cced4..f1fab7cc 100644 --- a/src/libs/database/include/database/Release.hpp +++ b/src/libs/database/include/database/Release.hpp @@ -270,6 +270,7 @@ namespace lms::db // Get the artists of this release std::vector> getArtists(TrackArtistLinkType type = TrackArtistLinkType::Artist) const; + std::vector getArtistIds(TrackArtistLinkType type = TrackArtistLinkType::Artist) const; std::vector> getReleaseArtists() const { return getArtists(TrackArtistLinkType::ReleaseArtist); } bool hasVariousArtists() const; std::vector getSimilarReleases(std::optional offset = {}, std::optional count = {}) const; diff --git a/src/libs/database/test/Release.cpp b/src/libs/database/test/Release.cpp index 408b5ab0..73fa51dd 100644 --- a/src/libs/database/test/Release.cpp +++ b/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" }; diff --git a/src/libs/metadata/impl/Parser.cpp b/src/libs/metadata/impl/Parser.cpp index 2a0ab2ab..0dab9927 100644 --- a/src/libs/metadata/impl/Parser.cpp +++ b/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 artists, const std::optional artistTag, std::span artistsTag, std::span artistTagDelimiters) + std::string computeArtistDisplayName(std::span artists, const std::optional artistTag, std::span artistTagDelimiters) { std::string artistDisplayName; @@ -209,22 +209,12 @@ namespace lms::metadata 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 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(tagReader, TagType::Artist), getTagValuesAs(tagReader, TagType::Artists, {}), _artistTagDelimiters); + 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); @@ -415,7 +405,7 @@ namespace lms::metadata release->name = std::move(*releaseName); release->sortName = getTagValueAs(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(tagReader, TagType::AlbumArtist), getTagValuesAs(tagReader, TagType::AlbumArtists, {}), _artistTagDelimiters); + release->artistDisplayName = computeArtistDisplayName(release->artists, getTagValueAs(tagReader, TagType::AlbumArtist), _artistTagDelimiters); release->mbid = getTagValueAs(tagReader, TagType::MusicBrainzReleaseID); release->groupMBID = getTagValueAs(tagReader, TagType::MusicBrainzReleaseGroupID); release->mediumCount = getTagValueAs(tagReader, TagType::TotalDiscs); diff --git a/src/libs/metadata/test/Parser.cpp b/src/libs/metadata/test/Parser.cpp index 06e101fa..40871d43 100644 --- a/src/libs/metadata/test/Parser.cpp +++ b/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(parser).setArtistTagDelimiters(std::vector{ "; " }); + + std::unique_ptr 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(parser).setArtistTagDelimiters(std::vector{ "; " }); + + std::unique_ptr 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(parser).setArtistTagDelimiters(std::vector{ " & " }); + static_cast(parser).setArtistTagDelimiters(std::vector{ "; " }); std::unique_ptr 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{ { diff --git a/src/lms/ui/Utils.cpp b/src/lms/ui/Utils.cpp index f0e39ec9..f353af0d 100644 --- a/src/lms/ui/Utils.cpp +++ b/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 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() }; res->addNew(Wt::WString::tr("Lms.Explore.various-artists"));