[Thelost77]

What was wrong

A review named four suspected faults. Each fault is confirmed in the code of pine, and a test reproduces it. Each fault has its own commit. "ABS" below is Audiobookshelf 2.37.1. The statements about ABS come from its source in the Docker image ghcr.io/advplyr/audiobookshelf:2.37.1.

1. The detail screen showed no bookmarks for an item that has bookmarks. fetchBookmarksCmd in internal/app/bookmarks.go read the key bookmarks of GET /api/me/progress/{id} (MediaProgressWithBookmarks in internal/abs/types.go). ABS gives no such key in that response (getOldMediaProgress in server/models/MediaProgress.js), and it answers 404 for an item with no progress (getMediaProgress in server/controllers/MeController.js). pine read the 404 as "no bookmarks". So the detail screen showed "No bookmarks yet" in the two cases. A bookmark that the user added was in the list only until the user left the detail screen. The fake server of the E2E tests gave bookmarks with the progress, so the tests passed.
2. A new player played at the speed 1.0, while the footer showed the speed of the model. Each Launch makes a new mpv handle (launch in internal/player/mpv.go), and start sets no speed option. Only the speed keys and the MPRIS rate called SetSpeedCmd. So after the start of a session, and after a track rollover, mpv played at the speed 1.0. The key speed of the config file had no effect on the player.
3. The time of a stalled stream counted as listening time. trackListeningTime in internal/app/playback.go counted the time of the clock while the player reported no pause. mpv reports no pause while it waits for data: the property pause stays false, time-pos does not move, and the properties paused-for-cache and core-idle are true.
4. The close of the terminal lost the last position and the listening time. The close of a terminal sends SIGHUP to the process. Go ends a process at SIGHUP if no code asked for the signal. Bubble Tea 1.3.10 asks only for SIGINT and SIGTERM (handleSignals in tea.go). So the process ended in p.Run, and main did not call Cleanup. SIGTERM had no fault: Bubble Tea sends QuitMsg, p.Run returns the final model, and main calls Cleanup.

What changes

• Fault 1. abs.Client.GetBookmarks reads GET /api/me/bookmarks/{id}. The response is {"bookmarks": [...]} (getBookmarksForLibraryItem in MeController.js). ABS has this route from 2.36.0. An older ABS has no handler for the path and answers 404. GetBookmarks then reads GET /api/me and keeps the bookmarks of the item. fetchBookmarksCmd calls GetBookmarks of the cache client. Its cache key is bookmarks:{id}, which the bookmark commands already remove after each change. The special case for a 404 is gone.
• Fault 2. handlePlayerReady sends SetSpeedCmd with the speed of the model, as spruce does.
• Fault 3. The Player interface has the new method GetIdle. Mpv.GetIdle reads the mpv property core-idle. A position tick reads it into the new field PositionMsg.Idle. A tick of a player that is paused or idle adds no listening time. The footer and MPRIS still show the pause state only.
• Fault 4. main calls the new function ui.RunProgram. At SIGHUP it calls p.Quit. p.Run then returns the final model, and main calls Cleanup.

Decisions:

• Fault 1: the fallback for an older ABS is on the status 404 of the new route. From 2.36.0, ABS answers 404 there only for an item that it does not have. pine then sends one more request, and gets no bookmarks.
• Fault 2: the speed goes to the player after the launch, not as a launch option. A launch option needs a change of Player.Launch. The probe below shows that mpv accepts the speed before the file is open.
• Fault 3: the tick reads core-idle, not paused-for-cache. core-idle is also true while mpv starts or seeks, and in that time no audio plays. The mpv manual says about core-idle: "it's only no/false if there's actually video playing". The probes below show the same for a file with audio only.
• Fault 3: a stall can lose up to 1 second of listening time, as a pause does: the time before the tick that reports it, and the time before the first tick after it.
• Fault 4: ui.RunProgram uses p.Quit, so the end of the program is the end of a quit with q.

How it was tested

Each fault has a test that failed before its correction.

Fault	Tests	Failure before the correction
1	TestGetBookmarksHTTP, TestGetBookmarksReadsUserBeforeABS236 (internal/abs), TestE2E_BookmarkFetchOnDetailEntry (internal/app)	bookmarks_test.go:41: expected 2 bookmarks, got 0 and e2e_test.go:657: detail bookmarks = 0, want 2
2	TestNewPlaybackGetsSpeedOfModel (internal/app): two sessions	playback_test.go:324: speed of the player in the session sess-first = 1, want 1.5
3	TestStalledPlayerAddsNoListeningTime (internal/app)	progress_sync_test.go:808: timeListened = 1 after one second of a player that waits for data, want 0
4	TestRunProgramReturnsAtHangup (internal/ui/program_unix_test.go). Its first name was TestHangupRunsCleanup in main_unix_test.go.	main_unix_test.go:79: after SIGHUP, the process printed [] and ended with the error signal: hangup, want the line cleanup and no error

Notes on the tests:

• Fault 1: the server of TestGetBookmarksHTTP answers as ABS does: the progress of the item has no bookmarks. The fake server of the E2E tests now has the bookmark route, and its progress route for a GET is gone. Three more E2E tests of the bookmarks failed with that server before the correction. TestClient_GetBookmarks_CacheHit (internal/cache) is new. The app did not use this cache path before, and the test checks it with a real store, also for an item with no bookmarks.
• Fault 2: the mock player of the app tests now has a speed. Its Launch sets the speed 1.0, as a new mpv handle.
• Fault 3: the test gets its tick from player.TickAfterCmd, with a mock player that is idle and not paused. TestListeningTimeIsTimeOfPlay has the new case "one minute of a player that waits for data". TestMpvIsIdleWhileItWaitsForData (internal/player) uses the real libmpv: its server sends 100 seconds of a file of 120 seconds, and then nothing. With GetIdle on the property pause, that test fails with paused = false, idle = false at the position 99.6.
• Fault 4: the test starts the test program again as a child process. The child runs a Bubble Tea program as main does, and prints a line at the place where main calls Cleanup. The test does not run on Windows.
• Fault 4: the first version of the test had a second case for SIGTERM. On macOS, that case passed before the correction and after it. In CI on Linux it failed: the child ended by SIGTERM. Bubble Tea asks for SIGINT and SIGTERM in a goroutine that p.Run starts, and the test sent the signal at the first line of the program. Inference: the signal came before that goroutine asked for it. The case is not in the test, because its result depends on that timing.

Probes that are not in the repository:

• libmpv 0.41.0, ao=null, a stream that stops after 40 seconds of data: pause=false paused-for-cache=true core-idle=true, and time-pos stays at 39.936.
• libmpv 0.41.0, the default audio output, a silent file: core-idle is false while the file plays, and true after a pause.
• libmpv 0.41.0: the speed is 1.5 after SetSpeed(1.5), and 1 after the next Launch. SetSpeed(8) directly after Launch gives the speed 8, while time-pos is not available yet.
• The pine program in a pseudo-terminal (Python pty), with an empty config folder, at the login screen. After the close of the pseudo-terminal, the program of 279e34f ended by SIGHUP, and its log has no line session ended. The program of this branch ended with the exit code 0, and its log has the lines terminal closed, quitting and session ended, 16 ms apart.

Checks on macOS 27.0 (Apple Silicon), on the last commit (19629b1):

• gofmt -l .: no file
• go build ./...: pass
• go vet ./...: pass
• golangci-lint run with version 2.14.0: 0 issues
• PINE_REQUIRE_LIBMPV=1 PINE_REQUIRE_DBUS=1 go test ./... -count=1: 28 packages pass
• go test -race ./... -count=1: 28 packages pass

Not tested

• No request went to a real ABS server. The statements about ABS come from its source.
• No ABS before 2.36.0 ran. The fallback comes from the source at the tag v2.35.1: server/routers/ApiRouter.js has no route for the bookmarks, and server/Server.js with the default client has no handler for an unknown path under /api. The tag v2.36.0 is the first tag whose ApiRouter.js has the route.
• No session played when the pseudo-terminal closed, and no window of a terminal program closed. The path from Cleanup to the server has the tests TestQuitWaitsForLastPosition and TestQuitSendsLastPositionOnce.
• On this machine, the libmpv test of the stall ran only with libmpv 0.41.0. In CI it passed in each of the six jobs, for example with libmpv 0.34.1 on Ubuntu 22.04.
• SIGTERM has no test. The statement about SIGTERM comes from the source of Bubble Tea 1.3.10, and from the SIGTERM case on macOS above. A SIGTERM in the first moment of the program can end the process before Bubble Tea asks for the signal. The program has no session at that time.
• Windows has no SIGHUP. The close of a console window on Windows was not examined.
• In the first CI run of this pull request, TestE2E_CacheSurvivesRestart failed on Windows with home should not be loading after first init. This pull request does not change that test or the home screen. The test passed on Windows in the second run. Its cause was not examined. Inference: a command of the test needed more than the 200 ms that feedCmd waits.
• The MPRIS server test is for Linux only. It did not run on macOS.
• spruce did not run in a terminal. Fault 12 below gives spruce the handler for SIGHUP.

Not changed

After the correction of fault 1, no code of the app calls GetMediaProgress. GetMediaProgress of internal/abs and of internal/cache, the type MediaProgressWithBookmarks with its key bookmarks, the cache key progress:{id}, and their tests are still in the repository. Their removal is a separate change.

Changes after the review

A review of this pull request found eight more faults. The commits below correct them. Three more commits remove two keys, remove a duplicate function, and correct two comments.

In the sections above, three names are new: quitOnHangup is now ui.RunProgram, the test of fault 4 is now TestRunProgramReturnsAtHangup, and the tick of the test of fault 3 comes from player.TickAfterCmd.

User-facing changes

• The keys n and N (next and previous chapter) are gone. The chapter list (C) and the command palette ("Next Chapter", "Previous Chapter") do that job.
• The config keys next_chapter and prev_chapter are gone. A config file that has them still loads, and the two keys have no effect. pine wrote the two keys to each config file that it made.
• b on the detail screen makes a bookmark only for the item that plays. For a different item, pine shows an error.
• ctrl+p does not open the command palette while a text input or a dialog gets the keys.

Put these changes in the release notes.

What was wrong

5. A text input lost keys. Update in internal/app/model.go gave some keys a global function or a playback function before the screen got them. A guard (isFiltering) held back these keys only while a list filter was active.
   • In the bookmark title editor, q quit the program, ? opened the help overlay, esc went to the previous screen, and the playback keys (n, l, h, p, space, S, +, -, [, ], >) did their function. The correction of fault 1 made this editor usable: before, the list had no bookmarks from the server.
   • On the login screen, ? opened the help overlay, and a typed C had no effect.
   • In the metadata editor, ? opened the help overlay.
   • In the confirmation dialog of the detail screen, q quit the program.
   • While the error banner was visible, the banner took the first key. While the chapter list was open, the list took j, k, H, L, and enter.
6. The detail screen showed the bookmarks of a different item. BookmarksUpdatedMsg had no item ID. The answer for an item can come after the user opened a different item. Before the correction of fault 1, this fault had no effect: the server gave no bookmarks.
7. For ABS before 2.36.0, each read of the bookmarks sent two requests. The first request got the expected 404, and abs.Client wrote an error line for it to the log.
8. The test of fault 4 passed also when main did not start the handler for SIGHUP. The test called quitOnHangup itself, and main called it in a line of its own.
9. b made a bookmark on an item that did not play. handleAddBookmark used the position of the player, and did not compare the item of the detail screen with the item that plays.
10. The left arrow closed the bookmark title editor. The editor used the back keys of the detail screen (esc and the left arrow) to cancel the edit. The text was lost.
11. A pause state, a speed, or a volume that the player refused gave no error. SetPauseCmd, SetSpeedCmd, and SetVolumeCmd reported the failure as a position tick with the generation 0. pine and spruce ignore a tick that is not of the current playback.
12. spruce did no cleanup at the close of the terminal. The main of spruce had no handler for SIGHUP, as the main of pine before fault 4. spruce did not close its media cache and its players.

What changes

Commit	Change
2aa3a4f	The keys n and N and their config keys are gone.
b94a4a7	Fault 5. capturesKeys (before: isFiltering) is true for the login screen, the metadata editor, a list filter, the bookmark title editor, and the confirmation dialog of the detail screen. Update then gives the key to the screen, before the global keys, the chapter overlay, the error banner, and the playback keys. ? 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+c quits the program on each screen.
bdaef5b	Fault 6. The message has the ID of its item. The root model ignores the message if the detail screen shows a different item, as it does for EpisodesLoadedMsg and BookDetailLoadedMsg.
da8bc75	Fault 7. The client 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.
7f54726	Fault 8. One function starts the handler for SIGHUP and runs the program. main and the child process of the test call it. quitOnHangup is gone.
b9c9388	Fault 9. handleAddBookmark refuses the bookmark and shows the error that the command palette shows in this case.
6ffd9ef	Fault 10. Only esc cancels the edit. The left arrow moves the cursor.
ab8d962	Fault 11. The three commands return the new player.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.
a92ed6d	player.TickCmd and tickCmd of the app had the same body. player.TickAfterCmd has the body now, and tickCmd of the app is gone.
c7aa399	The comments of GetMediaProgress and MediaProgressWithBookmarks said that the progress has the bookmarks. They now say that it has none.
1588e30	Fault 12. The function of fault 8 is now ui.RunProgram in the shared package internal/ui. The main of pine and the main of spruce call it.

How it was tested

Fault	Tests	Failure before the correction
5	TestE2E_BookmarkTitleEditGetsEachKey, TestE2E_BookmarkTitleEditGetsKeysOfBannerAndChapterList, TestLoginCapturesGlobalKeys, TestMetadataEditorCapturesHelpKey, TestDetailConfirmCapturesQuitAndBackKeys, TestHelpKeyClosesHelpOnLoginScreen, TestHintsShowNoKeyThatTheScreenCaptures (internal/app)	For example: screen = Home, want Detail (esc in the editor), server bookmark title = "Oldk", want Oldjk (error banner), server field = "", want "?Cq" (login screen), after ?, the help overlay is open
6	TestE2E_DetailIgnoresBookmarksOfOtherItem (internal/app)	detail of item-002 has the bookmarks [{... Title:Bookmark of the first item ...}], want item-002 and no bookmarks
7	TestGetBookmarksReadsUserBeforeABS236 (internal/abs)	requests = map[/api/me:2 /api/me/bookmarks/li-001:2], want 1 for the bookmark route and 2 for the user
8	TestRunProgramReturnsAtHangup (internal/ui)	With no handler in the function: after SIGHUP, the process printed [] and ended with the error signal: hangup
9	TestE2E_NoBookmarkForItemThatDoesNotPlay (internal/app)	1 bookmark requests for an item that does not play, want 0: [{Method:POST Path:/api/me/item/item-002/bookmark Body:map[time:100 title:Bookmark at 1:40]}]
10	TestLeftKey_MovesCursorWhileEditingBookmark (internal/screens/detail)	after the left arrow, the bookmark editor is not open
11	TestStateCommandsReportFailure (internal/player), TestPlayerCommandErrorShowsErrorBanner (internal/app and internal/spruce/app)	the command returned player.PositionMsg{...}, want CommandErrMsg with the error of the player and the error banner is not visible
12	TestRunProgramReturnsAtHangup (internal/ui)	The test is for the function that the two main functions call. No test runs the main of spruce.

Notes on the tests:

• Fault 5: the esc case of TestDetailConfirmCapturesQuitAndBackKeys and the ctrl+p check of TestLoginCapturesGlobalKeys passed before the correction. They cover behaviour that had no test.
• Fault 6: the tests of the three bookmark commands check that each message has the ID of its item.
• Fault 9: three tests of handleAddBookmark had a session and no item that plays. They now have the item.
• The keys n and N: three chapter tests now use the palette actions. TestE2E_ChapterNavigation checks that the palette offers the two actions.
• The tick command: TestStalledPlayerAddsNoListeningTime had one case for each of the two functions. It now has one case.

Checks on Linux, on the last commit (1588e30):

• gofmt -l .: no file
• go vet ./...: pass
• golangci-lint run with version 2.14.0: 0 issues
• PINE_REQUIRE_LIBMPV=1 PINE_REQUIRE_DBUS=1 go test ./... -count=1: 855 tests pass
• The same command with -race: pass
• GOOS=windows go build ./... and GOOS=darwin go build ./...: pass
• The two checks of scripts/release.sh for the packages of each binary: pass

Not tested

• pine and spruce did not run in a terminal for these commits. The tests send messages to Update and read the view functions.
• No test runs the main of pine or the main of spruce: a main needs a terminal. A main that calls p.Run and not ui.RunProgram fails no test.
• No ABS before 2.36.0 ran for fault 7. The test server answers 404 for the bookmark route.
• Fault 11: no test makes libmpv refuse a value. The tests use a player that fails each of the three calls.
• In the CI run of 7f54726, TestE2E_LoginFailure failed on Windows with expected login error after failed attempt, after 0.22 seconds. The job passed in the second run of the same commit. The test asks an address that has no server, and feedCmd waits 200 ms for the answer. Inference: the refusal came later than 200 ms. This pull request does not change that test.

Not changed

• If the chapter list is open and the user then opens the bookmark title editor, the keys go to the editor, and the list stays above it until the editor closes.
• SeekCmd and SeekRelativeCmd report a failure as a position tick with the generation 0. After a seek that fails, the next position tick has the position of the player.

🤖 Generated with Claude Code