From bb3bb281234bfbef57e71632424471b8e962942a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Wed, 14 Oct 2026 00:02:02 +0200 Subject: [PATCH 1/2] fix(bookmarks): play the item of the bookmark Enter on a bookmark seeked the player whatever played: a bookmark of book B moved book A. Stop the other playback and start the book or the episode of the bookmark at its time. ABS bookmarks have no episode ID. Find the episode of a podcast bookmark from the title prefix that handleAddBookmark writes. Show an error if no episode matches and the podcast does not play. --- AGENTS.md | 2 + internal/app/bookmarks.go | 76 +++++++++-- internal/app/bookmarks_test.go | 29 +++++ internal/app/e2e_test.go | 216 +++++++++++++++++++++++++++++++ internal/app/playback.go | 77 +++++++---- internal/screens/detail/model.go | 8 +- 6 files changed, 371 insertions(+), 37 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index fafe92a..3bf4cb5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -148,6 +148,8 @@ Styles from theme config. Format helpers (timestamps, durations). `RunProgram` r - Each launch makes a new mpv handle, which plays at the speed 1.0. `handlePlayerReady` sets the speed of the model in the new player. - The listening time (`timeListened`) is the time of the clock between two position ticks of a player that plays (`trackListeningTime`). It is not the change of the position: at the speed 2.0, the position moves two seconds in each second. A player that is paused or idle does not play. mpv reports no pause while it waits for data, so a position tick also reads the mpv property `core-idle` (`GetIdle`, `PositionMsg.Idle`). A test moves the clock with `Model.now`. - pine finds the end of a track through `errors.Is(err, player.ErrClosed)` on a position tick. spruce uses `PlayerEndMsg`, and a tick with `ErrClosed` can arrive before it. Do not match the text of a player error. +- Enter on a bookmark (`handleSeekToBookmark`) seeks the player only if the book or the episode of the bookmark plays. If not, it stops the playback and starts that book or that episode at the time of the bookmark. +- A bookmark of ABS has no episode ID. `handleAddBookmark` puts the episode title and `episodeTitleSeparator` before the title of a podcast bookmark, and `bookmarkEpisode` finds the episode from that title. The bookmarks on a server have that format, so do not change it. ## Testing diff --git a/internal/app/bookmarks.go b/internal/app/bookmarks.go index 6e9891a..d5dfc22 100644 --- a/internal/app/bookmarks.go +++ b/internal/app/bookmarks.go @@ -3,6 +3,7 @@ package app import ( "context" "fmt" + "strings" "time" "github.com/Thelost77/pine/internal/abs" @@ -37,7 +38,7 @@ func (m Model) handleAddBookmark(msg detail.AddBookmarkCmd) (Model, tea.Cmd) { currentTime := m.player.Position title := fmt.Sprintf("Bookmark at %s", ui.FormatTimestamp(currentTime)) if m.episodeID != "" && m.player.Title != "" { - title = fmt.Sprintf("%s — %s", m.player.Title, title) + title = m.player.Title + episodeTitleSeparator + title } logger.Info("creating bookmark", "itemID", itemID, "time", currentTime, "title", title) existing := m.detail.Bookmarks() @@ -58,16 +59,73 @@ func (m Model) handleAddBookmark(msg detail.AddBookmarkCmd) (Model, tea.Cmd) { } } -// handleSeekToBookmark seeks the player to a bookmark's timestamp (book-global). -func (m Model) handleSeekToBookmark(msg detail.SeekToBookmarkCmd) (Model, tea.Cmd) { - if !m.isPlaying() { - if m.client == nil || msg.Item.ID == "" { - return m, nil +// episodeTitleSeparator is between the episode title and the bookmark title +// in the title of a bookmark of a podcast episode. +const episodeTitleSeparator = " — " + +// bookmarkEpisode finds the episode of a podcast bookmark. A bookmark of ABS +// has no episode ID, so handleAddBookmark puts the episode title before the +// bookmark title. If more than one episode title is before the bookmark title, +// the longest is the episode. +func bookmarkEpisode(title string, episodes []abs.PodcastEpisode) (abs.PodcastEpisode, bool) { + var episode abs.PodcastEpisode + found := false + for _, ep := range episodes { + if ep.Title == "" || !strings.HasPrefix(title, ep.Title+episodeTitleSeparator) { + continue } - m.setSeriesContext(msg.Item) - return m, m.startPlaybackAtBookPositionCmd(msg.Item, msg.Time) + if !found || len(ep.Title) > len(episode.Title) { + episode, found = ep, true + } + } + return episode, found +} + +// handleSeekToBookmark moves the playback to a bookmark's timestamp +// (book-global). If the book or the episode of the bookmark plays, it seeks +// the player. If not, it starts the book or the episode at that timestamp. +func (m Model) handleSeekToBookmark(msg detail.SeekToBookmarkCmd) (Model, tea.Cmd) { + isPodcast := msg.Item.MediaType == "podcast" + var episode abs.PodcastEpisode + hasEpisode := false + if isPodcast { + episode, hasEpisode = bookmarkEpisode(msg.Title, m.detail.Episodes()) + } + + // The title of a podcast bookmark can give no episode, for example after + // an edit of the title. Then a bookmark of the podcast that plays seeks + // the episode that plays. + if m.isPlaying() && m.itemID == msg.Item.ID && (!hasEpisode || m.episodeID == episode.ID) { + return m.seekToBookGlobalPosition(msg.Time) + } + if m.client == nil || msg.Item.ID == "" { + return m, nil + } + if isPodcast && !hasEpisode { + logger.Warn("bookmark seek skipped: episode not found", "itemID", msg.Item.ID, "title", msg.Title) + cmd := m.err.SetError(fmt.Errorf("cannot find the episode of this bookmark")) + m.propagateSize() + return m, cmd + } + + // Different item or episode playing → stop it first + var closeCmd, stopCmd tea.Cmd + if m.isPlaying() { + logger.Info("switching playback", "from", m.itemID+"/"+m.episodeID, "to", msg.Item.ID+"/"+episode.ID) + m, closeCmd, stopCmd = m.stopPlaybackCmds() + } + + m.setSeriesContext(msg.Item) + var startCmd tea.Cmd + if hasEpisode { + startCmd = m.startEpisodePlaybackAtPositionCmd(msg.Item, episode, msg.Time) + } else { + startCmd = m.startPlaybackAtBookPositionCmd(msg.Item, msg.Time) + } + if stopCmd != nil { + return m, tea.Batch(stopCmd, closeThen(closeCmd, startCmd)) } - return m.seekToBookGlobalPosition(msg.Time) + return m, startCmd } // handleDeleteBookmark deletes a bookmark and removes it from the local list. diff --git a/internal/app/bookmarks_test.go b/internal/app/bookmarks_test.go index 1b94c0e..f1d820d 100644 --- a/internal/app/bookmarks_test.go +++ b/internal/app/bookmarks_test.go @@ -288,3 +288,32 @@ func TestHandleSeekToBookmarkStartsPlaybackWhenStopped(t *testing.T) { t.Errorf("expected no progress patch before starting playback, got %v", progressBody) } } + +func TestBookmarkEpisode(t *testing.T) { + episodes := []abs.PodcastEpisode{ + {ID: "ep-1", Title: "Pilot"}, + {ID: "ep-2", Title: "Pilot — Part 2"}, + {ID: "ep-3", Title: ""}, + } + + tests := []struct { + name string + title string + wantID string + wantOK bool + }{ + {"title of handleAddBookmark", "Pilot — Bookmark at 2:00", "ep-1", true}, + {"longest episode title", "Pilot — Part 2 — Bookmark at 2:00", "ep-2", true}, + {"no episode title", "Bookmark at 2:00", "", false}, + {"episode title with no separator", "Pilot", "", false}, + {"empty episode title", " — Bookmark at 2:00", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + episode, ok := bookmarkEpisode(tt.title, episodes) + if ok != tt.wantOK || episode.ID != tt.wantID { + t.Errorf("bookmarkEpisode(%q) = %q, %v, want %q, %v", tt.title, episode.ID, ok, tt.wantID, tt.wantOK) + } + }) + } +} diff --git a/internal/app/e2e_test.go b/internal/app/e2e_test.go index c538ddf..e65408b 100644 --- a/internal/app/e2e_test.go +++ b/internal/app/e2e_test.go @@ -576,6 +576,222 @@ func TestE2E_BookmarkEnterStartsPlaybackWhenStopped(t *testing.T) { } } +func TestE2E_BookmarkEnterSwitchesToItemOfBookmark(t *testing.T) { + log := &apiLog{} + state := &e2eServerState{bookmarks: make(map[string][]abs.Bookmark)} + state.bookmarks["item-002"] = []abs.Bookmark{ + testBookmark("In the other book", 300.0), + } + srv := newFullMockABSServer(log, state) + defer srv.Close() + + mp := &mockPlayer{position: 50, duration: 3600} + m := newE2EModelAuthenticated(srv, mp) + m = e2eSetSize(m, 120, 40) + + // Set up: item-001 plays + m.sessionID = "sess-other" + m.itemID = "item-001" + m.player.Playing = true + m.player.Position = 50.0 + m.player.Duration = 3600.0 + + // Open the detail screen of item-002 and select its bookmark + item := testLibraryItem("item-002", "1984") + res, cmd := m.Update(home.NavigateDetailMsg{Item: item}) + m = res.(Model) + assertScreen(t, m, ScreenDetail) + m = feedCmdChain(m, cmd, 5) + + m, _ = e2ePressSpecial(m, tea.KeyTab) + if !m.detail.FocusBookmarks() { + t.Fatal("expected bookmark focus after Tab") + } + + m, cmd = e2ePressSpecial(m, tea.KeyEnter) + m, cmd = feedCmd(m, cmd) + m = feedCmdChain(m, cmd, 2) + + assertAPICallMade(t, log, "POST", "/api/session/sess-other/close") + assertAPICallMade(t, log, "POST", "/api/items/item-002/play") + if m.itemID != "item-002" { + t.Fatalf("itemID = %q, want item-002", m.itemID) + } + if m.sessionID != "sess-e2e" { + t.Fatalf("sessionID = %q, want sess-e2e", m.sessionID) + } + if m.player.Position != 300.0 { + t.Fatalf("player position = %f, want 300.0", m.player.Position) + } + if !mp.launched { + t.Fatal("expected player launch after bookmark enter") + } +} + +func TestE2E_PodcastBookmarkEnterSwitchesToEpisodeOfBookmark(t *testing.T) { + log := &apiLog{} + state := &e2eServerState{bookmarks: make(map[string][]abs.Bookmark)} + state.bookmarks["pod-001"] = []abs.Bookmark{ + testBookmark("Episode 2 - Deep Dive — Bookmark at 5:00", 300.0), + } + srv := newFullMockABSServer(log, state) + defer srv.Close() + + mp := &mockPlayer{position: 50, duration: 3600} + m := newE2EModelAuthenticated(srv, mp) + m = e2eSetSize(m, 120, 40) + + // Set up: a book plays + m.sessionID = "sess-other" + m.itemID = "item-001" + m.player.Playing = true + m.player.Position = 50.0 + m.player.Duration = 3600.0 + + // Open the detail screen of the podcast and select its bookmark + item := testPodcastItem("pod-001", "Tech Talk Daily") + res, cmd := m.Update(home.NavigateDetailMsg{Item: item}) + m = res.(Model) + assertScreen(t, m, ScreenDetail) + m = feedCmdChain(m, cmd, 5) + + m, _ = e2ePressSpecial(m, tea.KeyTab) + if !m.detail.FocusBookmarks() { + t.Fatal("expected bookmark focus after Tab") + } + + m, cmd = e2ePressSpecial(m, tea.KeyEnter) + m, cmd = feedCmd(m, cmd) + m = feedCmdChain(m, cmd, 2) + + assertAPICallMade(t, log, "POST", "/api/session/sess-other/close") + assertAPICallMade(t, log, "POST", "/api/items/pod-001/play/pod-001-ep-002") + if m.itemID != "pod-001" || m.episodeID != "pod-001-ep-002" { + t.Fatalf("playing %q/%q, want pod-001/pod-001-ep-002", m.itemID, m.episodeID) + } + if m.player.Position != 300.0 { + t.Fatalf("player position = %f, want 300.0", m.player.Position) + } + if !mp.launched { + t.Fatal("expected player launch after bookmark enter") + } +} + +func TestE2E_PodcastBookmarkEnterWithSamePodcastPlaying(t *testing.T) { + log := &apiLog{} + state := &e2eServerState{bookmarks: make(map[string][]abs.Bookmark)} + state.bookmarks["pod-001"] = []abs.Bookmark{ + testBookmark("Episode 1 - Pilot — Bookmark at 2:00", 120.0), + testBookmark("Episode 2 - Deep Dive — Bookmark at 5:00", 300.0), + } + srv := newFullMockABSServer(log, state) + defer srv.Close() + + mp := &mockPlayer{position: 50, duration: testEpisodeDuration} + m := newE2EModelAuthenticated(srv, mp) + m = e2eSetSize(m, 120, 40) + + // Set up: episode 1 of the podcast plays + m.sessionID = "sess-ep-one" + m.itemID = "pod-001" + m.episodeID = "pod-001-ep-001" + m.player.Playing = true + m.player.Position = 50.0 + m.player.Duration = testEpisodeDuration + + item := testPodcastItem("pod-001", "Tech Talk Daily") + res, cmd := m.Update(home.NavigateDetailMsg{Item: item}) + m = res.(Model) + assertScreen(t, m, ScreenDetail) + m = feedCmdChain(m, cmd, 5) + + m, _ = e2ePressSpecial(m, tea.KeyTab) + if !m.detail.FocusBookmarks() { + t.Fatal("expected bookmark focus after Tab") + } + + // --- Bookmark of the episode that plays: seek --- + m, cmd = e2ePressSpecial(m, tea.KeyEnter) + m, cmd = feedCmd(m, cmd) + m = feedCmdChain(m, cmd, 2) + + assertNoAPICall(t, log, "POST", "/api/items/pod-001/play") + if m.sessionID != "sess-ep-one" { + t.Fatalf("sessionID = %q, want sess-ep-one", m.sessionID) + } + mp.mu.Lock() + pos := mp.position + mp.mu.Unlock() + if pos != 120.0 { + t.Fatalf("mock player position = %f, want 120.0 (bookmark time)", pos) + } + + // --- Bookmark of a different episode: start that episode --- + m, _ = e2ePressKey(m, 'j') + m, cmd = e2ePressSpecial(m, tea.KeyEnter) + m, cmd = feedCmd(m, cmd) + m = feedCmdChain(m, cmd, 2) + + assertAPICallMade(t, log, "POST", "/api/session/sess-ep-one/close") + assertAPICallMade(t, log, "POST", "/api/items/pod-001/play/pod-001-ep-002") + if m.itemID != "pod-001" || m.episodeID != "pod-001-ep-002" { + t.Fatalf("playing %q/%q, want pod-001/pod-001-ep-002", m.itemID, m.episodeID) + } + if m.player.Position != 300.0 { + t.Fatalf("player position = %f, want 300.0", m.player.Position) + } +} + +func TestE2E_PodcastBookmarkEnterWithUnknownEpisodeKeepsPlayback(t *testing.T) { + log := &apiLog{} + state := &e2eServerState{bookmarks: make(map[string][]abs.Bookmark)} + state.bookmarks["pod-001"] = []abs.Bookmark{ + testBookmark("A title with no episode", 300.0), + } + srv := newFullMockABSServer(log, state) + defer srv.Close() + + mp := &mockPlayer{position: 50, duration: 3600} + m := newE2EModelAuthenticated(srv, mp) + m = e2eSetSize(m, 120, 40) + + // Set up: a book plays + m.sessionID = "sess-other" + m.itemID = "item-001" + m.player.Playing = true + m.player.Position = 50.0 + m.player.Duration = 3600.0 + + item := testPodcastItem("pod-001", "Tech Talk Daily") + res, cmd := m.Update(home.NavigateDetailMsg{Item: item}) + m = res.(Model) + assertScreen(t, m, ScreenDetail) + m = feedCmdChain(m, cmd, 5) + + m, _ = e2ePressSpecial(m, tea.KeyTab) + if !m.detail.FocusBookmarks() { + t.Fatal("expected bookmark focus after Tab") + } + + m, cmd = e2ePressSpecial(m, tea.KeyEnter) + m, _ = feedCmd(m, cmd) + + assertNoAPICall(t, log, "POST", "/api/session/sess-other/close") + assertNoAPICall(t, log, "POST", "/api/items/pod-001/play") + if m.sessionID != "sess-other" || m.itemID != "item-001" { + t.Fatalf("playing %q in session %q, want item-001 in sess-other", m.itemID, m.sessionID) + } + mp.mu.Lock() + pos := mp.position + mp.mu.Unlock() + if pos != 50.0 { + t.Fatalf("mock player position = %f, want 50.0 (no seek)", pos) + } + if !m.err.HasError() { + t.Fatal("expected an error for a bookmark with no known episode") + } +} + func TestE2E_BookmarkTitleEdit(t *testing.T) { log := &apiLog{} state := &e2eServerState{bookmarks: make(map[string][]abs.Bookmark)} diff --git a/internal/app/playback.go b/internal/app/playback.go index a1c9d3e..678a6ba 100644 --- a/internal/app/playback.go +++ b/internal/app/playback.go @@ -228,32 +228,11 @@ func (m Model) handlePlayEpisodeCmd(msg detail.PlayEpisodeCmd) (Model, tea.Cmd) if err != nil { return PlaybackErrorMsg{Err: err} } - if len(session.AudioTracks) == 0 { - return PlaybackErrorMsg{Err: fmt.Errorf("no audio tracks")} - } - - streamURL := client.BaseURL() + session.AudioTracks[0].ContentURL - logger.Info("track selected for episode playback", "itemID", item.ID, "episodeID", episode.ID, "sessionID", session.ID, "trackIndex", session.AudioTracks[0].Index, "trackStart", session.AudioTracks[0].StartOffset, "trackDuration", session.AudioTracks[0].Duration, "bookPosition", session.CurrentTime) - - return PlaySessionMsg{ - Session: PlaySessionData{ - SessionID: session.ID, - ItemID: item.ID, - EpisodeID: episode.ID, - CurrentTime: session.CurrentTime, - SyncedTime: session.CurrentTime, - Duration: resolveEpisodeDuration(episode, session), - Title: episode.Title, - Authors: mprisAuthors(item), - Chapters: playSessionChapters(session), - TrackStartOffset: session.AudioTracks[0].StartOffset, - TrackDuration: session.AudioTracks[0].Duration, - TrackFilename: session.AudioTracks[0].Metadata.Filename, - TranscriptIno: session.TranscriptIno(session.AudioTracks[0].Metadata.Filename), - }, - StreamURL: streamURL, - AuthToken: client.Token(), + playMsg, err := buildEpisodePlaySessionMsg(client, session, item, episode, session.CurrentTime) + if err != nil { + return PlaybackErrorMsg{Err: err} } + return playMsg } if stopCmd != nil { @@ -565,6 +544,22 @@ func (m Model) startPlaybackAtBookPositionCmd(item abs.LibraryItem, bookPos floa } } +func (m Model) startEpisodePlaybackAtPositionCmd(item abs.LibraryItem, episode abs.PodcastEpisode, pos float64) tea.Cmd { + client := m.client + return func() tea.Msg { + device := m.playbackDevice() + session, err := client.StartEpisodePlaySession(context.Background(), item.ID, episode.ID, device) + if err != nil { + return PlaybackErrorMsg{Err: err} + } + playMsg, err := buildEpisodePlaySessionMsg(client, session, item, episode, pos) + if err != nil { + return PlaybackErrorMsg{Err: err} + } + return playMsg + } +} + func (m Model) startRestorePlaybackCmd(msg RestoreSessionMsg) tea.Cmd { if msg.Item == nil || m.client == nil { return nil @@ -770,6 +765,38 @@ func buildBookPlaySessionMsg(client *cache.Client, session *abs.PlaySession, ite }, nil } +// buildEpisodePlaySessionMsg makes the PlaySessionMsg that starts a podcast +// episode at the position pos. +func buildEpisodePlaySessionMsg(client *cache.Client, session *abs.PlaySession, item abs.LibraryItem, episode abs.PodcastEpisode, pos float64) (PlaySessionMsg, error) { + if len(session.AudioTracks) == 0 { + return PlaySessionMsg{}, fmt.Errorf("no audio tracks") + } + + track := session.AudioTracks[0] + streamURL := client.BaseURL() + track.ContentURL + logger.Info("track selected for episode playback", "itemID", item.ID, "episodeID", episode.ID, "sessionID", session.ID, "trackIndex", track.Index, "trackStart", track.StartOffset, "trackDuration", track.Duration, "bookPosition", pos) + + return PlaySessionMsg{ + Session: PlaySessionData{ + SessionID: session.ID, + ItemID: item.ID, + EpisodeID: episode.ID, + CurrentTime: pos, + SyncedTime: session.CurrentTime, + Duration: resolveEpisodeDuration(episode, session), + Title: episode.Title, + Authors: mprisAuthors(item), + Chapters: playSessionChapters(session), + TrackStartOffset: track.StartOffset, + TrackDuration: track.Duration, + TrackFilename: track.Metadata.Filename, + TranscriptIno: session.TranscriptIno(track.Metadata.Filename), + }, + StreamURL: streamURL, + AuthToken: client.Token(), + }, nil +} + func resolveEpisodeDuration(episode abs.PodcastEpisode, session *abs.PlaySession) float64 { if episode.Duration > 0 { return episode.Duration diff --git a/internal/screens/detail/model.go b/internal/screens/detail/model.go index 56aa5e2..17324e0 100644 --- a/internal/screens/detail/model.go +++ b/internal/screens/detail/model.go @@ -55,9 +55,11 @@ type NavigateSeriesMsg struct { } // SeekToBookmarkCmd requests seeking the player to a bookmark's timestamp. +// The root model finds the episode of a podcast bookmark from Title. type SeekToBookmarkCmd struct { - Item abs.LibraryItem - Time float64 + Item abs.LibraryItem + Time float64 + Title string } // DeleteBookmarkCmd requests deletion of a bookmark. @@ -598,7 +600,7 @@ func (m Model) Update(msg tea.Msg) (Model, tea.Cmd) { item := m.item bm := m.bookmarks[m.selectedBookmark] return m, func() tea.Msg { - return SeekToBookmarkCmd{Item: item, Time: bm.Time} + return SeekToBookmarkCmd{Item: item, Time: bm.Time, Title: bm.Title} } } if !m.focusBookmarks && !m.focusEpisodes && !m.focusSeries { From fbfc50e4aedc0a18447068bc7bf3c4e1846976b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Wed, 14 Oct 2026 00:03:00 +0200 Subject: [PATCH 2/2] test(bookmarks): run the command of the error case The unknown-episode test did not run the command that Enter returns, so its checks for no seek and no request could not fail. --- internal/app/e2e_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/app/e2e_test.go b/internal/app/e2e_test.go index e65408b..0b84395 100644 --- a/internal/app/e2e_test.go +++ b/internal/app/e2e_test.go @@ -774,7 +774,8 @@ func TestE2E_PodcastBookmarkEnterWithUnknownEpisodeKeepsPlayback(t *testing.T) { } m, cmd = e2ePressSpecial(m, tea.KeyEnter) - m, _ = feedCmd(m, cmd) + m, cmd = feedCmd(m, cmd) + m = feedCmdChain(m, cmd, 2) assertNoAPICall(t, log, "POST", "/api/session/sess-other/close") assertNoAPICall(t, log, "POST", "/api/items/pod-001/play")