nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-05-03 06:51:16 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(artwork): allow shared disc art from unnumbered filenames in single-folder albums (#5344)

Browse Source 
* 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.
This commit is contained in:
Deluan Quintão 2026-04-11 21:19:57 -04:00 committed by GitHub
parent 1f3a7efa75
commit de6475bb49
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 233 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

102
core/artwork/reader_disc.go
View File

@ -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)

194
core/artwork/reader_disc_test.go
View File

@ -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")