Repository navigation
Conversation
…ookmark edit/delete
Auto Sleep Timer, Global Speed Control and Progress Bar Seeking were saved
and shown but read by no playback code. All three now behave as on iOS:
- Auto Sleep Timer re-arms the timer the user last set on every
user-initiated play (never on the autoplay advance into the next book).
The last value lives in a new pref using the
deep-link encoding (-1 off, -2 end of chapter, else seconds); an explicit
Off is remembered, a natural expiry is not.
- Global Speed Control off (the default) means per-book speed. Books gain a
nullable column (Room 10 -> 11), written on every speed change for
the loaded book, sent as on metadata updates and merged back from
fetches (server null keeps local). A book without its own speed falls
back to the last-used speed so nothing snaps to 1x on update.
- Progress Bar Seeking off withholds the seek commands from OS controllers
(notification/lock screen, Auto, Bluetooth) and re-sends them live when
the toggle flips; the in-app controller is never gated.
Bookmarks now sync both ways. GET /v1/library/bookmarks was declared with
the wrong response shape and never called; it returns {bookmarks: [...]}.
Opening the Bookmarks sheet pulls and merges the server list (match on
whole seconds, server note wins, nothing local deleted, skipped while sync
tasks are pending). The sheet gains a pencil to edit the note and an
end-to-start swipe to delete after confirmation, both also exposed as
accessibility actions.
Also fixed while here: the sheet's bookmark collector was launched inside
collectLatest but parented to the outer scope, so every book played kept
a live collector and a stale one could overwrite the list with a previous
book's bookmarks; note edits mutated the listed entity in place, which made
the StateFlow drop Room's re-emission. Android Auto browse-play now goes
through shouldRestartFromZero (behavior unchanged: a manual tap always
restarts a finished book).
Mirror the iOS feature/videoplayer settings under Player Controls → Video
Playback:
- Continue Video Audio in Background (default on). Off pauses a playing
video when the whole app leaves the foreground (ProcessLifecycleOwner,
so a picker or the Auth Tab covering the activity doesn't count).
- Picture in Picture (default off, needs the first). Leaving the app from
the player while a video plays shrinks it into a PiP window: auto-enter
on Android 12+, onUserLeaveHint before; aspect ratio from the video,
clamped to the platform range; rewind / play-pause / forward actions via
a non-exported broadcast receiver. In PiP the player renders only the
video (fullscreen layout, overlay controls hidden).
Video detection moves to :core (VideoTracks) so PlaybackManager can expose
hasVideo for the activity. Manifest: supportsPictureInPicture plus the
configChanges PiP needs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (width <= 0 || height <= 0) return null | ||
| val ratio = width.toDouble() / height | ||
| val clamped = ratio.coerceIn(1 / MAX_RATIO, MAX_RATIO) | ||
| return if (clamped == ratio) width to height else (clamped * 10_000).roundToInt() to 10_000 |
There was a problem hiding this comment.
🟡 WARN — Clamped tall ratio is still outside the range Android accepts, so the app can crash. 1 / 2.39 = 0.418410…; (0.418410 * 10_000).roundToInt() gives 4184, and 4184/10000 = 0.4184 is below the platform minimum (config_pictureInPictureMinAspectRatio ≈ 0.41841). setPictureInPictureParams / enterPictureInPictureMode then throw IllegalArgumentException("Aspect ratio is too extreme"). MainActivity.applyPictureInPictureParams and onUserLeaveHint only catch IllegalStateException, so this crashes the collector in lifecycleScope. It is rare (videos taller than 1:2.39), and OEMs can also configure narrower ranges.
Fix: round toward the inside of the range (ceil when clamping to the min, floor for the max), or clamp to 1 / MAX_RATIO + 1e-4. Also catch IllegalArgumentException next to IllegalStateException in both MainActivity call sites. Update the test that currently pins 4_184 to 10_000.
| val toInsert = mutableListOf<BookmarkEntity>() | ||
| val toUpdate = mutableListOf<BookmarkEntity>() | ||
| val seenSeconds = mutableSetOf<Long>() | ||
| for (row in remote) { |
There was a problem hiding this comment.
🟡 WARN — Deleted bookmarks may come back. Deleting a bookmark doesn't remove it on the server: DeleteBookmarkProcessor sends setBookmark with "active" to false. The KDoc on BookmarksResponse says each row carries {title, key, note, time, active}, but SyncableBookmark doesn't map active and plan() inserts every row that has no local match. If GET /v1/library/bookmarks returns soft-deleted rows, a bookmark the user deleted is inserted again as a USER bookmark the next time the list opens (the queue-empty guard only protects the moment before the delete uploads).
Fix: add @SerializedName("active") val active: Boolean? = null to SyncableBookmark and skip active == false rows in plan(), with a test case for it. Or confirm the endpoint only returns active rows and say so in the KDoc.
| if (!remoteSeekEnabled && !isInAppController(session, controller)) { | ||
| builder | ||
| .remove(Player.COMMAND_SEEK_IN_CURRENT_MEDIA_ITEM) | ||
| .remove(Player.COMMAND_SEEK_TO_MEDIA_ITEM) |
There was a problem hiding this comment.
🟡 WARN — Progress Bar Seeking off removes more than the scrubber. Only COMMAND_SEEK_IN_CURRENT_MEDIA_ITEM backs the lock-screen/notification scrubber (legacy ACTION_SEEK_TO). Removing COMMAND_SEEK_TO_MEDIA_ITEM as well also turns off Media3's ACTION_SKIP_TO_QUEUE_ITEM. So with the setting off, Android Auto (and any other queue-showing controller) can no longer jump to a chapter from the queue: each chapter is its own window in chapter-context mode. iOS's setting only toggles changePlaybackPositionCommand, the scrubber.
Fix: gate only COMMAND_SEEK_IN_CURRENT_MEDIA_ITEM and leave COMMAND_SEEK_TO_MEDIA_ITEM available so chapter/queue navigation keeps working.
| return BookmarkSync.pull(delegate, syncTaskRepository, item) | ||
| } | ||
|
|
||
| override suspend fun updateItemSpeed(uuid: String, speed: Double) { |
There was a problem hiding this comment.
🟡 WARN — New repository sync behavior has no tests. The test fakes only stub updateItemSpeed and syncBookmarksFromCloud. Nothing checks that:
updateItemSpeedwrites the row and enqueues an update task only when subscribed, with the freshspeedin the payload;syncBookmarksFromCloudno-ops when not subscribed and merges through the plain delegate, so noset_bookmarkechoes are created;BookmarkSync.pullskips when the sync queue is non-empty and bails when the book was deleted mid-request.
BookmarkSync.plan is well covered, but these gates are where regressions would silently duplicate tasks or overwrite local edits. Add a SyncingLibraryRepositoryTest case for each (the fake task repo already exists there).
| <string name="player_bookmarks_title">Bookmarks</string> | ||
| <string name="player_add_bookmark">Create bookmark</string> | ||
| <string name="player_bookmarks_manual">Manual</string> | ||
| <string name="player_bookmark_edit_note">Edit note</string> |
There was a problem hiding this comment.
🔵 INFO — The 7 new strings (player_bookmark_edit_note, player_bookmark_delete, player_bookmark_delete_title, player_settings_video_*) exist only in values/. All 10 values-* locales (which already translate neighbouring keys such as player_settings_use_chapter_context) will fall back to English for the new Video Playback section and the bookmark actions. Consider adding the translations or tracking them in a follow-up.
🟡 Claude PR Review —
|
This branch was successfully deployed
1 participant
Purpose
Bring video playback back and make the player settings that were showing but doing nothing actually work.
Audio in Background (default on) and Picture in Picture (default off).
Bar Seeking off hides the lock-screen scrubber.
Bugfix
The bookmark list could show a previous book's bookmarks after an edit, and an edited note didn't refresh until something else changed. Both fixed.
Related tasks
feature/videoplayerbranch and the existing iOS speed, sleep-timer and bookmark behavior.Approach
speedcolumn to library items (Room migration 10 → 11) that rides along with the existing metadata sync.covering the screen.