fix(server): prefer cover.jpg over cover.1.jpg (#4684)

* fix(reader): prioritize cover art selection by base filename without numeric suffixes Signed-off-by: Deluan <deluan@navidrome.org> * fix(reader): update image file comparison to use natural sorting and prioritize files without numeric suffixes Signed-off-by: Deluan <deluan@navidrome.org> * refactor(reader): simplify comparison, add case-sensitivity test case Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2025-12-23 23:18:05 -05:00 · 2025-11-14 12:19:10 -05:00
parent 2385c8a548
commit a10f839221
4 changed files with 98 additions and 29 deletions
--- 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),
+	)
+}
--- 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")))
 		})
 	})
 })
--- 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
--- 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=