fix: correct bookmarks, playback faults, and the keys of text inputs

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 |
@@ -69,9 +69,9 @@ 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. |
+| `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. |
@@ -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/)
 
@@ -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
 
@@ -139,11 +139,14 @@ 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.
-- `ctrl+p` opens the global command palette from any screen
+- 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)
-- 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`.
+- Each launch makes a new mpv handle, which plays at the speed 1.0. `handlePlayerReady` sets the speed of the model in the new player.
+- The listening time (`timeListened`) is the time of the clock between two position ticks of a player that plays (`trackListeningTime`). It is not the change of the position: at the speed 2.0, the position moves two seconds in each second. A player that is paused or idle does not play. mpv reports no pause while it waits for data, so a position tick also reads the mpv property `core-idle` (`GetIdle`, `PositionMsg.Idle`). A test moves the clock with `Model.now`.
 - pine finds the end of a track through `errors.Is(err, player.ErrClosed)` on a position tick. spruce uses `PlayerEndMsg`, and a tick with `ErrClosed` can arrive before it. Do not match the text of a player error.
 
 ## Testing
@@ -152,6 +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
+- `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.

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]

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)

internal/abs/bookmarks.go

@@ -2,6 +2,7 @@ package abs
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"strconv"
@@ -13,13 +14,46 @@ 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, 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) {
-	progress, err := c.GetMediaProgress(ctx, itemID)
+	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)
+	}
 	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
+	}
+	c.noBookmarkRoute.Store(true)
+	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.

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,42 @@ 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. 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
+		}
+		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")
+	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 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)
+	}
+}
+
 func TestCreateBookmarkHTTP(t *testing.T) {
 	var capturedBody []byte
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -160,7 +197,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 +240,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()
 

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.

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)

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"`

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
@@ -46,7 +54,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 +96,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 +129,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}
 	}
 }
 
@@ -132,14 +140,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{ItemID: itemID, Err: err}
 		}
-		return detail.BookmarksUpdatedMsg{Bookmarks: progress.Bookmarks}
+		return detail.BookmarksUpdatedMsg{ItemID: itemID, Bookmarks: bookmarks}
 	}
 }