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

@@ -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 {
+var (
-	// Check if the /.nddockerenv file exists
+	ServerStart = time.Now()
-	if _, err := os.Stat("/.nddockerenv"); err == nil {
+
-		return true
+	InContainer = func() bool {
-	}
+		// Check if the /.nddockerenv file exists
-	return false
+		if _, err := os.Stat("/.nddockerenv"); err == nil {
-}()
+			return true
+		}
+		return false
+	}()
+)

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

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 {
+func (md Metadata) mapDisplayName(singularTagName, pluralTagName model.TagName) string {
-	for _, tagName := range tagNames {
+	return cmp.Or(
-		values := md.Strings(tagName)
+		strings.Join(md.tags[singularTagName], consts.ArtistJoiner),
-		if len(values) > 0 {
+		strings.Join(md.tags[pluralTagName], consts.ArtistJoiner),
-			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) mapDisplayArtist(mf model.MediaFile) string {
+func (md Metadata) mapDisplayArtist() string {
-	return md.mapDisplayRole(mf, model.RoleArtist, model.TagTrackArtist, model.TagTrackArtists)
+	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,
+	)
 }

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() {
+			It("should use the full string as display name", func() {
-				By("keeping the first artist as the display name")
 				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() {
+			It("should concatenate all ARTIST values as display name", func() {
-				Expect(mf.Artist).To(Equal("First Artist"))
+				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() {
+			It("should use ARTIST values concatenated as a display name ", func() {
-				Expect(mf.Artist).To(Equal("First Artist + Second Artist"))
+				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 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() {

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