diff --git a/core/artwork/reader_disc.go b/core/artwork/reader_disc.go index 7548f76d2..5a7a8a65e 100644 --- a/core/artwork/reader_disc.go +++ b/core/artwork/reader_disc.go @@ -168,47 +168,38 @@ func (d *discArtworkReader) fromDiscSubtitle(ctx context.Context, subtitle strin } } -// extractDiscNumber extracts a disc number from a filename based on a glob pattern. -// It finds the portion of the filename that the wildcard matched and parses leading -// digits as the disc number. Returns (0, false) if the pattern doesn't match or -// no leading digits are found in the wildcard portion. +// globMetaChars holds the substitution metacharacters understood by +// filepath.Match. The '\' escape character is intentionally excluded: +// disc art patterns come from user config and never include escaped +// metachars in practice, and treating '\' as a metachar would misalign +// the literal-prefix extraction in extractDiscNumber. +const globMetaChars = "*?[" + +// extractDiscNumber parses the disc number from a filename matched by a +// filepath.Match-style glob pattern. +// +// Both pattern and filename must already be lowercased by the caller, which +// is also expected to have verified that filepath.Match(pattern, filename) +// is true before calling this function. func extractDiscNumber(pattern, filename string) (int, bool) { - filename = strings.ToLower(filename) - pattern = strings.ToLower(pattern) - - matched, err := filepath.Match(pattern, filename) - if err != nil || !matched { + metaIdx := strings.IndexAny(pattern, globMetaChars) + if metaIdx < 0 { return 0, false } - - // Find the prefix before the first '*' in the pattern - starIdx := strings.IndexByte(pattern, '*') - if starIdx < 0 { - return 0, false - } - prefix := pattern[:starIdx] - - // Strip the prefix from the filename to get the wildcard-matched portion + prefix := pattern[:metaIdx] if !strings.HasPrefix(filename, prefix) { return 0, false } - remainder := filename[len(prefix):] - // Extract leading ASCII digits from the remainder - var digits []byte - for _, r := range remainder { - if r >= '0' && r <= '9' { - digits = append(digits, byte(r)) - } else { - break - } + start := len(prefix) + end := start + for end < len(filename) && filename[end] >= '0' && filename[end] <= '9' { + end++ } - - if len(digits) == 0 { + if end == start { return 0, false } - - num, err := strconv.Atoi(string(digits)) + num, err := strconv.Atoi(filename[start:end]) if err != nil { return 0, false } @@ -216,20 +207,16 @@ func extractDiscNumber(pattern, filename string) (int, bool) { } // fromExternalFile returns a sourceFunc that matches image files against a glob -// pattern with disc-number-aware filtering. -// -// Matching rules: -// - If a disc number can be extracted from the filename, the file matches only if -// the number equals the target disc number. -// - If no number is found and this is a multi-folder album, the file matches if -// it's in a folder containing tracks for this disc. -// - If no number is found and this is a single-folder album, the file is skipped -// (ambiguous). +// pattern. A numbered filename whose number equals the target disc wins over +// any unnumbered candidate; callers must pass a lowercase pattern. func (d *discArtworkReader) fromExternalFile(ctx context.Context, pattern string) sourceFunc { + isLiteral := !strings.ContainsAny(pattern, globMetaChars) return func() (io.ReadCloser, string, error) { + var fallbacks []string for _, file := range d.imgFiles { _, name := filepath.Split(file) - match, err := filepath.Match(pattern, strings.ToLower(name)) + name = strings.ToLower(name) + match, err := filepath.Match(pattern, name) if err != nil { log.Warn(ctx, "Error matching disc art file to pattern", "pattern", pattern, "file", file) continue @@ -238,24 +225,27 @@ func (d *discArtworkReader) fromExternalFile(ctx context.Context, pattern string continue } - // Try to extract disc number from filename - num, hasNum := extractDiscNumber(pattern, name) - if hasNum { - // File has a disc number — must match target disc - if num != d.discNumber { - continue + if !isLiteral { + if num, hasNum := extractDiscNumber(pattern, name); hasNum { + if num != d.discNumber { + continue + } + f, err := os.Open(file) + if err != nil { + log.Warn(ctx, "Could not open disc art file", "file", file, err) + continue + } + return f, file, nil } - } else if d.isMultiFolder { - // No number, multi-folder: match by folder association - dir := filepath.Dir(file) - if !d.discFolders[dir] { - continue - } - } else { - // No number, single-folder: ambiguous, skip - continue } + if d.isMultiFolder && !d.discFolders[filepath.Dir(file)] { + continue + } + fallbacks = append(fallbacks, file) + } + + for _, file := range fallbacks { f, err := os.Open(file) if err != nil { log.Warn(ctx, "Could not open disc art file", "file", file, err) diff --git a/core/artwork/reader_disc_test.go b/core/artwork/reader_disc_test.go index f8193e24e..7b633342f 100644 --- a/core/artwork/reader_disc_test.go +++ b/core/artwork/reader_disc_test.go @@ -42,11 +42,24 @@ var _ = Describe("Disc Artwork Reader", func() { // Case insensitive (filename already lowered by caller) Entry("Disc1.jpg lowered", "disc*.*", "disc1.jpg", 1, true), - // Pattern doesn't match - Entry("cover.jpg doesn't match disc*.*", "disc*.*", "cover.jpg", 0, false), + // HasPrefix guard: filename doesn't share the pattern's literal prefix + Entry("cover.jpg with disc*.* (no prefix match)", "disc*.*", "cover.jpg", 0, false), // Pattern with no wildcard before dot Entry("front1.jpg with front*.*", "front*.*", "front1.jpg", 1, true), + + // '?' single-char wildcard + Entry("disc?.jpg with disc1.jpg", "disc?.jpg", "disc1.jpg", 1, true), + Entry("disc?.jpg with disc2.jpg", "disc?.jpg", "disc2.jpg", 2, true), + Entry("cd??.jpg with cd07.jpg", "cd??.jpg", "cd07.jpg", 7, true), + + // '[...]' character class wildcard + Entry("cd[12].jpg with cd1.jpg", "cd[12].jpg", "cd1.jpg", 1, true), + Entry("cd[12].jpg with cd2.jpg", "cd[12].jpg", "cd2.jpg", 2, true), + Entry("disc[0-9].jpg with disc5.jpg", "disc[0-9].jpg", "disc5.jpg", 5, true), + + // Literal pattern (no wildcard) returns false + Entry("shellac.png literal", "shellac.png", "shellac.png", 0, false), ) }) @@ -85,19 +98,186 @@ var _ = Describe("Disc Artwork Reader", func() { Expect(path).To(Equal(f1)) }) - It("skips file without number in single-folder album", func() { - f1 := createFile("album/disc.jpg") + It("matches file without number in single-folder album (shared disc art)", func() { + f1 := createFile("album/cover.png") reader := &discArtworkReader{ discNumber: 1, imgFiles: []string{f1}, discFolders: map[string]bool{filepath.Join(tmpDir, "album"): true}, } - sf := reader.fromExternalFile(ctx, "disc*.*") - r, _, _ := sf() - Expect(r).To(BeNil()) + sf := reader.fromExternalFile(ctx, "cover.*") + r, path, err := sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(f1)) }) + It("returns shared disc art for every disc number in single-folder album", func() { + f1 := createFile("album/shellac.png") + makeReader := func(discNum int) *discArtworkReader { + return &discArtworkReader{ + discNumber: discNum, + imgFiles: []string{f1}, + discFolders: map[string]bool{filepath.Join(tmpDir, "album"): true}, + } + } + + for _, disc := range []int{1, 2, 5} { + sf := makeReader(disc).fromExternalFile(ctx, "shellac.png") + r, path, err := sf() + Expect(err).ToNot(HaveOccurred(), "disc %d", disc) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(f1), "disc %d", disc) + } + }) + + It("numbered and unnumbered patterns both resolve against the same reader", func() { + f1 := createFile("album/cover.png") + f2 := createFile("album/disc1.jpg") + f3 := createFile("album/disc2.jpg") + reader := &discArtworkReader{ + discNumber: 2, + imgFiles: []string{f1, f2, f3}, + discFolders: map[string]bool{filepath.Join(tmpDir, "album"): true}, + } + + sf := reader.fromExternalFile(ctx, "disc*.*") + r, path, err := sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(f3)) + + sf = reader.fromExternalFile(ctx, "cover.*") + r, path, err = sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(f1)) + }) + + It("respects DiscArtPriority order when both numbered and unnumbered patterns match", func() { + f1 := createFile("album/cover.png") + f2 := createFile("album/disc1.jpg") + reader := &discArtworkReader{ + discNumber: 1, + imgFiles: []string{f1, f2}, + discFolders: map[string]bool{filepath.Join(tmpDir, "album"): true}, + } + + ff := reader.fromDiscArtPriority(ctx, nil, "disc*.*, cover.*") + Expect(ff).To(HaveLen(2)) + r, path, err := ff[0]() + Expect(err).ToNot(HaveOccurred()) + Expect(path).To(Equal(f2)) + r.Close() + + ff = reader.fromDiscArtPriority(ctx, nil, "cover.*, disc*.*") + Expect(ff).To(HaveLen(2)) + r, path, err = ff[0]() + Expect(err).ToNot(HaveOccurred()) + Expect(path).To(Equal(f1)) + r.Close() + }) + + DescribeTable("numbered match wins over shared fallback within a pattern", + func(discNumber, expectedIdx int) { + files := []string{ + createFile("album/disc.jpg"), + createFile("album/disc1.jpg"), + createFile("album/disc2.jpg"), + } + reader := &discArtworkReader{ + discNumber: discNumber, + imgFiles: files, + discFolders: map[string]bool{filepath.Join(tmpDir, "album"): true}, + } + + sf := reader.fromExternalFile(ctx, "disc*.*") + r, path, err := sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(files[expectedIdx])) + }, + Entry("disc 2 picks disc2.jpg over the shared disc.jpg", 2, 2), + Entry("disc 3 falls back to disc.jpg when no numbered match exists", 3, 0), + ) + + It("tries the next fallback candidate when the first one cannot be opened", func() { + f1 := createFile("album/cover.jpg") + f2 := createFile("album/cover.png") + // Remove f1 so os.Open will fail on it; f2 should still win. + Expect(os.Remove(f1)).To(Succeed()) + reader := &discArtworkReader{ + discNumber: 1, + imgFiles: []string{f1, f2}, + discFolders: map[string]bool{filepath.Join(tmpDir, "album"): true}, + } + + sf := reader.fromExternalFile(ctx, "cover.*") + r, path, err := sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(f2)) + }) + + It("keeps scanning literal-pattern matches so fallback retry still works", func() { + // Guards against an 'early break on first literal match' optimization. + // Multiple imgFiles entries can share a basename (symlinks, case-variant + // duplicates on case-sensitive filesystems). If the loop breaks after + // recording just the first, the fallback retry cannot recover when + // that first file is unreadable. + f1 := createFile("album/stale/cover.png") + f2 := createFile("album/cover.png") + Expect(os.Remove(f1)).To(Succeed()) + reader := &discArtworkReader{ + discNumber: 1, + imgFiles: []string{f1, f2}, + discFolders: map[string]bool{ + filepath.Join(tmpDir, "album"): true, + filepath.Join(tmpDir, "album/stale"): true, + }, + isMultiFolder: true, + } + + sf := reader.fromExternalFile(ctx, "cover.png") + r, path, err := sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(f2)) + }) + + DescribeTable("filters by disc number for non-'*' wildcard patterns", + func(pattern string, discNumber, expectedIdx int) { + files := []string{ + createFile("album/disc1.jpg"), + createFile("album/disc2.jpg"), + } + reader := &discArtworkReader{ + discNumber: discNumber, + imgFiles: files, + discFolders: map[string]bool{filepath.Join(tmpDir, "album"): true}, + } + + sf := reader.fromExternalFile(ctx, pattern) + r, path, err := sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(files[expectedIdx])) + }, + Entry("disc?.jpg, target disc 1 → disc1.jpg", "disc?.jpg", 1, 0), + Entry("disc?.jpg, target disc 2 → disc2.jpg", "disc?.jpg", 2, 1), + Entry("disc[0-9].jpg, target disc 1 → disc1.jpg", "disc[0-9].jpg", 1, 0), + Entry("disc[0-9].jpg, target disc 2 → disc2.jpg", "disc[0-9].jpg", 2, 1), + ) + It("matches file without number in multi-folder album by folder", func() { f1 := createFile("album/cd1/disc.jpg") f2 := createFile("album/cd2/disc.jpg")