diff --git a/core/artwork/e2e/artist_test.go b/core/artwork/e2e/artist_test.go index f912805bf..5a9396cc3 100644 --- a/core/artwork/e2e/artist_test.go +++ b/core/artwork/e2e/artist_test.go @@ -74,6 +74,89 @@ 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": smallPNG("artist-folder"), + }) + scan() + expectArtistFolder(soleArtist(), "Artist/artist.jpg") + }) + }) + + 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": smallPNG("artist-folder"), + "Artist/Album/artist.jpg": smallPNG("album-artist"), + }) + scan() + expectArtistFolder(soleArtist(), "Artist/artist.jpg") + }) + }) + + 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": smallPNG("artist-folder"), + "Artist/Album1/artist.jpg": smallPNG("album-artist"), + }) + scan() + expectArtistFolder(soleArtist(), "Artist/artist.jpg") + }) + }) + + When("every album of the artist has its tracks in disc subfolders", func() { + // Artist/ + // ├── artist.jpg ← wins + // ├── Album1/ + // │ ├── artist.jpg + // │ ├── CD1/01 - Track.mp3 + // │ └── CD2/02 - Track.mp3 + // └── Album2/ + // ├── CD1/03 - Track.mp3 + // └── CD2/04 - 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/CD1/03 - Track.mp3": trackFile(3, "Track 3", map[string]any{"albumartist": "Artist", "album": "Album2"}), + "Artist/Album2/CD2/04 - Track.mp3": trackFile(4, "Track 4", map[string]any{"albumartist": "Artist", "album": "Album2"}), + "Artist/artist.jpg": smallPNG("artist-folder"), + "Artist/Album1/artist.jpg": smallPNG("album-artist"), + }) + scan() + expectArtistFolder(soleArtist(), "Artist/artist.jpg") + }) + }) + When("an artist has an uploaded image and a matching artist.* file", func() { // / // └── artwork/ diff --git a/core/artwork/folders_album.go b/core/artwork/folders_album.go index 84099db6f..88f0181a3 100644 --- a/core/artwork/folders_album.go +++ b/core/artwork/folders_album.go @@ -13,19 +13,16 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/natural" + "github.com/navidrome/navidrome/utils/slice" ) -func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...model.Album) ([]string, []string, *time.Time, error) { - var folderIDs []string - for _, album := range albums { - folderIDs = append(folderIDs, album.FolderIDs...) - } - folders, err := ds.Folder(ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"folder.id": folderIDs, "missing": false}}) +func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, album model.Album) ([]string, []string, *time.Time, error) { + folders, err := loadFolders(ctx, ds, album.FolderIDs) if err != nil { return nil, nil, nil, err } - parent, err := albumRootParent(ctx, ds, folders, folderIDs) + parent, err := albumRootParent(ctx, ds, folders, album.FolderIDs) if err != nil { return nil, nil, nil, err } @@ -33,11 +30,21 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo folders = append(folders, *parent) } - var paths []string + paths := slice.Map(folders, func(f model.Folder) string { return f.AbsolutePath() }) + imgFiles, updatedAt := folderImages(folders) + return paths, imgFiles, &updatedAt, nil +} + +func loadFolders(ctx context.Context, ds model.DataStore, folderIDs []string) ([]model.Folder, error) { + return ds.Folder(ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"folder.id": folderIDs, "missing": false}}) +} + +// folderImages collects the folders' image files, sorted so files without +// numeric suffixes win (e.g. cover.jpg over cover.1.jpg). +func folderImages(folders []model.Folder) ([]string, time.Time) { var imgFiles []string var updatedAt time.Time for _, f := range folders { - paths = append(paths, f.AbsolutePath()) if f.ImagesUpdatedAt.After(updatedAt) { updatedAt = f.ImagesUpdatedAt } @@ -46,13 +53,8 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo imgFiles = append(imgFiles, path.Join(rel, img)) } } - - // Sort image files to ensure consistent selection of cover art - // This prioritizes files without numeric suffixes (e.g., cover.jpg over cover.1.jpg) - // by comparing base filenames without extensions slices.SortFunc(imgFiles, compareImageFiles) - - return paths, imgFiles, &updatedAt, nil + return imgFiles, updatedAt } // albumRootParent returns the common parent of the album's folders when it diff --git a/core/artwork/folders_artist.go b/core/artwork/folders_artist.go index bf44b7e21..1ca1ce034 100644 --- a/core/artwork/folders_artist.go +++ b/core/artwork/folders_artist.go @@ -110,17 +110,64 @@ func escapeGlobLiteral(s string) string { return b.String() } +// loadArtistAlbumRoots returns one path per album — the deepest folder holding +// all of that album's tracks — so an album split into disc subfolders can't +// pull the artist folder's common prefix below the artist level. +func loadArtistAlbumRoots(ctx context.Context, ds model.DataStore, albums model.Albums) ([]string, []string, *time.Time, error) { + var folderIDs []string + for _, album := range albums { + folderIDs = append(folderIDs, album.FolderIDs...) + } + folders, err := loadFolders(ctx, ds, folderIDs) + if err != nil { + return nil, nil, nil, err + } + + pathByID := slice.ToMap(folders, func(f model.Folder) (string, string) { + return f.ID, f.AbsolutePath() + }) + var roots []string + for _, album := range albums { + var albumPaths []string + for _, fid := range album.FolderIDs { + if p, ok := pathByID[fid]; ok { + albumPaths = append(albumPaths, p) + } + } + if len(albumPaths) > 0 { + roots = append(roots, commonDir(albumPaths)) + } + } + + imgFiles, updatedAt := folderImages(folders) + return roots, imgFiles, &updatedAt, nil +} + +// 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 // TODO: Support albums spanning multiple libraries - folderPath := str.LongestCommonPrefix(paths) - if !strings.HasSuffix(folderPath, string(filepath.Separator)) { - folderPath, _ = filepath.Split(folderPath) + // paths holds one root per album: two or more distinct roots already meet at + // the artist folder, while a single root is an album folder needing a climb. + roots := slices.Compact(slices.Sorted(slices.Values(paths))) + folderPath := commonDir(roots) + if len(roots) < 2 { + folderPath = filepath.Dir(folderPath) } - folderPath = filepath.Dir(folderPath) // TODO: Hacky, but the easiest way to get the folder ID ATM libPath := core.AbsolutePath(ctx, ds, libID, "") diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index e50532d7b..11711469d 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -175,7 +175,7 @@ func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resoluti if err != nil { return resolution{}, err } - albumPaths, imgFiles, _, err := loadAlbumFoldersPaths(ctx, r.ds, als...) + albumPaths, imgFiles, _, err := loadArtistAlbumRoots(ctx, r.ds, als) if err != nil { return resolution{}, err }