mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
fix(artwork): resolve artist folder for albums with disc subfolders
Dropping the promoted album root from the artist reader's paths fixed the flat-album case but broke albums whose tracks live in disc subfolders: the promoted root was what kept the byte-wise longest common prefix on a directory boundary. Without it, CD1/CD2 siblings share the fragment "Album/CD", so the artist folder resolved one level too deep and album/artist.* won over artist.*. loadAlbumFoldersPaths now collapses each album to its own root before the paths are compared across albums, so a disc-split album contributes its album folder rather than each disc folder. Folders no album claims (the promoted parent) are still returned unchanged, keeping the album, disc and mediafile readers unaffected. The prefix math moves into commonDir, which appends a trailing separator so the comparison lands on segment boundaries - this also fixes sibling folders sharing a name prefix (Album/Album2). Adds e2e coverage for the case the branch fixes (artist.* with no album/ fallback, which failed before this branch), the disc-subfolder regression, and a multi-album guard that pins the scope boundary.
This commit is contained in:
parent
a9a5372399
commit
89389fa21f
@ -80,6 +80,73 @@ var _ = Describe("Artist artwork resolution", func() {
|
||||
})
|
||||
})
|
||||
|
||||
When("ArtistArtPriority has no album/ fallback", func() {
|
||||
// Artist/
|
||||
// ├── artist.jpg ← must resolve via the artist folder itself
|
||||
// └── Album/
|
||||
// └── 01 - Track.mp3
|
||||
It("still resolves the artist folder and returns artist.*", func() {
|
||||
conf.Server.ArtistArtPriority = "artist.*"
|
||||
setLayout(fstest.MapFS{
|
||||
"Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}),
|
||||
"Artist/artist.jpg": imageFile("artist-folder"),
|
||||
})
|
||||
scan()
|
||||
|
||||
ar := soleArtist()
|
||||
artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil)
|
||||
Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder")))
|
||||
})
|
||||
})
|
||||
|
||||
When("the artist's only album has its tracks in disc subfolders", func() {
|
||||
// Artist/
|
||||
// ├── artist.jpg ← wins (artist.* before album/artist.*)
|
||||
// └── Album/
|
||||
// ├── artist.jpg
|
||||
// ├── CD1/01 - Track.mp3
|
||||
// └── CD2/02 - Track.mp3
|
||||
It("prefers the artist-folder image over the album-folder one", func() {
|
||||
conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external"
|
||||
setLayout(fstest.MapFS{
|
||||
"Artist/Album/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album"}),
|
||||
"Artist/Album/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album"}),
|
||||
"Artist/artist.jpg": imageFile("artist-folder"),
|
||||
"Artist/Album/artist.jpg": imageFile("album-artist"),
|
||||
})
|
||||
scan()
|
||||
|
||||
ar := soleArtist()
|
||||
artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil)
|
||||
Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder")))
|
||||
})
|
||||
})
|
||||
|
||||
When("one album has disc subfolders and another sits at artist level", func() {
|
||||
// Artist/
|
||||
// ├── artist.jpg ← wins
|
||||
// ├── Album1/
|
||||
// │ ├── artist.jpg
|
||||
// │ ├── CD1/01 - Track.mp3
|
||||
// │ └── CD2/02 - Track.mp3
|
||||
// └── Album2/03 - Track.mp3
|
||||
It("prefers the artist-folder image over the album-folder one", func() {
|
||||
conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external"
|
||||
setLayout(fstest.MapFS{
|
||||
"Artist/Album1/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album1"}),
|
||||
"Artist/Album1/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album1"}),
|
||||
"Artist/Album2/03 - Track.mp3": trackFile(3, "Track 3", map[string]any{"albumartist": "Artist", "album": "Album2"}),
|
||||
"Artist/artist.jpg": imageFile("artist-folder"),
|
||||
"Artist/Album1/artist.jpg": imageFile("album-artist"),
|
||||
})
|
||||
scan()
|
||||
|
||||
ar := soleArtist()
|
||||
artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil)
|
||||
Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder")))
|
||||
})
|
||||
})
|
||||
|
||||
When("an artist has an uploaded image and a matching artist.* file", func() {
|
||||
// <DataFolder>/
|
||||
// └── artwork/
|
||||
|
||||
@ -20,6 +20,7 @@ import (
|
||||
"github.com/navidrome/navidrome/model"
|
||||
"github.com/navidrome/navidrome/utils"
|
||||
"github.com/navidrome/navidrome/utils/natural"
|
||||
"github.com/navidrome/navidrome/utils/slice"
|
||||
)
|
||||
|
||||
type albumArtworkReader struct {
|
||||
@ -123,11 +124,36 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, includeParen
|
||||
}
|
||||
}
|
||||
|
||||
// Collapse each album to its own root so an album split into disc
|
||||
// subfolders can't pull a shared prefix below the artist folder.
|
||||
pathByID := slice.ToMap(folders, func(f model.Folder) (string, string) {
|
||||
return f.ID, f.AbsolutePath()
|
||||
})
|
||||
var paths []string
|
||||
var claimedIDs []string
|
||||
for _, album := range albums {
|
||||
var albumPaths []string
|
||||
for _, fid := range album.FolderIDs {
|
||||
if p, ok := pathByID[fid]; ok {
|
||||
albumPaths = append(albumPaths, p)
|
||||
claimedIDs = append(claimedIDs, fid)
|
||||
}
|
||||
}
|
||||
if len(albumPaths) > 0 {
|
||||
paths = append(paths, commonDir(albumPaths))
|
||||
}
|
||||
}
|
||||
// Folders no album claims (e.g. the promoted parent) stay as-is.
|
||||
claimed := slice.ToSet(claimedIDs)
|
||||
for _, f := range folders {
|
||||
if _, ok := claimed[f.ID]; !ok {
|
||||
paths = append(paths, f.AbsolutePath())
|
||||
}
|
||||
}
|
||||
|
||||
var imgFiles []string
|
||||
var updatedAt time.Time
|
||||
for _, f := range folders {
|
||||
paths = append(paths, f.AbsolutePath())
|
||||
if f.ImagesUpdatedAt.After(updatedAt) {
|
||||
updatedAt = f.ImagesUpdatedAt
|
||||
}
|
||||
|
||||
@ -19,6 +19,7 @@ import (
|
||||
"github.com/navidrome/navidrome/core/external"
|
||||
"github.com/navidrome/navidrome/log"
|
||||
"github.com/navidrome/navidrome/model"
|
||||
"github.com/navidrome/navidrome/utils/slice"
|
||||
"github.com/navidrome/navidrome/utils/str"
|
||||
)
|
||||
|
||||
@ -232,17 +233,30 @@ func escapeGlobLiteral(s string) string {
|
||||
return b.String()
|
||||
}
|
||||
|
||||
// commonDir returns the deepest directory containing all paths. Trailing
|
||||
// separators keep the comparison on segment boundaries, so a shared name
|
||||
// fragment (".../Album" and ".../Album2") is never read as a shared directory.
|
||||
func commonDir(paths []string) string {
|
||||
sep := string(filepath.Separator)
|
||||
common := str.LongestCommonPrefix(slice.Map(paths, func(p string) string { return p + sep }))
|
||||
if !strings.HasSuffix(common, sep) {
|
||||
common, _ = filepath.Split(common)
|
||||
}
|
||||
return filepath.Clean(common)
|
||||
}
|
||||
|
||||
func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albums, paths []string) (string, time.Time, error) {
|
||||
if len(albums) == 0 {
|
||||
return "", time.Time{}, nil
|
||||
}
|
||||
libID := albums[0].LibraryID // Just need one of the albums, as they should all be in the same Library - for now! TODO: Support multiple libraries
|
||||
|
||||
folderPath := str.LongestCommonPrefix(paths)
|
||||
if !strings.HasSuffix(folderPath, string(filepath.Separator)) {
|
||||
folderPath, _ = filepath.Split(folderPath)
|
||||
// paths holds one root per album, so their common directory is already the
|
||||
// artist folder; a single root is the album itself, so climb one level.
|
||||
folderPath := commonDir(paths)
|
||||
if len(paths) < 2 {
|
||||
folderPath = filepath.Dir(folderPath)
|
||||
}
|
||||
folderPath = filepath.Dir(folderPath)
|
||||
|
||||
// Manipulate the path to get the folder ID
|
||||
// TODO: This is a bit hacky, but it's the easiest way to get the folder ID, ATM
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user