From 49a14d45838ff6f95157ea3d55b70e28d0f66866 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Fri, 13 Mar 2026 18:33:18 -0400 Subject: [PATCH] feat(artwork): add per-disc cover art support (#5182) * feat(artwork): add KindDiscArtwork and ParseDiscArtworkID Add new disc artwork kind with 'dc' prefix for per-disc cover art support. The composite ID format is albumID:discNumber, parsed by the new ParseDiscArtworkID helper. * feat(conf): add DiscArtPriority configuration option Default: 'disc*.*, cd*.*, embedded'. Controls how per-disc cover art is resolved, following the same pattern as CoverArtPriority and ArtistArtPriority. * feat(artwork): implement extractDiscNumber helper Extracts disc number from filenames based on glob patterns by parsing leading digits from the wildcard-matched portion. Used for matching disc-specific artwork files like disc1.jpg. * feat(artwork): implement fromDiscExternalFile source function Disc-aware variant of fromExternalFile that filters image files by disc number (extracted from filename) or folder association (for multi-folder albums). * feat(artwork): implement discArtworkReader Resolves disc artwork using DiscArtPriority config patterns. Supports glob patterns with disc number extraction, embedded images from first track, and falls back to album cover art. Handles both multi-folder and single-folder multi-disc albums. * feat(artwork): register disc artwork reader in dispatcher Add KindDiscArtwork case to getArtworkReader switch, routing disc artwork requests to the new discArtworkReader. * feat(subsonic): add CoverArt field to DiscTitle response Implements OpenSubsonic PR #220: optional cover art ID in DiscTitle responses for per-disc artwork support. * feat(subsonic): populate CoverArt in DiscTitle responses Each DiscTitle now includes a disc artwork ID (dc-albumID:discNum) that clients can use with getCoverArt to retrieve per-disc artwork. * style: fix file permission in test to satisfy gosec * feat(ui): add disc cover art display and lightbox functionality Signed-off-by: Deluan * refactor: simplify disc artwork code - Add DiscArtworkID constructor to encapsulate the "albumID:discNumber" format in one place - Convert fromDiscExternalFile to a method on discArtworkReader, reducing parameter count from 6 to 2 - Remove unused rootFolder field from discArtworkReader * style: fix prettier formatting in subsonic index * style(ui): move cursor style to makeStyles in SongDatagrid * feat(artwork): add discsubtitle option to DiscArtPriority Allow matching disc cover art by the disc's subtitle/name. When the "discsubtitle" keyword is in the priority list, image files whose stem matches the disc subtitle (case-insensitive) are used. This is useful for box sets with named discs (e.g., "The Blue Disc.jpg"). * feat(configuration): update discartpriority to include cover art options Signed-off-by: Deluan --------- Signed-off-by: Deluan --- conf/configuration.go | 2 + core/artwork/artwork.go | 2 + core/artwork/reader_disc.go | 268 +++++++++++++++++++++++ core/artwork/reader_disc_test.go | 285 +++++++++++++++++++++++++ model/artwork_id.go | 18 ++ model/artwork_id_test.go | 34 +++ server/subsonic/helpers.go | 8 +- server/subsonic/helpers_test.go | 33 ++- server/subsonic/responses/responses.go | 5 +- ui/src/common/SongDatagrid.jsx | 88 ++++++-- ui/src/i18n/en.json | 1 + ui/src/subsonic/index.js | 11 + ui/src/subsonic/index.test.js | 50 +++++ 13 files changed, 782 insertions(+), 23 deletions(-) create mode 100644 core/artwork/reader_disc.go create mode 100644 core/artwork/reader_disc_test.go diff --git a/conf/configuration.go b/conf/configuration.go index b867e0d5..dc5fd1b5 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -71,6 +71,7 @@ type configOptions struct { CoverArtPriority string CoverArtQuality int ArtistArtPriority string + DiscArtPriority string LyricsPriority string EnableGravatar bool EnableFavourites bool @@ -660,6 +661,7 @@ func setViperDefaults() { viper.SetDefault("coverartpriority", "cover.*, folder.*, front.*, embedded, external") viper.SetDefault("coverartquality", 75) viper.SetDefault("artistartpriority", "artist.*, album/artist.*, external") + viper.SetDefault("discartpriority", "disc*.*, cd*.*, cover.*, folder.*, front.*, discsubtitle, embedded") viper.SetDefault("lyricspriority", ".lrc,.txt,embedded") viper.SetDefault("enablegravatar", false) viper.SetDefault("enablefavourites", true) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index 2e92b24c..4a0a32af 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -122,6 +122,8 @@ func (a *artwork) getArtworkReader(ctx context.Context, artID model.ArtworkID, s artReader, err = newMediafileArtworkReader(ctx, a, artID) case model.KindPlaylistArtwork: artReader, err = newPlaylistArtworkReader(ctx, a, artID) + case model.KindDiscArtwork: + artReader, err = newDiscArtworkReader(ctx, a, artID) default: return nil, ErrUnavailable } diff --git a/core/artwork/reader_disc.go b/core/artwork/reader_disc.go new file mode 100644 index 00000000..7548f76d --- /dev/null +++ b/core/artwork/reader_disc.go @@ -0,0 +1,268 @@ +package artwork + +import ( + "context" + "crypto/md5" + "fmt" + "io" + "os" + "path/filepath" + "strconv" + "strings" + "time" + + "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core/ffmpeg" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" +) + +type discArtworkReader struct { + cacheKey + a *artwork + album model.Album + discNumber int + imgFiles []string + discFolders map[string]bool + isMultiFolder bool + firstTrackPath string + updatedAt *time.Time +} + +func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID) (*discArtworkReader, error) { + albumID, discNumber, err := model.ParseDiscArtworkID(artID.ID) + if err != nil { + return nil, fmt.Errorf("invalid disc artwork id '%s': %w", artID.ID, err) + } + + al, err := a.ds.Album(ctx).Get(albumID) + if err != nil { + return nil, err + } + + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, a.ds, *al) + if err != nil { + return nil, err + } + + // Query mediafiles for this album + disc to find folder associations and first track + mfs, err := a.ds.MediaFile(ctx).GetAll(model.QueryOptions{ + Sort: "track_number", + Order: "ASC", + Filters: squirrel.Eq{"album_id": albumID, "disc_number": discNumber}, + }) + if err != nil { + return nil, err + } + + // Build disc folder set and find first track + discFolders := make(map[string]bool) + var firstTrackPath string + allFolderIDs := make(map[string]bool) + for _, mf := range mfs { + allFolderIDs[mf.FolderID] = true + if firstTrackPath == "" { + firstTrackPath = mf.Path + } + } + + // Resolve folder IDs to absolute paths + if len(allFolderIDs) > 0 { + folderIDs := make([]string, 0, len(allFolderIDs)) + for id := range allFolderIDs { + folderIDs = append(folderIDs, id) + } + folders, err := a.ds.Folder(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"folder.id": folderIDs}, + }) + if err != nil { + return nil, err + } + for _, f := range folders { + discFolders[f.AbsolutePath()] = true + } + } + + isMultiFolder := len(al.FolderIDs) > 1 + + r := &discArtworkReader{ + a: a, + album: *al, + discNumber: discNumber, + imgFiles: imgFiles, + discFolders: discFolders, + isMultiFolder: isMultiFolder, + firstTrackPath: core.AbsolutePath(ctx, a.ds, al.LibraryID, firstTrackPath), + updatedAt: imagesUpdatedAt, + } + r.cacheKey.artID = artID + if r.updatedAt != nil && r.updatedAt.After(al.UpdatedAt) { + r.cacheKey.lastUpdate = *r.updatedAt + } else { + r.cacheKey.lastUpdate = al.UpdatedAt + } + return r, nil +} + +func (d *discArtworkReader) Key() string { + hash := md5.Sum([]byte(conf.Server.DiscArtPriority)) + return fmt.Sprintf( + "%s.%x", + d.cacheKey.Key(), + hash, + ) +} + +func (d *discArtworkReader) LastUpdated() time.Time { + return d.album.UpdatedAt +} + +func (d *discArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) { + var ff = d.fromDiscArtPriority(ctx, d.a.ffmpeg, conf.Server.DiscArtPriority) + // Fallback to album cover art + albumArtID := model.NewArtworkID(model.KindAlbumArtwork, d.album.ID, &d.album.UpdatedAt) + ff = append(ff, fromAlbum(ctx, d.a, albumArtID)) + return selectImageReader(ctx, d.cacheKey.artID, ff...) +} + +func (d *discArtworkReader) fromDiscArtPriority(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string) []sourceFunc { + var ff []sourceFunc + for pattern := range strings.SplitSeq(strings.ToLower(priority), ",") { + pattern = strings.TrimSpace(pattern) + switch { + case pattern == "embedded": + ff = append(ff, fromTag(ctx, d.firstTrackPath), fromFFmpegTag(ctx, ffmpeg, d.firstTrackPath)) + case pattern == "external": + // Not supported for disc art, silently ignore + case pattern == "discsubtitle": + if subtitle := strings.TrimSpace(d.album.Discs[d.discNumber]); subtitle != "" { + ff = append(ff, d.fromDiscSubtitle(ctx, subtitle)) + } + case len(d.imgFiles) > 0: + ff = append(ff, d.fromExternalFile(ctx, pattern)) + } + } + return ff +} + +// fromDiscSubtitle returns a sourceFunc that matches image files whose stem +// (filename without extension) equals the disc subtitle (case-insensitive). +func (d *discArtworkReader) fromDiscSubtitle(ctx context.Context, subtitle string) sourceFunc { + return func() (io.ReadCloser, string, error) { + for _, file := range d.imgFiles { + _, name := filepath.Split(file) + stem := strings.TrimSuffix(name, filepath.Ext(name)) + if !strings.EqualFold(stem, subtitle) { + 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 + } + return nil, "", fmt.Errorf("disc %d: no image file matching subtitle %q", d.discNumber, subtitle) + } +} + +// 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. +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 { + 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 + 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 + } + } + + if len(digits) == 0 { + return 0, false + } + + num, err := strconv.Atoi(string(digits)) + if err != nil { + return 0, false + } + return num, true +} + +// 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). +func (d *discArtworkReader) fromExternalFile(ctx context.Context, pattern string) sourceFunc { + return func() (io.ReadCloser, string, error) { + for _, file := range d.imgFiles { + _, name := filepath.Split(file) + match, err := filepath.Match(pattern, strings.ToLower(name)) + if err != nil { + log.Warn(ctx, "Error matching disc art file to pattern", "pattern", pattern, "file", file) + continue + } + if !match { + 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 + } + } 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 + } + + 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 + } + return nil, "", fmt.Errorf("disc %d: pattern '%s' not matched by files", d.discNumber, pattern) + } +} diff --git a/core/artwork/reader_disc_test.go b/core/artwork/reader_disc_test.go new file mode 100644 index 00000000..f8193e24 --- /dev/null +++ b/core/artwork/reader_disc_test.go @@ -0,0 +1,285 @@ +package artwork + +import ( + "context" + "os" + "path/filepath" + + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Disc Artwork Reader", func() { + Describe("extractDiscNumber", func() { + DescribeTable("extracts disc number from filename based on glob pattern", + func(pattern, filename string, expectedNum int, expectedOk bool) { + num, ok := extractDiscNumber(pattern, filename) + Expect(ok).To(Equal(expectedOk)) + if expectedOk { + Expect(num).To(Equal(expectedNum)) + } + }, + // Standard disc patterns + Entry("disc1.jpg", "disc*.*", "disc1.jpg", 1, true), + Entry("disc2.png", "disc*.*", "disc2.png", 2, true), + Entry("disc01.jpg", "disc*.*", "disc01.jpg", 1, true), + Entry("disc02.png", "disc*.*", "disc02.png", 2, true), + Entry("disc10.jpg", "disc*.*", "disc10.jpg", 10, true), + + // CD patterns + Entry("cd1.jpg", "cd*.*", "cd1.jpg", 1, true), + Entry("cd02.png", "cd*.*", "cd02.png", 2, true), + + // No number in filename + Entry("disc.jpg has no number", "disc*.*", "disc.jpg", 0, false), + Entry("cd.jpg has no number", "cd*.*", "cd.jpg", 0, false), + + // Extra text after number + Entry("disc2-bonus.jpg", "disc*.*", "disc2-bonus.jpg", 2, true), + Entry("disc01_front.png", "disc*.*", "disc01_front.png", 1, true), + + // 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), + + // Pattern with no wildcard before dot + Entry("front1.jpg with front*.*", "front*.*", "front1.jpg", 1, true), + ) + }) + + Describe("fromExternalFile", func() { + var ( + ctx context.Context + tmpDir string + ) + + BeforeEach(func() { + ctx = context.Background() + tmpDir = GinkgoT().TempDir() + }) + + createFile := func(path string) string { + fullPath := filepath.Join(tmpDir, filepath.FromSlash(path)) + Expect(os.MkdirAll(filepath.Dir(fullPath), 0755)).To(Succeed()) + Expect(os.WriteFile(fullPath, []byte("image data"), 0600)).To(Succeed()) + return fullPath + } + + It("matches file with disc number in single-folder album", func() { + f1 := createFile("album/disc1.jpg") + f2 := createFile("album/disc2.jpg") + reader := &discArtworkReader{ + discNumber: 1, + imgFiles: []string{f1, f2}, + 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(f1)) + }) + + It("skips file without number in single-folder album", func() { + f1 := createFile("album/disc.jpg") + 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()) + }) + + It("matches file without number in multi-folder album by folder", func() { + f1 := createFile("album/cd1/disc.jpg") + f2 := createFile("album/cd2/disc.jpg") + reader := &discArtworkReader{ + discNumber: 1, + imgFiles: []string{f1, f2}, + discFolders: map[string]bool{filepath.Join(tmpDir, "album", "cd1"): true}, + isMultiFolder: true, + } + + sf := reader.fromExternalFile(ctx, "disc*.*") + r, path, err := sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(f1)) + }) + + It("prefers disc number over folder when number is present", func() { + // disc2.jpg in cd1 folder should match disc 2, not disc 1 + f1 := createFile("album/cd1/disc2.jpg") + reader := &discArtworkReader{ + discNumber: 2, + imgFiles: []string{f1}, + discFolders: map[string]bool{filepath.Join(tmpDir, "album", "cd1"): true}, + isMultiFolder: true, + } + + sf := reader.fromExternalFile(ctx, "disc*.*") + r, path, err := sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(f1)) + }) + + It("does not match disc2.jpg when looking for disc 1", func() { + f1 := createFile("album/disc2.jpg") + 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()) + }) + }) + + Describe("fromDiscSubtitle", func() { + var ( + ctx context.Context + tmpDir string + ) + + BeforeEach(func() { + ctx = context.Background() + tmpDir = GinkgoT().TempDir() + }) + + createFile := func(path string) string { + fullPath := filepath.Join(tmpDir, filepath.FromSlash(path)) + Expect(os.MkdirAll(filepath.Dir(fullPath), 0755)).To(Succeed()) + Expect(os.WriteFile(fullPath, []byte("image data"), 0600)).To(Succeed()) + return fullPath + } + + It("matches image file whose stem equals the disc subtitle (case-insensitive)", func() { + f1 := createFile("album/The Blue Disc.jpg") + reader := &discArtworkReader{ + discNumber: 1, + imgFiles: []string{f1}, + } + + sf := reader.fromDiscSubtitle(ctx, "The Blue Disc") + r, path, err := sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(f1)) + }) + + It("matches case-insensitively", func() { + f1 := createFile("album/bonus tracks.png") + reader := &discArtworkReader{ + discNumber: 2, + imgFiles: []string{f1}, + } + + sf := reader.fromDiscSubtitle(ctx, "Bonus Tracks") + r, path, err := sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(f1)) + }) + + It("returns error when no matching file found", func() { + f1 := createFile("album/cover.jpg") + reader := &discArtworkReader{ + discNumber: 1, + imgFiles: []string{f1}, + } + + sf := reader.fromDiscSubtitle(ctx, "The Blue Disc") + _, _, err := sf() + Expect(err).To(HaveOccurred()) + }) + + It("matches first file when multiple extensions exist", func() { + f1 := createFile("album/The Blue Disc.jpg") + f2 := createFile("album/The Blue Disc.png") + reader := &discArtworkReader{ + discNumber: 1, + imgFiles: []string{f1, f2}, + } + + sf := reader.fromDiscSubtitle(ctx, "The Blue Disc") + r, path, err := sf() + Expect(err).ToNot(HaveOccurred()) + Expect(r).ToNot(BeNil()) + r.Close() + Expect(path).To(Equal(f1)) + }) + }) + + Describe("discArtworkReader", func() { + Describe("fromDiscArtPriority", func() { + var reader *discArtworkReader + + BeforeEach(func() { + reader = &discArtworkReader{ + discNumber: 2, + isMultiFolder: true, + discFolders: map[string]bool{"/music/album/cd2": true}, + imgFiles: []string{ + "/music/album/cd1/disc.jpg", + "/music/album/cd2/disc.jpg", + "/music/album/cd2/disc2.jpg", + }, + firstTrackPath: "/music/album/cd2/track1.flac", + } + }) + + It("returns source funcs for glob patterns", func() { + ff := reader.fromDiscArtPriority(context.Background(), nil, "disc*.*") + Expect(ff).To(HaveLen(1)) + }) + + It("returns source funcs for embedded pattern", func() { + ff := reader.fromDiscArtPriority(context.Background(), nil, "embedded") + Expect(ff).To(HaveLen(2)) // fromTag + fromFFmpegTag + }) + + It("handles multiple comma-separated patterns", func() { + ff := reader.fromDiscArtPriority(context.Background(), nil, "disc*.*, cd*.*, embedded") + Expect(ff).To(HaveLen(4)) // disc*.* + cd*.* + fromTag + fromFFmpegTag + }) + + It("ignores 'external' pattern silently", func() { + ff := reader.fromDiscArtPriority(context.Background(), nil, "external") + Expect(ff).To(HaveLen(0)) + }) + + It("returns no source funcs when imgFiles is empty and pattern is not embedded", func() { + reader.imgFiles = nil + ff := reader.fromDiscArtPriority(context.Background(), nil, "disc*.*") + Expect(ff).To(HaveLen(0)) + }) + + It("returns source func for discsubtitle pattern", func() { + reader.album = model.Album{Discs: model.Discs{2: "Bonus Tracks"}} + ff := reader.fromDiscArtPriority(context.Background(), nil, "discsubtitle") + Expect(ff).To(HaveLen(1)) + }) + + It("returns no source func for discsubtitle when disc has no subtitle", func() { + reader.album = model.Album{Discs: model.Discs{2: ""}} + ff := reader.fromDiscArtPriority(context.Background(), nil, "discsubtitle") + Expect(ff).To(HaveLen(0)) + }) + }) + }) +}) diff --git a/model/artwork_id.go b/model/artwork_id.go index 36026dd0..8d935f42 100644 --- a/model/artwork_id.go +++ b/model/artwork_id.go @@ -22,6 +22,7 @@ var ( KindArtistArtwork = Kind{"ar", "artist"} KindAlbumArtwork = Kind{"al", "album"} KindPlaylistArtwork = Kind{"pl", "playlist"} + KindDiscArtwork = Kind{"dc", "disc"} ) var artworkKindMap = map[string]Kind{ @@ -29,6 +30,7 @@ var artworkKindMap = map[string]Kind{ KindArtistArtwork.prefix: KindArtistArtwork, KindAlbumArtwork.prefix: KindAlbumArtwork, KindPlaylistArtwork.prefix: KindPlaylistArtwork, + KindDiscArtwork.prefix: KindDiscArtwork, } type ArtworkID struct { @@ -91,6 +93,22 @@ func MustParseArtworkID(id string) ArtworkID { return artID } +func DiscArtworkID(albumID string, discNumber int) string { + return fmt.Sprintf("%s:%d", albumID, discNumber) +} + +func ParseDiscArtworkID(id string) (albumID string, discNumber int, err error) { + parts := strings.SplitN(id, ":", 2) + if len(parts) != 2 || parts[1] == "" { + return "", 0, errors.New("invalid disc artwork id") + } + num, err := strconv.Atoi(parts[1]) + if err != nil { + return "", 0, fmt.Errorf("invalid disc number in artwork id: %w", err) + } + return parts[0], num, nil +} + func artworkIDFromAlbum(al Album) ArtworkID { return ArtworkID{ Kind: KindAlbumArtwork, diff --git a/model/artwork_id_test.go b/model/artwork_id_test.go index 2f42217f..b634e7cb 100644 --- a/model/artwork_id_test.go +++ b/model/artwork_id_test.go @@ -28,6 +28,40 @@ var _ = Describe("ArtworkID", func() { Expect(parsedId.LastUpdate.Unix()).To(Equal(id.LastUpdate.Unix())) }) }) + Describe("ParseArtworkID - disc kind", func() { + It("parses a disc artwork ID with dc prefix", func() { + now := time.Now() + id := model.NewArtworkID(model.KindDiscArtwork, "albumid123:2", &now) + parsedId, err := model.ParseArtworkID(id.String()) + Expect(err).ToNot(HaveOccurred()) + Expect(parsedId.Kind).To(Equal(model.KindDiscArtwork)) + Expect(parsedId.ID).To(Equal("albumid123:2")) + Expect(parsedId.LastUpdate.Unix()).To(Equal(now.Unix())) + }) + }) + + Describe("ParseDiscArtworkID", func() { + DescribeTable("parses composite disc artwork IDs", + func(id string, expectedAlbum string, expectedDisc int, expectErr bool) { + albumID, discNumber, err := model.ParseDiscArtworkID(id) + if expectErr { + Expect(err).To(HaveOccurred()) + } else { + Expect(err).ToNot(HaveOccurred()) + Expect(albumID).To(Equal(expectedAlbum)) + Expect(discNumber).To(Equal(expectedDisc)) + } + }, + Entry("valid id", "albumid123:2", "albumid123", 2, false), + Entry("disc number 1", "abc:1", "abc", 1, false), + Entry("large disc number", "abc:10", "abc", 10, false), + Entry("missing colon", "albumid123", "", 0, true), + Entry("missing disc number", "albumid123:", "", 0, true), + Entry("non-numeric disc", "albumid123:abc", "", 0, true), + Entry("empty string", "", "", 0, true), + ) + }) + Describe("ParseArtworkID()", func() { It("parses album artwork ids", func() { id, err := model.ParseArtworkID("al-1234") diff --git a/server/subsonic/helpers.go b/server/subsonic/helpers.go index 59834690..e930aa63 100644 --- a/server/subsonic/helpers.go +++ b/server/subsonic/helpers.go @@ -392,7 +392,13 @@ func buildDiscSubtitles(a model.Album) []responses.DiscTitle { } var discTitles []responses.DiscTitle for num, title := range a.Discs { - discTitles = append(discTitles, responses.DiscTitle{Disc: int32(num), Title: title}) + artID := model.NewArtworkID(model.KindDiscArtwork, + model.DiscArtworkID(a.ID, num), &a.UpdatedAt) + discTitles = append(discTitles, responses.DiscTitle{ + Disc: int32(num), + Title: title, + CoverArt: artID.String(), + }) } if len(discTitles) == 1 && discTitles[0].Title == "" { return nil diff --git a/server/subsonic/helpers_test.go b/server/subsonic/helpers_test.go index 2099c8f6..d20ca89a 100644 --- a/server/subsonic/helpers_test.go +++ b/server/subsonic/helpers_test.go @@ -3,6 +3,7 @@ package subsonic import ( "context" "net/http/httptest" + "time" "github.com/go-chi/jwtauth/v5" "github.com/navidrome/navidrome/conf" @@ -103,27 +104,43 @@ var _ = Describe("helpers", func() { Expect(buildDiscSubtitles(album)).To(BeNil()) }) - It("should return the disc title for a single disc", func() { + It("should return the disc title with cover art for a single disc", func() { + updatedAt := time.Now().Truncate(time.Second) album := model.Album{ + ID: "album1", + UpdatedAt: updatedAt, Discs: map[int]string{ 1: "Special Edition", }, } - Expect(buildDiscSubtitles(album)).To(Equal([]responses.DiscTitle{{Disc: 1, Title: "Special Edition"}})) + result := buildDiscSubtitles(album) + Expect(result).To(HaveLen(1)) + Expect(result[0].Disc).To(Equal(int32(1))) + Expect(result[0].Title).To(Equal("Special Edition")) + expectedArtID := model.NewArtworkID(model.KindDiscArtwork, "album1:1", &updatedAt) + Expect(result[0].CoverArt).To(Equal(expectedArtID.String())) }) - It("should return correct disc titles when album has discs with valid disc numbers", func() { + It("should return correct disc titles with cover art when album has multiple discs", func() { + updatedAt := time.Now().Truncate(time.Second) album := model.Album{ + ID: "album1", + UpdatedAt: updatedAt, Discs: map[int]string{ 1: "Disc 1", 2: "Disc 2", }, } - expected := []responses.DiscTitle{ - {Disc: 1, Title: "Disc 1"}, - {Disc: 2, Title: "Disc 2"}, - } - Expect(buildDiscSubtitles(album)).To(Equal(expected)) + result := buildDiscSubtitles(album) + Expect(result).To(HaveLen(2)) + Expect(result[0].Disc).To(Equal(int32(1))) + Expect(result[0].Title).To(Equal("Disc 1")) + expectedArtID1 := model.NewArtworkID(model.KindDiscArtwork, "album1:1", &updatedAt) + Expect(result[0].CoverArt).To(Equal(expectedArtID1.String())) + Expect(result[1].Disc).To(Equal(int32(2))) + Expect(result[1].Title).To(Equal("Disc 2")) + expectedArtID2 := model.NewArtworkID(model.KindDiscArtwork, "album1:2", &updatedAt) + Expect(result[1].CoverArt).To(Equal(expectedArtID2.String())) }) }) diff --git a/server/subsonic/responses/responses.go b/server/subsonic/responses/responses.go index be59e585..c550ce61 100644 --- a/server/subsonic/responses/responses.go +++ b/server/subsonic/responses/responses.go @@ -575,8 +575,9 @@ func (r ReplayGain) MarshalXML(e *xml.Encoder, start xml.StartElement) error { } type DiscTitle struct { - Disc int32 `xml:"disc,attr" json:"disc"` - Title string `xml:"title,attr" json:"title"` + Disc int32 `xml:"disc,attr" json:"disc"` + Title string `xml:"title,attr" json:"title"` + CoverArt string `xml:"coverArt,attr,omitempty" json:"coverArt,omitempty"` } type ItemDate struct { diff --git a/ui/src/common/SongDatagrid.jsx b/ui/src/common/SongDatagrid.jsx index 3586cf22..fbf2b9e3 100644 --- a/ui/src/common/SongDatagrid.jsx +++ b/ui/src/common/SongDatagrid.jsx @@ -1,4 +1,10 @@ -import React, { isValidElement, useMemo, useCallback, forwardRef } from 'react' +import React, { + isValidElement, + useMemo, + useCallback, + useState, + forwardRef, +} from 'react' import { useDispatch } from 'react-redux' import { Datagrid, @@ -17,7 +23,10 @@ import { makeStyles } from '@material-ui/core/styles' import AlbumIcon from '@material-ui/icons/Album' import clsx from 'clsx' import { useDrag } from 'react-dnd' +import Lightbox from 'react-image-lightbox' +import 'react-image-lightbox/style.css' import { playTracks } from '../actions' +import subsonic from '../subsonic' import { AlbumContextMenu } from '../common' import { DraggableTypes } from '../consts' import { formatFullDate } from '../utils' @@ -28,10 +37,20 @@ const useStyles = makeStyles({ overflow: 'hidden', textOverflow: 'ellipsis', verticalAlign: 'middle', + display: 'flex', + alignItems: 'center', }, discIcon: { - verticalAlign: 'text-top', - marginRight: '4px', + marginRight: '14px', + }, + discCoverArt: { + width: '48px', + height: '48px', + marginRight: '14px', + objectFit: 'cover', + borderRadius: '4px', + flexShrink: 0, + cursor: 'pointer', }, row: { cursor: 'pointer', @@ -61,19 +80,45 @@ const useStyles = makeStyles({ const DiscSubtitleRow = forwardRef( ({ record, onClick, colSpan, contextAlwaysVisible }, ref) => { + const translate = useTranslate() const isDesktop = useMediaQuery((theme) => theme.breakpoints.up('md')) const classes = useStyles({ isDesktop }) + const [imageError, setImageError] = useState(false) + const [isLightboxOpen, setLightboxOpen] = useState(false) const handlePlaySubset = (discNumber) => () => { onClick(discNumber) } - let subtitle = [] - if (record.discNumber > 0) { - subtitle.push(record.discNumber) - } - if (record.discSubtitle) { - subtitle.push(record.discSubtitle) - } + const coverArtUrl = subsonic.getDiscCoverArtUrl( + record.albumId, + record.discNumber, + record.updatedAt, + 96, + ) + + const fullImageUrl = subsonic.getDiscCoverArtUrl( + record.albumId, + record.discNumber, + record.updatedAt, + ) + + const handleOpenLightbox = useCallback( + (e) => { + if (!imageError) { + e.stopPropagation() + setLightboxOpen(true) + } + }, + [imageError], + ) + + const handleCloseLightbox = useCallback(() => setLightboxOpen(false), []) + + const subtitle = record.discSubtitle + ? record.discSubtitle + : translate('resources.song.fields.disc', { + discNumber: record.discNumber, + }) return ( - - {subtitle.join(': ')} + {!imageError ? ( + setImageError(true)} + /> + ) : ( + + )} + {subtitle} + {isLightboxOpen && !imageError && ( + + )} { } } +const getDiscCoverArtUrl = (albumId, discNumber, updatedAt, size) => { + const options = { + ...(updatedAt && { _: updatedAt }), + ...(size && { size }), + } + return baseUrl( + url('getCoverArt', 'dc-' + albumId + ':' + discNumber, options), + ) +} + const getArtistInfo = (id) => { return httpClient(url('getArtistInfo', id)) } @@ -129,6 +139,7 @@ export default { getScanStatus, getNowPlaying, getCoverArtUrl, + getDiscCoverArtUrl, getAvatarUrl, streamUrl, getAlbumInfo, diff --git a/ui/src/subsonic/index.test.js b/ui/src/subsonic/index.test.js index 1e0fbeaa..2544e03f 100644 --- a/ui/src/subsonic/index.test.js +++ b/ui/src/subsonic/index.test.js @@ -105,6 +105,56 @@ describe('getCoverArtUrl', () => { }) }) +describe('getDiscCoverArtUrl', () => { + beforeEach(() => { + const localStorageMock = { + getItem: vi.fn((key) => { + const values = { + username: 'testuser', + 'subsonic-token': 'testtoken', + 'subsonic-salt': 'testsalt', + } + return values[key] || null + }), + } + Object.defineProperty(window, 'localStorage', { value: localStorageMock }) + }) + + it('should construct URL with dc-albumId:discNumber format, size, and cache param', () => { + const url = subsonic.getDiscCoverArtUrl( + 'album-123', + 2, + '2023-01-01T00:00:00Z', + 48, + ) + + expect(url).toContain('getCoverArt') + expect(url).toContain('id=dc-album-123%3A2') + expect(url).toContain('size=48') + expect(url).toContain('_=2023-01-01T00%3A00%3A00Z') + }) + + it('should handle missing updatedAt', () => { + const url = subsonic.getDiscCoverArtUrl('album-123', 1, undefined, 48) + + expect(url).toContain('id=dc-album-123%3A1') + expect(url).toContain('size=48') + expect(url).not.toContain('_=') + }) + + it('should handle missing size', () => { + const url = subsonic.getDiscCoverArtUrl( + 'album-123', + 1, + '2023-01-01T00:00:00Z', + ) + + expect(url).toContain('id=dc-album-123%3A1') + expect(url).toContain('_=2023-01-01T00%3A00%3A00Z') + expect(url).not.toContain('size=') + }) +}) + describe('getAvatarUrl', () => { beforeEach(() => { // Mock localStorage values required by subsonic