diff --git a/docs/provider-development.md b/docs/provider-development.md index 58fb028..1ba753d 100644 --- a/docs/provider-development.md +++ b/docs/provider-development.md @@ -40,7 +40,7 @@ interfaces are defined in `provider/interfaces.go`. | `ArtistBrowser` | Hierarchical artist browsing | `Artists()`, `ArtistAlbums(id)` | | `AlbumBrowser` | Paginated album browsing with sort | `AlbumList(sort, offset, size)`, `AlbumSortTypes()` | | `AlbumTrackLoader` | Album track listing | `AlbumTracks(albumID)` | -| `Scrobbler` | Playback reporting | `Scrobble(track, submission)` | +| `PlaybackReporter` | Playback reporting at track start and finish | `CanReportPlayback(track)`, `ReportNowPlaying(track, position, canSeek) error`, `ReportScrobble(track, elapsed, duration, canSeek) error` | | `PlaylistWriter` | Add track to playlist | `AddTrackToPlaylist(ctx, playlistID, track)` | | `PlaylistCreator` | Create new playlist | `CreatePlaylist(ctx, name)` | | `PlaylistDeleter` | Remove playlists/tracks | `DeletePlaylist(name)`, `RemoveTrack(name, index)` | @@ -49,7 +49,7 @@ interfaces are defined in `provider/interfaces.go`. | `Closer` | Cleanup on shutdown | `Close()` | | `Authenticator` | Interactive sign-in flow | `Authenticate() error` (in `playlist` package) | | `ResumeTarget` | Server-side resume position | `ResumeTarget(playlistID, tracks)` | -| `ProgressReporter` | Interim position updates while playing, in addition to `PlaybackReporter`'s start/finish reports | `ReportProgress(track, position)` | +| `ProgressReporter` | Interim position updates while playing, in addition to `PlaybackReporter`'s start/finish reports | `ReportProgress(track, position) error` | | `BrowseLabeler` | Relabel the browse overlay's two levels (e.g. Authors/Books instead of Artists/Albums) | `BrowseLabels()` | ## Steps @@ -190,7 +190,7 @@ implements, the UI will automatically: - Show the browse overlay ("N") if any registered provider implements `ArtistBrowser` or `AlbumBrowser` - Show the search overlay ("F") if any registered provider implements `Searcher` - Enable add-to-playlist in search results if the searched provider implements `PlaylistWriter` -- Scrobble playback if `Scrobbler` is implemented +- Report playback at track start and finish if `PlaybackReporter` is implemented, logging any failure the provider returns - Run interactive auth on first use if `Authenticator` is implemented - Place the cursor on the in-progress track and start it at the stored position if `ResumeTarget` is implemented - Push an interim listening position every 15 seconds while a track plays if `ProgressReporter` is implemented diff --git a/external/audiobookshelf/provider.go b/external/audiobookshelf/provider.go index 01e60d4..e45d18b 100644 --- a/external/audiobookshelf/provider.go +++ b/external/audiobookshelf/provider.go @@ -9,7 +9,6 @@ import ( "sync" "time" - "github.com/bjarneo/cliamp/applog" "github.com/bjarneo/cliamp/config" "github.com/bjarneo/cliamp/playlist" "github.com/bjarneo/cliamp/provider" @@ -501,26 +500,26 @@ func (p *Provider) CanReportPlayback(track playlist.Track) bool { return track.Meta(provider.MetaAudiobookshelfID) != "" } -func (p *Provider) ReportNowPlaying(track playlist.Track, position time.Duration, _ bool) { +func (p *Provider) ReportNowPlaying(track playlist.Track, position time.Duration, _ bool) error { if position <= 0 { - return + return nil } - p.report(track, position, false) + return p.report(track, position, false) } -func (p *Provider) ReportScrobble(track playlist.Track, elapsed, _ time.Duration, _ bool) { - p.report(track, elapsed, true) +func (p *Provider) ReportScrobble(track playlist.Track, elapsed, _ time.Duration, _ bool) error { + return p.report(track, elapsed, true) } // ReportProgress pushes an interim listening position. -func (p *Provider) ReportProgress(track playlist.Track, position time.Duration) { - p.report(track, position, false) +func (p *Provider) ReportProgress(track playlist.Track, position time.Duration) error { + return p.report(track, position, false) } -func (p *Provider) report(track playlist.Track, position time.Duration, complete bool) { +func (p *Provider) report(track playlist.Track, position time.Duration, complete bool) error { itemID := track.Meta(provider.MetaAudiobookshelfID) if itemID == "" { - return + return nil } offset := metaFloat(track, provider.MetaAudiobookshelfOffset) total := metaFloat(track, provider.MetaAudiobookshelfTotal) @@ -531,8 +530,9 @@ func (p *Provider) report(track playlist.Track, position time.Duration, complete finished := complete && total > 0 && current >= total-finishSlack episodeID := track.Meta(provider.MetaAudiobookshelfEpisode) if err := p.client.UpdateProgress(itemID, episodeID, current, total, finished); err != nil { - applog.Warn("audiobookshelf: progress update failed for item %s episode %q: %v", itemID, episodeID, err) + return fmt.Errorf("audiobookshelf: update progress for item %s: %w", itemID, err) } + return nil } // ResumeTarget returns where to continue an item: the track index and the diff --git a/external/audiobookshelf/provider_test.go b/external/audiobookshelf/provider_test.go index 103d7d8..3e0b78b 100644 --- a/external/audiobookshelf/provider_test.go +++ b/external/audiobookshelf/provider_test.go @@ -827,3 +827,30 @@ func TestSearchTracksHonoursCancellation(t *testing.T) { }) } } + +func TestReportProgressReturnsTheFailure(t *testing.T) { + p := mockProvider(func(req *http.Request) (*http.Response, error) { + return statusResponse(http.StatusNotFound, "404 Not Found"), nil + }) + + track := playlist.Track{ + DurationSecs: 3600, + ProviderMeta: map[string]string{ + provider.MetaAudiobookshelfID: "book-1", + provider.MetaAudiobookshelfTotal: "7200", + }, + } + + err := p.ReportProgress(track, 120*time.Second) + if err == nil { + t.Fatal("ReportProgress() error = nil, want the rejected write surfaced to the caller") + } + if !strings.Contains(err.Error(), "book-1") || !strings.Contains(err.Error(), "404") { + t.Fatalf("error = %v, want the item id and the status", err) + } + + // A track this provider does not own is not an error. + if err := p.ReportProgress(playlist.Track{}, 120*time.Second); err != nil { + t.Fatalf("ReportProgress() for a foreign track = %v, want nil", err) + } +} diff --git a/external/emby/provider.go b/external/emby/provider.go index a5fa69b..95894fa 100644 --- a/external/emby/provider.go +++ b/external/emby/provider.go @@ -94,12 +94,12 @@ func (p *Provider) CanReportPlayback(track playlist.Track) bool { return track.Meta(provider.MetaEmbyID) != "" } -func (p *Provider) ReportNowPlaying(track playlist.Track, position time.Duration, canSeek bool) { - _ = p.client.ReportNowPlaying(track, position, canSeek) +func (p *Provider) ReportNowPlaying(track playlist.Track, position time.Duration, canSeek bool) error { + return p.client.ReportNowPlaying(track, position, canSeek) } -func (p *Provider) ReportScrobble(track playlist.Track, elapsed, _ time.Duration, canSeek bool) { - _ = p.client.ReportScrobble(track, elapsed, canSeek) +func (p *Provider) ReportScrobble(track playlist.Track, elapsed, _ time.Duration, canSeek bool) error { + return p.client.ReportScrobble(track, elapsed, canSeek) } // Playlists returns all albums across all Emby music views. diff --git a/external/jellyfin/provider.go b/external/jellyfin/provider.go index 0ae5212..d13af34 100644 --- a/external/jellyfin/provider.go +++ b/external/jellyfin/provider.go @@ -82,12 +82,12 @@ func (p *Provider) CanReportPlayback(track playlist.Track) bool { return track.Meta(provider.MetaJellyfinID) != "" } -func (p *Provider) ReportNowPlaying(track playlist.Track, position time.Duration, canSeek bool) { - _ = p.client.ReportNowPlaying(track, position, canSeek) +func (p *Provider) ReportNowPlaying(track playlist.Track, position time.Duration, canSeek bool) error { + return p.client.ReportNowPlaying(track, position, canSeek) } -func (p *Provider) ReportScrobble(track playlist.Track, elapsed, _ time.Duration, canSeek bool) { - _ = p.client.ReportScrobble(track, elapsed, canSeek) +func (p *Provider) ReportScrobble(track playlist.Track, elapsed, _ time.Duration, canSeek bool) error { + return p.client.ReportScrobble(track, elapsed, canSeek) } // Playlists returns all albums across all Jellyfin music views. diff --git a/external/navidrome/client.go b/external/navidrome/client.go index b8dea8e..d91274d 100644 --- a/external/navidrome/client.go +++ b/external/navidrome/client.go @@ -514,21 +514,21 @@ func (c *NavidromeClient) CanReportPlayback(track playlist.Track) bool { return !c.scrobbleDisabled && track.Meta(provider.MetaNavidromeID) != "" } -func (c *NavidromeClient) ReportNowPlaying(track playlist.Track, _ time.Duration, _ bool) { - c.scrobble(track.Meta(provider.MetaNavidromeID), false) +func (c *NavidromeClient) ReportNowPlaying(track playlist.Track, _ time.Duration, _ bool) error { + return c.scrobble(track.Meta(provider.MetaNavidromeID), false) } -func (c *NavidromeClient) ReportScrobble(track playlist.Track, _, _ time.Duration, _ bool) { - c.scrobble(track.Meta(provider.MetaNavidromeID), true) +func (c *NavidromeClient) ReportScrobble(track playlist.Track, _, _ time.Duration, _ bool) error { + return c.scrobble(track.Meta(provider.MetaNavidromeID), true) } // scrobble reports playback of a track to the Subsonic server. // If submission is false, it registers a "now playing" notification only. // If submission is true, it records a full play (updates play count, last.fm, etc.). -// The call is best-effort: errors are silently discarded. -func (c *NavidromeClient) scrobble(id string, submission bool) { +// The call is best-effort: the error is returned for logging, never acted on. +func (c *NavidromeClient) scrobble(id string, submission bool) error { if id == "" { - return + return nil } params := url.Values{ "id": {id}, @@ -541,7 +541,8 @@ func (c *NavidromeClient) scrobble(id string, submission bool) { } resp, err := httpClient.Get(c.buildURL("scrobble", params)) if err != nil { - return // fire-and-forget; ignore network errors + return fmt.Errorf("navidrome: scrobble: %w", err) } resp.Body.Close() + return nil } diff --git a/provider/interfaces.go b/provider/interfaces.go index 4ac4ada..912546c 100644 --- a/provider/interfaces.go +++ b/provider/interfaces.go @@ -44,8 +44,10 @@ type AlbumTrackLoader interface { // playback-completion reports for tracks they originated. type PlaybackReporter interface { CanReportPlayback(track playlist.Track) bool - ReportNowPlaying(track playlist.Track, position time.Duration, canSeek bool) - ReportScrobble(track playlist.Track, elapsed, duration time.Duration, canSeek bool) + // ReportNowPlaying and ReportScrobble return the failure so the caller can + // record it; reporting is best-effort and never blocks playback. + ReportNowPlaying(track playlist.Track, position time.Duration, canSeek bool) error + ReportScrobble(track playlist.Track, elapsed, duration time.Duration, canSeek bool) error } // ProgressReporter is implemented by providers that track listening position @@ -54,7 +56,7 @@ type PlaybackReporter interface { type ProgressReporter interface { PlaybackReporter // ReportProgress sends an interim position update for a playing track. - ReportProgress(track playlist.Track, position time.Duration) + ReportProgress(track playlist.Track, position time.Duration) error } // ResumeTarget is implemented by providers that track listening position diff --git a/ui/model/audiobookshelf_resume_test.go b/ui/model/audiobookshelf_resume_test.go index fda87db..b8b2bc5 100644 --- a/ui/model/audiobookshelf_resume_test.go +++ b/ui/model/audiobookshelf_resume_test.go @@ -148,12 +148,15 @@ type progressProv struct { func (p *progressProv) CanReportPlayback(playlist.Track) bool { return true } -func (p *progressProv) ReportNowPlaying(playlist.Track, time.Duration, bool) {} +func (p *progressProv) ReportNowPlaying(playlist.Track, time.Duration, bool) error { return nil } -func (p *progressProv) ReportScrobble(playlist.Track, time.Duration, time.Duration, bool) {} +func (p *progressProv) ReportScrobble(playlist.Track, time.Duration, time.Duration, bool) error { + return nil +} -func (p *progressProv) ReportProgress(_ playlist.Track, position time.Duration) { +func (p *progressProv) ReportProgress(_ playlist.Track, position time.Duration) error { p.reports <- position + return nil } func TestTickProgressReportThrottles(t *testing.T) { diff --git a/ui/model/notifications.go b/ui/model/notifications.go index 72a1205..0fe342b 100644 --- a/ui/model/notifications.go +++ b/ui/model/notifications.go @@ -4,6 +4,7 @@ import ( "strings" "time" + "github.com/bjarneo/cliamp/applog" "github.com/bjarneo/cliamp/internal/playback" "github.com/bjarneo/cliamp/luaplugin" "github.com/bjarneo/cliamp/playlist" @@ -115,7 +116,12 @@ func (m *Model) nowPlaying(track playlist.Track) { return } canSeek := m.player.Seekable() - go reporter.ReportNowPlaying(track, m.player.Position(), canSeek) + position := m.player.Position() + go func() { + if err := reporter.ReportNowPlaying(track, position, canSeek); err != nil { + applog.Warn("now-playing report failed for %q: %v", track.Title, err) + } + }() } // maybeScrobble fires a playback-complete report for the given track if all @@ -163,7 +169,11 @@ func (m *Model) maybeScrobble(track playlist.Track, elapsed, duration time.Durat return // less than 50% played } canSeek := m.player.Seekable() - go reporter.ReportScrobble(track, elapsed, duration, canSeek) + go func() { + if err := reporter.ReportScrobble(track, elapsed, duration, canSeek); err != nil { + applog.Warn("scrobble failed for %q: %v", track.Title, err) + } + }() } // findPlaybackReporter returns the first registered provider that can report @@ -213,5 +223,10 @@ func (m *Model) tickProgressReport(now time.Time) { return } m.lastProgressReport = now - go reporter.ReportProgress(track, m.player.Position()) + position := m.player.Position() + go func() { + if err := reporter.ReportProgress(track, position); err != nil { + applog.Warn("progress report failed for %q: %v", track.Title, err) + } + }() }