nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-05-03 06:51:16 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(artwork): prefer album-root images over disc-subfolder images for multi-disc albums (#5451)

Browse Source 
Fixed two bugs in album cover art resolution for multi-disc layouts:

1. compareImageFiles now sorts by path depth (shallower first) when basenames
   tie, so album-root images like Artist/Album/cover.jpg are preferred over
   disc-subfolder images like Artist/Album/CD1/cover.jpg.

2. commonParentFolder now includes the parent folder for single-disc-subfolder
   albums, with a Path != "." guard to avoid pulling artist-folder images.

Closes #5376
This commit is contained in:
Deluan Quintão 2026-05-02 19:48:44 -04:00 committed by GitHub
parent ae0e0c89d9
commit a00152397e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 98 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
core/artwork/e2e/album_test.go
View File

@ -39,7 +39,7 @@ var _ = Describe("Album artwork resolution", func() {
// Bug 2 variant: cover.* basenames tie across album-root and per-disc folders;
// compareImageFiles' lexicographic full-path tiebreaker ranks disc-subfolder
// files first. Flip from PIt to It once it prefers shorter/parent paths.
// files first.
When("a multi-disc album has a cover.jpg at the album root and per-disc covers", func() {
// Artist/
// └── Album/
@ -50,7 +50,7 @@ var _ = Describe("Album artwork resolution", func() {
// │ ├── 01 - Track.mp3
// │ └── cover.jpg
// └── cover.jpg ← should win (album-root fallback)
PIt("uses the album-root cover (currently picks a disc subfolder image — bug)", func() {
It("prefers the album-root cover over per-disc covers", func() {
conf.Server.CoverArtPriority = defaultCoverPriority
setLayout(fstest.MapFS{
"Artist/Album/CD1/01 - Track.mp3": trackFile(1, "Track CD1"),
@ -71,7 +71,6 @@ var _ = Describe("Album artwork resolution", func() {
// Bug 2: folder.jpg basenames tie across album-root and per-disc folders;
// the lexicographic full-path tiebreaker in compareImageFiles ranks
// "Artist/Album/CD1/folder.jpg" ahead of "Artist/Album/folder.jpg".
// Flip from PIt to It once compareImageFiles prefers shorter/parent paths.
When("a multi-disc album has folder.jpg at the album root AND in each disc subfolder", func() {
// Artist/
// └── Album/
@ -82,7 +81,7 @@ var _ = Describe("Album artwork resolution", func() {
// │ ├── 01 - Track.mp3
// │ └── folder.jpg
// └── folder.jpg ← should win (album-root fallback)
PIt("uses the album-root folder.jpg (currently picks a disc subfolder image — bug)", func() {
It("prefers the album-root folder.jpg over per-disc folder.jpg", func() {
conf.Server.CoverArtPriority = defaultCoverPriority
setLayout(fstest.MapFS{
"Artist/Album/CD1/01 - Track.mp3": trackFile(1, "Track CD1"),
@ -100,15 +99,14 @@ var _ = Describe("Album artwork resolution", func() {
// Bug 1: commonParentFolder's `len(folders) < 2` guard skips the parent-folder
// lookup whenever an album lives entirely under a single subfolder, so an
// album-root cover is never considered. Flip from PIt to It once the guard
// accepts single-folder albums whose parent isn't already in the folder set.
// album-root cover is never considered.
When("an album lives entirely under a single disc subfolder with cover.jpg at the parent", func() {
// Artist/
// └── Album/
// ├── disc1/
// │ └── 01 - Track.mp3
// └── cover.jpg ← should win (parent-folder fallback, currently ignored — bug)
PIt("uses the parent-folder cover (currently ignored — bug)", func() {
It("uses the parent-folder cover for single-disc-subfolder albums", func() {
conf.Server.CoverArtPriority = defaultCoverPriority
setLayout(fstest.MapFS{
"Artist/Album/disc1/01 - Track.mp3": trackFile(1, "Track"),

48
core/artwork/reader_album.go
View File

@ -118,19 +118,22 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo
folderIDSet[id] = true
}
// For multi-disc albums (2+ folders), check if all folders share a common parent
// that is not already included. This finds cover art in the album root folder
// (e.g., "Artist/Album/cover.jpg" when tracks are in "Artist/Album/CD1/" and "Artist/Album/CD2/").
// We skip single-folder albums to avoid pulling images from the artist folder.
// Check if all folders share a common parent that is not already included.
// This finds cover art in the album root folder (e.g., "Artist/Album/cover.jpg"
// when tracks are in disc subfolders like "Artist/Album/CD1/" and "Artist/Album/CD2/").
// For single-folder albums, the parent is only included when the folder has no
// images of its own (indicating a disc subfolder needing parent artwork).
if commonParentID := commonParentFolder(folders, folderIDSet); commonParentID != "" {
parentFolder, err := ds.Folder(ctx).Get(commonParentID)
if errors.Is(err, model.ErrNotFound) {
log.Warn(ctx, "Parent folder not found for album cover art lookup", "parentID", commonParentID)
} else if err != nil {
return nil, nil, nil, err
}
if parentFolder != nil {
folders = append(folders, *parentFolder)
if len(folders) >= 2 || !anyFolderHasImages(folders) {
parentFolder, err := ds.Folder(ctx).Get(commonParentID)
if errors.Is(err, model.ErrNotFound) {
log.Warn(ctx, "Parent folder not found for album cover art lookup", "parentID", commonParentID)
} else if err != nil {
return nil, nil, nil, err
}
if parentFolder != nil && parentFolder.Path != "." {
folders = append(folders, *parentFolder)
}
}
}
@ -156,10 +159,19 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo
return paths, imgFiles, &updatedAt, nil
}
func anyFolderHasImages(folders []model.Folder) bool {
for _, f := range folders {
if len(f.ImageFiles) > 0 {
return true
}
}
return false
}
// commonParentFolder returns the shared parent folder ID when all folders have the
// same parent and that parent is not already in folderIDSet. Returns "" otherwise.
func commonParentFolder(folders []model.Folder, folderIDSet map[string]bool) string {
if len(folders) < 2 {
if len(folders) == 0 {
return ""
}
parentID := folders[0].ParentID
@ -174,11 +186,8 @@ func commonParentFolder(folders []model.Folder, folderIDSet map[string]bool) str
return parentID
}
// compareImageFiles compares two image file paths for sorting.
// It extracts the base filename (without extension) and compares case-insensitively.
// This ensures that "cover.jpg" sorts before "cover.1.jpg" since "cover" < "cover.1".
// Note: This function is called O(n log n) times during sorting, but in practice albums
// typically have only 1-20 image files, making the repeated string operations negligible.
// compareImageFiles sorts image paths by: base filename (natural order),
// then path depth (shallower first), then full path (stable tiebreaker).
func compareImageFiles(a, b string) int {
// Case-insensitive comparison
a = strings.ToLower(a)
@ -188,9 +197,10 @@ func compareImageFiles(a, b string) int {
baseA := strings.TrimSuffix(path.Base(a), path.Ext(a))
baseB := strings.TrimSuffix(path.Base(b), path.Ext(b))
// Compare base names first, then full paths if equal
// Compare base names first, then prefer shallower paths, then full path as tiebreaker
return cmp.Or(
natural.Compare(baseA, baseB),
cmp.Compare(strings.Count(a, "/"), strings.Count(b, "/")),
natural.Compare(a, b),
)
}

68
core/artwork/reader_album_test.go
View File

@ -213,9 +213,42 @@ var _ = Describe("Album Artwork Reader", func() {
Expect(repo.getCallCount).To(Equal(0))
})
It("does not query parent for single-folder albums", func() {
// A single-folder album's parent is typically the artist folder,
// which should not be searched for cover art
It("does not include top-level parent for multi-folder albums", func() {
// Two album parts under the same artist folder — parent is artist-level
repo.result = []model.Folder{
{
ID: "folder1",
Path: ".",
Name: "AlbumPart1",
ParentID: "artistFolder",
ImagesUpdatedAt: now,
ImageFiles: []string{"cover.jpg"},
},
{
ID: "folder2",
Path: ".",
Name: "AlbumPart2",
ParentID: "artistFolder",
ImagesUpdatedAt: now,
ImageFiles: []string{},
},
}
repo.parentResult = &model.Folder{
ID: "artistFolder",
Path: ".",
Name: "Artist",
ImageFiles: []string{"artist.jpg"},
}
_, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album)
Expect(err).ToNot(HaveOccurred())
Expect(imgFiles).To(HaveLen(1))
Expect(imgFiles[0]).To(Equal("AlbumPart1/cover.jpg"))
Expect(repo.getCallCount).To(Equal(1))
})
It("does not query parent for single-folder albums that already have images", func() {
repo.result = []model.Folder{
{
ID: "folder1",
@ -232,10 +265,37 @@ var _ = Describe("Album Artwork Reader", func() {
Expect(err).ToNot(HaveOccurred())
Expect(imgFiles).To(HaveLen(1))
Expect(imgFiles[0]).To(Equal("Artist/Album/cover.jpg"))
// Get should not have been called (single folder, no parent lookup)
Expect(repo.getCallCount).To(Equal(0))
})
It("includes parent images for single-disc-subfolder albums", func() {
repo.result = []model.Folder{
{
ID: "folder1",
Path: "Artist/Album",
Name: "disc1",
ParentID: "albumFolder",
ImagesUpdatedAt: now,
ImageFiles: []string{},
},
}
repo.parentResult = &model.Folder{
ID: "albumFolder",
Path: "Artist",
Name: "Album",
ImagesUpdatedAt: expectedAt,
ImageFiles: []string{"cover.jpg"},
}
_, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album)
Expect(err).ToNot(HaveOccurred())
Expect(*imagesUpdatedAt).To(Equal(expectedAt))
Expect(imgFiles).To(HaveLen(1))
Expect(imgFiles[0]).To(Equal("Artist/Album/cover.jpg"))
Expect(repo.getCallCount).To(Equal(1))
})
It("propagates non-ErrNotFound errors from parent folder lookup", func() {
repo.result = []model.Folder{
{