From a550529e43cb7fbff224ddd29d3e0083ef04942f Mon Sep 17 00:00:00 2001 From: emeric Date: Tue, 10 Jun 2025 10:20:40 +0200 Subject: [PATCH] Better determinism --- src/libs/database/impl/TrackEmbeddedImage.cpp | 17 +-- src/libs/database/include/database/Types.hpp | 4 +- src/libs/database/test/TrackEmbeddedImage.cpp | 118 +++++++----------- .../services/artwork/impl/ArtworkService.cpp | 53 +++++--- 4 files changed, 91 insertions(+), 101 deletions(-) diff --git a/src/libs/database/impl/TrackEmbeddedImage.cpp b/src/libs/database/impl/TrackEmbeddedImage.cpp index 4aa512d9..9b844b8d 100644 --- a/src/libs/database/impl/TrackEmbeddedImage.cpp +++ b/src/libs/database/impl/TrackEmbeddedImage.cpp @@ -44,8 +44,8 @@ namespace lms::db || params.release.isValid() || params.trackList.isValid() || !params.imageTypes.empty() - || params.sortMethod == TrackEmbeddedImageSortMethod::MediaTypeThenFrontTypeThenSizeDescDesc - || params.sortMethod == TrackEmbeddedImageSortMethod::FrontTypeThenSizeDesc) + || params.sortMethod == TrackEmbeddedImageSortMethod::DiscNumberThenTrackNumberThenSizeDesc + || params.sortMethod == TrackEmbeddedImageSortMethod::TrackNumberThenSizeDesc) { query.join("track_embedded_image_link t_e_i_l ON t_e_i_l.track_embedded_image_id = t_e_i.id"); @@ -72,7 +72,10 @@ namespace lms::db if (params.track.isValid()) query.where("t_e_i_l.track_id = ?").bind(params.track); - if (params.release.isValid() || params.discNumber.has_value()) + if (params.release.isValid() + || params.discNumber.has_value() + || params.sortMethod == TrackEmbeddedImageSortMethod::DiscNumberThenTrackNumberThenSizeDesc + || params.sortMethod == TrackEmbeddedImageSortMethod::TrackNumberThenSizeDesc) { query.join("track t ON t_e_i_l.track_id = t.id"); if (params.release.isValid()) @@ -109,11 +112,11 @@ namespace lms::db case TrackEmbeddedImageSortMethod::SizeDesc: query.orderBy("t_e_i.size DESC"); break; - case TrackEmbeddedImageSortMethod::MediaTypeThenFrontTypeThenSizeDescDesc: - query.orderBy("CASE t_e_i_l.type WHEN ? THEN 1 WHEN ? THEN 2 ELSE 3 END, t_e_i.size DESC").bind(ImageType::Media).bind(ImageType::FrontCover); + case TrackEmbeddedImageSortMethod::DiscNumberThenTrackNumberThenSizeDesc: + query.orderBy("t.disc_number, t.track_number, t_e_i.size DESC"); break; - case TrackEmbeddedImageSortMethod::FrontTypeThenSizeDesc: - query.orderBy("CASE WHEN t_e_i_l.type = ? THEN 0 ELSE 1 END, t_e_i.size DESC").bind(ImageType::FrontCover); + case TrackEmbeddedImageSortMethod::TrackNumberThenSizeDesc: + query.orderBy("t.track_number, t_e_i.size DESC"); break; case TrackEmbeddedImageSortMethod::TrackListIndexAscThenSizeDesc: assert(params.trackList.isValid()); diff --git a/src/libs/database/include/database/Types.hpp b/src/libs/database/include/database/Types.hpp index 1a5d2406..6139f902 100644 --- a/src/libs/database/include/database/Types.hpp +++ b/src/libs/database/include/database/Types.hpp @@ -172,8 +172,8 @@ namespace lms::db { None, SizeDesc, - MediaTypeThenFrontTypeThenSizeDescDesc, - FrontTypeThenSizeDesc, + TrackNumberThenSizeDesc, + DiscNumberThenTrackNumberThenSizeDesc, TrackListIndexAscThenSizeDesc, }; diff --git a/src/libs/database/test/TrackEmbeddedImage.cpp b/src/libs/database/test/TrackEmbeddedImage.cpp index a0149f3d..16169592 100644 --- a/src/libs/database/test/TrackEmbeddedImage.cpp +++ b/src/libs/database/test/TrackEmbeddedImage.cpp @@ -130,54 +130,6 @@ namespace lms::db::tests link.get().modify()->setType(ImageType::FrontCover); } - { - auto transaction{ session.createReadTransaction() }; - - TrackEmbeddedImage::FindParameters params; - params.setSortMethod(TrackEmbeddedImageSortMethod::FrontTypeThenSizeDesc); - - bool visited{}; - TrackEmbeddedImage::find(session, params, [&](const auto&) { visited = true; }); - EXPECT_TRUE(visited); - } - - { - auto transaction{ session.createReadTransaction() }; - - TrackEmbeddedImage::FindParameters params; - params.setImageTypes({ ImageType::FrontCover }); - params.setSortMethod(TrackEmbeddedImageSortMethod::FrontTypeThenSizeDesc); - - bool visited{}; - TrackEmbeddedImage::find(session, params, [&](const auto&) { visited = true; }); - EXPECT_TRUE(visited); - } - - { - auto transaction{ session.createReadTransaction() }; - - TrackEmbeddedImage::FindParameters params; - params.setImageTypes({ ImageType::Media }); - params.setSortMethod(TrackEmbeddedImageSortMethod::FrontTypeThenSizeDesc); - - bool visited{}; - TrackEmbeddedImage::find(session, params, [&](const auto&) { visited = true; }); - EXPECT_FALSE(visited); - } - - { - auto transaction{ session.createReadTransaction() }; - - TrackEmbeddedImage::FindParameters params; - params.setRelease(release.getId()); - params.setImageTypes({ ImageType::Media, ImageType::FrontCover }); - params.setSortMethod(TrackEmbeddedImageSortMethod::MediaTypeThenFrontTypeThenSizeDescDesc); - - bool visited{}; - TrackEmbeddedImage::find(session, params, [&](const auto&) { visited = true; }); - EXPECT_FALSE(visited); - } - { auto transaction{ session.createWriteTransaction() }; track.get().modify()->setRelease(release.get()); @@ -188,7 +140,7 @@ namespace lms::db::tests TrackEmbeddedImage::FindParameters params; params.setRelease(release.getId()); - params.setSortMethod(TrackEmbeddedImageSortMethod::MediaTypeThenFrontTypeThenSizeDescDesc); + params.setSortMethod(TrackEmbeddedImageSortMethod::DiscNumberThenTrackNumberThenSizeDesc); bool visited{}; TrackEmbeddedImage::find(session, params, [&](const auto&) { visited = true; }); @@ -200,7 +152,7 @@ namespace lms::db::tests TrackEmbeddedImage::FindParameters params; params.setTrack(track.getId()); - params.setSortMethod(TrackEmbeddedImageSortMethod::FrontTypeThenSizeDesc); + params.setSortMethod(TrackEmbeddedImageSortMethod::TrackNumberThenSizeDesc); bool visited{}; TrackEmbeddedImage::find(session, params, [&](const auto&) { visited = true; }); @@ -213,32 +165,62 @@ namespace lms::db::tests ScopedTrackEmbeddedImage image1{ session }; ScopedTrackEmbeddedImage image2{ session }; ScopedTrackEmbeddedImage image3{ session }; - ScopedTrack track{ session }; + ScopedTrackEmbeddedImage image4{ session }; + ScopedTrack track1{ session }; + ScopedTrack track2{ session }; ScopedRelease release{ session, "MyRelease" }; - ScopedTrackEmbeddedImageLink link1{ session, track.lockAndGet(), image1.lockAndGet() }; - ScopedTrackEmbeddedImageLink link2{ session, track.lockAndGet(), image2.lockAndGet() }; - ScopedTrackEmbeddedImageLink link3{ session, track.lockAndGet(), image3.lockAndGet() }; + ScopedTrackEmbeddedImageLink link1{ session, track1.lockAndGet(), image1.lockAndGet() }; + ScopedTrackEmbeddedImageLink link2{ session, track1.lockAndGet(), image2.lockAndGet() }; + ScopedTrackEmbeddedImageLink link3{ session, track1.lockAndGet(), image3.lockAndGet() }; + ScopedTrackEmbeddedImageLink link4{ session, track2.lockAndGet(), image4.lockAndGet() }; { auto transaction{ session.createWriteTransaction() }; + track1.get().modify()->setRelease(release.get()); + track1.get().modify()->setTrackNumber(2); + link1.get().modify()->setType(ImageType::FrontCover); image1.get().modify()->setSize(750); link2.get().modify()->setType(ImageType::Media); image2.get().modify()->setSize(1000); link3.get().modify()->setType(ImageType::Media); image3.get().modify()->setSize(2000); + + track2.get().modify()->setRelease(release.get()); + track2.get().modify()->setTrackNumber(1); + + link4.get().modify()->setType(ImageType::Media); + image4.get().modify()->setSize(1500); } { auto transaction{ session.createReadTransaction() }; TrackEmbeddedImage::FindParameters params; - params.setSortMethod(TrackEmbeddedImageSortMethod::FrontTypeThenSizeDesc); + params.setRelease(release.getId()); + params.setImageTypes({ ImageType::Media }); + params.setSortMethod(TrackEmbeddedImageSortMethod::SizeDesc); std::vector visitedIds; TrackEmbeddedImage::find(session, params, [&](const TrackEmbeddedImage::pointer& image) { visitedIds.push_back(image->getId()); }); ASSERT_EQ(visitedIds.size(), 3); - EXPECT_EQ(visitedIds[0], image1.getId()); + EXPECT_EQ(visitedIds[0], image3.getId()); + EXPECT_EQ(visitedIds[1], image4.getId()); + EXPECT_EQ(visitedIds[2], image2.getId()); + } + + { + auto transaction{ session.createReadTransaction() }; + + TrackEmbeddedImage::FindParameters params; + params.setRelease(release.getId()); + params.setImageTypes({ ImageType::Media }); + params.setSortMethod(TrackEmbeddedImageSortMethod::TrackNumberThenSizeDesc); + + std::vector visitedIds; + TrackEmbeddedImage::find(session, params, [&](const TrackEmbeddedImage::pointer& image) { visitedIds.push_back(image->getId()); }); + ASSERT_EQ(visitedIds.size(), 3); + EXPECT_EQ(visitedIds[0], image4.getId()); EXPECT_EQ(visitedIds[1], image3.getId()); EXPECT_EQ(visitedIds[2], image2.getId()); } @@ -247,28 +229,16 @@ namespace lms::db::tests auto transaction{ session.createReadTransaction() }; TrackEmbeddedImage::FindParameters params; - params.setSortMethod(TrackEmbeddedImageSortMethod::MediaTypeThenFrontTypeThenSizeDescDesc); + params.setRelease(release.getId()); + params.setImageTypes({ ImageType::Media }); + params.setSortMethod(TrackEmbeddedImageSortMethod::DiscNumberThenTrackNumberThenSizeDesc); std::vector visitedIds; TrackEmbeddedImage::find(session, params, [&](const TrackEmbeddedImage::pointer& image) { visitedIds.push_back(image->getId()); }); ASSERT_EQ(visitedIds.size(), 3); - EXPECT_EQ(visitedIds[0], image3.getId()); - EXPECT_EQ(visitedIds[1], image2.getId()); - EXPECT_EQ(visitedIds[2], image1.getId()); - } - - { - auto transaction{ session.createReadTransaction() }; - - TrackEmbeddedImage::FindParameters params; - params.setSortMethod(TrackEmbeddedImageSortMethod::SizeDesc); - - std::vector visitedIds; - TrackEmbeddedImage::find(session, params, [&](const TrackEmbeddedImage::pointer& image) { visitedIds.push_back(image->getId()); }); - ASSERT_EQ(visitedIds.size(), 3); - EXPECT_EQ(visitedIds[0], image3.getId()); - EXPECT_EQ(visitedIds[2], image1.getId()); - EXPECT_EQ(visitedIds[1], image2.getId()); + EXPECT_EQ(visitedIds[0], image4.getId()); + EXPECT_EQ(visitedIds[1], image3.getId()); + EXPECT_EQ(visitedIds[2], image2.getId()); } } diff --git a/src/libs/services/artwork/impl/ArtworkService.cpp b/src/libs/services/artwork/impl/ArtworkService.cpp index 1213acc3..0f083634 100644 --- a/src/libs/services/artwork/impl/ArtworkService.cpp +++ b/src/libs/services/artwork/impl/ArtworkService.cpp @@ -181,7 +181,7 @@ namespace lms::artwork if (isImageFound(res)) return res; - // fallback on another track of the same disc + // Fallback on another track of the same disc const db::Track::pointer track{ db::Track::find(session, trackId) }; if (!track) return res; @@ -195,7 +195,7 @@ namespace lms::artwork params.setRelease(releaseId); params.setDiscNumber(track->getDiscNumber()); params.setImageTypes({ db::ImageType::Media }); - params.setSortMethod(db::TrackEmbeddedImageSortMethod::SizeDesc); + params.setSortMethod(db::TrackEmbeddedImageSortMethod::TrackNumberThenSizeDesc); params.setRange(db::Range{ .offset = 0, .size = 1 }); db::TrackEmbeddedImage::find(session, params, [&](const db::TrackEmbeddedImage::pointer& image) { res = image->getId(); }); } @@ -267,7 +267,7 @@ namespace lms::artwork params.setRelease(releaseId); params.setDiscNumber(track->getDiscNumber()); params.setImageTypes({ db::ImageType::Media }); - params.setSortMethod(db::TrackEmbeddedImageSortMethod::SizeDesc); + params.setSortMethod(db::TrackEmbeddedImageSortMethod::TrackNumberThenSizeDesc); params.setRange(db::Range{ .offset = 0, .size = 1 }); db::TrackEmbeddedImage::find(session, params, [&](const db::TrackEmbeddedImage::pointer& image) { res = image->getId(); }); } @@ -282,22 +282,39 @@ namespace lms::artwork auto transaction{ session.createReadTransaction() }; ImageFindResult res; - if (const db::Release::pointer release{ db::Release::find(session, releaseId) }) - { - if (const db::ImageId imageId{ release->getImageId() }; imageId.isValid()) - { - res = imageId; - } - else - { - db::TrackEmbeddedImage::FindParameters params; - params.setRelease(releaseId); - params.setImageTypes({ db::ImageType::FrontCover, db::ImageType::Media, db::ImageType::Unknown /* give unknown a chance */ }); - params.setSortMethod(db::TrackEmbeddedImageSortMethod::FrontTypeThenSizeDesc); - params.setRange(db::Range{ .offset = 0, .size = 1 }); + const db::Release::pointer release{ db::Release::find(session, releaseId) }; + if (!release) + return res; - db::TrackEmbeddedImage::find(session, params, [&](const db::TrackEmbeddedImage::pointer& image) { res = image->getId(); }); - } + if (const db::ImageId imageId{ release->getImageId() }; imageId.isValid()) + res = imageId; + + if (isImageFound(res)) + return res; + + // Fallback on embedded Front image + { + db::TrackEmbeddedImage::FindParameters params; + params.setRelease(releaseId); + params.setImageTypes({ db::ImageType::FrontCover }); + params.setSortMethod(db::TrackEmbeddedImageSortMethod::DiscNumberThenTrackNumberThenSizeDesc); + params.setRange(db::Range{ .offset = 0, .size = 1 }); + + db::TrackEmbeddedImage::find(session, params, [&](const db::TrackEmbeddedImage::pointer& image) { res = image->getId(); }); + } + + if (isImageFound(res)) + return res; + + // Fallback on embedded media image + { + db::TrackEmbeddedImage::FindParameters params; + params.setRelease(releaseId); + params.setImageTypes({ db::ImageType::Media }); + params.setSortMethod(db::TrackEmbeddedImageSortMethod::DiscNumberThenTrackNumberThenSizeDesc); + params.setRange(db::Range{ .offset = 0, .size = 1 }); + + db::TrackEmbeddedImage::find(session, params, [&](const db::TrackEmbeddedImage::pointer& image) { res = image->getId(); }); } return res;