From 26f0033ebb5f8e7c204a042709e3dfa885eac868 Mon Sep 17 00:00:00 2001
From: Florian Fischer <florian.fischer@muhq.space>
Date: Sat, 22 Jul 2023 15:15:47 +0200
Subject: fix applyTimeBonus by always using Song pointers

Storing song objects in the Songs slice and using pointers to the
underlying array slots breaks if the underlying array must be grown.
When the array is grown the objects are copied to the new array and
the pointers used in queue point to the old objects.
This causes that Playlist.applyTimeBonus may not correctly apply
to the most recent list of songs.

This is fixed by always using Song pointers.
---
 backend.go       |  8 ++++----
 localBackend.go  |  4 ++--
 player.go        |  6 +++---
 playlist.go      |  3 ++-
 playlist_test.go | 38 ++++++++++++++++++------------------
 spotify.go       |  6 +++---
 upload.go        |  2 +-
 wrms.go          | 59 ++++++++++++++++++++++++++++----------------------------
 wrms_test.go     | 42 ++++++++++++++++++++++++++++------------
 youtube.go       |  4 ++--
 10 files changed, 96 insertions(+), 76 deletions(-)

diff --git a/backend.go b/backend.go
index dd34fcc..fb9a3d1 100644
--- a/backend.go
+++ b/backend.go
@@ -7,7 +7,7 @@ import (
 
 type Backend interface {
 	Play(song *Song, player Player)
-	Search(map[string]string) []Song
+	Search(map[string]string) []*Song
 	OnSongFinished(song *Song)
 }
 
@@ -15,12 +15,12 @@ type DummyBackend struct{}
 
 func (dummy *DummyBackend) Play(song *Song, player Player) {}
 func (dummy *DummyBackend) OnSongFinished(song *Song)      {}
-func (dummy *DummyBackend) Search(map[string]string) []Song {
+func (dummy *DummyBackend) Search(map[string]string) []*Song {
 	s := NewDummySong("Dummy Mc Crashtest", "foo")
-	return []Song{s}
+	return []*Song{s}
 }
 
-func NewDummySong(title, artist string) Song {
+func NewDummySong(title, artist string) *Song {
 	h := sha1.New()
 	h.Write([]byte(title))
 	h.Write([]byte(artist))
diff --git a/localBackend.go b/localBackend.go
index cb07ab1..e59bf03 100644
--- a/localBackend.go
+++ b/localBackend.go
@@ -114,7 +114,7 @@ func (b *LocalBackend) findSongs() {
 		s := NewSong(m.Title(), m.Artist(), "local", p)
 		s.Album = m.Album()
 		s.Year = m.Year()
-		songs = append(songs, &s)
+		songs = append(songs, s)
 		return nil
 	})
 
@@ -129,7 +129,7 @@ func (b *LocalBackend) Play(song *Song, player Player) {
 	player.PlayUri("file://" + song.Uri)
 }
 
-func (b *LocalBackend) Search(patterns map[string]string) (results []Song) {
+func (b *LocalBackend) Search(patterns map[string]string) (results []*Song) {
 	pattern := strings.ToLower(patterns["pattern"])
 
 	query := fmt.Sprintf("SELECT * FROM songs WHERE Title LIKE '%%%s%%' OR Artist LIKE '%%%s%%'",
diff --git a/player.go b/player.go
index 461677c..795978b 100644
--- a/player.go
+++ b/player.go
@@ -14,7 +14,7 @@ import (
 type Player interface {
 	Play(*Song)
 	Playing() bool
-	Search(map[string]string) chan []Song
+	Search(map[string]string) chan []*Song
 	PlayUri(string)
 	PlayData(io.Reader)
 	Pause()
@@ -232,11 +232,11 @@ func (player *MpvPlayer) _stop() {
 	player.mpv.Store(nil)
 }
 
-func (player *MpvPlayer) Search(pattern map[string]string) chan []Song {
+func (player *MpvPlayer) Search(pattern map[string]string) chan []*Song {
 	var wg sync.WaitGroup
 	wg.Add(len(player.Backends))
 
-	ch := make(chan []Song)
+	ch := make(chan []*Song)
 
 	for _, backend := range player.Backends {
 		backend := backend
diff --git a/playlist.go b/playlist.go
index aeb1297..cb9c90c 100644
--- a/playlist.go
+++ b/playlist.go
@@ -78,7 +78,8 @@ func (pl *Playlist) OrderedList() []*Song {
 func (pl *Playlist) applyTimeBonus(timeBonus float64) {
 	for _, s := range *pl {
 		s.Weight += timeBonus
+		llog.DDebug("%p %v", s, s.Weight)
 	}
 	heap.Init(pl)
-	llog.DDebug("applying time bonus to the playlist")
+	llog.DDebug("applying time bonus to the playlist %v", *pl)
 }
diff --git a/playlist_test.go b/playlist_test.go
index 2ccad7c..8730737 100644
--- a/playlist_test.go
+++ b/playlist_test.go
@@ -5,7 +5,7 @@ import "testing"
 func TestPlAdd(t *testing.T) {
 	var pl Playlist
 	s := NewDummySong("Foo", "Bar")
-	pl.Add(&s)
+	pl.Add(s)
 	if len(pl) != 1 {
 		t.Log("len should be 1")
 		t.Fail()
@@ -15,7 +15,7 @@ func TestPlAdd(t *testing.T) {
 func TestPlAddPop(t *testing.T) {
 	var pl Playlist
 	s := NewDummySong("Foo", "Bar")
-	pl.Add(&s)
+	pl.Add(s)
 	if len(pl) != 1 {
 		t.Log("len should be 1")
 		t.Fail()
@@ -27,7 +27,7 @@ func TestPlAddPop(t *testing.T) {
 		t.Fail()
 	}
 
-	if ps != &s {
+	if ps != s {
 		t.Log("the popped ptr should match the one added")
 		t.Fail()
 	}
@@ -37,8 +37,8 @@ func TestPlAddPopTwice(t *testing.T) {
 	var pl Playlist
 	s1 := NewDummySong("Foo", "Bar")
 	s2 := NewDummySong("Nasen", "Baer")
-	pl.Add(&s1)
-	pl.Add(&s2)
+	pl.Add(s1)
+	pl.Add(s2)
 	if len(pl) != 2 {
 		t.Log("len should be 2")
 		t.Fail()
@@ -51,12 +51,12 @@ func TestPlAddPopTwice(t *testing.T) {
 		t.Fail()
 	}
 
-	if ps1 != &s1 {
+	if ps1 != s1 {
 		t.Log("the popped ptr should match the one added")
 		t.Fail()
 	}
 
-	if ps2 != &s2 {
+	if ps2 != s2 {
 		t.Log("the popped ptr should match the one added")
 		t.Fail()
 	}
@@ -66,15 +66,15 @@ func TestPlAdd2AdjustPop(t *testing.T) {
 	var pl Playlist
 	s1 := NewDummySong("Foo", "Bar")
 	s2 := NewDummySong("Nasen", "Baer")
-	pl.Add(&s1)
-	pl.Add(&s2)
+	pl.Add(s1)
+	pl.Add(s2)
 	if len(pl) != 2 {
 		t.Log("len should be 2")
 		t.Fail()
 	}
 
 	s2.Weight += 1
-	pl.Adjust(&s2)
+	pl.Adjust(s2)
 
 	ps := pl.PopSong()
 	if len(pl) != 1 {
@@ -82,7 +82,7 @@ func TestPlAdd2AdjustPop(t *testing.T) {
 		t.Fail()
 	}
 
-	if ps != &s2 {
+	if ps != s2 {
 		t.Log("the popped ptr should match the one adjusted")
 		t.Fail()
 	}
@@ -93,9 +93,9 @@ func TestPlAdd3AdjustAdjustPop(t *testing.T) {
 	s1 := NewDummySong("Foo", "Bar")
 	s2 := NewDummySong("Nasen", "Baer")
 	s3 := NewDummySong("Hut", "Traeger")
-	pl.Add(&s1)
-	pl.Add(&s2)
-	pl.Add(&s3)
+	pl.Add(s1)
+	pl.Add(s2)
+	pl.Add(s3)
 
 	if len(pl) != 3 {
 		t.Log("len should be 3")
@@ -103,13 +103,13 @@ func TestPlAdd3AdjustAdjustPop(t *testing.T) {
 	}
 
 	s1.Weight += 1
-	pl.Adjust(&s1)
+	pl.Adjust(s1)
 
 	s2.Weight -= 1
-	pl.Adjust(&s2)
+	pl.Adjust(s2)
 
 	s3.Weight += 1
-	pl.Adjust(&s3)
+	pl.Adjust(s3)
 
 	ps := pl.PopSong()
 	if len(pl) != 2 {
@@ -117,7 +117,7 @@ func TestPlAdd3AdjustAdjustPop(t *testing.T) {
 		t.Fail()
 	}
 
-	if ps == &s2 {
+	if ps == s2 {
 		t.Log("the popped ptr is the downvoted one")
 		t.Fail()
 	}
@@ -128,7 +128,7 @@ func TestPlAdd3AdjustAdjustPop(t *testing.T) {
 		t.Fail()
 	}
 
-	if ps == &s2 {
+	if ps == s2 {
 		t.Log("the popped ptr is the downvoted one")
 		t.Fail()
 	}
diff --git a/spotify.go b/spotify.go
index 324f645..e15501a 100644
--- a/spotify.go
+++ b/spotify.go
@@ -96,7 +96,7 @@ func (spotify *SpotifyBackend) Play(song *Song, player Player) {
 	player.PlayData(audioFile)
 }
 
-func (spotify *SpotifyBackend) Search(patterns map[string]string) []Song {
+func (spotify *SpotifyBackend) Search(patterns map[string]string) []*Song {
 	session := spotify.session
 	keyword := ""
 	for _, p := range []string{"pattern", "title", "artist", "album"} {
@@ -110,7 +110,7 @@ func (spotify *SpotifyBackend) Search(patterns map[string]string) []Song {
 
 	if err != nil {
 		llog.Error("Failed to search: %s", err)
-		return []Song{}
+		return nil
 	}
 
 	res := resp.Results
@@ -124,7 +124,7 @@ func (spotify *SpotifyBackend) Search(patterns map[string]string) []Song {
 
 	llog.Debug("\nTracks: %d (total %d)\n", len(res.Tracks.Hits), res.Tracks.Total)
 
-	results := []Song{}
+	results := []*Song{}
 	for _, track := range res.Tracks.Hits {
 		uriParts := strings.Split(track.Uri, ":")
 		s := NewSong(track.Name, track.Artists[0].Name, "spotify", uriParts[len(uriParts)-1])
diff --git a/upload.go b/upload.go
index 68c6adb..7a0ecc8 100644
--- a/upload.go
+++ b/upload.go
@@ -95,6 +95,6 @@ func (b *UploadBackend) Play(song *Song, player Player) {
 	player.PlayUri("file://" + path.Join(b.uploadDir, song.Uri))
 }
 
-func (b *UploadBackend) Search(map[string]string) []Song {
+func (b *UploadBackend) Search(map[string]string) []*Song {
 	return nil
 }
diff --git a/wrms.go b/wrms.go
index 02b0860..f225b2a 100644
--- a/wrms.go
+++ b/wrms.go
@@ -26,8 +26,8 @@ type Song struct {
 	Downvotes map[uuid.UUID]struct{} `json:"-"`
 }
 
-func NewSong(title, artist, source, uri string) Song {
-	return Song{
+func NewSong(title, artist, source, uri string) *Song {
+	return &Song{
 		Title:     title,
 		Artist:    artist,
 		Source:    source,
@@ -37,33 +37,33 @@ func NewSong(title, artist, source, uri string) Song {
 	}
 }
 
-func NewDetailedSong(title, artist, source, uri, album string, year int) Song {
+func NewDetailedSong(title, artist, source, uri, album string, year int) *Song {
 	s := NewSong(title, artist, source, uri)
 	s.Album = album
 	s.Year = year
 	return s
 }
 
-func NewSongFromJson(data []byte) (Song, error) {
+func NewSongFromJson(data []byte) (*Song, error) {
 	var s Song
 	err := json.Unmarshal(data, &s)
 	if err != nil {
 		llog.Error("Failed to parse song data %s with %s", string(data), err)
-		return s, err
+		return nil, err
 	}
 
 	s.Upvotes = map[uuid.UUID]struct{}{}
 	s.Downvotes = map[uuid.UUID]struct{}{}
-	return s, nil
+	return &s, nil
 }
 
 type Event struct {
-	Event string `json:"cmd"`
-	Id    uint64 `json:"id"`
-	Songs []Song `json:"songs"`
+	Event string  `json:"cmd"`
+	Id    uint64  `json:"id"`
+	Songs []*Song `json:"songs"`
 }
 
-func (wrms *Wrms) newEvent(event string, songs []Song) Event {
+func (wrms *Wrms) newEvent(event string, songs []*Song) Event {
 	return Event{Id: wrms.eventId.Add(1), Event: event, Songs: songs}
 }
 
@@ -188,7 +188,7 @@ type Wrms struct {
 	nextConnNr  atomic.Uint64
 	// The rwlock must be held when using most internal state
 	rwlock      sync.RWMutex
-	Songs       []Song
+	Songs       []*Song
 	queue       Playlist
 	CurrentSong atomic.Pointer[Song]
 	Player      Player
@@ -234,23 +234,23 @@ func (wrms *Wrms) initConn(conn *Connection) error {
 	}
 
 	if wrms.playing {
-		var songs []Song
+		var songs []*Song
 		if currentSong := wrms.CurrentSong.Load(); currentSong != nil {
-			songs = []Song{*currentSong}
+			songs = []*Song{currentSong}
 		}
 		initialCmds = append(initialCmds, wrms.newEvent("play", songs))
 	}
 
-	upvoted := []Song{}
-	downvoted := []Song{}
+	upvoted := []*Song{}
+	downvoted := []*Song{}
 	if len(wrms.Songs) > 0 {
 		initialCmds = append(initialCmds, wrms.newEvent("add", wrms.Songs))
 
 		for _, song := range wrms.queue.OrderedList() {
 			if _, ok := song.Upvotes[conn.Id]; ok {
-				upvoted = append(upvoted, *song)
+				upvoted = append(upvoted, song)
 			} else if _, ok := song.Downvotes[conn.Id]; ok {
-				downvoted = append(downvoted, *song)
+				downvoted = append(downvoted, song)
 			}
 		}
 	}
@@ -291,20 +291,21 @@ func (wrms *Wrms) Broadcast(ev Event) {
 	})
 }
 
-func (wrms *Wrms) AddSong(song Song) {
+func (wrms *Wrms) AddSong(song *Song) {
 	wrms.rwlock.Lock()
 
 	startPlayingAgain := wrms.playing && wrms.CurrentSong.Load() == nil
-	wrms.Songs = append(wrms.Songs, song)
-	s := &wrms.Songs[len(wrms.Songs)-1]
-	wrms.queue.Add(s)
 
 	if wrms.Config.timeBonus != 0 {
 		llog.Info("Apply time bonus %v", wrms.Config.timeBonus)
 		wrms.queue.applyTimeBonus(wrms.Config.timeBonus)
 	}
 
-	ev := wrms.newEvent("add", []Song{song})
+	wrms.Songs = append(wrms.Songs, song)
+	s := wrms.Songs[len(wrms.Songs)-1]
+	wrms.queue.Add(s)
+
+	ev := wrms.newEvent("add", []*Song{song})
 	wrms.rwlock.Unlock()
 
 	llog.Info("Added song %s (ptr=%p) to Songs", s.Uri, s)
@@ -319,7 +320,7 @@ func (wrms *Wrms) DeleteSong(songUri string) {
 	wrms.rwlock.Lock()
 
 	for i := 0; i < len(wrms.Songs); i++ {
-		s := &wrms.Songs[i]
+		s := wrms.Songs[i]
 		if s.Uri != songUri {
 			continue
 		}
@@ -329,7 +330,7 @@ func (wrms *Wrms) DeleteSong(songUri string) {
 
 		wrms.queue.RemoveSong(s)
 
-		ev := wrms.newEvent("delete", []Song{*s})
+		ev := wrms.newEvent("delete", []*Song{s})
 		wrms.rwlock.Unlock()
 
 		wrms.Broadcast(ev)
@@ -383,7 +384,7 @@ func (wrms *Wrms) _next() {
 		cmd = "play"
 	}
 
-	ev := wrms.newEvent(cmd, []Song{*next})
+	ev := wrms.newEvent(cmd, []*Song{next})
 	wrms.rwlock.Unlock()
 
 	wrms.Broadcast(ev)
@@ -421,7 +422,7 @@ func (wrms *Wrms) PlayPause() {
 		wrms.Player.Play(currentSong)
 	}
 
-	ev := wrms.newEvent("play", []Song{*currentSong})
+	ev := wrms.newEvent("play", []*Song{currentSong})
 	wrms.rwlock.Unlock()
 
 	wrms.Broadcast(ev)
@@ -431,7 +432,7 @@ func (wrms *Wrms) AdjustSongWeight(connId uuid.UUID, songUri string, vote string
 	wrms.rwlock.Lock()
 
 	for i := 0; i < len(wrms.Songs); i++ {
-		s := &wrms.Songs[i]
+		s := wrms.Songs[i]
 		if s.Uri != songUri {
 			continue
 		}
@@ -488,7 +489,7 @@ func (wrms *Wrms) AdjustSongWeight(connId uuid.UUID, songUri string, vote string
 		}
 
 		wrms.queue.Adjust(s)
-		ev := wrms.newEvent("update", []Song{*s})
+		ev := wrms.newEvent("update", []*Song{s})
 		wrms.rwlock.Unlock()
 
 		wrms.Broadcast(ev)
@@ -496,6 +497,6 @@ func (wrms *Wrms) AdjustSongWeight(connId uuid.UUID, songUri string, vote string
 	}
 }
 
-func (wrms *Wrms) Search(pattern map[string]string) chan []Song {
+func (wrms *Wrms) Search(pattern map[string]string) chan []*Song {
 	return wrms.Player.Search(pattern)
 }
diff --git a/wrms_test.go b/wrms_test.go
index 7d0c4d1..29ee2b2 100644
--- a/wrms_test.go
+++ b/wrms_test.go
@@ -1,22 +1,22 @@
 package main
 
 import (
+	"fmt"
 	"github.com/google/uuid"
 	"io"
-	"muhq.space/go/wrms/llog"
 	"testing"
 )
 
 type mockPlayer struct{}
 
-func (p *mockPlayer) Play(*Song)                                         {}
-func (p *mockPlayer) PlayUri(string)                                     {}
-func (p *mockPlayer) PlayData(io.Reader)                                 {}
-func (p *mockPlayer) Search(pattern map[string]string) (res chan []Song) { return }
-func (p *mockPlayer) Playing() bool                                      { return false }
-func (p *mockPlayer) Pause()                                             {}
-func (p *mockPlayer) Continue()                                          {}
-func (p *mockPlayer) Stop()                                              {}
+func (p *mockPlayer) Play(*Song)                                          {}
+func (p *mockPlayer) PlayUri(string)                                      {}
+func (p *mockPlayer) PlayData(io.Reader)                                  {}
+func (p *mockPlayer) Search(pattern map[string]string) (res chan []*Song) { return }
+func (p *mockPlayer) Playing() bool                                       { return false }
+func (p *mockPlayer) Pause()                                              {}
+func (p *mockPlayer) Continue()                                           {}
+func (p *mockPlayer) Stop()                                               {}
 
 var alice, _ = uuid.NewRandom()
 
@@ -33,7 +33,7 @@ func TestWrmsAdd(t *testing.T) {
 
 func TestWrmsAdd3Next3(t *testing.T) {
 	wrms := Wrms{Player: &mockPlayer{}}
-	songs := []Song{
+	songs := []*Song{
 		NewDummySong("song1", "snfmt"),
 		NewDummySong("song2", "snfmt"),
 		NewDummySong("song3", "snfmt"),
@@ -94,7 +94,6 @@ func TestWrmsUpUpDownNextNext(t *testing.T) {
 	wrms.AddSong(s2)
 	wrms.AddSong(s3)
 
-	llog.Info("UpUpDown test")
 	wrms.AdjustSongWeight(alice, s1.Uri, "up")
 	wrms.AdjustSongWeight(alice, s2.Uri, "down")
 	wrms.AdjustSongWeight(alice, s3.Uri, "up")
@@ -114,7 +113,6 @@ func TestWrmsDoubleAdd(t *testing.T) {
 	wrms := Wrms{Player: &mockPlayer{}}
 	s1 := NewDummySong("song1", "snfmt")
 
-	llog.Info("Double Add test")
 	wrms.AddSong(s1)
 	wrms.AdjustSongWeight(alice, s1.Uri, "up")
 	wrms.Next()
@@ -141,3 +139,23 @@ func TestFlipVote(t *testing.T) {
 		t.Fail()
 	}
 }
+
+func TestTimeBonus(t *testing.T) {
+	wrms := Wrms{Player: &mockPlayer{}}
+	wrms.Config.timeBonus = 0.1
+	songs := []*Song{
+		NewDummySong("song1", "snfmt"),
+		NewDummySong("song2", "snfmt"),
+		NewDummySong("song3", "snfmt"),
+	}
+
+	for _, s := range songs {
+		wrms.AddSong(s)
+	}
+
+	exp := 0.1 * float64(len(songs)-1)
+	if wrms.Songs[0].Weight != exp {
+		t.Log(fmt.Sprintf("TimeBonus not correctly applied song %p %v expected: %v", &wrms.Songs[0], wrms.Songs[0], exp))
+		t.Fail()
+	}
+}
diff --git a/youtube.go b/youtube.go
index 1b5bda6..5b97e50 100644
--- a/youtube.go
+++ b/youtube.go
@@ -27,7 +27,7 @@ type YoutubeDlSearchResult struct {
 	Title string
 }
 
-func (b *YoutubeBackend) Search(patterns map[string]string) []Song {
+func (b *YoutubeBackend) Search(patterns map[string]string) []*Song {
 	pattern := ""
 	for _, v := range patterns {
 		pattern += v + " "
@@ -41,7 +41,7 @@ func (b *YoutubeBackend) Search(patterns map[string]string) []Song {
 		llog.Error("youtube-dl failed with: %s", err)
 	}
 
-	songs := []Song{}
+	songs := []*Song{}
 	for _, l := range strings.Split(string(results), "\n") {
 		if l == "" {
 			continue
-- 
cgit v1.2.3