diff --git a/backend/player/emit_test.go b/backend/player/emit_test.go new file mode 100644 index 0000000..d677afb --- /dev/null +++ b/backend/player/emit_test.go @@ -0,0 +1,85 @@ +package player + +import ( + "context" + "log/slog" + "testing" + "time" + + "yellowjacket/backend/events" +) + +// recordedPlayer is a player with an event sink installed and no audio +// device: enough to assert on what the frontend would receive from the +// paths that do not touch the speaker. +func recordedPlayer(t *testing.T) (*Player, *events.Recorder) { + t.Helper() + + p := NewPlayer(slog.Default(), nil) + rec := events.NewRecorder() + p.SetContext(events.WithSink(t.Context(), rec)) + + return p, rec +} + +func TestSeek_WithNoTrackEmitsSeekFailed(t *testing.T) { + t.Parallel() + + p, rec := recordedPlayer(t) + + if err := p.Seek(5); err == nil { + t.Fatal("Seek with no track loaded returned nil error") + } + + // C2: the frontend has made an optimistic move it now has to take + // back, and this is the only thing that tells it so. + if _, ok := rec.Last(events.SeekFailed); !ok { + t.Errorf("no SeekFailed emitted; got %v", rec.Names()) + } +} + +func TestPositionTicker_SilentWhileNotPlaying(t *testing.T) { + t.Parallel() + + _, rec := recordedPlayer(t) + + // The ticker is running (SetContext started it) but nothing is + // playing, so a paused app must not push a position a second + // forever. + time.Sleep(positionTickInterval * 2) + + if got := rec.Count(events.PlaybackPositionChanged); got != 0 { + t.Errorf("position emitted %d times while stopped, want 0", got) + } +} + +func TestEmitPosition_CarriesLengthAndSequence(t *testing.T) { + t.Parallel() + + p := NewPlayer(slog.Default(), nil) + rec := events.NewRecorder() + p.ctx = events.WithSink(context.Background(), rec) + + p.mu.Lock() + p.emitPositionLocked() + p.emitPositionLocked() + p.mu.Unlock() + + ticks := rec.Named(events.PlaybackPositionChanged) + if len(ticks) != 2 { + t.Fatalf("emitted %d positions, want 2", len(ticks)) + } + + first, ok := ticks[0].Payload().(PositionInfo) + if !ok { + t.Fatalf("payload is %T, want player.PositionInfo", ticks[0].Payload()) + } + + second, _ := ticks[1].Payload().(PositionInfo) + + // The sequence is what lets the seek bar reset its interpolation + // on a tick that reports the same second twice. + if second.Seq <= first.Seq { + t.Errorf("seq did not advance: %d then %d", first.Seq, second.Seq) + } +} diff --git a/backend/player/player.go b/backend/player/player.go index 1e6e46b..641d661 100644 --- a/backend/player/player.go +++ b/backend/player/player.go @@ -61,6 +61,26 @@ type Player struct { // inflated for files with multiple ID3v2 tags, so this value // is preferred for display and position calculations. trackLengthMs int64 + + // positionTickerOnce guards the 1 Hz position ticker so repeated + // SetContext calls (tests, re-init) cannot start a second one. + positionTickerOnce sync.Once + + // positionSeq increments on every emitted position, so a + // consumer can tell "the same second, again" from "a fresh + // reading" and reset its interpolation on both. + positionSeq uint64 +} + +// PositionInfo is the payload of the PlaybackPositionChanged event: +// the player's own answer to "where are we", which the seek bar +// renders instead of counting. +type PositionInfo struct { + PositionSeconds int `json:"positionSeconds"` + TrackLength int `json:"trackLength"` + TrackChangeID uint64 `json:"trackChangeId"` + Seq uint64 `json:"seq"` + Playing bool `json:"playing"` } // State represents the current playback state. @@ -173,6 +193,74 @@ func (p *Player) SetContext(ctx context.Context) { p.ctx = ctx p.restoreStateLocked() + p.startPositionTicker() +} + +// positionTickInterval is how often the backend reports its own +// playback position while playing. +const positionTickInterval = time.Second + +// startPositionTicker runs the 1 Hz position report for the life of +// the Wails context. Must be called with p.mu held. +func (p *Player) startPositionTicker() { + if p.ctx == nil { + return + } + + ctx := p.ctx + + p.positionTickerOnce.Do(func() { + go func() { + ticker := time.NewTicker(positionTickInterval) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + p.emitPositionIfPlaying() + } + } + }() + }) +} + +// emitPositionIfPlaying reports the position only while audio is +// actually moving; a paused or stopped player has already emitted its +// final position at the transition. +func (p *Player) emitPositionIfPlaying() { + p.mu.Lock() + defer p.mu.Unlock() + + if p.state != Playing || p.currentFile == nil { + return + } + + p.emitPositionLocked() +} + +// emitPositionLocked pushes the current position to the frontend. +// Must be called with p.mu held. +func (p *Player) emitPositionLocked() { + if p.ctx == nil { + return + } + + length, err := p.trackLengthLocked() + if err != nil { + length = 0 + } + + p.positionSeq++ + + events.Emit(p.ctx, events.PlaybackPositionChanged, PositionInfo{ + PositionSeconds: p.displayPositionSecsLocked(), + TrackLength: length, + TrackChangeID: p.trackChangeID, + Seq: p.positionSeq, + Playing: p.state == Playing, + }) } // --------------------------------------------------------------- @@ -381,6 +469,13 @@ func (p *Player) onPlaybackFinished() { p.state = Stopped handler := p.playbackFinishedHandler mc := p.mediaControls + + // Rewind so the position the UI is told is the truth: a finished + // track sits at 0:00, ready to play again, rather than reporting + // its own length forever. Play() rebuilds the streamer chain from + // the Stopped state anyway, so this only moves the decoder. + p.rewindLocked() + p.emitPositionLocked() p.mu.Unlock() // Emit Wails events outside the lock — these are non-blocking @@ -476,6 +571,7 @@ func (p *Player) loadFileLocked(filePath string) error { p.startPaused() p.emitPlaybackStateChanged(p.state) p.emitTrackChanged() + p.emitPositionLocked() p.saveState() p.logger.Info( "File loaded, state set to paused", "file", filePath, @@ -553,6 +649,7 @@ func (p *Player) Play() error { p.state = Playing p.emitPlaybackStateChanged(p.state) + p.emitPositionLocked() p.logger.Info("Started playback") return nil @@ -581,6 +678,7 @@ func (p *Player) Pause() error { p.state = Paused p.logger.Info("Paused playback") p.emitPlaybackStateChanged(p.state) + p.emitPositionLocked() p.saveState() } else { p.logger.Info("Already paused or not playing") @@ -771,6 +869,29 @@ func (p *Player) Seek(targetSeconds int) error { return p.seekLocked(targetSeconds) } +// rewindLocked returns the decoder to the start of the track without +// touching playback state. Must be called with p.mu held. +func (p *Player) rewindLocked() { + if p.seeker == nil { + return + } + + // Same source lock the seek path takes: the read-ahead goroutine + // must not be inside Read while the decoder seeks. + if p.buffered != nil { + p.buffered.LockSource() + defer p.buffered.UnlockSource() + } + + speaker.Lock() + err := p.seeker.Seek(0) + speaker.Unlock() + + if err != nil { + p.logger.Warn("Failed to rewind finished track", "err", err) + } +} + func (p *Player) seekLocked(targetSeconds int) error { if p.seeker == nil { events.Emit(p.ctx, events.SeekFailed) @@ -846,6 +967,11 @@ func (p *Player) seekLocked(targetSeconds int) error { "err", seekErr, ) + // The optimistic move the UI already made has to be taken + // back, and only the backend knows it did not happen. + events.Emit(p.ctx, events.SeekFailed) + p.emitPositionLocked() + return fmt.Errorf("failed to seek: %w", seekErr) } @@ -862,6 +988,11 @@ func (p *Player) seekLocked(targetSeconds int) error { p.mediaControls.NotifySeek(targetSeconds) } + // Report the landing position immediately rather than leaving the + // UI to guess until the next tick — this is the half of H-3 that + // desynced the seek bar by 30 s over four keyboard seeks. + p.emitPositionLocked() + return nil } diff --git a/backend/queue/emit.go b/backend/queue/emit.go index 45eb617..8d7a6e2 100644 --- a/backend/queue/emit.go +++ b/backend/queue/emit.go @@ -43,6 +43,28 @@ func (q *Queue) emitModeChanged() { ) } +// emitPlaybackFailed tells the frontend that a track could not be +// played. Before this existed the failure was logged, the index was +// reverted and nothing reached the user: a moved file was a button +// that did nothing, twice, forever (errors.C1). +func (q *Queue) emitPlaybackFailed(track Track, reason error) { + msg := "" + if reason != nil { + msg = reason.Error() + } + + events.Emit( + q.ctx, + events.PlaybackFailed, + PlaybackFailure{ + FilePath: track.FilePath, + Title: track.Title, + Artist: track.Artist, + Reason: msg, + }, + ) +} + // emitTracksModified emits a delta update for track list changes. func (q *Queue) emitTracksModified( action string, diff --git a/backend/queue/handlers.go b/backend/queue/handlers.go index 5b671f2..b3d44e9 100644 --- a/backend/queue/handlers.go +++ b/backend/queue/handlers.go @@ -29,18 +29,20 @@ func (q *Queue) OnPlaybackFinished() { nextIdx := q.nextIndex() if nextIdx == -1 { // Queue exhausted — this is the extension point for a future fallback playlist. - q.onQueueExhausted() + q.onQueueExhausted(false) q.mu.Unlock() q.recordPlay(finishedID) return } - prevIndex := q.currentIndex q.currentIndex = nextIdx - if !q.playCurrentTrack() { - q.currentIndex = prevIndex + // Skip over tracks that cannot be played instead of reverting. + // Reverting stopped playback dead on the first moved file and left + // Next unable to get past it, since Next hit the same track. + if !q.playCurrentOrSkip(true, q.nextIndex) { + q.onQueueExhausted(false) q.mu.Unlock() q.recordPlay(finishedID) diff --git a/backend/queue/playbackfailed_test.go b/backend/queue/playbackfailed_test.go new file mode 100644 index 0000000..dfebade --- /dev/null +++ b/backend/queue/playbackfailed_test.go @@ -0,0 +1,213 @@ +package queue + +import ( + "context" + "errors" + "log/slog" + "testing" + + "yellowjacket/backend/database" + "yellowjacket/backend/events" +) + +var errFileMissing = errors.New("no such file or directory") + +// failingLoader is a TrackLoader that refuses to load a named set of +// paths — a moved file, in other words, which is the whole of +// errors.C1. +type failingLoader struct { + mockTrackLoader + fails map[string]bool + unloaded int +} + +func (f *failingLoader) LoadFile(filePath string) error { + if f.fails[filePath] { + return errFileMissing + } + + return f.mockTrackLoader.LoadFile(filePath) +} + +func (f *failingLoader) UnloadTrack() { f.unloaded++ } + +// setupFailingQueue is setupRecordedQueue with a loader that fails on +// the given paths. +func setupFailingQueue( + t *testing.T, +) (*Queue, *database.DB, *events.Recorder, *failingLoader) { + t.Helper() + + db := database.NewTestDB(t) + q := NewQueue(slog.Default(), db) + loader := &failingLoader{fails: map[string]bool{}} + q.SetPlayer(loader) + + rec := events.NewRecorder() + q.SetContext(events.WithSink(context.Background(), rec)) + + return q, db, rec, loader +} + +// failureOf returns the payload of the most recent PlaybackFailed. +func failureOf(t *testing.T, rec *events.Recorder) PlaybackFailure { + t.Helper() + + ev, ok := rec.Last(events.PlaybackFailed) + if !ok { + t.Fatalf("no PlaybackFailed emitted; got %v", rec.Names()) + } + + failure, ok := ev.Payload().(PlaybackFailure) + if !ok { + t.Fatalf( + "PlaybackFailed payload is %T, want queue.PlaybackFailure", + ev.Payload(), + ) + } + + return failure +} + +func TestPlaybackFailed_EmittedForAMissingFile(t *testing.T) { + t.Parallel() + + q, db, rec, loader := setupFailingQueue(t) + paths := seedAudioFiles(t, db, 3) + loader.fails[paths[1]] = true + + q.SetQueue(paths, 0, false) + q.PlayIndex(1) + + failure := failureOf(t, rec) + if failure.FilePath != paths[1] { + t.Errorf("filePath: got %q, want %q", failure.FilePath, paths[1]) + } + + if failure.Reason == "" { + t.Error("reason is empty; the frontend has nothing to log") + } + + if failure.Title == "" { + t.Error("title is empty; a message cannot name the track") + } +} + +func TestPlaybackFailed_AutoAdvanceSkipsPastIt(t *testing.T) { + t.Parallel() + + q, db, rec, loader := setupFailingQueue(t) + paths := seedAudioFiles(t, db, 3) + loader.fails[paths[1]] = true + + q.SetQueue(paths, 0, false) + q.Play() + + // The first track finished: auto-advance lands on the missing + // file and must step over it rather than stopping dead. + q.OnPlaybackFinished() + + if got := q.GetState().CurrentIndex; got != 2 { + t.Errorf("currentIndex after skipping: got %d, want 2", got) + } + + if _, ok := rec.Last(events.PlaybackFailed); !ok { + t.Errorf("skipped silently; events were %v", rec.Names()) + } +} + +func TestPlaybackFailed_NextSkipsPastIt(t *testing.T) { + t.Parallel() + + q, db, _, loader := setupFailingQueue(t) + paths := seedAudioFiles(t, db, 4) + loader.fails[paths[1]] = true + loader.fails[paths[2]] = true + + q.SetQueue(paths, 0, false) + q.Next() + + if got := q.GetState().CurrentIndex; got != 3 { + t.Errorf( + "currentIndex after two unplayable tracks: got %d, want 3", + got, + ) + } +} + +func TestPlaybackFailed_WholeQueueUnplayableStopsOnce(t *testing.T) { + t.Parallel() + + q, db, rec, loader := setupFailingQueue(t) + paths := seedAudioFiles(t, db, 3) + + for _, p := range paths { + loader.fails[p] = true + } + + q.SetQueue(paths, 0, false) + q.repeatMode = RepeatAll + + rec.Reset() + q.Next() + + // Every file is gone (a disconnected drive). One pass, then + // stop — not an endless wrap around a RepeatAll queue. + if got := q.GetState().CurrentIndex; got != -1 { + t.Errorf("currentIndex: got %d, want -1 (exhausted)", got) + } + + if got := rec.Count(events.PlaybackFailed); got != len(paths) { + t.Errorf( + "PlaybackFailed count: got %d, want %d (one pass)", + got, len(paths), + ) + } + + if loader.unloaded != 0 { + t.Errorf( + "player unloaded %d times; the finished track should stay "+ + "on the now-playing bar", + loader.unloaded, + ) + } +} + +func TestQueueExhausted_KeepsTheFinishedTrackLoaded(t *testing.T) { + t.Parallel() + + q, db, _, loader := setupFailingQueue(t) + paths := seedAudioFiles(t, db, 1) + + q.SetQueue(paths, 0, false) + q.Play() + q.OnPlaybackFinished() + + if q.GetState().CurrentIndex != -1 { + t.Errorf( + "currentIndex: got %d, want -1", + q.GetState().CurrentIndex, + ) + } + + // H-18: the bar used to blank while the queue panel still listed + // what had just played. + if loader.unloaded != 0 { + t.Errorf("player unloaded %d times, want 0", loader.unloaded) + } +} + +func TestQueueExhausted_UnloadsWhenTheTrackIsRemoved(t *testing.T) { + t.Parallel() + + q, db, _, loader := setupFailingQueue(t) + paths := seedAudioFiles(t, db, 1) + + q.SetQueue(paths, 0, false) + q.RemoveTrack(0) + + // Nothing left to show, so the bar clears. + if loader.unloaded == 0 { + t.Error("player not unloaded after its track left the queue") + } +} diff --git a/backend/queue/queue.go b/backend/queue/queue.go index 2415d29..528fa2d 100644 --- a/backend/queue/queue.go +++ b/backend/queue/queue.go @@ -113,6 +113,16 @@ type ModeChanged struct { RepeatMode RepeatMode `json:"repeatMode"` } +// PlaybackFailure is the payload for the PlaybackFailed event. It +// carries enough to name the track in a message without the frontend +// having to look anything up, and the raw reason for the log. +type PlaybackFailure struct { + FilePath string `json:"filePath"` + Title string `json:"title"` + Artist string `json:"artist"` + Reason string `json:"reason"` +} + // TracksModified is the payload for the QueueTracksModified event. type TracksModified struct { Action string `json:"action"` @@ -910,16 +920,15 @@ func (q *Queue) Next() { nextIdx := q.nextIndex() if nextIdx == -1 { - q.onQueueExhausted() + q.onQueueExhausted(false) return } - prevIndex := q.currentIndex q.currentIndex = nextIdx - if !q.playOrLoadCurrentTrack(wasPlaying) { - q.currentIndex = prevIndex + if !q.playCurrentOrSkip(wasPlaying, q.nextIndex) { + q.onQueueExhausted(false) return } @@ -974,7 +983,9 @@ func (q *Queue) Previous() { prevCurrentIndex := q.currentIndex q.currentIndex = prevIdx - if !q.playOrLoadCurrentTrack(wasPlaying) { + // Backwards for the same reason Next skips forwards: otherwise a + // bad file behind you makes Previous a button that does nothing. + if !q.playCurrentOrSkip(wasPlaying, q.previousIndex) { q.currentIndex = prevCurrentIndex return @@ -1041,7 +1052,7 @@ func (q *Queue) playFromStart() { q.currentIndex = 0 } - if !q.playCurrentTrack() { + if !q.playCurrentOrSkip(true, q.nextIndex) { q.currentIndex = -1 return @@ -1208,6 +1219,7 @@ func (q *Queue) loadCurrentTrack() bool { "Failed to load file from queue", "filePath", track.FilePath, "err", err, ) + q.emitPlaybackFailed(track, err) return false } @@ -1231,6 +1243,7 @@ func (q *Queue) playCurrentTrack() bool { "Failed to play file from queue", "filePath", track.FilePath, "err", err, ) + q.emitPlaybackFailed(track, err) return false } @@ -1238,12 +1251,39 @@ func (q *Queue) playCurrentTrack() bool { return true } +// playCurrentOrSkip plays (or loads) the track at currentIndex, and on +// failure steps to the next candidate rather than giving up. A moved +// or unreadable file in the middle of a queue used to stop playback +// dead, and Next did not help because it hit the same track and +// reverted. +// +// The attempt count is bounded by the queue length so a queue whose +// files have all gone (a disconnected drive) stops after one pass +// instead of spinning forever through a RepeatAll wrap. Returns false +// when nothing reachable can be played. +func (q *Queue) playCurrentOrSkip(autoPlay bool, step func() int) bool { + for range len(q.tracks) { + if q.playOrLoadCurrentTrack(autoPlay) { + return true + } + + next := step() + if next == -1 { + return false + } + + q.currentIndex = next + } + + return false +} + // handleCurrentTrackRemoved handles the case where the currently loaded // track was removed from the queue. If tracks remain it loads the track // now at currentIndex (paused); otherwise it exhausts the queue. func (q *Queue) handleCurrentTrackRemoved() { if len(q.tracks) == 0 { - q.onQueueExhausted() + q.onQueueExhausted(true) return } @@ -1252,14 +1292,21 @@ func (q *Queue) handleCurrentTrackRemoved() { } // onQueueExhausted is called when there are no more tracks to play. -// It unloads the current track, resets the index to -1 (no current track), -// and notifies the frontend. -func (q *Queue) onQueueExhausted() { - q.logger.Info("Queue exhausted, unloading track") +// It resets the index to -1 (no current track) and notifies the +// frontend. +// +// unload says whether the player should also let go of the file it +// holds. When the queue simply ran out, it should not: the finished +// track stays on the now-playing bar, paused at 0:00, rather than the +// bar blanking while the queue panel still lists what just played +// (H-18). When the current track was removed from the queue, or the +// queue was cleared, there is nothing left to show and it does. +func (q *Queue) onQueueExhausted(unload bool) { + q.logger.Info("Queue exhausted", "unload", unload) q.currentIndex = -1 - if q.player != nil { + if unload && q.player != nil { q.player.UnloadTrack() }