Feature/videoplayer#1572
Open
Hirobreak wants to merge 13 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Adds video playback as a rendering layer on top of the existing shared AVPlayer, including an inline video surface in the player artwork area, a custom fullscreen experience, and Picture in Picture / background-audio behavior controlled via new Settings toggles.
Changes:
- Detect video chapters via UTType conformance and switch the player UI from artwork to an inline video surface + fullscreen presenter.
- Add settings + UserDefaults keys for “background audio for video” and “Picture in Picture”.
- Introduce a new
VideoPlayerView.swiftimplementation (surface, PiP coordinator, fullscreen controller) and expose the underlyingAVPlayerfromPlayerManager.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Shared/CoreData/Lightweight-Models/PlayableChapter.swift | Adds isVideo computed property via UTType to detect video chapters. |
| Shared/Constants.swift | Adds UserDefaults keys for video background audio + PiP. |
| BookPlayer/Settings/Sections/PlayerControls/VideoPlaybackSectionView.swift | New settings section with toggles for background audio and PiP. |
| BookPlayer/Settings/Sections/PlayerControls/SettingsPlayerControlsView.swift | Wires the new video playback settings section into the controls settings screen. |
| BookPlayer/Player/Views/VideoPlayerView.swift | New inline surface + PiP coordination + fullscreen controller/presenter. |
| BookPlayer/Player/Views/PlayerView.swift | Replaces artwork with video surface when the current item is video. |
| BookPlayer/Player/ViewModels/PlayerViewModel.swift | Tracks isVideoItem, presents fullscreen, and wires PiP close behavior to resume audio-only playback. |
| BookPlayer/Player/PlayerManager.swift | Exposes underlying AVPlayer for video rendering. |
| BookPlayer/en.lproj/Localizable.strings | Adds new strings for video settings + fullscreen. |
| BookPlayer/Base.lproj/Localizable.strings | Adds same new strings to Base localization. |
| BookPlayer.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved | Updates Kingfisher dependency version/revision. |
| BookPlayer.xcodeproj/project.pbxproj | Adds new Swift source files to the Xcode project build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+348
to
+350
| .opacity(controlsVisible ? 1 : 0) | ||
| .allowsHitTesting(controlsVisible) | ||
| } |
Comment on lines
+12
to
+15
| "settings_video_background_playback_title" = "Continue Video Audio in Background"; | ||
| "settings_video_background_playback_description" = "Keep listening to a video's audio when you leave the app or lock the screen."; | ||
| "settings_video_pip_title" = "Picture in Picture"; | ||
| "video_fullscreen_title" = "Fullscreen"; |
Hirobreak
force-pushed
the
feature/videoplayer
branch
from
July 16, 2026 20:43
e5ba82a to
d0a3c1b
Compare
- Reclaim inline video rendering when returning from fullscreen: the fullscreen surface shares the same AVPlayer and takes over its video output, and the inline layer's player reference is left untouched, so a plain reconnect is a no-op. Force a detach/reattach via a fullscreen-dismiss notification so the inline surface doesn't stay black. - Hide the fullscreen/AirPlay controls from VoiceOver while they're hidden; opacity(0) alone leaves them focusable in the accessibility tree.
GianniCarlo
force-pushed
the
feature/videoplayer
branch
from
July 19, 2026 20:35
d0a3c1b to
07a069b
Compare
VideoPiPCoordinator.didStop resumed audio-only playback whenever PiP stopped while backgrounded and no UI restore was in flight. But stop() is also called programmatically on book end and when switching to a non-video item, so a book ending or the queue advancing in the background would fire an unintended playerManager.play(). Track programmatic stops with a flag (mirroring isRestoringUserInterface) and skip the audio-only handoff for them; only a user-initiated PiP close resumes playback.
isStoppingProgrammatically was only reset in the didStop delegate callback. On a non-video switch the surface dismantles and release() nils the PiP controller/delegate in the same turn as stop(), which can beat the async didStop — leaving the flag stuck true on the singleton and misclassifying the next genuine user PiP close (playback fails to resume). Reset the flag in release() (controller teardown) and host() (fresh session) so it can't survive a controller lifecycle.
Consolidate the three duplicated 'path extension -> UTType' checks (PlayableChapter.isVideo and PlayerManager's getNextPlayableBook / getNextPlayableChapter) behind a single URL.fileType computed property. isVideo also stops routing through fileURL, which resolves (and creates) the processed folder just to read an extension. Behavior is unchanged: the dropped '!fileExtension.isEmpty' guard is redundant since UTType(filenameExtension: "") is already nil. Adds PlayableChapterTests covering video/audio extensions, case-insensitivity, nested and dotted paths, and missing extensions.
The video-extensions test asserted .mkv detects as video, which holds on macOS but not on the iOS simulator (whose UTType database doesn't classify .mkv as public.movie). AVFoundation can't play .mkv anyway. Assert only the reliably-classified, playable containers (mp4/mov/m4v).
Drop the 'Continue Video Audio in Background' setting. This is an audiobook app where every item already keeps playing in the background, so a video's audio now always continues there too (like any other item) instead of being user-gated. Removes the UserDefault + its registered default, the PlayerManager background-pause observer, and the background checks in the video surface and PiP coordinator — including the awkward 'PiP disabled unless background on' dependency. Picture in Picture stays as the single opt-in, gated only on device support. Adds a PiP footer description and removes the now-dead background-playback localization keys.
Add settings_video_pip_description (introduced in Base/en with the video settings simplification) to the remaining 24 locales, so the PR ships consistent translations for the new footer. Each matches its locale's existing 'Picture in Picture' term. Lokalise remains the source of truth.
- Add a 'Show Video' master toggle (off by default) gating video rendering: when off, video files play as audio and fall back to ArtworkView (restoring the cover + VoiceOver title/author label), and the Picture in Picture toggle is disabled. Aimed at the audiobook-first / VoiceOver audience. - PlayerViewModel gates isVideoItem on the setting and re-evaluates on player appear (refreshVideoState) so a settings change takes effect. - Move fullscreen presentation out of the view model: PlayerView calls VideoFullscreenPresenter directly (the VC stays imperative UIKit for its rotation animation); the VM just exposes playPause(). Keeps the VM free of UIKit/window access. - Move the video settings to the bottom of Player Controls; both toggles in a single section. - Localize the new Show Video strings across all locales; drop the now-unused PiP description string.
pull_request runs triggered from a fork don't receive repo secrets, so the reviewer's ANTHROPIC_API_KEY is empty and the step hard-fails. Gate the job on the PR head being the same repository, so fork PRs skip it (a neutral 'skipped' check) instead of a red failure. The review is advisory and never blocked merges.
- Declare PlayerViewModel.playerManager as concrete PlayerManager (the init already required it), dropping the redundant videoPlayerManager reference that existed only to reach getAVPlayer(). getAVPlayer stays off the protocol, keeping the raw-player escape hatch contained. - Replace the globally-mutable VideoPiPCoordinator.onClosedInBackground closure (set from the view model) with a .videoPiPClosedInBackground notification that PlayerManager (a lifetime object) observes to resume audio-only playback. Removes the shared-mutable-closure smell, and the resume no longer depends on the player screen's view model being alive.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Adds video playback support. When the current item is a video:
non-video items or playback ending dismiss it.
Sync, remote URLs, and downloads are untouched — video is only a rendering layer over the existing player.
Approach
Video renders through layers attached to the existing shared
AVPlayer— no changes to playback, sync, or the shared player protocol. All new UI lives inVideoPlayerView.swift(inline surface, fullscreencontroller, PiP coordinator), plus a new settings section, two user-defaults keys, and
PlayableChapter.isVideo(detected by file extension).Things to be aware of / Things to focus on
.mp4shows a black surface instead of artwork.Screenshots