From 2adb098f3232f4597f404ce9f5bdac7c3c5e26b5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= <deluan@navidrome.org>
Date: Mon, 17 Mar 2025 19:21:33 -0400
Subject: [PATCH] fix(scanner): fix displayArtist logic (#3835)
* fix displayArtist logic
Signed-off-by: Deluan <deluan@navidrome.org>
* remove unneeded value
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor
Signed-off-by: Deluan <deluan@navidrome.org>
* Use first albumartist if it cannot figure out the display name
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
---
consts/consts.go | 20 +--
model/metadata/map_mediafile.go | 2 +-
model/metadata/map_participants.go | 43 +++----
model/metadata/map_participants_test.go | 163 ++++++++++++++++++++++--
server/subsonic/helpers.go | 2 +-
5 files changed, 186 insertions(+), 44 deletions(-)
diff --git a/consts/consts.go b/consts/consts.go
index 7f46fe39a..75271bec8 100644
--- a/consts/consts.go
+++ b/consts/consts.go
@@ -151,13 +151,17 @@ var (
UnknownArtistID = id.NewHash(strings.ToLower(UnknownArtist))
VariousArtistsMbzId = "89ad4ac3-39f7-470e-963a-56509c546377"
- ServerStart = time.Now()
+ ArtistJoiner = " • "
)
-var InContainer = func() bool {
- // Check if the /.nddockerenv file exists
- if _, err := os.Stat("/.nddockerenv"); err == nil {
- return true
- }
- return false
-}()
+var (
+ ServerStart = time.Now()
+
+ InContainer = func() bool {
+ // Check if the /.nddockerenv file exists
+ if _, err := os.Stat("/.nddockerenv"); err == nil {
+ return true
+ }
+ return false
+ }()
+)
diff --git a/model/metadata/map_mediafile.go b/model/metadata/map_mediafile.go
index 53c5a8db2..47d2578ec 100644
--- a/model/metadata/map_mediafile.go
+++ b/model/metadata/map_mediafile.go
@@ -72,7 +72,7 @@ func (md Metadata) ToMediaFile(libID int, folderID string) model.MediaFile {
mf.UpdatedAt = md.ModTime()
mf.Participants = md.mapParticipants()
- mf.Artist = md.mapDisplayArtist(mf)
+ mf.Artist = md.mapDisplayArtist()
mf.AlbumArtist = md.mapDisplayAlbumArtist(mf)
// Persistent IDs
diff --git a/model/metadata/map_participants.go b/model/metadata/map_participants.go
index 9d47c676d..9305d8791 100644
--- a/model/metadata/map_participants.go
+++ b/model/metadata/map_participants.go
@@ -2,6 +2,7 @@ package metadata
import (
"cmp"
+ "strings"
"github.com/navidrome/navidrome/consts"
"github.com/navidrome/navidrome/model"
@@ -199,32 +200,28 @@ func (md Metadata) getArtistValues(single, multi model.TagName) []string {
return vSingle
}
-func (md Metadata) getTags(tagNames ...model.TagName) []string {
- for _, tagName := range tagNames {
- values := md.Strings(tagName)
- if len(values) > 0 {
- return values
- }
- }
- return nil
-}
-func (md Metadata) mapDisplayRole(mf model.MediaFile, role model.Role, tagNames ...model.TagName) string {
- artistNames := md.getTags(tagNames...)
- values := []string{
- "",
- mf.Participants.First(role).Name,
- consts.UnknownArtist,
- }
- if len(artistNames) == 1 {
- values[0] = artistNames[0]
- }
- return cmp.Or(values...)
+func (md Metadata) mapDisplayName(singularTagName, pluralTagName model.TagName) string {
+ return cmp.Or(
+ strings.Join(md.tags[singularTagName], consts.ArtistJoiner),
+ strings.Join(md.tags[pluralTagName], consts.ArtistJoiner),
+ )
}
-func (md Metadata) mapDisplayArtist(mf model.MediaFile) string {
- return md.mapDisplayRole(mf, model.RoleArtist, model.TagTrackArtist, model.TagTrackArtists)
+func (md Metadata) mapDisplayArtist() string {
+ return cmp.Or(
+ md.mapDisplayName(model.TagTrackArtist, model.TagTrackArtists),
+ consts.UnknownArtist,
+ )
}
func (md Metadata) mapDisplayAlbumArtist(mf model.MediaFile) string {
- return md.mapDisplayRole(mf, model.RoleAlbumArtist, model.TagAlbumArtist, model.TagAlbumArtists)
+ fallbackName := consts.UnknownArtist
+ if md.Bool(model.TagCompilation) {
+ fallbackName = consts.VariousArtists
+ }
+ return cmp.Or(
+ md.mapDisplayName(model.TagAlbumArtist, model.TagAlbumArtists),
+ mf.Participants.First(model.RoleAlbumArtist).Name,
+ fallbackName,
+ )
}
diff --git a/model/metadata/map_participants_test.go b/model/metadata/map_participants_test.go
index a1c8ed527..5317a4bcf 100644
--- a/model/metadata/map_participants_test.go
+++ b/model/metadata/map_participants_test.go
@@ -45,6 +45,10 @@ var _ = Describe("Participants", func() {
mf = toMediaFile(model.RawTags{})
})
+ It("should set the display name to Unknown Artist", func() {
+ Expect(mf.Artist).To(Equal("[Unknown Artist]"))
+ })
+
It("should set artist to Unknown Artist", func() {
Expect(mf.Artist).To(Equal("[Unknown Artist]"))
})
@@ -92,6 +96,7 @@ var _ = Describe("Participants", func() {
Expect(artist.MbzArtistID).To(Equal(mbid1))
})
})
+
Context("Multiple values in a Single-valued ARTIST tags, no ARTISTS tags", func() {
BeforeEach(func() {
mf = toMediaFile(model.RawTags{
@@ -101,12 +106,13 @@ var _ = Describe("Participants", func() {
})
})
- It("should split the tag", func() {
- By("keeping the first artist as the display name")
+ It("should use the full string as display name", func() {
Expect(mf.Artist).To(Equal("Artist Name feat. Someone Else"))
Expect(mf.SortArtistName).To(Equal("Name, Artist"))
Expect(mf.OrderArtistName).To(Equal("artist name"))
+ })
+ It("should split the tag", func() {
participants := mf.Participants
Expect(participants).To(SatisfyAll(
HaveKeyWithValue(model.RoleArtist, HaveLen(2)),
@@ -130,6 +136,7 @@ var _ = Describe("Participants", func() {
Expect(artist1.SortArtistName).To(Equal("Else, Someone"))
Expect(artist1.MbzArtistID).To(BeEmpty())
})
+
It("should split the tag using case-insensitive separators", func() {
mf = toMediaFile(model.RawTags{
"ARTIST": {"A1 FEAT. A2"},
@@ -167,8 +174,8 @@ var _ = Describe("Participants", func() {
})
})
- It("should use the first artist name as display name", func() {
- Expect(mf.Artist).To(Equal("First Artist"))
+ It("should concatenate all ARTIST values as display name", func() {
+ Expect(mf.Artist).To(Equal("First Artist • Second Artist"))
})
It("should populate the participants with all artists", func() {
@@ -194,6 +201,101 @@ var _ = Describe("Participants", func() {
})
})
+ Context("Single-valued ARTIST tag, single-valued ARTISTS tag, same values", func() {
+ BeforeEach(func() {
+ mf = toMediaFile(model.RawTags{
+ "ARTIST": {"Artist Name"},
+ "ARTISTS": {"Artist Name"},
+ "ARTISTSORT": {"Name, Artist"},
+ "MUSICBRAINZ_ARTISTID": {mbid1},
+ })
+ })
+
+ It("should use the ARTIST tag as display name", func() {
+ Expect(mf.Artist).To(Equal("Artist Name"))
+ })
+
+ It("should populate the participants with the ARTIST", func() {
+ participants := mf.Participants
+ Expect(participants).To(HaveLen(2)) // ARTIST and ALBUMARTIST
+ Expect(participants).To(SatisfyAll(
+ HaveKeyWithValue(model.RoleArtist, HaveLen(1)),
+ ))
+
+ artist := participants[model.RoleArtist][0]
+ Expect(artist.ID).ToNot(BeEmpty())
+ Expect(artist.Name).To(Equal("Artist Name"))
+ Expect(artist.OrderArtistName).To(Equal("artist name"))
+ Expect(artist.SortArtistName).To(Equal("Name, Artist"))
+ Expect(artist.MbzArtistID).To(Equal(mbid1))
+ })
+ })
+
+ Context("Single-valued ARTIST tag, single-valued ARTISTS tag, different values", func() {
+ BeforeEach(func() {
+ mf = toMediaFile(model.RawTags{
+ "ARTIST": {"Artist Name"},
+ "ARTISTS": {"Artist Name 2"},
+ "ARTISTSORT": {"Name, Artist"},
+ "MUSICBRAINZ_ARTISTID": {mbid1},
+ })
+ })
+
+ It("should use the ARTIST tag as display name", func() {
+ Expect(mf.Artist).To(Equal("Artist Name"))
+ })
+
+ It("should use only artists from ARTISTS", func() {
+ participants := mf.Participants
+ Expect(participants).To(HaveLen(2)) // ARTIST and ALBUMARTIST
+ Expect(participants).To(SatisfyAll(
+ HaveKeyWithValue(model.RoleArtist, HaveLen(1)),
+ ))
+
+ artist := participants[model.RoleArtist][0]
+ Expect(artist.ID).ToNot(BeEmpty())
+ Expect(artist.Name).To(Equal("Artist Name 2"))
+ Expect(artist.OrderArtistName).To(Equal("artist name 2"))
+ Expect(artist.SortArtistName).To(Equal("Name, Artist"))
+ Expect(artist.MbzArtistID).To(Equal(mbid1))
+ })
+ })
+
+ Context("No ARTIST tag, multi-valued ARTISTS tag", func() {
+ BeforeEach(func() {
+ mf = toMediaFile(model.RawTags{
+ "ARTISTS": {"First Artist", "Second Artist"},
+ "ARTISTSSORT": {"Name, First Artist", "Name, Second Artist"},
+ })
+ })
+
+ It("should concatenate ARTISTS as display name", func() {
+ Expect(mf.Artist).To(Equal("First Artist • Second Artist"))
+ })
+
+ It("should populate the participants with all artists", func() {
+ participants := mf.Participants
+ Expect(participants).To(HaveLen(2)) // ARTIST and ALBUMARTIST
+ Expect(participants).To(SatisfyAll(
+ HaveKeyWithValue(model.RoleArtist, HaveLen(2)),
+ ))
+
+ artist0 := participants[model.RoleArtist][0]
+ Expect(artist0.ID).ToNot(BeEmpty())
+ Expect(artist0.Name).To(Equal("First Artist"))
+ Expect(artist0.OrderArtistName).To(Equal("first artist"))
+ Expect(artist0.SortArtistName).To(Equal("Name, First Artist"))
+ Expect(artist0.MbzArtistID).To(BeEmpty())
+
+ artist1 := participants[model.RoleArtist][1]
+ Expect(artist1.ID).ToNot(BeEmpty())
+ Expect(artist1.Name).To(Equal("Second Artist"))
+ Expect(artist1.OrderArtistName).To(Equal("second artist"))
+ Expect(artist1.SortArtistName).To(Equal("Name, Second Artist"))
+ Expect(artist1.MbzArtistID).To(BeEmpty())
+ })
+ })
+
Context("Single-valued ARTIST tags, multi-valued ARTISTS tags", func() {
BeforeEach(func() {
mf = toMediaFile(model.RawTags{
@@ -231,6 +333,7 @@ var _ = Describe("Participants", func() {
})
})
+ // Not a good tagging strategy, but supported anyway.
Context("Multi-valued ARTIST tags, multi-valued ARTISTS tags", func() {
BeforeEach(func() {
mf = toMediaFile(model.RawTags{
@@ -242,13 +345,8 @@ var _ = Describe("Participants", func() {
})
})
- XIt("should use the values concatenated as a display name ", func() {
- Expect(mf.Artist).To(Equal("First Artist + Second Artist"))
- })
-
- // TODO: remove when the above is implemented
- It("should use the first artist name as display name", func() {
- Expect(mf.Artist).To(Equal("First Artist 2"))
+ It("should use ARTIST values concatenated as a display name ", func() {
+ Expect(mf.Artist).To(Equal("First Artist • Second Artist"))
})
It("should prioritize ARTISTS tags", func() {
@@ -275,6 +373,7 @@ var _ = Describe("Participants", func() {
})
Describe("ALBUMARTIST(S) tags", func() {
+ // Only test specific scenarios for ALBUMARTIST(S) tags, as the logic is the same as for ARTIST(S) tags.
Context("No ALBUMARTIST/ALBUMARTISTS tags", func() {
When("the COMPILATION tag is not set", func() {
BeforeEach(func() {
@@ -305,6 +404,35 @@ var _ = Describe("Participants", func() {
})
})
+ When("the COMPILATION tag is not set and there is no ALBUMARTIST tag", func() {
+ BeforeEach(func() {
+ mf = toMediaFile(model.RawTags{
+ "ARTIST": {"Artist Name", "Another Artist"},
+ "ARTISTSORT": {"Name, Artist", "Artist, Another"},
+ })
+ })
+
+ It("should use the first ARTIST as ALBUMARTIST", func() {
+ Expect(mf.AlbumArtist).To(Equal("Artist Name"))
+ })
+
+ It("should add the ARTIST to participants as ALBUMARTIST", func() {
+ participants := mf.Participants
+ Expect(participants).To(HaveLen(2))
+ Expect(participants).To(SatisfyAll(
+ HaveKeyWithValue(model.RoleAlbumArtist, HaveLen(2)),
+ ))
+
+ albumArtist := participants[model.RoleAlbumArtist][0]
+ Expect(albumArtist.Name).To(Equal("Artist Name"))
+ Expect(albumArtist.SortArtistName).To(Equal("Name, Artist"))
+
+ albumArtist = participants[model.RoleAlbumArtist][1]
+ Expect(albumArtist.Name).To(Equal("Another Artist"))
+ Expect(albumArtist.SortArtistName).To(Equal("Artist, Another"))
+ })
+ })
+
When("the COMPILATION tag is true", func() {
BeforeEach(func() {
mf = toMediaFile(model.RawTags{
@@ -331,6 +459,19 @@ var _ = Describe("Participants", func() {
Expect(albumArtist.MbzArtistID).To(Equal(consts.VariousArtistsMbzId))
})
})
+
+ When("the COMPILATION tag is true and there are ALBUMARTIST tags", func() {
+ BeforeEach(func() {
+ mf = toMediaFile(model.RawTags{
+ "COMPILATION": {"1"},
+ "ALBUMARTIST": {"Album Artist Name 1", "Album Artist Name 2"},
+ })
+ })
+
+ It("should use the ALBUMARTIST names as display name", func() {
+ Expect(mf.AlbumArtist).To(Equal("Album Artist Name 1 • Album Artist Name 2"))
+ })
+ })
})
Context("ALBUMARTIST tag is set", func() {
diff --git a/server/subsonic/helpers.go b/server/subsonic/helpers.go
index 0fb35a7b7..fa98a985b 100644
--- a/server/subsonic/helpers.go
+++ b/server/subsonic/helpers.go
@@ -241,7 +241,7 @@ func osChildFromMediaFile(ctx context.Context, mf model.MediaFile) *responses.Op
child.DisplayAlbumArtist = mf.AlbumArtist
child.AlbumArtists = artistRefs(mf.Participants[model.RoleAlbumArtist])
var contributors []responses.Contributor
- child.DisplayComposer = mf.Participants[model.RoleComposer].Join(" • ")
+ child.DisplayComposer = mf.Participants[model.RoleComposer].Join(consts.ArtistJoiner)
for role, participants := range mf.Participants {
if role == model.RoleArtist || role == model.RoleAlbumArtist {
continue