From de6475bb497bfbda4f1dc945efe203963839b2d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sat, 11 Apr 2026 21:19:57 -0400 Subject: [PATCH] fix(artwork): allow shared disc art from unnumbered filenames in single-folder albums (#5344) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(artwork): expect shared disc art for unnumbered filenames in single-folder albums * fix(artwork): match unnumbered disc art for every disc in single-folder albums * test(artwork): verify shared disc art resolves for every disc number * test(artwork): regression guard for numbered disc filter with mixed filenames * test(artwork): verify DiscArtPriority order decides numbered vs shared disc art * test(artwork): strengthen regression guard to exercise both disc art branches * refactor(artwork): simplify disc art matching and drop redundant comments - Lowercase the pattern and filename once in fromExternalFile and pass lowered values into extractDiscNumber, eliminating the duplicate strings.ToLower calls inside that helper. - Drop narrating comments in reader_disc.go and reader_disc_test.go that duplicated information already conveyed by nearby code or doc comments. * fix(artwork): prefer numbered disc art over shared fallback within a pattern Review feedback: with files [disc.jpg, disc1.jpg, disc2.jpg] in a single folder, the previous single-folder fall-through returned the first match in imgFiles order. Because compareImageFiles sorts 'disc' before 'disc1' and 'disc2', disc.jpg would mask the per-disc numbered files for every disc, regressing the behavior from before the shared-disc-art change. Within a single pattern the loop now records the first viable unnumbered candidate as a fallback and keeps scanning for a numbered match equal to the target disc. Numbered matches still win immediately; the shared file is only returned when no numbered match for the target disc exists. Also drops the redundant strings.ToLower(pattern) at the top of fromExternalFile; fromDiscArtPriority already lowercases the whole priority string before splitting, so the function contract is now 'pattern must be lowercase' (documented on the function). * refactor(artwork): trim disc art matching comments and table-drive tests Doc comment on fromExternalFile is trimmed to the one non-obvious contract (caller must pre-lowercase the pattern) plus the headline behavior; the bulleted restatement of the branch logic went away. Two inline comments that narrated what the code already shows are also gone. Hoisting a `hasWildcard := strings.ContainsRune(pattern, '*')` check out of the loop avoids per-iteration extractDiscNumber calls for literal patterns (e.g. `shellac.png`) and lets the loop break as soon as a viable fallback is found, since literal patterns can never be beaten by a numbered match. Wildcard patterns keep the original scan-to-end-for-numbered-match behavior. The two regression tests added in the previous commit were structurally identical apart from discNumber/expected, so they are collapsed into a DescribeTable with two entries — matching the existing table style used for extractDiscNumber tests in the same file. * fix(artwork): support '?' and '[...]' wildcards in disc art patterns filepath.Match understands three glob metacharacters ('*', '?', '[') but extractDiscNumber only looked for '*'. A pattern like 'disc?.jpg' or 'cd[12].jpg' would therefore be treated as unnumbered, and every disc of a multi-disc album would resolve to the same (first-sorted) file instead of the per-disc numbered art. extractDiscNumber now finds the literal prefix of the pattern by scanning for the first '*', '?', or '[' (via strings.IndexAny), strips it from the filename, and parses the leading digits that follow. The standalone filepath.Match check is dropped; HasPrefix plus the leading-digits requirement is enough to reject non-matches, and the caller already verifies the glob match before calling. fromExternalFile's literal-pattern optimization is widened correspondingly: a pattern is treated as literal only when it contains none of '*', '?', '['. Any wildcard form now keeps the scan-to-end behavior so a numbered match can beat a fallback. Adds table entries for both the extractDiscNumber parser and the fromExternalFile higher-level behavior, covering '?' and '[...]' patterns as well as a literal-pattern baseline. * refactor(artwork): tidy extractDiscNumber after glob-wildcard support - Name the '*?[' charset as globMetaChars, used by both extractDiscNumber and fromExternalFile so the two call sites can't drift. - Trim the extractDiscNumber doc comment: keep the non-obvious caller contract, drop the algorithm narration. - Replace the byte-slice digit accumulator with a direct filename slice fed to strconv.Atoi. - Rename the four new non-'*' wildcard Entry descriptions so they read like the existing extractDiscNumber table ('pattern, target → expected') instead of the ambiguous 'disc 1' shorthand. * fix(artwork): retry remaining fallbacks when the first one fails to open Review feedback: the previous shape remembered only the first unnumbered candidate and fell through to a generic error if os.Open failed on it, even though other matching unnumbered files in imgFiles could have succeeded. The pre-PR code was more resilient because it looped and continued on open failure. fromExternalFile now collects every viable unnumbered candidate into a slice during the scan, then tries them in order after the loop, mirroring the pre-PR retry-on-open-failure behavior. Numbered matches still return immediately on first success and skip the candidate list entirely — an open failure on a numbered match means no other file has that number anyway. Also: - globMetaChars doc comment now notes that '\' escape is intentionally excluded (filepath.Match supports it but treating it as a metachar here would misalign extractDiscNumber's literal-prefix extraction with no benefit for realistic config patterns). - The 'cover.jpg doesn't match disc*.*' Entry in the extractDiscNumber table is renamed to 'cover.jpg with disc*.* (no prefix match)' to reflect that the test now exercises the HasPrefix defensive guard, not the removed internal filepath.Match check. Regression test added: a single-folder album with a deleted first candidate file resolves to the second candidate. * fix(artwork): scan all literal-pattern matches so fallback retry works Review feedback: the 'break on first literal match' optimization assumed only one file in imgFiles could match a literal basename, but filepath.Match compares basenames only — multiple folders can contribute files with the same basename, and the fallback-list retry in 5d79f751c is defeated if the loop breaks after recording just the first one. Removing the break makes literal and wildcard patterns follow the same scan-to-end path, preserving the retry-on-open-failure resilience regained in 5d79f751c. The efficiency cost is negligible — imgFiles is 5-20 entries per album and this is a cache-miss path. --- core/artwork/reader_disc.go | 102 ++++++++-------- core/artwork/reader_disc_test.go | 194 +++++++++++++++++++++++++++++-- 2 files changed, 233 insertions(+), 63 deletions(-) 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")