diff --git a/core/artwork/artwork_internal_test.go b/core/artwork/artwork_internal_test.go index c95371959..5bcd82f23 100644 --- a/core/artwork/artwork_internal_test.go +++ b/core/artwork/artwork_internal_test.go @@ -202,6 +202,7 @@ var _ = Describe("Artwork", func() { repoRoot, err := os.Getwd() Expect(err).ToNot(HaveOccurred()) folderRepo.result = []model.Folder{{ + ID: "f1", LibraryPath: testFileLibPath(repoRoot), Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"artist.png"}, diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 7ad099bc2..58a47c8ae 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -38,7 +38,7 @@ func newAlbumArtworkReader(ctx context.Context, artwork *artwork, artID model.Ar if err != nil { return nil, err } - _, imgFiles, imagesUpdateAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, true, *al) + _, imgFiles, imagesUpdateAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, *al) if err != nil { return nil, err } @@ -104,53 +104,32 @@ func (a *albumArtworkReader) fromCoverArtPriority(ctx context.Context, ffmpeg ff return ff } -func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, includeParent bool, 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 } - if includeParent { - parent, err := albumRootParent(ctx, ds, folders, folderIDs) - if err != nil { - return nil, nil, nil, err - } - if parent != nil { - folders = append(folders, *parent) - } + parent, err := albumRootParent(ctx, ds, folders, album.FolderIDs) + if err != nil { + return nil, nil, nil, err + } + if parent != nil { + folders = append(folders, *parent) } - // 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()) - } - } + 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 { @@ -162,13 +141,8 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, includeParen 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/reader_album_test.go b/core/artwork/reader_album_test.go index 3c0a7d023..c95ddedaa 100644 --- a/core/artwork/reader_album_test.go +++ b/core/artwork/reader_album_test.go @@ -58,7 +58,7 @@ var _ = Describe("Album Artwork Reader", func() { }, } - _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(*imagesUpdatedAt).To(Equal(expectedAt)) @@ -86,7 +86,7 @@ var _ = Describe("Album Artwork Reader", func() { }, } - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(imgFiles).To(HaveLen(3)) @@ -107,7 +107,7 @@ var _ = Describe("Album Artwork Reader", func() { }, } - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(imgFiles).To(HaveLen(3)) @@ -147,7 +147,7 @@ var _ = Describe("Album Artwork Reader", func() { ImageFiles: []string{"cover.jpg", "back.jpg"}, } - _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(*imagesUpdatedAt).To(Equal(expectedAt)) @@ -176,7 +176,7 @@ var _ = Describe("Album Artwork Reader", func() { }, } - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(imgFiles).To(HaveLen(1)) @@ -206,7 +206,7 @@ var _ = Describe("Album Artwork Reader", func() { }, } - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(imgFiles).To(HaveLen(1)) @@ -243,7 +243,7 @@ var _ = Describe("Album Artwork Reader", func() { ImageFiles: []string{"unrelated.jpg"}, } - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(imgFiles).To(HaveLen(1)) @@ -280,7 +280,7 @@ var _ = Describe("Album Artwork Reader", func() { ImageFiles: []string{"cover.jpg"}, } - _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(*imagesUpdatedAt).To(Equal(expectedAt)) @@ -303,7 +303,7 @@ var _ = Describe("Album Artwork Reader", func() { }, } - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(imgFiles).To(HaveLen(1)) @@ -331,7 +331,7 @@ var _ = Describe("Album Artwork Reader", func() { ImageFiles: []string{"cover.jpg"}, } - _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(*imagesUpdatedAt).To(Equal(expectedAt)) @@ -340,7 +340,7 @@ var _ = Describe("Album Artwork Reader", func() { Expect(repo.getCallCount).To(Equal(1)) }) - It("properly responds whether to add parent or not", func() { + It("promotes the album root parent into the returned paths", func() { repo.result = []model.Folder{ { ID: "folder1", @@ -360,17 +360,10 @@ var _ = Describe("Album Artwork Reader", func() { ImageFiles: []string{"folder.jpg"}, } - paths, _, _, err := loadAlbumFoldersPaths(ctx, ds, false, album) - - fsPath := filepath.Join("Artist", "Album") + paths, _, _, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) - Expect(paths).To(Equal([]string{fsPath})) - - paths, _, _, err = loadAlbumFoldersPaths(ctx, ds, true, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(paths).To(Equal([]string{fsPath, "Artist"})) + Expect(paths).To(Equal([]string{filepath.Join("Artist", "Album"), "Artist"})) }) It("does not include parent images when other albums' audio lives under the parent", func() { @@ -396,7 +389,7 @@ var _ = Describe("Album Artwork Reader", func() { } repo.hasOtherAudio = true - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(imgFiles).To(BeEmpty()) @@ -423,7 +416,7 @@ var _ = Describe("Album Artwork Reader", func() { } repo.otherAudioErr = errors.New("db connection failed") - _, _, _, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, _, _, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).To(MatchError("db connection failed")) }) @@ -449,7 +442,7 @@ var _ = Describe("Album Artwork Reader", func() { } repo.getErr = errors.New("db connection failed") - _, _, _, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, _, _, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).To(MatchError("db connection failed")) Expect(repo.getCallCount).To(Equal(1)) @@ -477,7 +470,7 @@ var _ = Describe("Album Artwork Reader", func() { } // parentResult is nil, so Get will return ErrNotFound - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, true, album) + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(imgFiles).To(HaveLen(1)) diff --git a/core/artwork/reader_artist.go b/core/artwork/reader_artist.go index 68e2e78d5..fe5bcb196 100644 --- a/core/artwork/reader_artist.go +++ b/core/artwork/reader_artist.go @@ -55,7 +55,7 @@ func newArtistArtworkReader(ctx context.Context, artwork *artwork, artID model.A if err != nil { return nil, err } - albumPaths, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, false, als...) + albumPaths, imgFiles, imagesUpdatedAt, err := loadArtistAlbumRoots(ctx, artwork.ds, als) if err != nil { return nil, err } @@ -233,6 +233,39 @@ 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. @@ -251,10 +284,11 @@ func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albu } 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 - // 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 { + // 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) } diff --git a/core/artwork/reader_artist_test.go b/core/artwork/reader_artist_test.go index 6d6d58fc5..c75f8447e 100644 --- a/core/artwork/reader_artist_test.go +++ b/core/artwork/reader_artist_test.go @@ -85,6 +85,19 @@ var _ = Describe("artistArtworkReader", func() { }) }) + When("two albums share the same folder", func() { + It("climbs above the shared album folder", func() { + paths = []string{ + filepath.FromSlash("/music/artist/split"), + filepath.FromSlash("/music/artist/split"), + } + folder, upd, err := loadArtistFolder(ctx, fds, albums, paths) + Expect(err).ToNot(HaveOccurred()) + Expect(folder).To(Equal(filepath.FromSlash("/music/artist"))) + Expect(upd).To(Equal(expectedUpdTime)) + }) + }) + When("the album paths contain same prefix", func() { It("returns the common prefix", func() { paths = []string{ diff --git a/core/artwork/reader_disc.go b/core/artwork/reader_disc.go index 0f000cc61..0f648c987 100644 --- a/core/artwork/reader_disc.go +++ b/core/artwork/reader_disc.go @@ -43,7 +43,7 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID return nil, err } - _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, a.ds, true, *al) + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, a.ds, *al) if err != nil { return nil, err } diff --git a/core/artwork/reader_mediafile.go b/core/artwork/reader_mediafile.go index cebd3f06a..eac3c5e70 100644 --- a/core/artwork/reader_mediafile.go +++ b/core/artwork/reader_mediafile.go @@ -27,7 +27,7 @@ func newMediafileArtworkReader(ctx context.Context, artwork *artwork, artID mode if err != nil { return nil, err } - _, _, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, true, *al) + _, _, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, *al) if err != nil { return nil, err }