From 9607dfa1d7758d3cba86b2d04d39d00b49a86564 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 21:43:39 +0200 Subject: [PATCH 01/15] fix(bookmarks): read bookmarks from the bookmark route - The detail screen read the bookmarks from the key `bookmarks` of GET /api/me/progress/{id}. ABS gives no such key in that response (getOldMediaProgress in server/models/MediaProgress.js), and it answers 404 for an item with no progress. So the detail screen showed "No bookmarks yet" for an item that has bookmarks. - GetBookmarks now reads GET /api/me/bookmarks/{id}, which ABS has from 2.36.0. An older ABS answers 404 for that route. GetBookmarks then reads the bookmarks of the item from GET /api/me. - The detail screen calls GetBookmarks of the cache client. The cache key is `bookmarks:{id}`, which the bookmark commands already remove. - The fake server of the E2E tests gave bookmarks with the progress. It now has the bookmark route, as ABS 2.37.1. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 4 +-- internal/abs/bookmarks.go | 32 +++++++++++++++-- internal/abs/bookmarks_test.go | 59 ++++++++++++++++++++++++-------- internal/app/bookmarks.go | 7 ++-- internal/app/e2e_helpers_test.go | 43 ++++++++++------------- internal/app/e2e_test.go | 6 ++-- internal/cache/client_test.go | 36 +++++++++++++++++++ 7 files changed, 134 insertions(+), 53 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d4ce576..8685b0b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -116,8 +116,8 @@ The server is written from the MPRIS 2.2 specification. The Go MPRIS server libr | `types.go` | All ABS data types. Custom UnmarshalJSON for MediaMetadata.Series (handles both object and array). | | `libraries.go` | GetLibraries, GetPersonalized, GetLibraryItems (paginated), GetLibraryItemsBatch (10 concurrent), GetLibrarySeries, GetSeriesContents, FilterAudioLibraries (sample-based audio detection) | | `playback.go` | StartPlaySession, StartEpisodePlaySession, SyncSession, CloseSession, CloseSessionNoSync (sends no position) | -| `progress.go`, `me.go` | UpdateProgress, UpdatePosition (no `isFinished` key), GetMediaProgress (with bookmarks), UpdateEpisodeProgress, GetMe (user + all media progress) | -| `bookmarks.go` | CreateBookmark, UpdateBookmark, DeleteBookmark, GetBookmarks | +| `progress.go`, `me.go` | UpdateProgress, UpdatePosition (no `isFinished` key), GetMediaProgress (ABS gives no bookmarks with the progress), UpdateEpisodeProgress, GetMe (user + all media progress) | +| `bookmarks.go` | CreateBookmark, UpdateBookmark, DeleteBookmark, GetBookmarks (`GET /api/me/bookmarks/{id}`, or `GET /api/me` for ABS before 2.36.0) | ### Data (internal/db/) diff --git a/internal/abs/bookmarks.go b/internal/abs/bookmarks.go index 096e432..eb4b2b4 100644 --- a/internal/abs/bookmarks.go +++ b/internal/abs/bookmarks.go @@ -2,6 +2,7 @@ package abs import ( "context" + "encoding/json" "fmt" "net/http" "strconv" @@ -13,13 +14,38 @@ type createBookmarkRequest struct { Title string `json:"title"` } -// GetBookmarks returns bookmarks for a library item via the per-item progress endpoint. +// bookmarksResponse is the body of GET /api/me/bookmarks/{id}. The body of +// GET /api/me has the same key, with the bookmarks of all items. +type bookmarksResponse struct { + Bookmarks []Bookmark `json:"bookmarks"` +} + +// GetBookmarks returns the bookmarks of the user for a library item. ABS +// before 2.36.0 has no route for the bookmarks of one item, and answers 404. +// GetBookmarks then reads the bookmarks of the item from GET /api/me. func (c *Client) GetBookmarks(ctx context.Context, itemID string) ([]Bookmark, error) { - progress, err := c.GetMediaProgress(ctx, itemID) + resp, err := c.do(ctx, http.MethodGet, "/api/me/bookmarks/"+itemID, nil) + allItems := IsHTTPStatus(err, http.StatusNotFound) + if allItems { + resp, err = c.do(ctx, http.MethodGet, "/api/me", nil) + } if err != nil { return nil, fmt.Errorf("get bookmarks: %w", err) } - return progress.Bookmarks, nil + var body bookmarksResponse + if err := json.Unmarshal(resp, &body); err != nil { + return nil, fmt.Errorf("get bookmarks: unmarshal: %w", err) + } + if !allItems { + return body.Bookmarks, nil + } + bookmarks := []Bookmark{} + for _, bookmark := range body.Bookmarks { + if bookmark.LibraryItemID == itemID { + bookmarks = append(bookmarks, bookmark) + } + } + return bookmarks, nil } // CreateBookmark creates a bookmark on a library item at the given time. diff --git a/internal/abs/bookmarks_test.go b/internal/abs/bookmarks_test.go index 93637b5..d772ef2 100644 --- a/internal/abs/bookmarks_test.go +++ b/internal/abs/bookmarks_test.go @@ -9,25 +9,26 @@ import ( "testing" ) +// TestGetBookmarksHTTP verifies that GetBookmarks reads the route of the +// bookmarks. The server answers as ABS 2.37.1 does: the progress of an item +// has no bookmarks. func TestGetBookmarksHTTP(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/api/me/progress/li-001" { - t.Errorf("path = %q, want /api/me/progress/li-001", r.URL.Path) - } if r.Method != http.MethodGet { t.Errorf("method = %q, want GET", r.Method) } w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{ - "libraryItemId": "li-001", - "currentTime": 0, - "progress": 0, - "isFinished": false, - "bookmarks": [ - {"libraryItemId": "li-001", "title": "Great passage", "time": 300.5, "createdAt": 1700000000000}, - {"libraryItemId": "li-001", "title": "Important quote", "time": 1500.0, "createdAt": 1700001000000} - ] - }`)) + switch r.URL.Path { + case "/api/me/bookmarks/li-001": + _, _ = w.Write([]byte(`{"bookmarks": [ + {"libraryItemId": "li-001", "time": 300.5, "title": "Great passage", "createdAt": 1700000000000}, + {"libraryItemId": "li-001", "time": 1500, "title": "Important quote", "createdAt": 1700001000000} + ]}`)) + case "/api/me/progress/li-001": + _, _ = w.Write([]byte(`{"id": "mp-001", "libraryItemId": "li-001", "episodeId": null, "duration": 3600, "progress": 0.1, "currentTime": 360, "isFinished": false}`)) + default: + http.NotFound(w, r) + } })) defer srv.Close() @@ -47,6 +48,34 @@ func TestGetBookmarksHTTP(t *testing.T) { } } +// TestGetBookmarksReadsUserBeforeABS236 verifies that GetBookmarks reads the +// bookmarks of the item from the user, if the server has no route for the +// bookmarks of one item. ABS before 2.36.0 answers 404 for that route. +func TestGetBookmarksReadsUserBeforeABS236(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/me" { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id": "usr-001", "bookmarks": [ + {"libraryItemId": "li-001", "time": 300.5, "title": "Great passage", "createdAt": 1700000000000}, + {"libraryItemId": "li-002", "time": 10, "title": "Other book", "createdAt": 1700000500000}, + {"libraryItemId": "li-001", "time": 1500, "title": "Important quote", "createdAt": 1700001000000} + ]}`)) + })) + defer srv.Close() + + c := NewClient(srv.URL, "tok") + bookmarks, err := c.GetBookmarks(context.Background(), "li-001") + if err != nil { + t.Fatalf("GetBookmarks() error: %v", err) + } + if len(bookmarks) != 2 || bookmarks[0].Title != "Great passage" || bookmarks[1].Title != "Important quote" { + t.Errorf("bookmarks = %+v, want the two bookmarks of li-001", bookmarks) + } +} + func TestCreateBookmarkHTTP(t *testing.T) { var capturedBody []byte srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -160,7 +189,7 @@ func TestDeleteBookmarkHTTPPreservesBookmarkPrecision(t *testing.T) { func TestGetBookmarksEmptyResponse(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"libraryItemId":"li-002","currentTime":0,"progress":0,"isFinished":false,"bookmarks":[]}`)) + _, _ = w.Write([]byte(`{"bookmarks":[]}`)) })) defer srv.Close() @@ -203,7 +232,7 @@ func TestGetBookmarksServerErrorReturnsError(t *testing.T) { func TestGetBookmarksMalformedResponseReturnsError(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"libraryItemId":`)) + _, _ = w.Write([]byte(`{"bookmarks":`)) })) defer srv.Close() diff --git a/internal/app/bookmarks.go b/internal/app/bookmarks.go index 322c49f..3383d12 100644 --- a/internal/app/bookmarks.go +++ b/internal/app/bookmarks.go @@ -132,14 +132,11 @@ func (m Model) fetchBookmarksCmd(itemID string) tea.Cmd { } client := m.client return func() tea.Msg { - progress, err := client.GetMediaProgress(context.Background(), itemID) + bookmarks, err := client.GetBookmarks(context.Background(), itemID) if err != nil { - if abs.IsHTTPStatus(err, 404) { - return detail.BookmarksUpdatedMsg{Bookmarks: nil} - } return detail.BookmarksUpdatedMsg{Err: err} } - return detail.BookmarksUpdatedMsg{Bookmarks: progress.Bookmarks} + return detail.BookmarksUpdatedMsg{Bookmarks: bookmarks} } } diff --git a/internal/app/e2e_helpers_test.go b/internal/app/e2e_helpers_test.go index 553a4c1..152fbd3 100644 --- a/internal/app/e2e_helpers_test.go +++ b/internal/app/e2e_helpers_test.go @@ -108,12 +108,12 @@ func testBookmark(title string, seconds float64) abs.Bookmark { // e2eServerState holds mutable state for the mock server. type e2eServerState struct { - mu sync.Mutex - bookmarks map[string][]abs.Bookmark // itemID → bookmarks - progressStatus map[string]int // itemID → GET /api/me/progress status override - meStatus int // GET /api/me status override - force401 bool // when true, next API call returns 401 - noTrackEpisode map[string]bool // itemID|episodeID → return play session without tracks + mu sync.Mutex + bookmarks map[string][]abs.Bookmark // itemID → bookmarks + bookmarksStatus map[string]int // itemID → GET /api/me/bookmarks status override + meStatus int // GET /api/me status override + force401 bool // when true, next API call returns 401 + noTrackEpisode map[string]bool // itemID|episodeID → return play session without tracks } func (s *e2eServerState) shouldReturnNoTracks(itemID, episodeID string) bool { @@ -161,13 +161,13 @@ func (s *e2eServerState) allBookmarks() []abs.Bookmark { return all } -func (s *e2eServerState) getProgressStatus(itemID string) int { +func (s *e2eServerState) getBookmarksStatus(itemID string) int { s.mu.Lock() defer s.mu.Unlock() - if s.progressStatus == nil { + if s.bookmarksStatus == nil { return http.StatusOK } - if status, ok := s.progressStatus[itemID]; ok { + if status, ok := s.bookmarksStatus[itemID]; ok { return status } return http.StatusOK @@ -333,24 +333,17 @@ func newFullMockABSServer(log *apiLog, state *e2eServerState) *httptest.Server { w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"error":"item not found"}`)) - // --- Bookmarks (via progress endpoint) --- - case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/api/me/progress/"): - itemID := strings.TrimPrefix(r.URL.Path, "/api/me/progress/") - status := state.getProgressStatus(itemID) - if status != http.StatusOK { - if status == http.StatusNotFound { - http.Error(w, `{"error":"not found"}`, status) - } else { - http.Error(w, `{"error":"boom"}`, status) - } + // --- Bookmarks of one item. The progress of an item has no bookmarks. --- + case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/api/me/bookmarks/"): + itemID := strings.TrimPrefix(r.URL.Path, "/api/me/bookmarks/") + if status := state.getBookmarksStatus(itemID); status != http.StatusOK { + http.Error(w, `{"error":"boom"}`, status) return } - bms := state.getBookmarks(itemID) - resp := abs.MediaProgressWithBookmarks{ - LibraryItemID: itemID, - CurrentTime: 42.0, - Progress: 0.0117, - Bookmarks: bms, + resp := struct { + Bookmarks []abs.Bookmark `json:"bookmarks"` + }{ + Bookmarks: state.getBookmarks(itemID), } _ = json.NewEncoder(w).Encode(resp) diff --git a/internal/app/e2e_test.go b/internal/app/e2e_test.go index dde29f1..6456fc4 100644 --- a/internal/app/e2e_test.go +++ b/internal/app/e2e_test.go @@ -650,7 +650,7 @@ func TestE2E_BookmarkFetchOnDetailEntry(t *testing.T) { // Execute the bookmark fetch m = feedCmdChain(m, cmd, 5) - assertAPICallMade(t, log, "GET", "/api/me") + assertAPICallMade(t, log, "GET", "/api/me/bookmarks/item-001") // Verify bookmarks are on the detail screen if len(m.detail.Bookmarks()) != 2 { @@ -689,8 +689,8 @@ func TestE2E_BookmarkFetchOnDetailEntryShowsEmptyStateForNoBookmarks(t *testing. func TestE2E_BookmarkFetchOnDetailEntryShowsBookmarkErrorForServerFailure(t *testing.T) { log := &apiLog{} state := &e2eServerState{ - bookmarks: make(map[string][]abs.Bookmark), - progressStatus: map[string]int{"item-001": http.StatusInternalServerError}, + bookmarks: make(map[string][]abs.Bookmark), + bookmarksStatus: map[string]int{"item-001": http.StatusInternalServerError}, } srv := newFullMockABSServer(log, state) defer srv.Close() diff --git a/internal/cache/client_test.go b/internal/cache/client_test.go index 5b5caef..b3f6f17 100644 --- a/internal/cache/client_test.go +++ b/internal/cache/client_test.go @@ -274,6 +274,42 @@ func TestClient_GetLibraryItem_CacheHit(t *testing.T) { } } +// TestClient_GetBookmarks_CacheHit verifies that the cache keeps the bookmarks +// of an item, also if the item has no bookmarks. +func TestClient_GetBookmarks_CacheHit(t *testing.T) { + requestCount := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestCount++ + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/api/me/bookmarks/li1": + _, _ = w.Write([]byte(`{"bookmarks":[{"libraryItemId":"li1","time":30,"title":"mark1","createdAt":1700000000000}]}`)) + case "/api/me/bookmarks/li2": + _, _ = w.Write([]byte(`{"bookmarks":[]}`)) + default: + t.Errorf("unexpected path %q", r.URL.Path) + } + })) + defer srv.Close() + + client := NewClient(abs.NewClient(srv.URL, "tok"), NewStore(openTestDB(t))) + + for itemID, want := range map[string]int{"li1": 1, "li2": 0} { + for call := 1; call <= 2; call++ { + bookmarks, err := client.GetBookmarks(context.Background(), itemID) + if err != nil { + t.Fatalf("GetBookmarks(%s) call %d error: %v", itemID, call, err) + } + if len(bookmarks) != want { + t.Errorf("GetBookmarks(%s) call %d = %d bookmarks, want %d", itemID, call, len(bookmarks), want) + } + } + } + if requestCount != 2 { + t.Errorf("%d requests, want 1 for each item", requestCount) + } +} + func TestClient_GetRecentlyAdded_CacheHit(t *testing.T) { requestCount := 0 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From 48fa28a1b8c982248c94c4f5a53b97706999c659 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 21:44:41 +0200 Subject: [PATCH 02/15] fix(playback): set the speed of the model in each new player - Each launch makes a new mpv handle, and a new handle plays at the speed 1.0. Only the speed keys and the MPRIS rate set the speed of the player. So after the start of a session, and after a track rollover, the player played at the speed 1.0, while the footer showed the speed of the model. The speed of the config file had no effect on the player. - The model now sets its speed in the player when the player is ready, as spruce does. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 1 + internal/app/model_test.go | 14 +++++++++++--- internal/app/playback.go | 5 ++++- internal/app/playback_test.go | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8685b0b..ef3553a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -143,6 +143,7 @@ Styles from theme config. Format helpers (timestamps, durations). Components: He - A quit (`q` or ctrl+c) returns only `tea.Quit` and keeps the session in the model. `Cleanup` sends the last position, and `main` waits for it. Bubble Tea does not wait for a command at a quit, so a quit must not send the position in a command. - `ctrl+p` opens the global command palette from any screen - Duration from ABS takes precedence over mpv-reported duration (streaming content reports 0) +- 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 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. diff --git a/internal/app/model_test.go b/internal/app/model_test.go index 2c5abf3..8c17241 100644 --- a/internal/app/model_test.go +++ b/internal/app/model_test.go @@ -33,13 +33,16 @@ type mockPlayer struct { position float64 duration float64 paused bool + speed float64 launchErr error } +// Launch starts a new playback, which has the speed 1.0, as a new mpv handle. func (p *mockPlayer) Launch(url, startTime string, paused bool, httpHeaders []string, volume int) error { p.mu.Lock() defer p.mu.Unlock() p.launched = true + p.speed = 1.0 return p.launchErr } func (p *mockPlayer) GetPosition() (float64, error) { @@ -75,9 +78,14 @@ func (p *mockPlayer) SeekRelative(seconds float64) error { p.position += seconds return nil } -func (p *mockPlayer) SetSpeed(speed float64) error { return nil } -func (p *mockPlayer) SetVolume(vol int) error { return nil } -func (p *mockPlayer) GetVolume() (int, error) { return 100, nil } +func (p *mockPlayer) SetSpeed(speed float64) error { + p.mu.Lock() + defer p.mu.Unlock() + p.speed = speed + return nil +} +func (p *mockPlayer) SetVolume(vol int) error { return nil } +func (p *mockPlayer) GetVolume() (int, error) { return 100, nil } func (p *mockPlayer) Quit() error { p.mu.Lock() defer p.mu.Unlock() diff --git a/internal/app/playback.go b/internal/app/playback.go index c44ab8a..7059aa6 100644 --- a/internal/app/playback.go +++ b/internal/app/playback.go @@ -342,7 +342,9 @@ func loadCaptionsCmd(client *cache.Client, itemID, audioFilename, ino string, ge } } -// handlePlayerReady starts the position tick and sync tick. +// handlePlayerReady starts the position tick and sync tick. It sets the speed +// of the model in the player: each launch makes a new player, which plays at +// the speed 1.0. // Stale PlayerReadyMsg events (from a superseded play session) are ignored so // duplicate polling ticks don't race with the active session. func (m Model) handlePlayerReady(msg player.PlayerReadyMsg) (Model, tea.Cmd) { @@ -353,6 +355,7 @@ func (m Model) handlePlayerReady(msg player.PlayerReadyMsg) (Model, tea.Cmd) { logger.Info("player ready, starting position ticks", "generation", m.playGeneration) m.propertyUnavailableCount = 0 return m, tea.Batch( + player.SetSpeedCmd(m.mpv, m.player.Speed), player.TickCmd(m.mpv, m.playGeneration), syncTickCmd(), ) diff --git a/internal/app/playback_test.go b/internal/app/playback_test.go index dbedd82..30c9428 100644 --- a/internal/app/playback_test.go +++ b/internal/app/playback_test.go @@ -297,6 +297,38 @@ func TestPlaybackLifecycleIntegration(t *testing.T) { } } +// TestNewPlaybackGetsSpeedOfModel verifies that each new playback plays at the +// speed of the model, which the footer shows. Each launch makes a new player, +// and a new player plays at the speed 1.0. A new session and a track rollover +// make a launch. +func TestNewPlaybackGetsSpeedOfModel(t *testing.T) { + cfg := config.Default() + cfg.Player.Speed = 1.5 + mp := &mockPlayer{} + m := NewWithPlayer(cfg, nil, nil, nil, mp) + + for _, sessionID := range []string{"sess-first", "sess-next-track"} { + result, cmd := m.Update(PlaySessionMsg{ + Session: PlaySessionData{SessionID: sessionID, ItemID: "item-001", Duration: 3600}, + StreamURL: "http://test/audio.mp3", + }) + // The launch gives PlayerReadyMsg, and the model gives its commands. + var ready tea.Cmd + m, ready = feedCmd(result.(Model), cmd) + executeBatchCmds(ready) + + mp.mu.Lock() + speed := mp.speed + mp.mu.Unlock() + if speed != 1.5 { + t.Errorf("speed of the player in the session %s = %v, want 1.5", sessionID, speed) + } + if m.player.Speed != 1.5 { + t.Errorf("speed of the model in the session %s = %v, want 1.5", sessionID, m.player.Speed) + } + } +} + func TestPlayEpisodeCmdUsesSessionDurationWhenEpisodeDurationMissing(t *testing.T) { log := &apiLog{} srv := newMockABSServer(log) From 41596fa3160439ccd1ed7c61beb51165736a93d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 21:49:00 +0200 Subject: [PATCH 03/15] fix(playback): count no listening time while the player waits for data - The listening time counted the time of the clock while the player reported no pause. mpv reports no pause while it fills its cache: the property `pause` stays false, the position does not move, and the property `core-idle` is true. So the time of a stalled stream counted as listening time. - A position tick now also reads `core-idle` (GetIdle, PositionMsg.Idle). A player that is paused or idle adds no listening time. The footer and MPRIS still show the pause state only. - The Player interface has the new method GetIdle. spruce gets the new field in its ticks and does not use it. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 4 +- internal/app/model_test.go | 6 +++ internal/app/playback.go | 18 +++++---- internal/app/progress_sync_test.go | 53 ++++++++++++++++++++++--- internal/player/commands.go | 13 +++++-- internal/player/commands_test.go | 1 + internal/player/mpv.go | 29 +++++++++----- internal/player/mpv_test.go | 62 ++++++++++++++++++++++++++++++ 8 files changed, 159 insertions(+), 27 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ef3553a..9ead3d5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -69,7 +69,7 @@ The Architecture Map below describes pine and the shared packages. | File | Responsibility | |------|---------------| -| `mpv.go` | Mpv struct: Launch (creates a libmpv handle and loads the file), GetPosition/GetDuration/GetPaused, Seek, SetSpeed/SetVolume, Quit. `ErrClosed` and `ErrPropertyUnavailable`. Writes the warnings and errors of mpv to the app log, without URL queries. Runtime directory for the media cache. | +| `mpv.go` | Mpv struct: Launch (creates a libmpv handle and loads the file), GetPosition/GetDuration/GetPaused/GetIdle, Seek, SetSpeed/SetVolume, Quit. `ErrClosed` and `ErrPropertyUnavailable`. Writes the warnings and errors of mpv to the app log, without URL queries. Runtime directory for the media cache. | | `libmpv.go`, `libmpv_unix.go`, `libmpv_windows.go` | Binding to libmpv through purego, with no cgo. The library loads on the first Launch. Library search order for each system. | | `commands.go` | tea.Cmd factories: TickCmd (500ms poll), LaunchCmd, SetPauseCmd, SetSpeedCmd, SetVolumeCmd, QuitCmd. WatchEvents returns PlayerEndMsg at the end of a playback. | | `model.go` | Player sub-model: Position/Duration/Speed/Volume state, key handling (play/pause, seek, speed, volume) | @@ -144,7 +144,7 @@ Styles from theme config. Format helpers (timestamps, durations). Components: He - `ctrl+p` opens the global command palette from any screen - Duration from ABS takes precedence over mpv-reported duration (streaming content reports 0) - 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 test moves the clock with `Model.now`. +- 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. ## Testing diff --git a/internal/app/model_test.go b/internal/app/model_test.go index 8c17241..f7c2783 100644 --- a/internal/app/model_test.go +++ b/internal/app/model_test.go @@ -33,6 +33,7 @@ type mockPlayer struct { position float64 duration float64 paused bool + idle bool speed float64 launchErr error } @@ -60,6 +61,11 @@ func (p *mockPlayer) GetPaused() (bool, error) { defer p.mu.Unlock() return p.paused, nil } +func (p *mockPlayer) GetIdle() (bool, error) { + p.mu.Lock() + defer p.mu.Unlock() + return p.idle, nil +} func (p *mockPlayer) SetPause(paused bool) error { p.mu.Lock() defer p.mu.Unlock() diff --git a/internal/app/playback.go b/internal/app/playback.go index 7059aa6..88653bc 100644 --- a/internal/app/playback.go +++ b/internal/app/playback.go @@ -75,11 +75,13 @@ func (m Model) clock() time.Time { } // trackListeningTime adds the time since the last position tick to the -// listening time, if the player played at that tick and plays now. The -// listening time is the time of the clock. It is not the change of the -// position: at the speed 2.0, the position moves two seconds in each second. -func (m *Model) trackListeningTime(paused bool) { - if paused { +// listening time, if the player played at that tick and plays now. A player +// that is paused or idle does not play: mpv reports no pause while it waits +// for data. The listening time is the time of the clock. It is not the change +// of the position: at the speed 2.0, the position moves two seconds in each +// second. +func (m *Model) trackListeningTime(playing bool) { + if !playing { m.lastPlayingTick = time.Time{} return } @@ -408,7 +410,7 @@ func (m Model) handlePositionMsg(msg player.PositionMsg) (Model, tea.Cmd) { } m.propertyUnavailableCount = 0 - m.trackListeningTime(msg.Paused) + m.trackListeningTime(!msg.Paused && !msg.Idle) // Convert track-relative position to book-global bookPos := msg.Position + m.trackStartOffset @@ -1158,8 +1160,9 @@ func tickCmd(p player.Player, generation uint64, interval time.Duration) tea.Cmd pos, posErr := p.GetPosition() dur, durErr := p.GetDuration() paused, pauseErr := p.GetPaused() + idle, idleErr := p.GetIdle() - for _, err := range []error{posErr, durErr, pauseErr} { + for _, err := range []error{posErr, durErr, pauseErr, idleErr} { if err != nil { return player.PositionMsg{Err: err, Generation: generation} } @@ -1169,6 +1172,7 @@ func tickCmd(p player.Player, generation uint64, interval time.Duration) tea.Cmd Position: pos, Duration: dur, Paused: paused, + Idle: idle, Generation: generation, } }) diff --git a/internal/app/progress_sync_test.go b/internal/app/progress_sync_test.go index af963ed..8e1efac 100644 --- a/internal/app/progress_sync_test.go +++ b/internal/app/progress_sync_test.go @@ -720,11 +720,11 @@ func TestQuitSendsLastPositionOnce(t *testing.T) { // speed 2.0, the position moves two seconds in each second. The server adds // the timeListened of each sync to the listening time of the user. func TestListeningTimeIsTimeOfPlay(t *testing.T) { - // pausedTick moves the clock by seconds, and then sends a position tick - // of a paused player. - pausedTick := func(m Model, seconds float64) Model { + // idleTick moves the clock by seconds, and then sends a position tick of + // a player that does not play: it is paused, or it waits for data. + idleTick := func(m Model, seconds float64, paused bool) Model { m = advanceClock(m, seconds) - result, _ := m.Update(player.PositionMsg{Position: m.player.Position, Duration: m.player.Duration, Paused: true}) + result, _ := m.Update(player.PositionMsg{Position: m.player.Position, Duration: m.player.Duration, Paused: paused, Idle: true}) return result.(Model) } tests := []struct { @@ -747,13 +747,25 @@ func TestListeningTimeIsTimeOfPlay(t *testing.T) { play: func(m Model) Model { m = listen(m, 110) for i := 0; i < 120; i++ { - m = pausedTick(m, 0.5) + m = idleTick(m, 0.5, true) } // The tick after the pause starts the count again. return listen(playTick(m, 0.5, 110), 120) }, want: map[string]interface{}{"currentTime": 120.0, "timeListened": 20.0}, }, + { + name: "one minute of a player that waits for data", + play: func(m Model) Model { + m = listen(m, 110) + for i := 0; i < 120; i++ { + m = idleTick(m, 0.5, false) + } + // The tick after the wait starts the count again. + return listen(playTick(m, 0.5, 110), 120) + }, + want: map[string]interface{}{"currentTime": 120.0, "timeListened": 20.0}, + }, { name: "a tick after a suspend of the machine", play: func(m Model) Model { @@ -782,3 +794,34 @@ func TestListeningTimeIsTimeOfPlay(t *testing.T) { }) } } + +// TestStalledPlayerAddsNoListeningTime verifies that the time during which +// the player waits for data is not listening time. mpv reports no pause while +// it fills its cache: its position does not move, and it is idle. +func TestStalledPlayerAddsNoListeningTime(t *testing.T) { + mp := &mockPlayer{position: 100, duration: 3600, idle: true} + ticks := map[string]func(generation uint64) tea.Cmd{ + "position tick": func(generation uint64) tea.Cmd { + return player.TickCmd(mp, generation) + }, + "position tick after a property that was not ready": func(generation uint64) tea.Cmd { + return tickCmd(mp, generation, time.Millisecond) + }, + } + for name, tick := range ticks { + t.Run(name, func(t *testing.T) { + _, client := newFakeABS(t, nil, nil) + m := advanceClock(newSyncTestModel(client), 1) + + result, _ := m.Update(tick(m.playGeneration)()) + m = result.(Model) + + if m.timeListened != 0 { + t.Errorf("timeListened = %v after one second of a player that waits for data, want 0", m.timeListened) + } + if !m.player.Playing { + t.Error("the model shows a pause for a player that waits for data") + } + }) + } +} diff --git a/internal/player/commands.go b/internal/player/commands.go index 2ad796e..e8247d9 100644 --- a/internal/player/commands.go +++ b/internal/player/commands.go @@ -10,9 +10,12 @@ import ( // PositionMsg carries the current playback position polled from mpv. type PositionMsg struct { - Position float64 - Duration float64 - Paused bool + Position float64 + Duration float64 + Paused bool + // Idle is true if no audio plays now. A player that waits for data is + // idle and not paused. + Idle bool Err error Generation uint64 // ties this tick to a specific play session } @@ -34,9 +37,10 @@ func TickCmd(p Player, generation uint64) tea.Cmd { pos, posErr := p.GetPosition() dur, durErr := p.GetDuration() paused, pauseErr := p.GetPaused() + idle, idleErr := p.GetIdle() // Return first error encountered - for _, err := range []error{posErr, durErr, pauseErr} { + for _, err := range []error{posErr, durErr, pauseErr, idleErr} { if err != nil { return PositionMsg{Err: err, Generation: generation} } @@ -46,6 +50,7 @@ func TickCmd(p Player, generation uint64) tea.Cmd { Position: pos, Duration: dur, Paused: paused, + Idle: idle, Generation: generation, } }) diff --git a/internal/player/commands_test.go b/internal/player/commands_test.go index 7dc84f2..d4fe98d 100644 --- a/internal/player/commands_test.go +++ b/internal/player/commands_test.go @@ -84,6 +84,7 @@ func (launchFailPlayer) Launch(url, startTime string, paused bool, httpHeaders [ func (launchFailPlayer) GetPosition() (float64, error) { return 0, nil } func (launchFailPlayer) GetDuration() (float64, error) { return 0, nil } func (launchFailPlayer) GetPaused() (bool, error) { return false, nil } +func (launchFailPlayer) GetIdle() (bool, error) { return false, nil } func (launchFailPlayer) SetPause(paused bool) error { return nil } func (launchFailPlayer) Seek(seconds float64) error { return nil } func (launchFailPlayer) SeekRelative(seconds float64) error { return nil } diff --git a/internal/player/mpv.go b/internal/player/mpv.go index 4427db6..2785501 100644 --- a/internal/player/mpv.go +++ b/internal/player/mpv.go @@ -77,6 +77,7 @@ type Player interface { GetPosition() (float64, error) GetDuration() (float64, error) GetPaused() (bool, error) + GetIdle() (bool, error) SetPause(paused bool) error Seek(seconds float64) error SeekRelative(seconds float64) error @@ -329,15 +330,13 @@ func (m *Mpv) GetDuration() (float64, error) { // GetPaused returns whether playback is paused. func (m *Mpv) GetPaused() (bool, error) { - var paused bool - err := m.withHandle("get pause", func(h mpvHandle) (err error) { - paused, err = h.getFlag("pause") - return err - }) - if err != nil { - logger.Debug("failed to query mpv property", "property", "pause", "err", err) - } - return paused, err + return m.getFlag("pause") +} + +// GetIdle returns whether no audio plays now. It is true for a paused player, +// and for a player that is not paused but waits for data, starts, or seeks. +func (m *Mpv) GetIdle() (bool, error) { + return m.getFlag("core-idle") } // SetPause pauses or resumes playback. @@ -434,6 +433,18 @@ func describeStreamURL(rawURL string) streamLogInfo { } } +func (m *Mpv) getFlag(property string) (bool, error) { + var value bool + err := m.withHandle("get "+property, func(h mpvHandle) (err error) { + value, err = h.getFlag(property) + return err + }) + if err != nil { + logger.Debug("failed to query mpv property", "property", property, "err", err) + } + return value, err +} + func (m *Mpv) getFloat(property string) (float64, error) { var value float64 err := m.withHandle("get "+property, func(h mpvHandle) (err error) { diff --git a/internal/player/mpv_test.go b/internal/player/mpv_test.go index 4e4d8d4..c417f51 100644 --- a/internal/player/mpv_test.go +++ b/internal/player/mpv_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "runtime" + "strconv" "sync" "testing" "time" @@ -318,6 +319,67 @@ func TestMpvControls(t *testing.T) { } } +// TestMpvIsIdleWhileItWaitsForData verifies that GetIdle separates a player +// that plays from a player that waits for data. The two are not paused. +func TestMpvIsIdleWhileItWaitsForData(t *testing.T) { + // state returns an error until the player has a position, is not paused, + // and has the idle state want. + state := func(m *Mpv, want bool) (position float64, err error) { + if position, err = m.GetPosition(); err != nil { + return 0, err + } + paused, err := m.GetPaused() + if err != nil { + return 0, err + } + idle, err := m.GetIdle() + if err != nil { + return 0, err + } + if paused || idle != want { + return 0, fmt.Errorf("paused = %v, idle = %v at the position %.1f", paused, idle, position) + } + return position, nil + } + + srv := newAudioServer(t) + m := newTestMpv(t) + if err := m.Launch(srv.URL+"/long.wav", "0", false, []string{testAuthHeader}, 100); err != nil { + t.Fatalf("Launch() error: %v", err) + } + eventually(t, "a player that plays is not idle", func() error { + _, err := state(m, false) + return err + }) + + // The server sends the first 100 seconds of the file, and then nothing. + const sent = 100 + wav := silentWAV(120) + release := make(chan struct{}) + stalled := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Length", strconv.Itoa(len(wav))) + _, _ = w.Write(wav[:44+8000*sent]) + w.(http.Flusher).Flush() + <-release + })) + t.Cleanup(stalled.Close) + t.Cleanup(func() { close(release) }) + if err := m.Launch(stalled.URL+"/stalled.wav", "0", false, nil, 100); err != nil { + t.Fatalf("Launch() error: %v", err) + } + // At this speed, the player needs one second for the data that it has. + if err := m.SetSpeed(100); err != nil { + t.Fatalf("SetSpeed() error: %v", err) + } + eventually(t, "a player that waits for data is idle", func() error { + position, err := state(m, true) + if err == nil && position < sent/2 { + return fmt.Errorf("idle at the position %.1f, before the end of the data", position) + } + return err + }) +} + func TestMpvEndOfFileGivesEOFAndClosesThePlayer(t *testing.T) { srv := newAudioServer(t) m := newTestMpv(t) From 19629b1939960c5e25058ab3352a7b1ea8674541 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 21:52:01 +0200 Subject: [PATCH 04/15] fix(main): send the last position when the terminal closes - The close of the terminal sends SIGHUP to the process. Go ends a process at SIGHUP if no code asked for the signal, and Bubble Tea asks only for SIGINT and SIGTERM. So the process ended in p.Run, and main did not call Cleanup: the last position and the listening time since the last sync were lost. - main now quits the program at SIGHUP. p.Run then returns the final model, and Cleanup sends the close of the session and the progress update. - SIGTERM needs no change: Bubble Tea quits the program at SIGTERM, and main calls Cleanup. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 ++ main.go | 16 ++++++++++ main_unix_test.go | 81 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+) create mode 100644 main_unix_test.go diff --git a/AGENTS.md b/AGENTS.md index 9ead3d5..dce7ce2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -141,6 +141,7 @@ Styles from theme config. Format helpers (timestamps, durations). Components: He - Cache-first data loading through `internal/cache.Client`: home and library views hit the persistent cache first; misses trigger a background rebuild - `q` always quits, `esc`/left goes back (never quits). `h`/`l` seek only, arrows navigate only - A quit (`q` or ctrl+c) returns only `tea.Quit` and keeps the session in the model. `Cleanup` sends the last position, and `main` waits for it. Bubble Tea does not wait for a command at a quit, so a quit must not send the position in a command. +- `main` quits the program at SIGHUP, which is the close of the terminal (`quitOnHangup`), so that `Cleanup` runs. Bubble Tea quits the program at SIGTERM. - `ctrl+p` opens the global command palette from any screen - Duration from ABS takes precedence over mpv-reported duration (streaming content reports 0) - 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. @@ -153,6 +154,7 @@ Styles from theme config. Format helpers (timestamps, durations). Components: He - E2E tests in `internal/app/e2e_test.go` + `e2e_helpers_test.go` use mock HTTP server + mock player - E2E tests drive the bubbletea Update loop programmatically (no real TUI) - `internal/app/model_test.go`, `playback_test.go`, `bookmarks_test.go`, `render_test.go` for app-level tests +- `main_unix_test.go` starts the test program again as a child process, sends SIGHUP to it, and checks that it gets to the place where `main` calls `Cleanup`. It does not run on Windows. - `internal/cache/*_test.go` for cache store and helper behavior - `internal/mpris/mpris_test.go` for the D-Bus adapter and bridge - `internal/mpris/server_linux_test.go` starts the MPRIS server on a private session bus and checks it as an MPRIS client. It skips if `dbus-daemon` is absent. With `PINE_REQUIRE_DBUS=1` it fails instead. CI and `scripts/release.sh` set that variable. diff --git a/main.go b/main.go index 75b28d3..d6b9a26 100644 --- a/main.go +++ b/main.go @@ -4,7 +4,9 @@ import ( "flag" "fmt" "os" + "os/signal" "path/filepath" + "syscall" "github.com/Thelost77/pine/internal/abs" "github.com/Thelost77/pine/internal/app" @@ -74,6 +76,7 @@ func main() { model := app.New(cfg, store, cachedClient, cacheStore) p := tea.NewProgram(model, tea.WithAltScreen()) model.SetProgram(p) + quitOnHangup(p) logger.Info("starting TUI") finalModel, err := p.Run() if err != nil { @@ -86,3 +89,16 @@ func main() { } logger.Info("session ended") } + +// quitOnHangup quits the program when the process gets SIGHUP, which is the +// close of its terminal. Without it, the process ends at the signal, and main +// does not call Cleanup. Bubble Tea quits the program at SIGTERM. +func quitOnHangup(p *tea.Program) { + hangup := make(chan os.Signal, 1) + signal.Notify(hangup, syscall.SIGHUP) + go func() { + <-hangup + logger.Info("terminal closed, quitting") + p.Quit() + }() +} diff --git a/main_unix_test.go b/main_unix_test.go new file mode 100644 index 0000000..925da53 --- /dev/null +++ b/main_unix_test.go @@ -0,0 +1,81 @@ +//go:build unix + +package main + +import ( + "bufio" + "fmt" + "io" + "os" + "os/exec" + "syscall" + "testing" + "time" + + tea "github.com/charmbracelet/bubbletea" +) + +// hangupChildEnv marks the process that TestHangupRunsCleanup starts. +const hangupChildEnv = "PINE_TEST_HANGUP_CHILD" + +// waitModel is a program that waits. Its Init prints a line when the program +// runs. +type waitModel struct{} + +func (waitModel) Init() tea.Cmd { + return func() tea.Msg { + fmt.Println("running") + return nil + } +} + +func (m waitModel) Update(tea.Msg) (tea.Model, tea.Cmd) { return m, nil } + +func (waitModel) View() string { return "" } + +// runHangupChild runs a program as main does, and prints a line at the place +// where main calls Cleanup. +func runHangupChild() { + p := tea.NewProgram(waitModel{}, tea.WithInput(nil), tea.WithOutput(io.Discard)) + quitOnHangup(p) + _, _ = p.Run() + fmt.Println("cleanup") + os.Exit(0) +} + +// TestHangupRunsCleanup verifies that the process gets to the place after the +// program when it gets SIGHUP, which is the close of the terminal. main calls +// Cleanup there, which sends the last position and the listening time. +func TestHangupRunsCleanup(t *testing.T) { + if os.Getenv(hangupChildEnv) == "1" { + runHangupChild() + } + child := exec.Command(os.Args[0], "-test.run=^TestHangupRunsCleanup$") + child.Env = append(os.Environ(), hangupChildEnv+"=1") + stdout, err := child.StdoutPipe() + if err != nil { + t.Fatal(err) + } + if err := child.Start(); err != nil { + t.Fatal(err) + } + // A child that does not end fails the test, and does not block it. + kill := time.AfterFunc(10*time.Second, func() { _ = child.Process.Kill() }) + defer kill.Stop() + + lines := bufio.NewScanner(stdout) + if !lines.Scan() || lines.Text() != "running" { + t.Fatalf("first line of the child = %q, want running", lines.Text()) + } + if err := child.Process.Signal(syscall.SIGHUP); err != nil { + t.Fatal(err) + } + var after []string + for lines.Scan() { + after = append(after, lines.Text()) + } + err = child.Wait() + if err != nil || len(after) != 1 || after[0] != "cleanup" { + t.Errorf("after SIGHUP, the process printed %q and ended with the error %v, want the line cleanup and no error", after, err) + } +} From 2aa3a4fccfe1ef008d4d4cb0e5b1bc14dd382810 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 22:50:17 +0200 Subject: [PATCH 05/15] feat(app): remove the n and N chapter keys - The keys n and N went to the next and the previous chapter during a playback. The chapter list (C) and the command palette do that job. - The config keys next_chapter and prev_chapter are gone. A config file that has them still loads: the two keys have no effect. - The command palette keeps "Next Chapter" and "Previous Chapter". The chapter tests use these actions, and one test checks that the palette offers them. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- README.md | 3 --- internal/app/e2e_test.go | 32 +++++++++++++++++++++----------- internal/app/help.go | 1 - internal/app/keymap.go | 10 ---------- internal/app/model.go | 8 -------- internal/config/config.go | 4 ---- 7 files changed, 22 insertions(+), 38 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index dce7ce2..5837e36 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -60,7 +60,7 @@ The Architecture Map below describes pine and the shared packages. | `bookmarks.go` | Bookmark CRUD, chapter next/prev, seek logic (in-track vs cross-track), fetchBookmarksCmd/fetchEpisodesCmd/fetchBookDetailCmd | | `metadata.go` | Metadata editor save flow: server PATCH, cache eviction, detail refresh | | `messages.go` | Screen enum, PlaySessionMsg, PlaySessionData, QueueEntry, all root-level message types | -| `keymap.go` | Global keybindings (quit, back, help, chapter overlay, queue, chapters, sleep, command palette). Key names for the player model. | +| `keymap.go` | Global keybindings (quit, back, help, chapter overlay, queue, sleep, command palette). Key names for the player model. | | `help.go` | Help overlay groups for pine | | `queue.go` | Queue entry enqueue/dequeue with dedup, clone helpers | | `mpris_state.go` | MPRIS bridge message conversion between player state and D-Bus adapter | diff --git a/README.md b/README.md index 3d91f35..f71ee70 100644 --- a/README.md +++ b/README.md @@ -91,7 +91,6 @@ Some keys are context-specific. | `space` / `p` | Play / pause | | `a` / `A` | Add to queue / play next | | `>` | Play next queued item | -| `n` / `N` | Next / previous chapter | | `+` / `-` | Speed up / down | | `]` / `[` | Volume up / down | | `c` | Toggle captions during playback | @@ -127,8 +126,6 @@ speed_up = "+" speed_down = "-" volume_up = "]" volume_down = "[" -next_chapter = "n" -prev_chapter = "N" sleep_timer = "S" [theme] diff --git a/internal/app/e2e_test.go b/internal/app/e2e_test.go index 6456fc4..9bb3a0a 100644 --- a/internal/app/e2e_test.go +++ b/internal/app/e2e_test.go @@ -1477,17 +1477,27 @@ func TestE2E_ChapterNavigation(t *testing.T) { t.Fatalf("expected 2 chapters from play session, got %d", len(m.chapters)) } + // The command palette offers the two chapter actions. + offered := map[components.PaletteAction]bool{} + playerItems, _ := m.buildStaticPaletteItems() + for _, item := range playerItems { + offered[item.Action] = true + } + if !offered[components.ActionNextChapter] || !offered[components.ActionPrevChapter] { + t.Fatalf("palette items = %+v, want Next Chapter and Previous Chapter", playerItems) + } + // Position is 42 (in chapter 1: 0-1800) - // Press 'n' → next chapter start (1800) - m, _ = e2ePressKey(m, 'n') + // Palette action "Next Chapter" → next chapter start (1800) + m, _ = m.handlePaletteAction(components.ActionNextChapter, "", "", "", nil) if m.player.Position != 1800.0 { - t.Errorf("after 'n', position = %f, want 1800.0", m.player.Position) + t.Errorf("after Next Chapter, position = %f, want 1800.0", m.player.Position) } - // Press 'N' → previous chapter start (0) - m, _ = e2ePressKey(m, 'N') + // Palette action "Previous Chapter" → previous chapter start (0) + m, _ = m.handlePaletteAction(components.ActionPrevChapter, "", "", "", nil) if m.player.Position != 0.0 { - t.Errorf("after 'N', position = %f, want 0.0", m.player.Position) + t.Errorf("after Previous Chapter, position = %f, want 0.0", m.player.Position) } } @@ -1733,14 +1743,14 @@ func TestE2E_WithinTrackSeek(t *testing.T) { t.Errorf("expected no new API calls for within-track seek, got %d new calls", apiCallsAfter-apiCallsBefore) } - // Press 'n' (next chapter) — chapter 2 starts at 900, within same track - m, cmd = e2ePressKey(m, 'n') + // Palette action "Next Chapter" — chapter 2 starts at 900, within same track + m, cmd = m.handlePaletteAction(components.ActionNextChapter, "", "", "", nil) if cmd != nil { m = feedCmdChain(m, cmd, 3) } if m.player.Position != 900.0 { - t.Errorf("position after 'n' = %f, want 900.0", m.player.Position) + t.Errorf("position after Next Chapter = %f, want 900.0", m.player.Position) } mp.mu.Lock() @@ -1825,9 +1835,9 @@ func TestE2E_CrossTrackChapterSeek(t *testing.T) { {ID: 1, Start: 1800, End: 3600, Title: "Chapter 2"}, } - // Press 'n' → chapter 2 at position 2000 is beyond current track (0-1800) + // Palette action "Next Chapter" → chapter 2 at position 2000 is beyond current track (0-1800) // This should trigger a cross-track restart - m, cmd := e2ePressKey(m, 'n') + m, cmd := m.handlePaletteAction(components.ActionNextChapter, "", "", "", nil) // Feed the cross-track restart command chain m = feedCmdChain(m, cmd, 10) diff --git a/internal/app/help.go b/internal/app/help.go index b80f067..f1feccc 100644 --- a/internal/app/help.go +++ b/internal/app/help.go @@ -49,7 +49,6 @@ func helpGroups() []components.HelpGroup { {Key: "] / [", Desc: "volume up / down"}, {Key: "C", Desc: "open chapters"}, {Key: "c", Desc: "toggle captions"}, - {Key: "n / N", Desc: "next / prev chapter"}, {Key: "S", Desc: "sleep timer"}, }, }, diff --git a/internal/app/keymap.go b/internal/app/keymap.go index 2959c4a..25510a7 100644 --- a/internal/app/keymap.go +++ b/internal/app/keymap.go @@ -14,8 +14,6 @@ type KeyMap struct { ChapterOverlay key.Binding Captions key.Binding NextInQueue key.Binding - NextChapter key.Binding - PrevChapter key.Binding SleepTimer key.Binding GlobalPalette key.Binding } @@ -47,14 +45,6 @@ func DefaultKeyMap(cfg config.KeybindsConfig) KeyMap { key.WithKeys(cfg.NextInQueue), key.WithHelp(">", "next queued"), ), - NextChapter: key.NewBinding( - key.WithKeys(cfg.NextChapter), - key.WithHelp("n", "next chapter"), - ), - PrevChapter: key.NewBinding( - key.WithKeys(cfg.PrevChapter), - key.WithHelp("N", "prev chapter"), - ), SleepTimer: key.NewBinding( key.WithKeys(cfg.SleepTimer), key.WithHelp(cfg.SleepTimer, "sleep timer"), diff --git a/internal/app/model.go b/internal/app/model.go index 7352cbf..fee0c56 100644 --- a/internal/app/model.go +++ b/internal/app/model.go @@ -851,14 +851,6 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if key.Matches(msg, m.keys.NextInQueue) { return m.skipToNextQueued() } - if len(m.chapters) > 0 { - if key.Matches(msg, m.keys.NextChapter) { - return m.seekToChapter(m.nextChapter()) - } - if key.Matches(msg, m.keys.PrevChapter) { - return m.seekToChapter(m.prevChapter()) - } - } if key.Matches(msg, m.keys.SleepTimer) { return m.cycleSleepTimer() } diff --git a/internal/config/config.go b/internal/config/config.go index c0ed653..2ef6f1d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -30,8 +30,6 @@ type KeybindsConfig struct { SpeedDown string `toml:"speed_down"` VolumeUp string `toml:"volume_up"` VolumeDown string `toml:"volume_down"` - NextChapter string `toml:"next_chapter"` - PrevChapter string `toml:"prev_chapter"` SleepTimer string `toml:"sleep_timer"` Back string `toml:"back"` } @@ -61,8 +59,6 @@ func Default() Config { SpeedDown: "-", VolumeUp: "]", VolumeDown: "[", - NextChapter: "n", - PrevChapter: "N", SleepTimer: "S", Back: "esc", }, From b94a4a72ffbc8171260c585de7aa9ec22a072c1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 22:53:05 +0200 Subject: [PATCH 06/15] fix(app): give each key to a text input or a dialog - The root model gave some keys a global function or a playback function before the screen got them. A guard held back these keys only while a list filter was active. So the bookmark title editor, the login screen, the metadata editor, and the confirmation dialog of the detail screen lost keys: q quit the program, ? opened the help overlay, a typed C had no effect, esc in the bookmark title editor went to the previous screen, and the playback keys did their function. - capturesKeys (before: isFiltering) is true in each of these cases. Update then gives the key to the screen, before the global keys, the chapter overlay, the error banner, and the playback keys. Before, the error banner took the first key, and the chapter overlay took j, k, H, L, and enter. - ? closes an open help overlay on each screen. - The hint line shows no global key and no playback key while the screen gets each key. - ctrl+p does not open the command palette while a text input or a dialog gets the keys. ctrl+c quits the program on each screen. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 5 +- internal/app/e2e_test.go | 95 +++++++++++++++++++++++++++++++ internal/app/model.go | 28 +++++---- internal/app/model_test.go | 109 ++++++++++++++++++++++++++++++++++++ internal/app/playback.go | 9 ++- internal/app/render.go | 12 +++- internal/app/render_test.go | 28 +++++++++ 7 files changed, 266 insertions(+), 20 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5837e36..d8ec252 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -139,10 +139,11 @@ Styles from theme config. Format helpers (timestamps, durations). Components: He - `tea.Batch` for concurrent async operations - Skeleton loading with generation-gated reveal delays (home: 500ms, library: 150ms) - Cache-first data loading through `internal/cache.Client`: home and library views hit the persistent cache first; misses trigger a background rebuild -- `q` always quits, `esc`/left goes back (never quits). `h`/`l` seek only, arrows navigate only +- `q` quits, and `esc`/left goes back (never quits). `h`/`l` seek only, arrows navigate only. These keys have no global function while a text input or a dialog gets the keys (`capturesKeys`). - A quit (`q` or ctrl+c) returns only `tea.Quit` and keeps the session in the model. `Cleanup` sends the last position, and `main` waits for it. Bubble Tea does not wait for a command at a quit, so a quit must not send the position in a command. - `main` quits the program at SIGHUP, which is the close of the terminal (`quitOnHangup`), so that `Cleanup` runs. Bubble Tea quits the program at SIGTERM. -- `ctrl+p` opens the global command palette from any screen +- `ctrl+p` opens the global command palette from each screen, but not while a text input or a dialog gets the keys +- A text input or a confirmation dialog of a screen gets each key, except ctrl+c. `capturesKeys` in `playback.go` is true for the login screen, the metadata editor, a list filter, and the bookmark title editor. It is also true for the confirmation dialog of the detail screen. `Update` in `model.go` then gives the key to the screen, before the global keys, the chapter overlay, the error banner, and the playback keys. The help overlay and the command palette get the keys first while they are open. If you add a text input or a dialog to a screen, add it to `capturesKeys`. - Duration from ABS takes precedence over mpv-reported duration (streaming content reports 0) - 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`. diff --git a/internal/app/e2e_test.go b/internal/app/e2e_test.go index 9bb3a0a..b1b8c98 100644 --- a/internal/app/e2e_test.go +++ b/internal/app/e2e_test.go @@ -620,6 +620,101 @@ func TestE2E_BookmarkTitleEdit(t *testing.T) { } } +// e2eDetailWithBookmark returns a model at the detail screen of an item that +// plays, has chapters, and has one bookmark with the title "Old". The bookmark +// list has the focus. +func e2eDetailWithBookmark(t *testing.T) (Model, *e2eServerState) { + t.Helper() + state := &e2eServerState{bookmarks: make(map[string][]abs.Bookmark)} + state.bookmarks["item-001"] = []abs.Bookmark{ + testBookmark("Old", 300.0), + } + srv := newFullMockABSServer(&apiLog{}, state) + t.Cleanup(srv.Close) + + m := newE2EModelAuthenticated(srv, &mockPlayer{position: 100, duration: 3600}) + m = e2eSetSize(m, 120, 40) + res, cmd := m.Update(home.NavigateDetailMsg{Item: testLibraryItem("item-001", "The Great Gatsby")}) + m = feedCmdChain(res.(Model), cmd, 5) + + m.sessionID = "sess-e2e" + m.itemID = "item-001" + m.player.Playing = true + m.player.Position = 100.0 + m.player.Duration = 3600.0 + m.chapters = []abs.Chapter{ + {ID: 0, Start: 0, End: 1800, Title: "Chapter 1"}, + {ID: 1, Start: 1800, End: 3600, Title: "Chapter 2"}, + } + m, _ = e2ePressSpecial(m, tea.KeyTab) + return m, state +} + +// e2eSaveBookmarkTitle presses enter in the bookmark title editor, and returns +// the title that the server has. +func e2eSaveBookmarkTitle(m Model, state *e2eServerState) string { + m, cmd := e2ePressSpecial(m, tea.KeyEnter) + m, cmd = feedCmd(m, cmd) + _ = feedCmdChain(m, cmd, 3) + return state.getBookmarks("item-001")[0].Title +} + +// TestE2E_BookmarkTitleEditGetsEachKey verifies that the bookmark title editor +// gets each key during a playback. Outside the editor, these keys have a +// global function or a playback function. +func TestE2E_BookmarkTitleEditGetsEachKey(t *testing.T) { + m, state := e2eDetailWithBookmark(t) + + // esc cancels the edit. It does not go back to the previous screen. + m, _ = e2ePressKey(m, 'e') + m, _ = e2ePressSpecial(m, tea.KeyEsc) + assertScreen(t, m, ScreenDetail) + if m.detail.EditingBookmark() { + t.Fatal("after esc, the bookmark editor is open") + } + + m, _ = e2ePressKey(m, 'e') + const typed = "nNqC?lhpS>+-[] c" + m = e2eTypeString(m, typed) + if !m.detail.EditingBookmark() { + t.Fatalf("after the keys %q, the bookmark editor is not open", typed) + } + if m.player.Position != 100.0 || !m.player.Playing { + t.Errorf("after the keys %q, position = %v and playing = %v, want 100 and true", typed, m.player.Position, m.player.Playing) + } + if got := e2eSaveBookmarkTitle(m, state); got != "Old"+typed { + t.Fatalf("server bookmark title = %q, want %q", got, "Old"+typed) + } +} + +// TestE2E_BookmarkTitleEditGetsKeysOfBannerAndChapterList verifies that the +// bookmark title editor gets each key while the error banner shows, and while +// the chapter list is open. Outside the editor, a key closes the banner, and +// the chapter list has the keys j, k, and enter. +func TestE2E_BookmarkTitleEditGetsKeysOfBannerAndChapterList(t *testing.T) { + tests := map[string]func(m Model) Model{ + "error banner": func(m Model) Model { + m, _ = e2ePressKey(m, 'e') + res, _ := m.Update(components.ErrMsg{Err: fmt.Errorf("failure")}) + return res.(Model) + }, + "chapter list": func(m Model) Model { + m, _ = e2ePressKey(m, 'C') + m, _ = e2ePressKey(m, 'e') + return m + }, + } + for name, openEditor := range tests { + t.Run(name, func(t *testing.T) { + m, state := e2eDetailWithBookmark(t) + m = e2eTypeString(openEditor(m), "jk") + if got := e2eSaveBookmarkTitle(m, state); got != "Oldjk" { + t.Fatalf("server bookmark title = %q, want Oldjk", got) + } + }) + } +} + // --------------------------------------------------------------------------- // E2E: Bookmarks fetched on detail entry // --------------------------------------------------------------------------- diff --git a/internal/app/model.go b/internal/app/model.go index fee0c56..ae7e22f 100644 --- a/internal/app/model.go +++ b/internal/app/model.go @@ -752,7 +752,7 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.Type == tea.KeyCtrlC { return m, tea.Quit } - if !m.isFiltering() && key.Matches(msg, m.keys.Help) { + if (m.help.Visible() || !m.capturesKeys()) && key.Matches(msg, m.keys.Help) { m.help.Toggle() return m, nil } @@ -776,7 +776,8 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } return m.updateScreen(msg) } - if m.screen == ScreenMetadataEdit { + // A text input or a confirmation dialog of the screen gets each key. + if m.capturesKeys() { return m.updateScreen(msg) } if m.chapterOverlayVisible { @@ -815,39 +816,36 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.propagateSize() return m, nil } - if !m.isFiltering() && key.Matches(msg, m.keys.GlobalPalette) { + if key.Matches(msg, m.keys.GlobalPalette) { m.openGlobalPalette() return m, nil } - if !m.isFiltering() && key.Matches(msg, m.keys.ChapterOverlay) { + if key.Matches(msg, m.keys.ChapterOverlay) { if m.canOpenChapterOverlay() { m.openChapterOverlay() } return m, nil } - if !m.isFiltering() && m.isPlaying() && len(m.captionCues) > 0 && key.Matches(msg, m.keys.Captions) { + if m.isPlaying() && len(m.captionCues) > 0 && key.Matches(msg, m.keys.Captions) { m.toggleCaptions() return m, nil } - // Global quit: q always quits the app. The session stays in the model: + // Global quit: q quits the app. The session stays in the model: // main calls Cleanup on the final model, and Cleanup waits until the // server has the last position. The program does not wait for a // command, so the request of a command can be lost. - if !m.isFiltering() && m.screen != ScreenLogin && key.Matches(msg, m.keys.Quit) { + if key.Matches(msg, m.keys.Quit) { return m, tea.Quit } // Global back: esc/left goes back but never quits. - if m.screen != ScreenLogin && !m.isFiltering() && !m.hasActiveFilter() { - isConfirmVisible := m.screen == ScreenDetail && m.detail.ConfirmOverlayVisible() - if !isConfirmVisible && key.Matches(msg, m.keys.Back) { - if len(m.backStack) > 0 { - return m.back() - } - return m, nil + if !m.hasActiveFilter() && key.Matches(msg, m.keys.Back) { + if len(m.backStack) > 0 { + return m.back() } + return m, nil } // When playing, playback keys take priority over screen keys. - if !m.isFiltering() && m.isPlaying() { + if m.isPlaying() { if key.Matches(msg, m.keys.NextInQueue) { return m.skipToNextQueued() } diff --git a/internal/app/model_test.go b/internal/app/model_test.go index f7c2783..9c14e91 100644 --- a/internal/app/model_test.go +++ b/internal/app/model_test.go @@ -453,6 +453,115 @@ func TestMetadataEditorCapturesPlaybackShortcutKeys(t *testing.T) { } } +// TestMetadataEditorCapturesHelpKey verifies that the metadata editor gets the +// key of the help overlay. +func TestMetadataEditorCapturesHelpKey(t *testing.T) { + m := newTestModelAuthenticated() + m.screen = ScreenMetadataEdit + m.metadataEdit = metadataedit.New(m.styles, abs.LibraryItem{ + ID: "item-1", + MediaType: "book", + Media: abs.Media{Metadata: abs.MediaMetadata{ + Title: "Old Title", + }}, + }) + + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'?'}}) + m = result.(Model) + if m.help.Visible() { + t.Fatal("after ?, the help overlay is open") + } + + _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEnter}) + if cmd == nil { + t.Fatal("cmd = nil, want SaveCmd") + } + save, ok := cmd().(metadataedit.SaveCmd) + if !ok || save.Request.Metadata.Title == nil || *save.Request.Metadata.Title != "Old Title?" { + t.Fatalf("save = %+v, want a SaveCmd with the title %q", save, "Old Title?") + } +} + +// TestLoginCapturesGlobalKeys verifies that a field of the login screen gets +// the keys that have a global function on the other screens. +func TestLoginCapturesGlobalKeys(t *testing.T) { + m := newTestModel() + + const typed = "?Cq" + for _, r := range typed { + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{r}}) + m = result.(Model) + } + if m.help.Visible() { + t.Error("after ?, the help overlay is open") + } + if got := m.login.ServerURL(); got != typed { + t.Errorf("server field = %q, want %q", got, typed) + } + + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyCtrlP}) + m = result.(Model) + if m.palette.Visible() { + t.Error("after ctrl+p, the command palette is open") + } +} + +// TestHelpKeyClosesHelpOnLoginScreen verifies that ? closes the help overlay +// on a screen that gets each key. A 401 moves to the login screen while the +// overlay is open. +func TestHelpKeyClosesHelpOnLoginScreen(t *testing.T) { + m := newTestModelAuthenticated() + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'?'}}) + result, _ = result.(Model).Update(components.ErrMsg{Err: errors.New("status 401")}) + m = result.(Model) + if m.ActiveScreen() != ScreenLogin || !m.help.Visible() { + t.Fatalf("ActiveScreen() = %v, help open = %v, want ScreenLogin and true", m.ActiveScreen(), m.help.Visible()) + } + + result, _ = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'?'}}) + if result.(Model).help.Visible() { + t.Fatal("after ?, the help overlay is open") + } +} + +// TestDetailConfirmCapturesQuitAndBackKeys verifies that q and esc cancel the +// confirmation dialog of the detail screen. They do not quit the program, and +// they do not go back to the previous screen. +func TestDetailConfirmCapturesQuitAndBackKeys(t *testing.T) { + keys := map[string]tea.KeyMsg{ + "q": {Type: tea.KeyRunes, Runes: []rune{'q'}}, + "esc": {Type: tea.KeyEsc}, + } + for name, key := range keys { + t.Run(name, func(t *testing.T) { + m := newTestModelAuthenticated() + m.detail = detail.New(m.styles, abs.LibraryItem{ID: "item-1", MediaType: "book"}) + m.screen = ScreenDetail + m.backStack = []Screen{ScreenHome} + + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'D'}}) + m = result.(Model) + if !m.detail.ConfirmOverlayVisible() { + t.Fatal("after D, the confirmation dialog is not open") + } + + result, cmd := m.Update(key) + m = result.(Model) + if cmd != nil { + if _, quit := cmd().(tea.QuitMsg); quit { + t.Fatal("the key quits the program") + } + } + if m.detail.ConfirmOverlayVisible() { + t.Error("after the key, the confirmation dialog is open") + } + if m.ActiveScreen() != ScreenDetail { + t.Errorf("ActiveScreen() = %v, want ScreenDetail", m.ActiveScreen()) + } + }) + } +} + func TestContentSearchContextUsesActiveLibraryScreen(t *testing.T) { m := newTestModelAuthenticated() m.home = home.New(m.styles, m.client) diff --git a/internal/app/playback.go b/internal/app/playback.go index 88653bc..01f44d5 100644 --- a/internal/app/playback.go +++ b/internal/app/playback.go @@ -100,12 +100,19 @@ func (m Model) hasUnsyncedProgress() bool { return m.timeListened >= syncMinChange || math.Abs(m.player.Position-m.lastSyncPos) >= syncMinChange } -func (m Model) isFiltering() bool { +// capturesKeys returns true if the screen gets each key: a text input or a +// confirmation dialog is active. Update then gives the key to the screen, and +// the global keys and the playback keys have no function. +func (m Model) capturesKeys() bool { switch m.screen { + case ScreenLogin, ScreenMetadataEdit: + return true case ScreenHome: return m.home.IsFiltering() case ScreenLibrary: return m.library.IsFiltering() + case ScreenDetail: + return m.detail.EditingBookmark() || m.detail.ConfirmOverlayVisible() case ScreenSeries: return m.series.IsFiltering() case ScreenSeriesList: diff --git a/internal/app/render.go b/internal/app/render.go index 5c7e6d3..14f7750 100644 --- a/internal/app/render.go +++ b/internal/app/render.go @@ -258,6 +258,8 @@ func (m Model) viewHints() string { } var parts []string + // A screen that gets each key shows no global key and no playback key. + capturesKeys := m.capturesKeys() switch m.screen { case ScreenHome: @@ -275,6 +277,10 @@ func (m Model) viewHints() string { } parts = append(parts, key("←/esc", "back")) case ScreenDetail: + // The bookmark title editor and the confirmation dialog show their keys. + if capturesKeys { + break + } parts = append(parts, key("enter/p", "play")) parts = append(parts, key("b", "bookmark")) if _, _, ok := m.detail.MetadataEditTarget(); ok { @@ -299,7 +305,7 @@ func (m Model) viewHints() string { return "" } - if m.isPlaying() { + if m.isPlaying() && !capturesKeys { parts = append(parts, key("space", "pause")) parts = append(parts, key("h/l", "seek")) parts = append(parts, key("-/+", "speed")) @@ -318,7 +324,9 @@ func (m Model) viewHints() string { if len(m.queue) > 0 { parts = append(parts, m.styles.Muted.Render(fmt.Sprintf("%d queued", len(m.queue)))) } - parts = append(parts, key("?", "help")) + if !capturesKeys { + parts = append(parts, key("?", "help")) + } style := m.styles.StatusBar if m.width > 0 { diff --git a/internal/app/render_test.go b/internal/app/render_test.go index 99f7607..dc23da6 100644 --- a/internal/app/render_test.go +++ b/internal/app/render_test.go @@ -6,7 +6,9 @@ import ( "github.com/Thelost77/pine/internal/abs" "github.com/Thelost77/pine/internal/captions" + "github.com/Thelost77/pine/internal/screens/detail" "github.com/Thelost77/pine/internal/screens/library" + "github.com/Thelost77/pine/internal/screens/metadataedit" tea "github.com/charmbracelet/bubbletea" ) @@ -188,6 +190,32 @@ func TestPaletteDoesNotHidePlaybackFooter(t *testing.T) { } } +// TestHintsShowNoKeyThatTheScreenCaptures verifies that the hint line shows no +// global key and no playback key while a text input or a dialog gets the keys. +func TestHintsShowNoKeyThatTheScreenCaptures(t *testing.T) { + m := newTestModelAuthenticated() + m.sessionID = "sess-123" + m.player.Playing = true + m.width = 160 + item := abs.LibraryItem{ID: "item-1", MediaType: "book"} + + m.screen = ScreenMetadataEdit + m.metadataEdit = metadataedit.New(m.styles, item) + hints := m.viewHints() + if !strings.Contains(hints, "save") || strings.Contains(hints, "help") || strings.Contains(hints, "pause") { + t.Errorf("hints of the metadata editor = %q, want the keys of the editor only", hints) + } + + m.screen = ScreenDetail + m.detail = detail.New(m.styles, item) + result, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'D'}}) + m = result.(Model) + hints = m.viewHints() + if strings.TrimSpace(hints) != "" || strings.Count(hints, "\n") != 0 { + t.Errorf("hints with the confirmation dialog open = %q, want one empty line", hints) + } +} + func TestViewMaintainsStrictHeightConstraint(t *testing.T) { m := newTestModelAuthenticated() m.screen = ScreenHome From bdaef5bb1c781631a27b9ed82cf98076aacb7665 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 22:54:12 +0200 Subject: [PATCH 07/15] fix(bookmarks): ignore the bookmarks of an item that is not shown - BookmarksUpdatedMsg had no item ID. The answer for an item can come after the user opened a different item. The detail screen then showed the bookmarks of the first item. enter on such a bookmark moved the second item to a time of the first item. - The message now has the ID of its item. The root model ignores the message if the detail screen shows a different item, as it does for the episodes and for the detail of a book. - The fault had no effect before the read of the bookmark route: the server gave no bookmarks. Co-Authored-By: Claude Opus 5.5 --- internal/app/bookmarks.go | 10 +++++----- internal/app/bookmarks_test.go | 9 +++++++++ internal/app/e2e_test.go | 26 ++++++++++++++++++++++++++ internal/app/model.go | 3 +++ internal/screens/detail/model.go | 5 ++++- 5 files changed, 47 insertions(+), 6 deletions(-) diff --git a/internal/app/bookmarks.go b/internal/app/bookmarks.go index 3383d12..b393412 100644 --- a/internal/app/bookmarks.go +++ b/internal/app/bookmarks.go @@ -46,7 +46,7 @@ func (m Model) handleAddBookmark(msg detail.AddBookmarkCmd) (Model, tea.Cmd) { newBm := abs.Bookmark{LibraryItemID: itemID, Title: title, Time: currentTime, CreatedAt: time.Now().UnixMilli()} updated := append(append([]abs.Bookmark{}, existing...), newBm) logger.Info("bookmark added locally", "itemID", itemID, "count", len(updated)) - return detail.BookmarksUpdatedMsg{Bookmarks: updated} + return detail.BookmarksUpdatedMsg{ItemID: itemID, Bookmarks: updated} } } @@ -88,7 +88,7 @@ func (m Model) handleDeleteBookmark(msg detail.DeleteBookmarkCmd) (Model, tea.Cm } } logger.Info("bookmark deleted locally", "itemID", itemID, "count", len(updated)) - return detail.BookmarksUpdatedMsg{Bookmarks: updated} + return detail.BookmarksUpdatedMsg{ItemID: itemID, Bookmarks: updated} } } @@ -121,7 +121,7 @@ func (m Model) handleUpdateBookmark(msg detail.UpdateBookmarkCmd) (Model, tea.Cm } } logger.Info("bookmark updated locally", "itemID", itemID, "count", len(updated)) - return detail.BookmarksUpdatedMsg{Bookmarks: updated} + return detail.BookmarksUpdatedMsg{ItemID: itemID, Bookmarks: updated} } } @@ -134,9 +134,9 @@ func (m Model) fetchBookmarksCmd(itemID string) tea.Cmd { return func() tea.Msg { bookmarks, err := client.GetBookmarks(context.Background(), itemID) if err != nil { - return detail.BookmarksUpdatedMsg{Err: err} + return detail.BookmarksUpdatedMsg{ItemID: itemID, Err: err} } - return detail.BookmarksUpdatedMsg{Bookmarks: bookmarks} + return detail.BookmarksUpdatedMsg{ItemID: itemID, Bookmarks: bookmarks} } } diff --git a/internal/app/bookmarks_test.go b/internal/app/bookmarks_test.go index 1bc8317..c99c6e8 100644 --- a/internal/app/bookmarks_test.go +++ b/internal/app/bookmarks_test.go @@ -57,6 +57,9 @@ func TestHandleAddBookmarkAppendsOptimistically(t *testing.T) { if updateMsg.Err != nil { t.Fatalf("unexpected error: %v", updateMsg.Err) } + if updateMsg.ItemID != "item-001" { + t.Errorf("ItemID = %q, want item-001", updateMsg.ItemID) + } if len(updateMsg.Bookmarks) != 2 { t.Fatalf("expected 2 bookmarks, got %d", len(updateMsg.Bookmarks)) } @@ -162,6 +165,9 @@ func TestHandleDeleteBookmarkReturnsEmptyBookmarksWhenLastBookmarkRemoved(t *tes if updateMsg.Err != nil { t.Fatalf("expected empty bookmark update, got error %v", updateMsg.Err) } + if updateMsg.ItemID != "item-001" { + t.Errorf("ItemID = %q, want item-001", updateMsg.ItemID) + } if len(updateMsg.Bookmarks) != 0 { t.Fatalf("expected empty bookmarks after delete, got %d", len(updateMsg.Bookmarks)) } @@ -203,6 +209,9 @@ func TestHandleUpdateBookmarkUpdatesTitle(t *testing.T) { if updateMsg.Err != nil { t.Fatalf("expected no refresh error, got %v", updateMsg.Err) } + if updateMsg.ItemID != "item-001" { + t.Errorf("ItemID = %q, want item-001", updateMsg.ItemID) + } if len(updateMsg.Bookmarks) != 1 { t.Fatalf("expected 1 bookmark, got %d", len(updateMsg.Bookmarks)) } diff --git a/internal/app/e2e_test.go b/internal/app/e2e_test.go index b1b8c98..4e5f71a 100644 --- a/internal/app/e2e_test.go +++ b/internal/app/e2e_test.go @@ -753,6 +753,32 @@ func TestE2E_BookmarkFetchOnDetailEntry(t *testing.T) { } } +// TestE2E_DetailIgnoresBookmarksOfOtherItem verifies that the detail screen +// ignores the bookmarks of an item that it does not show. The answer for an +// item can come after the user opened a different item. +func TestE2E_DetailIgnoresBookmarksOfOtherItem(t *testing.T) { + state := &e2eServerState{bookmarks: make(map[string][]abs.Bookmark)} + state.bookmarks["item-001"] = []abs.Bookmark{ + testBookmark("Bookmark of the first item", 300.0), + } + srv := newFullMockABSServer(&apiLog{}, state) + defer srv.Close() + + m := newE2EModelAuthenticated(srv, &mockPlayer{}) + m = e2eSetSize(m, 120, 40) + + res, firstCmd := m.Update(home.NavigateDetailMsg{Item: testLibraryItem("item-001", "First")}) + res, _ = res.(Model).Update(BackMsg{}) + res, secondCmd := res.(Model).Update(home.NavigateDetailMsg{Item: testLibraryItem("item-002", "Second")}) + m = feedCmdChain(res.(Model), secondCmd, 5) + // The answers for the first item come last. + m = feedCmdChain(m, firstCmd, 5) + + if m.detail.ItemID() != "item-002" || len(m.detail.Bookmarks()) != 0 { + t.Fatalf("detail of %s has the bookmarks %+v, want item-002 and no bookmarks", m.detail.ItemID(), m.detail.Bookmarks()) + } +} + func TestE2E_BookmarkFetchOnDetailEntryShowsEmptyStateForNoBookmarks(t *testing.T) { log := &apiLog{} state := &e2eServerState{bookmarks: make(map[string][]abs.Bookmark)} diff --git a/internal/app/model.go b/internal/app/model.go index ae7e22f..9373cc9 100644 --- a/internal/app/model.go +++ b/internal/app/model.go @@ -590,6 +590,9 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m.handleUpdateBookmark(msg) case detail.BookmarksUpdatedMsg: + if msg.ItemID != "" && msg.ItemID != m.detail.ItemID() { + return m, nil + } m.detail, _ = m.detail.Update(msg) return m, nil diff --git a/internal/screens/detail/model.go b/internal/screens/detail/model.go index 9f5c453..81ecab3 100644 --- a/internal/screens/detail/model.go +++ b/internal/screens/detail/model.go @@ -90,8 +90,11 @@ type MarkFinishedMsg struct { Progress *abs.UserMediaProgress } -// BookmarksUpdatedMsg updates the bookmark list after an add/delete operation. +// BookmarksUpdatedMsg updates the bookmark list after a load or an +// add/delete operation. ItemID is the item of the bookmarks: the answer for an +// item can come after the user opened a different item. type BookmarksUpdatedMsg struct { + ItemID string Bookmarks []abs.Bookmark Err error } From da8bc7550525734d635009d77c68032579e11b9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 22:55:16 +0200 Subject: [PATCH 08/15] fix(bookmarks): ask an older ABS for the bookmark route one time - For ABS before 2.36.0, each read of the bookmarks sent two requests: the bookmark route, which answers 404, and GET /api/me. Each read also wrote an error line for the 404 to the log. - The client now keeps the result of the first 404, and then reads GET /api/me directly. - ABS from 2.36.0 answers 404 only for an item that it does not have. After such a 404, the client also reads GET /api/me for the later reads. That response has the bookmarks of all items, so the result is correct. Co-Authored-By: Claude Opus 5.5 --- internal/abs/bookmarks.go | 14 +++++++++++--- internal/abs/bookmarks_test.go | 20 ++++++++++++++------ internal/abs/client.go | 4 ++++ 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/internal/abs/bookmarks.go b/internal/abs/bookmarks.go index eb4b2b4..ad71b37 100644 --- a/internal/abs/bookmarks.go +++ b/internal/abs/bookmarks.go @@ -22,10 +22,17 @@ type bookmarksResponse struct { // GetBookmarks returns the bookmarks of the user for a library item. ABS // before 2.36.0 has no route for the bookmarks of one item, and answers 404. -// GetBookmarks then reads the bookmarks of the item from GET /api/me. +// GetBookmarks then reads the bookmarks of the item from GET /api/me, and the +// client does not ask for the route again. A later ABS answers 404 only for +// an item that it does not have. GET /api/me is correct for that server too. func (c *Client) GetBookmarks(ctx context.Context, itemID string) ([]Bookmark, error) { - resp, err := c.do(ctx, http.MethodGet, "/api/me/bookmarks/"+itemID, nil) - allItems := IsHTTPStatus(err, http.StatusNotFound) + var resp []byte + var err error + allItems := c.noBookmarkRoute.Load() + if !allItems { + resp, err = c.do(ctx, http.MethodGet, "/api/me/bookmarks/"+itemID, nil) + allItems = IsHTTPStatus(err, http.StatusNotFound) + } if allItems { resp, err = c.do(ctx, http.MethodGet, "/api/me", nil) } @@ -39,6 +46,7 @@ func (c *Client) GetBookmarks(ctx context.Context, itemID string) ([]Bookmark, e if !allItems { return body.Bookmarks, nil } + c.noBookmarkRoute.Store(true) bookmarks := []Bookmark{} for _, bookmark := range body.Bookmarks { if bookmark.LibraryItemID == itemID { diff --git a/internal/abs/bookmarks_test.go b/internal/abs/bookmarks_test.go index d772ef2..bc045e0 100644 --- a/internal/abs/bookmarks_test.go +++ b/internal/abs/bookmarks_test.go @@ -50,9 +50,12 @@ func TestGetBookmarksHTTP(t *testing.T) { // TestGetBookmarksReadsUserBeforeABS236 verifies that GetBookmarks reads the // bookmarks of the item from the user, if the server has no route for the -// bookmarks of one item. ABS before 2.36.0 answers 404 for that route. +// bookmarks of one item. ABS before 2.36.0 answers 404 for that route. The +// client asks for that route only one time. func TestGetBookmarksReadsUserBeforeABS236(t *testing.T) { + requests := map[string]int{} srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests[r.URL.Path]++ if r.URL.Path != "/api/me" { http.NotFound(w, r) return @@ -67,12 +70,17 @@ func TestGetBookmarksReadsUserBeforeABS236(t *testing.T) { defer srv.Close() c := NewClient(srv.URL, "tok") - bookmarks, err := c.GetBookmarks(context.Background(), "li-001") - if err != nil { - t.Fatalf("GetBookmarks() error: %v", err) + for read := 1; read <= 2; read++ { + bookmarks, err := c.GetBookmarks(context.Background(), "li-001") + if err != nil { + t.Fatalf("GetBookmarks() read %d error: %v", read, err) + } + if len(bookmarks) != 2 || bookmarks[0].Title != "Great passage" || bookmarks[1].Title != "Important quote" { + t.Errorf("read %d: bookmarks = %+v, want the two bookmarks of li-001", read, bookmarks) + } } - if len(bookmarks) != 2 || bookmarks[0].Title != "Great passage" || bookmarks[1].Title != "Important quote" { - t.Errorf("bookmarks = %+v, want the two bookmarks of li-001", bookmarks) + if requests["/api/me/bookmarks/li-001"] != 1 || requests["/api/me"] != 2 { + t.Errorf("requests = %v, want 1 for the bookmark route and 2 for the user", requests) } } diff --git a/internal/abs/client.go b/internal/abs/client.go index d4df56b..f874fc7 100644 --- a/internal/abs/client.go +++ b/internal/abs/client.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "net/http" + "sync/atomic" "time" "github.com/Thelost77/pine/internal/logger" @@ -36,6 +37,9 @@ type Client struct { baseURL string token string httpClient *http.Client + // noBookmarkRoute is true after the server answered 404 for the bookmarks + // of one item. See GetBookmarks. + noBookmarkRoute atomic.Bool } // NewClient creates a new ABS API client. From 7f54726834242235d620762cb5907b8cb478098f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 22:55:45 +0200 Subject: [PATCH 09/15] refactor(main): run the program and the hangup handler in one function - TestHangupRunsCleanup called quitOnHangup, and then ran the program. main did the same in two lines of its own. So the test passed also when main did not call quitOnHangup. - runProgram now starts the handler for SIGHUP and runs the program. main and the child process of the test call runProgram. The test fails if runProgram has no handler. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 4 ++-- main.go | 13 +++++++------ main_unix_test.go | 7 +++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d8ec252..c4de9f3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -141,7 +141,7 @@ Styles from theme config. Format helpers (timestamps, durations). Components: He - Cache-first data loading through `internal/cache.Client`: home and library views hit the persistent cache first; misses trigger a background rebuild - `q` quits, and `esc`/left goes back (never quits). `h`/`l` seek only, arrows navigate only. These keys have no global function while a text input or a dialog gets the keys (`capturesKeys`). - A quit (`q` or ctrl+c) returns only `tea.Quit` and keeps the session in the model. `Cleanup` sends the last position, and `main` waits for it. Bubble Tea does not wait for a command at a quit, so a quit must not send the position in a command. -- `main` quits the program at SIGHUP, which is the close of the terminal (`quitOnHangup`), so that `Cleanup` runs. Bubble Tea quits the program at SIGTERM. +- `main` runs the program with `runProgram`, which quits the program at SIGHUP, the close of the terminal, so that `Cleanup` runs. Bubble Tea quits the program at SIGTERM. - `ctrl+p` opens the global command palette from each screen, but not while a text input or a dialog gets the keys - A text input or a confirmation dialog of a screen gets each key, except ctrl+c. `capturesKeys` in `playback.go` is true for the login screen, the metadata editor, a list filter, and the bookmark title editor. It is also true for the confirmation dialog of the detail screen. `Update` in `model.go` then gives the key to the screen, before the global keys, the chapter overlay, the error banner, and the playback keys. The help overlay and the command palette get the keys first while they are open. If you add a text input or a dialog to a screen, add it to `capturesKeys`. - Duration from ABS takes precedence over mpv-reported duration (streaming content reports 0) @@ -155,7 +155,7 @@ Styles from theme config. Format helpers (timestamps, durations). Components: He - E2E tests in `internal/app/e2e_test.go` + `e2e_helpers_test.go` use mock HTTP server + mock player - E2E tests drive the bubbletea Update loop programmatically (no real TUI) - `internal/app/model_test.go`, `playback_test.go`, `bookmarks_test.go`, `render_test.go` for app-level tests -- `main_unix_test.go` starts the test program again as a child process, sends SIGHUP to it, and checks that it gets to the place where `main` calls `Cleanup`. It does not run on Windows. +- `main_unix_test.go` starts the test program again as a child process, sends SIGHUP to it, and checks that it gets to the place where `main` calls `Cleanup`. The child runs its program with `runProgram`, as `main` does. It does not run on Windows. - `internal/cache/*_test.go` for cache store and helper behavior - `internal/mpris/mpris_test.go` for the D-Bus adapter and bridge - `internal/mpris/server_linux_test.go` starts the MPRIS server on a private session bus and checks it as an MPRIS client. It skips if `dbus-daemon` is absent. With `PINE_REQUIRE_DBUS=1` it fails instead. CI and `scripts/release.sh` set that variable. diff --git a/main.go b/main.go index d6b9a26..9edfed3 100644 --- a/main.go +++ b/main.go @@ -76,9 +76,8 @@ func main() { model := app.New(cfg, store, cachedClient, cacheStore) p := tea.NewProgram(model, tea.WithAltScreen()) model.SetProgram(p) - quitOnHangup(p) logger.Info("starting TUI") - finalModel, err := p.Run() + finalModel, err := runProgram(p) if err != nil { logger.Error("program exited with error", "err", err) fmt.Fprintf(os.Stderr, "error: %v\n", err) @@ -90,10 +89,11 @@ func main() { logger.Info("session ended") } -// quitOnHangup quits the program when the process gets SIGHUP, which is the -// close of its terminal. Without it, the process ends at the signal, and main -// does not call Cleanup. Bubble Tea quits the program at SIGTERM. -func quitOnHangup(p *tea.Program) { +// runProgram runs the program until it quits, and returns its final model. It +// quits the program when the process gets SIGHUP, which is the close of its +// terminal. Without that, the process ends at the signal, and main does not +// call Cleanup. Bubble Tea quits the program at SIGTERM. +func runProgram(p *tea.Program) (tea.Model, error) { hangup := make(chan os.Signal, 1) signal.Notify(hangup, syscall.SIGHUP) go func() { @@ -101,4 +101,5 @@ func quitOnHangup(p *tea.Program) { logger.Info("terminal closed, quitting") p.Quit() }() + return p.Run() } diff --git a/main_unix_test.go b/main_unix_test.go index 925da53..5668ef7 100644 --- a/main_unix_test.go +++ b/main_unix_test.go @@ -33,12 +33,11 @@ func (m waitModel) Update(tea.Msg) (tea.Model, tea.Cmd) { return m, nil } func (waitModel) View() string { return "" } -// runHangupChild runs a program as main does, and prints a line at the place -// where main calls Cleanup. +// runHangupChild runs a program with runProgram, as main does, and prints a +// line at the place where main calls Cleanup. func runHangupChild() { p := tea.NewProgram(waitModel{}, tea.WithInput(nil), tea.WithOutput(io.Discard)) - quitOnHangup(p) - _, _ = p.Run() + _, _ = runProgram(p) fmt.Println("cleanup") os.Exit(0) } From b9c9388d98a30ed280f4d1b473051b9d5f99f86e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 23:04:50 +0200 Subject: [PATCH 10/15] fix(bookmarks): add a bookmark only to the item that plays - b on the detail screen made a bookmark at the position of the player. handleAddBookmark did not compare the item of the detail screen with the item that plays. So b on a different item made a bookmark on that item, at a position of the item that plays. - handleAddBookmark now refuses the bookmark and shows the error that the command palette shows in this case. Co-Authored-By: Claude Opus 5.5 --- internal/app/bookmarks.go | 8 ++++++++ internal/app/bookmarks_test.go | 3 +++ internal/app/e2e_test.go | 31 +++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/internal/app/bookmarks.go b/internal/app/bookmarks.go index b393412..6e9891a 100644 --- a/internal/app/bookmarks.go +++ b/internal/app/bookmarks.go @@ -24,6 +24,14 @@ func (m Model) handleAddBookmark(msg detail.AddBookmarkCmd) (Model, tea.Cmd) { m.propagateSize() return m, cmd } + // The bookmark gets the position of the player, so it is for the item + // that plays only. + if m.itemID != msg.Item.ID { + logger.Warn("bookmark skipped: a different item plays", "itemID", msg.Item.ID, "playingItemID", m.itemID) + cmd := m.err.SetError(fmt.Errorf("can only add bookmarks to the currently playing item")) + m.propagateSize() + return m, cmd + } client := m.client itemID := msg.Item.ID currentTime := m.player.Position diff --git a/internal/app/bookmarks_test.go b/internal/app/bookmarks_test.go index c99c6e8..1b94c0e 100644 --- a/internal/app/bookmarks_test.go +++ b/internal/app/bookmarks_test.go @@ -39,6 +39,7 @@ func TestHandleAddBookmarkAppendsOptimistically(t *testing.T) { m := New(config.Default(), nil, cache.NewClient(abs.NewClient(srv.URL, "tok"), nil), nil) m.sessionID = "sess-001" + m.itemID = "item-001" m.player.Position = 120 m.detail.SetBookmarks([]abs.Bookmark{ {LibraryItemID: "item-001", Title: "Existing", Time: 60, CreatedAt: 1700000000000}, @@ -82,6 +83,7 @@ func TestHandleAddBookmarkReturnsPlaybackErrorOnFailure(t *testing.T) { m := New(config.Default(), nil, cache.NewClient(abs.NewClient(srv.URL, "tok"), nil), nil) m.sessionID = "sess-001" + m.itemID = "item-001" m.player.Position = 120 _, cmd := m.handleAddBookmark(detail.AddBookmarkCmd{Item: testBookmarkItem("item-001")}) @@ -116,6 +118,7 @@ func TestHandleAddBookmarkUsesEpisodeTitleForPodcastBookmarks(t *testing.T) { m := New(config.Default(), nil, cache.NewClient(abs.NewClient(srv.URL, "tok"), nil), nil) m.sessionID = "sess-001" + m.itemID = "item-001" m.episodeID = "ep-001" m.player.Title = "Episode 2199" m.player.Position = 120 diff --git a/internal/app/e2e_test.go b/internal/app/e2e_test.go index 4e5f71a..fe66a86 100644 --- a/internal/app/e2e_test.go +++ b/internal/app/e2e_test.go @@ -753,6 +753,37 @@ func TestE2E_BookmarkFetchOnDetailEntry(t *testing.T) { } } +// TestE2E_NoBookmarkForItemThatDoesNotPlay verifies that b on the detail +// screen of an item makes no bookmark while a different item plays. The +// bookmark would have the position of the item that plays. +func TestE2E_NoBookmarkForItemThatDoesNotPlay(t *testing.T) { + log := &apiLog{} + state := &e2eServerState{bookmarks: make(map[string][]abs.Bookmark)} + srv := newFullMockABSServer(log, state) + defer srv.Close() + + m := newE2EModelAuthenticated(srv, &mockPlayer{position: 100, duration: 3600}) + m = e2eSetSize(m, 120, 40) + m.sessionID = "sess-e2e" + m.itemID = "item-001" + m.player.Playing = true + m.player.Position = 100.0 + m.player.Duration = 3600.0 + m.detail = detail.New(m.styles, testLibraryItem("item-002", "Other Book")) + m, _ = m.navigate(ScreenDetail) + m = e2eSetSize(m, 120, 40) + + m, cmd := e2ePressKey(m, 'b') + m = feedCmdChain(m, cmd, 3) + + if n := log.count("POST", "/api/me/item/"); n != 0 { + t.Errorf("%d bookmark requests for an item that does not play, want 0: %+v", n, log.get()) + } + if !m.err.HasError() { + t.Error("the error banner is not visible") + } +} + // TestE2E_DetailIgnoresBookmarksOfOtherItem verifies that the detail screen // ignores the bookmarks of an item that it does not show. The answer for an // item can come after the user opened a different item. From 6ffd9efad0476149bff88a3973f561388a5bee89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 23:05:18 +0200 Subject: [PATCH 11/15] fix(detail): move the cursor with the left arrow in the bookmark editor - The bookmark title editor used the back keys of the detail screen to cancel the edit. These keys are esc and the left arrow. So the left arrow closed the editor, and the text was lost. - Only esc cancels the edit now. The left arrow moves the cursor. Co-Authored-By: Claude Opus 5.5 --- internal/screens/detail/model.go | 4 +++- internal/screens/detail/model_test.go | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/internal/screens/detail/model.go b/internal/screens/detail/model.go index 81ecab3..56aa5e2 100644 --- a/internal/screens/detail/model.go +++ b/internal/screens/detail/model.go @@ -776,7 +776,9 @@ func (m *Model) startBookmarkEdit() tea.Cmd { func (m Model) updateBookmarkEditor(msg tea.KeyMsg) (Model, tea.Cmd) { switch { - case key.Matches(msg, m.keys.Back): + // Only esc cancels the edit. The left arrow, which also goes back on the + // detail screen, moves the cursor. + case msg.Type == tea.KeyEsc: m.editingBookmark = false m.bookmarkEditErr = "" m.refreshContent() diff --git a/internal/screens/detail/model_test.go b/internal/screens/detail/model_test.go index 97013e4..6da39e5 100644 --- a/internal/screens/detail/model_test.go +++ b/internal/screens/detail/model_test.go @@ -627,6 +627,25 @@ func TestEKey_StartsBookmarkEdit(t *testing.T) { } } +// TestLeftKey_MovesCursorWhileEditingBookmark verifies that the left arrow +// moves the cursor in the bookmark title editor. It does not cancel the edit. +func TestLeftKey_MovesCursorWhileEditingBookmark(t *testing.T) { + m := newTestModelWithBookmarks() + m, _ = m.Update(tea.KeyMsg{Type: tea.KeyTab}) + m, _ = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'e'}}) + m.bookmarkInput.SetValue("ac") + + m, _ = m.Update(tea.KeyMsg{Type: tea.KeyLeft}) + m, _ = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'b'}}) + + if !m.EditingBookmark() { + t.Fatal("after the left arrow, the bookmark editor is not open") + } + if m.bookmarkInput.Value() != "abc" { + t.Fatalf("edit value = %q, want abc", m.bookmarkInput.Value()) + } +} + func TestEnterKey_UpdatesBookmarkTitleWhileEditing(t *testing.T) { m := newTestModelWithBookmarks() m, _ = m.Update(tea.KeyMsg{Type: tea.KeyTab}) From ab8d96270042c8f8d7bb90254b42f86a0428cffe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 23:06:48 +0200 Subject: [PATCH 12/15] fix(player): report a pause, speed, or volume that the player refuses - SetPauseCmd, SetSpeedCmd, and SetVolumeCmd reported a failure of the player as a position tick with the generation 0. pine and spruce ignore a tick that is not of the current playback. So the app showed a speed that the player did not have, and showed no error. - The three commands now return CommandErrMsg. pine and spruce show its error in the error banner. - A player that is closed gets no banner: it has no state, and the next launch sets the speed and the volume. - The seek commands are as they were. After a seek that fails, the next position tick has the position of the player. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- internal/app/model.go | 11 +++++++++++ internal/app/model_test.go | 19 +++++++++++++++++++ internal/player/commands.go | 14 +++++++++++--- internal/player/commands_test.go | 31 +++++++++++++++++++++++++++++++ internal/spruce/app/model_test.go | 19 +++++++++++++++++++ internal/spruce/app/playback.go | 8 ++++++++ 7 files changed, 100 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c4de9f3..542c16e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,7 +71,7 @@ The Architecture Map below describes pine and the shared packages. |------|---------------| | `mpv.go` | Mpv struct: Launch (creates a libmpv handle and loads the file), GetPosition/GetDuration/GetPaused/GetIdle, Seek, SetSpeed/SetVolume, Quit. `ErrClosed` and `ErrPropertyUnavailable`. Writes the warnings and errors of mpv to the app log, without URL queries. Runtime directory for the media cache. | | `libmpv.go`, `libmpv_unix.go`, `libmpv_windows.go` | Binding to libmpv through purego, with no cgo. The library loads on the first Launch. Library search order for each system. | -| `commands.go` | tea.Cmd factories: TickCmd (500ms poll), LaunchCmd, SetPauseCmd, SetSpeedCmd, SetVolumeCmd, QuitCmd. WatchEvents returns PlayerEndMsg at the end of a playback. | +| `commands.go` | tea.Cmd factories: TickCmd (500ms poll), LaunchCmd, SetPauseCmd, SetSpeedCmd, SetVolumeCmd, QuitCmd. A command for the pause state, the speed, or the volume returns CommandErrMsg if the player does not accept the value. WatchEvents returns PlayerEndMsg at the end of a playback. | | `model.go` | Player sub-model: Position/Duration/Speed/Volume state, key handling (play/pause, seek, speed, volume) | | `view.go` | Player footer bar: icon + title + time + speed + optional volume/sleep | | `cache.go`, `group.go` | Session media cache and player group for simultaneous playback. Only spruce uses them. | diff --git a/internal/app/model.go b/internal/app/model.go index 9373cc9..37f8e4a 100644 --- a/internal/app/model.go +++ b/internal/app/model.go @@ -2,6 +2,7 @@ package app import ( "context" + "errors" "fmt" "strings" "time" @@ -660,6 +661,16 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case player.PlayerQuitMsg: return m, nil + case player.CommandErrMsg: + // A player that is closed has no state. The next launch sets the + // speed and the volume. + if errors.Is(msg.Err, player.ErrClosed) { + return m, nil + } + cmd := m.err.SetError(msg.Err) + m.propagateSize() + return m, cmd + // --- MPRIS control messages --- case mpris.PlayPauseMsg: diff --git a/internal/app/model_test.go b/internal/app/model_test.go index 9c14e91..02a693e 100644 --- a/internal/app/model_test.go +++ b/internal/app/model_test.go @@ -562,6 +562,25 @@ func TestDetailConfirmCapturesQuitAndBackKeys(t *testing.T) { } } +// TestPlayerCommandErrorShowsErrorBanner verifies that the model shows the +// error of a player that did not accept a new speed. A player that is closed +// has no state, so its error has no banner. +func TestPlayerCommandErrorShowsErrorBanner(t *testing.T) { + m := newTestModelAuthenticated() + + result, _ := m.Update(player.CommandErrMsg{Err: fmt.Errorf("set speed: %w", player.ErrClosed)}) + m = result.(Model) + if m.err.HasError() { + t.Fatal("the error banner is visible for a player that is closed") + } + + result, _ = m.Update(player.CommandErrMsg{Err: errors.New("set speed: not accepted")}) + m = result.(Model) + if !m.err.HasError() { + t.Fatal("the error banner is not visible") + } +} + func TestContentSearchContextUsesActiveLibraryScreen(t *testing.T) { m := newTestModelAuthenticated() m.home = home.New(m.styles, m.client) diff --git a/internal/player/commands.go b/internal/player/commands.go index e8247d9..abaf5da 100644 --- a/internal/player/commands.go +++ b/internal/player/commands.go @@ -29,6 +29,14 @@ type PlayerEndMsg struct { Err error } +// CommandErrMsg reports that the player did not accept a new pause state, a +// new speed, or a new volume. The app then shows a state that the player does +// not have. A seek that fails needs no message: the next position tick has +// the position of the player. +type CommandErrMsg struct { + Err error +} + // TickCmd returns a command that fires twice per second and polls mpv state. // The generation parameter ties the tick to a specific play session so stale // ticks from a previous session can be safely ignored. @@ -61,7 +69,7 @@ func SetPauseCmd(p Player, paused bool) tea.Cmd { return func() tea.Msg { err := p.SetPause(paused) if err != nil { - return PositionMsg{Err: err} + return CommandErrMsg{Err: err} } return nil } @@ -72,7 +80,7 @@ func SetSpeedCmd(p Player, speed float64) tea.Cmd { return func() tea.Msg { err := p.SetSpeed(speed) if err != nil { - return PositionMsg{Err: err} + return CommandErrMsg{Err: err} } return nil } @@ -83,7 +91,7 @@ func SetVolumeCmd(p Player, vol int) tea.Cmd { return func() tea.Msg { err := p.SetVolume(vol) if err != nil { - return PositionMsg{Err: err} + return CommandErrMsg{Err: err} } return nil } diff --git a/internal/player/commands_test.go b/internal/player/commands_test.go index d4fe98d..6a6a5aa 100644 --- a/internal/player/commands_test.go +++ b/internal/player/commands_test.go @@ -3,6 +3,8 @@ package player import ( "fmt" "testing" + + tea "github.com/charmbracelet/bubbletea" ) // TestLaunchCmdPropagatesGenerationToLaunchError verifies that LaunchCmd @@ -98,3 +100,32 @@ var errLaunchSentinel = &sentinelError{"launch failed"} type sentinelError struct{ s string } func (e *sentinelError) Error() string { return e.s } + +// stateFailPlayer implements Player and fails SetPause, SetSpeed, and +// SetVolume. +type stateFailPlayer struct{ launchFailPlayer } + +func (stateFailPlayer) SetPause(paused bool) error { return errStateSentinel } +func (stateFailPlayer) SetSpeed(speed float64) error { return errStateSentinel } +func (stateFailPlayer) SetVolume(vol int) error { return errStateSentinel } + +var errStateSentinel = &sentinelError{"state not accepted"} + +// TestStateCommandsReportFailure verifies that a command for the pause state, +// the speed, or the volume reports a failure of the player as CommandErrMsg. +// The failure is not a position tick: an app ignores a tick that has no +// generation. +func TestStateCommandsReportFailure(t *testing.T) { + p := stateFailPlayer{} + cmds := map[string]tea.Cmd{ + "pause": SetPauseCmd(p, true), + "speed": SetSpeedCmd(p, 1.5), + "volume": SetVolumeCmd(p, 50), + } + for name, cmd := range cmds { + msg, ok := cmd().(CommandErrMsg) + if !ok || msg.Err != error(errStateSentinel) { + t.Errorf("%s: the command returned %#v, want CommandErrMsg with the error of the player", name, cmd()) + } + } +} diff --git a/internal/spruce/app/model_test.go b/internal/spruce/app/model_test.go index e0def65..3766b8b 100644 --- a/internal/spruce/app/model_test.go +++ b/internal/spruce/app/model_test.go @@ -253,6 +253,25 @@ func TestClosedPlayerTickDoesNotStopBeforeTheEndEvent(t *testing.T) { } } +// TestPlayerCommandErrorShowsErrorBanner verifies that the model shows the +// error of a player that did not accept a new speed. A player that is closed +// has no state, so its error has no banner. +func TestPlayerCommandErrorShowsErrorBanner(t *testing.T) { + m := New(nil, nil) + + updated, _ := m.Update(player.CommandErrMsg{Err: fmt.Errorf("set speed: %w", player.ErrClosed)}) + m = updated.(*Model) + if m.err.HasError() { + t.Fatal("the error banner is visible for a player that is closed") + } + + updated, _ = m.Update(player.CommandErrMsg{Err: errors.New("set speed: not accepted")}) + m = updated.(*Model) + if !m.err.HasError() { + t.Fatal("the error banner is not visible") + } +} + func TestMPRISSeekAbsoluteSeeksTheCurrentTrack(t *testing.T) { m := New(nil, player.NewMpv()) m.tracks = []jellyfin.Track{{ID: "t-1", Name: "Track 1"}} diff --git a/internal/spruce/app/playback.go b/internal/spruce/app/playback.go index 3d46554..221b05b 100644 --- a/internal/spruce/app/playback.go +++ b/internal/spruce/app/playback.go @@ -463,6 +463,14 @@ func (m *Model) handlePlayerEvent(msg tea.Msg) (*Model, tea.Cmd, bool) { newM, cmd := m.handlePositionMsg(msg) return newM, cmd, true + case player.CommandErrMsg: + // A player that is closed has no state. The next launch sets the + // speed and the volume. + if errors.Is(msg.Err, player.ErrClosed) { + return m, nil, true + } + return m, m.err.SetError(msg.Err), true + case player.PlayerReadyMsg: if msg.Generation != m.playGeneration { if m.mpv != nil && (m.isSubliminalActive() || !m.IsPlaying()) { From a92ed6d1ad5810433dda6c56198be6b2f55ea702 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 23:07:29 +0200 Subject: [PATCH 13/15] refactor(player): use one command for the position tick - player.TickCmd and tickCmd of the app had the same body. They were different only in the interval. A change of the tick, such as the read of core-idle, had to go in the two functions. - player.TickAfterCmd has the body and gets the interval. TickCmd calls it with 500 ms. The app calls it for the longer wait while mpv has no position. tickCmd of the app is gone. - TestStalledPlayerAddsNoListeningTime had one case for each function. It now has one case. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- internal/app/playback.go | 26 +------------------------- internal/app/progress_sync_test.go | 30 +++++++++--------------------- internal/player/commands.go | 8 +++++++- 4 files changed, 18 insertions(+), 48 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 542c16e..3b7b3ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,7 +71,7 @@ The Architecture Map below describes pine and the shared packages. |------|---------------| | `mpv.go` | Mpv struct: Launch (creates a libmpv handle and loads the file), GetPosition/GetDuration/GetPaused/GetIdle, Seek, SetSpeed/SetVolume, Quit. `ErrClosed` and `ErrPropertyUnavailable`. Writes the warnings and errors of mpv to the app log, without URL queries. Runtime directory for the media cache. | | `libmpv.go`, `libmpv_unix.go`, `libmpv_windows.go` | Binding to libmpv through purego, with no cgo. The library loads on the first Launch. Library search order for each system. | -| `commands.go` | tea.Cmd factories: TickCmd (500ms poll), LaunchCmd, SetPauseCmd, SetSpeedCmd, SetVolumeCmd, QuitCmd. A command for the pause state, the speed, or the volume returns CommandErrMsg if the player does not accept the value. WatchEvents returns PlayerEndMsg at the end of a playback. | +| `commands.go` | tea.Cmd factories: TickCmd (500ms poll), TickAfterCmd (poll after an interval), LaunchCmd, SetPauseCmd, SetSpeedCmd, SetVolumeCmd, QuitCmd. A command for the pause state, the speed, or the volume returns CommandErrMsg if the player does not accept the value. WatchEvents returns PlayerEndMsg at the end of a playback. | | `model.go` | Player sub-model: Position/Duration/Speed/Volume state, key handling (play/pause, seek, speed, volume) | | `view.go` | Player footer bar: icon + title + time + speed + optional volume/sleep | | `cache.go`, `group.go` | Session media cache and player group for simultaneous playback. Only spruce uses them. | diff --git a/internal/app/playback.go b/internal/app/playback.go index 01f44d5..a1c9d3e 100644 --- a/internal/app/playback.go +++ b/internal/app/playback.go @@ -406,7 +406,7 @@ func (m Model) handlePositionMsg(msg player.PositionMsg) (Model, tea.Cmd) { } interval := 500 * time.Millisecond << shift logger.Debug("mpv properties not ready yet, backing off", "attempt", m.propertyUnavailableCount, "nextTick", interval) - return m, tickCmd(m.mpv, m.playGeneration, interval) + return m, player.TickAfterCmd(m.mpv, m.playGeneration, interval) } m.propertyUnavailableCount = 0 @@ -1161,30 +1161,6 @@ func syncTickCmd() tea.Cmd { }) } -// tickCmd returns a command that polls mpv position after the given interval. -func tickCmd(p player.Player, generation uint64, interval time.Duration) tea.Cmd { - return tea.Tick(interval, func(_ time.Time) tea.Msg { - pos, posErr := p.GetPosition() - dur, durErr := p.GetDuration() - paused, pauseErr := p.GetPaused() - idle, idleErr := p.GetIdle() - - for _, err := range []error{posErr, durErr, pauseErr, idleErr} { - if err != nil { - return player.PositionMsg{Err: err, Generation: generation} - } - } - - return player.PositionMsg{ - Position: pos, - Duration: dur, - Paused: paused, - Idle: idle, - Generation: generation, - } - }) -} - // restoreSessionCmd fetches the last saved session from the DB and resolves // the full LibraryItem from ABS so playback can resume. func restoreSessionCmd(client *cache.Client, store *db.Store) tea.Cmd { diff --git a/internal/app/progress_sync_test.go b/internal/app/progress_sync_test.go index 8e1efac..9305587 100644 --- a/internal/app/progress_sync_test.go +++ b/internal/app/progress_sync_test.go @@ -800,28 +800,16 @@ func TestListeningTimeIsTimeOfPlay(t *testing.T) { // it fills its cache: its position does not move, and it is idle. func TestStalledPlayerAddsNoListeningTime(t *testing.T) { mp := &mockPlayer{position: 100, duration: 3600, idle: true} - ticks := map[string]func(generation uint64) tea.Cmd{ - "position tick": func(generation uint64) tea.Cmd { - return player.TickCmd(mp, generation) - }, - "position tick after a property that was not ready": func(generation uint64) tea.Cmd { - return tickCmd(mp, generation, time.Millisecond) - }, - } - for name, tick := range ticks { - t.Run(name, func(t *testing.T) { - _, client := newFakeABS(t, nil, nil) - m := advanceClock(newSyncTestModel(client), 1) + _, client := newFakeABS(t, nil, nil) + m := advanceClock(newSyncTestModel(client), 1) - result, _ := m.Update(tick(m.playGeneration)()) - m = result.(Model) + result, _ := m.Update(player.TickAfterCmd(mp, m.playGeneration, time.Millisecond)()) + m = result.(Model) - if m.timeListened != 0 { - t.Errorf("timeListened = %v after one second of a player that waits for data, want 0", m.timeListened) - } - if !m.player.Playing { - t.Error("the model shows a pause for a player that waits for data") - } - }) + if m.timeListened != 0 { + t.Errorf("timeListened = %v after one second of a player that waits for data, want 0", m.timeListened) + } + if !m.player.Playing { + t.Error("the model shows a pause for a player that waits for data") } } diff --git a/internal/player/commands.go b/internal/player/commands.go index abaf5da..6f8449b 100644 --- a/internal/player/commands.go +++ b/internal/player/commands.go @@ -41,7 +41,13 @@ type CommandErrMsg struct { // The generation parameter ties the tick to a specific play session so stale // ticks from a previous session can be safely ignored. func TickCmd(p Player, generation uint64) tea.Cmd { - return tea.Tick(500*time.Millisecond, func(_ time.Time) tea.Msg { + return TickAfterCmd(p, generation, 500*time.Millisecond) +} + +// TickAfterCmd returns a command that polls mpv state after the interval. An +// app uses it to wait longer while mpv has no position yet. +func TickAfterCmd(p Player, generation uint64, interval time.Duration) tea.Cmd { + return tea.Tick(interval, func(_ time.Time) tea.Msg { pos, posErr := p.GetPosition() dur, durErr := p.GetDuration() paused, pauseErr := p.GetPaused() From c7aa3999c01bd8821bb8ef4780fa48268a72b940 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 23:07:30 +0200 Subject: [PATCH 14/15] docs(abs): state that the progress of an item has no bookmarks - The comments of GetMediaProgress and MediaProgressWithBookmarks said that the progress has the bookmarks. ABS gives no bookmarks in that response. GetBookmarks reads them from the bookmark route. Co-Authored-By: Claude Opus 5.5 --- internal/abs/progress.go | 4 ++-- internal/abs/types.go | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/abs/progress.go b/internal/abs/progress.go index 038ee3e..aa85577 100644 --- a/internal/abs/progress.go +++ b/internal/abs/progress.go @@ -50,8 +50,8 @@ func (c *Client) UpdatePosition(ctx context.Context, itemID, episodeID string, c return nil } -// GetMediaProgress returns the user's media progress (including bookmarks) -// for a specific library item. +// GetMediaProgress returns the user's media progress for a specific library +// item. ABS gives no bookmarks in this response: GetBookmarks reads them. func (c *Client) GetMediaProgress(ctx context.Context, itemID string) (*MediaProgressWithBookmarks, error) { path := fmt.Sprintf("/api/me/progress/%s", itemID) resp, err := c.do(ctx, http.MethodGet, path, nil) diff --git a/internal/abs/types.go b/internal/abs/types.go index 02ab181..29f7e66 100644 --- a/internal/abs/types.go +++ b/internal/abs/types.go @@ -343,7 +343,9 @@ type Bookmark struct { CreatedAt int64 `json:"createdAt"` // unix timestamp ms } -// MediaProgressWithBookmarks extends progress with bookmark data. +// MediaProgressWithBookmarks is the body of GET /api/me/progress/{id}. ABS +// gives no key "bookmarks" in that body, so Bookmarks is always empty. +// GetBookmarks reads the bookmarks. type MediaProgressWithBookmarks struct { LibraryItemID string `json:"libraryItemId"` EpisodeID string `json:"episodeId,omitempty"` From 1588e3053fd6dff4b4dae6946a9c7f9ed9ab9c0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wiktor=20Zi=C4=99bka?= Date: Tue, 13 Oct 2026 23:09:12 +0200 Subject: [PATCH 15/15] fix(spruce): do the cleanup when the terminal closes - The main of spruce had no handler for SIGHUP. At the close of the terminal, the process ended in p.Run, and main did not call Cleanup. So spruce did not close its media cache and its players. - The function that runs the program and quits it at SIGHUP is now ui.RunProgram, in the shared package internal/ui. The main of pine and the main of spruce call it. - The test is now internal/ui/program_unix_test.go, with the name TestRunProgramReturnsAtHangup. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 6 ++--- cmd/spruce/main.go | 3 ++- internal/ui/program.go | 26 +++++++++++++++++++ .../ui/program_unix_test.go | 21 ++++++++------- main.go | 20 ++------------ 5 files changed, 44 insertions(+), 32 deletions(-) create mode 100644 internal/ui/program.go rename main_unix_test.go => internal/ui/program_unix_test.go (71%) diff --git a/AGENTS.md b/AGENTS.md index 3b7b3ca..af7adc1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -129,7 +129,7 @@ TOML config with defaults. Theme (Everforest Dark palette), player (speed, seek) ### UI (internal/ui/, shared) -Styles from theme config. Format helpers (timestamps, durations). Components: HelpOverlay (the app supplies the groups), ErrorBanner (5s auto-dismiss), ConfirmOverlay, CommandPalette, and the filter, flash, and skeleton helpers. +Styles from theme config. Format helpers (timestamps, durations). `RunProgram` runs a Bubble Tea program and quits it at SIGHUP. Components: HelpOverlay (the app supplies the groups), ErrorBanner (5s auto-dismiss), ConfirmOverlay, CommandPalette, and the filter, flash, and skeleton helpers. ## Key Patterns to Follow @@ -141,7 +141,7 @@ Styles from theme config. Format helpers (timestamps, durations). Components: He - Cache-first data loading through `internal/cache.Client`: home and library views hit the persistent cache first; misses trigger a background rebuild - `q` quits, and `esc`/left goes back (never quits). `h`/`l` seek only, arrows navigate only. These keys have no global function while a text input or a dialog gets the keys (`capturesKeys`). - A quit (`q` or ctrl+c) returns only `tea.Quit` and keeps the session in the model. `Cleanup` sends the last position, and `main` waits for it. Bubble Tea does not wait for a command at a quit, so a quit must not send the position in a command. -- `main` runs the program with `runProgram`, which quits the program at SIGHUP, the close of the terminal, so that `Cleanup` runs. Bubble Tea quits the program at SIGTERM. +- The `main` of pine and the `main` of spruce run the program with `ui.RunProgram`, which quits the program at SIGHUP, the close of the terminal, so that `Cleanup` runs. Bubble Tea quits the program at SIGTERM. - `ctrl+p` opens the global command palette from each screen, but not while a text input or a dialog gets the keys - A text input or a confirmation dialog of a screen gets each key, except ctrl+c. `capturesKeys` in `playback.go` is true for the login screen, the metadata editor, a list filter, and the bookmark title editor. It is also true for the confirmation dialog of the detail screen. `Update` in `model.go` then gives the key to the screen, before the global keys, the chapter overlay, the error banner, and the playback keys. The help overlay and the command palette get the keys first while they are open. If you add a text input or a dialog to a screen, add it to `capturesKeys`. - Duration from ABS takes precedence over mpv-reported duration (streaming content reports 0) @@ -155,7 +155,7 @@ Styles from theme config. Format helpers (timestamps, durations). Components: He - E2E tests in `internal/app/e2e_test.go` + `e2e_helpers_test.go` use mock HTTP server + mock player - E2E tests drive the bubbletea Update loop programmatically (no real TUI) - `internal/app/model_test.go`, `playback_test.go`, `bookmarks_test.go`, `render_test.go` for app-level tests -- `main_unix_test.go` starts the test program again as a child process, sends SIGHUP to it, and checks that it gets to the place where `main` calls `Cleanup`. The child runs its program with `runProgram`, as `main` does. It does not run on Windows. +- `internal/ui/program_unix_test.go` starts the test program again as a child process, sends SIGHUP to it, and checks that it gets to the place where a `main` calls `Cleanup`. The child runs its program with `ui.RunProgram`, as each `main` does. It does not run on Windows. - `internal/cache/*_test.go` for cache store and helper behavior - `internal/mpris/mpris_test.go` for the D-Bus adapter and bridge - `internal/mpris/server_linux_test.go` starts the MPRIS server on a private session bus and checks it as an MPRIS client. It skips if `dbus-daemon` is absent. With `PINE_REQUIRE_DBUS=1` it fails instead. CI and `scripts/release.sh` set that variable. diff --git a/cmd/spruce/main.go b/cmd/spruce/main.go index 0af710f..1917b13 100644 --- a/cmd/spruce/main.go +++ b/cmd/spruce/main.go @@ -11,6 +11,7 @@ import ( "github.com/Thelost77/pine/internal/spruce/app" "github.com/Thelost77/pine/internal/spruce/buildinfo" "github.com/Thelost77/pine/internal/spruce/config" + "github.com/Thelost77/pine/internal/ui" tea "github.com/charmbracelet/bubbletea" ) @@ -44,7 +45,7 @@ func main() { p := tea.NewProgram(rootModel, tea.WithAltScreen()) rootModel.SetProgram(p) - finalModel, err := p.Run() + finalModel, err := ui.RunProgram(p) if err != nil { logger.Error("program exited with error", "err", err) fmt.Fprintf(os.Stderr, "error: %v\n", err) diff --git a/internal/ui/program.go b/internal/ui/program.go new file mode 100644 index 0000000..00dfa0a --- /dev/null +++ b/internal/ui/program.go @@ -0,0 +1,26 @@ +package ui + +import ( + "os" + "os/signal" + "syscall" + + "github.com/Thelost77/pine/internal/logger" + tea "github.com/charmbracelet/bubbletea" +) + +// RunProgram runs the program until it quits, and returns its final model. It +// quits the program when the process gets SIGHUP, which is the close of its +// terminal. Without that, the process ends at the signal, and the app does +// not do its cleanup after the program. Bubble Tea quits the program at +// SIGTERM. +func RunProgram(p *tea.Program) (tea.Model, error) { + hangup := make(chan os.Signal, 1) + signal.Notify(hangup, syscall.SIGHUP) + go func() { + <-hangup + logger.Info("terminal closed, quitting") + p.Quit() + }() + return p.Run() +} diff --git a/main_unix_test.go b/internal/ui/program_unix_test.go similarity index 71% rename from main_unix_test.go rename to internal/ui/program_unix_test.go index 5668ef7..def2356 100644 --- a/main_unix_test.go +++ b/internal/ui/program_unix_test.go @@ -1,6 +1,6 @@ //go:build unix -package main +package ui import ( "bufio" @@ -15,7 +15,7 @@ import ( tea "github.com/charmbracelet/bubbletea" ) -// hangupChildEnv marks the process that TestHangupRunsCleanup starts. +// hangupChildEnv marks the process that TestRunProgramReturnsAtHangup starts. const hangupChildEnv = "PINE_TEST_HANGUP_CHILD" // waitModel is a program that waits. Its Init prints a line when the program @@ -33,23 +33,24 @@ func (m waitModel) Update(tea.Msg) (tea.Model, tea.Cmd) { return m, nil } func (waitModel) View() string { return "" } -// runHangupChild runs a program with runProgram, as main does, and prints a -// line at the place where main calls Cleanup. +// runHangupChild runs a program with RunProgram, as the main of an app does, +// and prints a line at the place where that main calls Cleanup. func runHangupChild() { p := tea.NewProgram(waitModel{}, tea.WithInput(nil), tea.WithOutput(io.Discard)) - _, _ = runProgram(p) + _, _ = RunProgram(p) fmt.Println("cleanup") os.Exit(0) } -// TestHangupRunsCleanup verifies that the process gets to the place after the -// program when it gets SIGHUP, which is the close of the terminal. main calls -// Cleanup there, which sends the last position and the listening time. -func TestHangupRunsCleanup(t *testing.T) { +// TestRunProgramReturnsAtHangup verifies that the process gets to the place +// after the program when it gets SIGHUP, which is the close of the terminal. +// The main of an app calls Cleanup there. The Cleanup of pine sends the last +// position and the listening time. +func TestRunProgramReturnsAtHangup(t *testing.T) { if os.Getenv(hangupChildEnv) == "1" { runHangupChild() } - child := exec.Command(os.Args[0], "-test.run=^TestHangupRunsCleanup$") + child := exec.Command(os.Args[0], "-test.run=^TestRunProgramReturnsAtHangup$") child.Env = append(os.Environ(), hangupChildEnv+"=1") stdout, err := child.StdoutPipe() if err != nil { diff --git a/main.go b/main.go index 9edfed3..d9fde60 100644 --- a/main.go +++ b/main.go @@ -4,9 +4,7 @@ import ( "flag" "fmt" "os" - "os/signal" "path/filepath" - "syscall" "github.com/Thelost77/pine/internal/abs" "github.com/Thelost77/pine/internal/app" @@ -15,6 +13,7 @@ import ( "github.com/Thelost77/pine/internal/db" "github.com/Thelost77/pine/internal/logger" "github.com/Thelost77/pine/internal/player" + "github.com/Thelost77/pine/internal/ui" tea "github.com/charmbracelet/bubbletea" ) @@ -77,7 +76,7 @@ func main() { p := tea.NewProgram(model, tea.WithAltScreen()) model.SetProgram(p) logger.Info("starting TUI") - finalModel, err := runProgram(p) + finalModel, err := ui.RunProgram(p) if err != nil { logger.Error("program exited with error", "err", err) fmt.Fprintf(os.Stderr, "error: %v\n", err) @@ -88,18 +87,3 @@ func main() { } logger.Info("session ended") } - -// runProgram runs the program until it quits, and returns its final model. It -// quits the program when the process gets SIGHUP, which is the close of its -// terminal. Without that, the process ends at the signal, and main does not -// call Cleanup. Bubble Tea quits the program at SIGTERM. -func runProgram(p *tea.Program) (tea.Model, error) { - hangup := make(chan os.Signal, 1) - signal.Notify(hangup, syscall.SIGHUP) - go func() { - <-hangup - logger.Info("terminal closed, quitting") - p.Quit() - }() - return p.Run() -}