From f85fbbe11e3a4f9d5975cb9b30e3ce6c681c2b80 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 27 Jul 2026 21:42:00 -0400 Subject: [PATCH] fix(artwork): resolve artist folder when albums share a folder loadArtistFolder decided whether to climb above the common directory by comparing len(paths) against 2. That count stands in for "how many distinct album roots are there", and the two diverge when an artist has two albums in the same folder: the slice holds two identical entries, the climb is skipped, and the artist folder resolves to the album folder. Deduplicate the roots so the branch tests the property it means to test. Replace the includeParent flag with two functions split by audience. The flag was a caller-identity switch - constant true at the album, disc and mediafile readers, all of which discard the returned paths, and constant false at the only caller that reads them. The artist path now has loadArtistAlbumRoots, which collapses each album to its own root and never consults albumRootParent; loadAlbumFoldersPaths goes back to taking a single album and always promoting the parent. Both share the folder load and image aggregation. This also makes albumRootParent's contract structural. Its guard only rejects an artist folder when it can find audio outside the album being resolved, so passing every album of an artist at once made the check meaningless; taking a single album means it can no longer be called that way. Drops the per-album grouping and unclaimed-folder reconciliation from the album path, where the result was discarded, and removes 15 mechanical true arguments from the tests. --- core/artwork/artwork_internal_test.go | 1 + core/artwork/reader_album.go | 68 +++++++++------------------ core/artwork/reader_album_test.go | 41 +++++++--------- core/artwork/reader_artist.go | 44 +++++++++++++++-- core/artwork/reader_artist_test.go | 13 +++++ core/artwork/reader_disc.go | 2 +- core/artwork/reader_mediafile.go | 2 +- 7 files changed, 93 insertions(+), 78 deletions(-) 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 }