diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 55d8b435..cb4db97f 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -1,6 +1,7 @@ package artwork import ( + "cmp" "context" "crypto/md5" "fmt" @@ -11,6 +12,7 @@ import ( "time" "github.com/Masterminds/squirrel" + "github.com/maruel/natural" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/external" @@ -116,8 +118,30 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo } // Sort image files to ensure consistent selection of cover art - // This prioritizes files from lower-numbered disc folders by sorting the paths - slices.Sort(imgFiles) + // 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 } + +// 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. +func compareImageFiles(a, b string) int { + // Case-insensitive comparison + a = strings.ToLower(a) + b = strings.ToLower(b) + + // Extract base filenames without extensions + baseA := strings.TrimSuffix(filepath.Base(a), filepath.Ext(a)) + baseB := strings.TrimSuffix(filepath.Base(b), filepath.Ext(b)) + + // Compare base names first, then full paths if equal + return cmp.Or( + natural.Compare(baseA, baseB), + natural.Compare(a, b), + ) +} diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go index 2665632b..fd5f8a2b 100644 --- a/core/artwork/reader_album_test.go +++ b/core/artwork/reader_album_test.go @@ -27,26 +27,7 @@ var _ = Describe("Album Artwork Reader", func() { expectedAt = now.Add(5 * time.Minute) // Set up the test folders with image files - repo = &fakeFolderRepo{ - result: []model.Folder{ - { - Path: "Artist/Album/Disc1", - ImagesUpdatedAt: expectedAt, - ImageFiles: []string{"cover.jpg", "back.jpg"}, - }, - { - Path: "Artist/Album/Disc2", - ImagesUpdatedAt: now, - ImageFiles: []string{"cover.jpg"}, - }, - { - Path: "Artist/Album/Disc10", - ImagesUpdatedAt: now, - ImageFiles: []string{"cover.jpg"}, - }, - }, - err: nil, - } + repo = &fakeFolderRepo{} ds = &fakeDataStore{ folderRepo: repo, } @@ -58,19 +39,82 @@ var _ = Describe("Album Artwork Reader", func() { }) It("returns sorted image files", func() { + repo.result = []model.Folder{ + { + Path: "Artist/Album/Disc1", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"cover.jpg", "back.jpg", "cover.1.jpg"}, + }, + { + Path: "Artist/Album/Disc2", + ImagesUpdatedAt: now, + ImageFiles: []string{"cover.jpg"}, + }, + { + Path: "Artist/Album/Disc10", + ImagesUpdatedAt: now, + ImageFiles: []string{"cover.jpg"}, + }, + } + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) Expect(err).ToNot(HaveOccurred()) Expect(*imagesUpdatedAt).To(Equal(expectedAt)) - // Check that image files are sorted alphabetically - Expect(imgFiles).To(HaveLen(4)) + // Check that image files are sorted by base name (without extension) + Expect(imgFiles).To(HaveLen(5)) - // The files should be sorted by full path + // Files should be sorted by base filename without extension, then by full path + // "back" < "cover", so back.jpg comes first + // Then all cover.jpg files, sorted by path Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/Disc1/back.jpg"))) Expect(imgFiles[1]).To(Equal(filepath.FromSlash("Artist/Album/Disc1/cover.jpg"))) - Expect(imgFiles[2]).To(Equal(filepath.FromSlash("Artist/Album/Disc10/cover.jpg"))) - Expect(imgFiles[3]).To(Equal(filepath.FromSlash("Artist/Album/Disc2/cover.jpg"))) + Expect(imgFiles[2]).To(Equal(filepath.FromSlash("Artist/Album/Disc2/cover.jpg"))) + Expect(imgFiles[3]).To(Equal(filepath.FromSlash("Artist/Album/Disc10/cover.jpg"))) + Expect(imgFiles[4]).To(Equal(filepath.FromSlash("Artist/Album/Disc1/cover.1.jpg"))) + }) + + It("prioritizes files without numeric suffixes", func() { + // Test case for issue #4683: cover.jpg should come before cover.1.jpg + repo.result = []model.Folder{ + { + Path: "Artist/Album", + ImagesUpdatedAt: now, + ImageFiles: []string{"cover.1.jpg", "cover.jpg", "cover.2.jpg"}, + }, + } + + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(imgFiles).To(HaveLen(3)) + + // cover.jpg should come first because "cover" < "cover.1" < "cover.2" + Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/cover.jpg"))) + Expect(imgFiles[1]).To(Equal(filepath.FromSlash("Artist/Album/cover.1.jpg"))) + Expect(imgFiles[2]).To(Equal(filepath.FromSlash("Artist/Album/cover.2.jpg"))) + }) + + It("handles case-insensitive sorting", func() { + // Test that Cover.jpg and cover.jpg are treated as equivalent + repo.result = []model.Folder{ + { + Path: "Artist/Album", + ImagesUpdatedAt: now, + ImageFiles: []string{"Folder.jpg", "cover.jpg", "BACK.jpg"}, + }, + } + + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(imgFiles).To(HaveLen(3)) + + // Files should be sorted case-insensitively: BACK, cover, Folder + Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/BACK.jpg"))) + Expect(imgFiles[1]).To(Equal(filepath.FromSlash("Artist/Album/cover.jpg"))) + Expect(imgFiles[2]).To(Equal(filepath.FromSlash("Artist/Album/Folder.jpg"))) }) }) }) diff --git a/go.mod b/go.mod index dcc77d06..5a6a9907 100644 --- a/go.mod +++ b/go.mod @@ -39,6 +39,7 @@ require ( github.com/knqyf263/go-plugin v0.9.0 github.com/kr/pretty v0.3.1 github.com/lestrrat-go/jwx/v2 v2.1.6 + github.com/maruel/natural v1.2.1 github.com/matoous/go-nanoid/v2 v2.1.0 github.com/mattn/go-sqlite3 v1.14.32 github.com/microcosm-cc/bluemonday v1.0.27 diff --git a/go.sum b/go.sum index 10feea90..7cda0ce8 100644 --- a/go.sum +++ b/go.sum @@ -162,8 +162,8 @@ github.com/lestrrat-go/jwx/v2 v2.1.6 h1:hxM1gfDILk/l5ylers6BX/Eq1m/pnxe9NBwW6lVf github.com/lestrrat-go/jwx/v2 v2.1.6/go.mod h1:Y722kU5r/8mV7fYDifjug0r8FK8mZdw0K0GpJw/l8pU= github.com/lestrrat-go/option v1.0.1 h1:oAzP2fvZGQKWkvHa1/SAcFolBEca1oN+mQ7eooNBEYU= github.com/lestrrat-go/option v1.0.1/go.mod h1:5ZHFbivi4xwXxhxY9XHDe2FHo6/Z7WWmtT7T5nBBp3I= -github.com/maruel/natural v1.1.1 h1:Hja7XhhmvEFhcByqDoHz9QZbkWey+COd9xWfCfn1ioo= -github.com/maruel/natural v1.1.1/go.mod h1:v+Rfd79xlw1AgVBjbO0BEQmptqb5HvL/k9GRHB7ZKEg= +github.com/maruel/natural v1.2.1 h1:G/y4pwtTA07lbQsMefvsmEO0VN0NfqpxprxXDM4R/4o= +github.com/maruel/natural v1.2.1/go.mod h1:v+Rfd79xlw1AgVBjbO0BEQmptqb5HvL/k9GRHB7ZKEg= github.com/matoous/go-nanoid/v2 v2.1.0 h1:P64+dmq21hhWdtvZfEAofnvJULaRR1Yib0+PnU669bE= github.com/matoous/go-nanoid/v2 v2.1.0/go.mod h1:KlbGNQ+FhrUNIHUxZdL63t7tl4LaPkZNpUULS8H4uVM= github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY=