Skip to content

fix(video): surface a full-screen toggle on the macOS player - #3

Merged
tsvb merged 2 commits into
mainfrom
macos-video-fullscreen-toggle
Aug 10, 2026
Merged

fix(video): surface a full-screen toggle on the macOS player#3
tsvb merged 2 commits into
mainfrom
macos-video-fullscreen-toggle

Conversation

@tsvb

@tsvb tsvb commented Aug 10, 2026

Copy link
Copy Markdown
Owner

What

One line in AppleNugs/macOS/VideoPlayerSurface.swift:

view.showsFullScreenToggleButton = true

…plus a correction to the doc comment above it.

Why

The Mac video surface had no full-screen affordance at all. AVPlayerView.showsFullScreenToggleButton is documented as Default is NO (AVPlayerView.h:198-201), and VideoPlayerSurface never set it — so AVKit drew no full-screen control, and NSWindow.toggleFullScreen is never called anywhere either. The button wasn't hidden; it was never enabled.

That matters because the inline 16:9 box lives in the NavigationSplitView detail column, squeezed between the sidebar, the TransportBar in a bottom safeAreaInset, and a 340pt inspector — so a concert plays in a roughly 480pt letterbox with no way to escape it.

The stale doc comment is the second half of the fix. It asserted that AVPlayerView "supplies the native scrubber, volume, fullscreen, AirPlay, and (free) Picture-in-Picture" — a false claim that made the gap invisible to anyone reading the file.

Behavior

All from AVKit, no app-drawn chrome:

  • Full-screen button in the floating control bar (revealed on hover over the video).
  • Same button flips to a shrink icon in full screen — one control enters and exits.
  • Esc also exits, matching Mac-standard expectations.

An always-visible custom button was considered and rejected: neither AVKit surface exposes a programmatic full-screen entry point, so it would have required the unrelated NSView.enterFullScreenMode mechanism — a second full-screen path that AVKit's own state doesn't know about.

iOS is untouched. AVPlayerViewController already draws its expand button inline and already carries a full AVPlayerViewControllerDelegate + OrientationGate rotation contract (AppleNugs/iOS/VideoPlayerSurface.swift:39-72).

Verification

  • AppleNugs (macOS) and AppleNugs-iOS both build clean with fresh -derivedDataPath under SWIFT_STRICT_CONCURRENCY=complete. Only warning is the pre-existing appintentsmetadataprocessor one.
  • AppleNugsTests: 81/81 pass.
  • ⚠️ Not visually confirmed. No unit test is possible (AVKit view config, no pure logic; the test target is host-free), and the usual swiftc + NSHostingView harness is blind here: AVKit renders .floating controls out of process. In a real, visible, hovered window the AVPlayerView subtree is only AVDesktopPlayerViewContentView → …ContentContainerView → …VideoContentView → …AnalysisPlayerLayerView — no control bar, no child windows, AXUnknown with zero children — and toggling the property changes the in-process tree not at all.

Reviewer notes

Worth eyeballing in a real build, since these only surface live:

  1. Playback across the transition. VideoDetailView.swift:68 is .onDisappear { app.video.stop() }, and stop() relinquishes Now Playing and resumes the audio queue. AVKit reparents the view into its own window while the SwiftUI tree stays mounted, so this shouldn't fire — but it's the one thing that would break the feature outright.
  2. Layout on exit. The surface carries .aspectRatio(16:9) and .clipShape(RoundedRectangle(cornerRadius: 8)); confirm no clipped corners in full screen and a correct inline box afterward.
  3. Overlays. liveBadge and the loadError view are SwiftUI siblings, not AVPlayerView subviews, so they won't appear in full screen (fine — AVKit shows its own chrome). Confirm the badge returns on exit.

If 1 or 2 misbehave, the fix is an AVPlayerViewDelegate coordinator (macOS 12+) guarding the teardown via playerViewWillEnterFullScreen / playerViewDidExitFullScreen — the same shape the iOS surface already uses. Not written speculatively.

🤖 Generated with Claude Code

Tim and others added 2 commits August 9, 2026 22:33
AVPlayerView.showsFullScreenToggleButton defaults to NO, and VideoPlayerSurface
never set it, so the Mac video surface had no full-screen affordance at all —
concerts played in a ~480pt letterbox wedged between the sidebar, the transport
bar, and the 340pt inspector, with no way out.

Turning it on gets AVKit's own control: it appears in the floating control bar,
flips to a shrink icon to exit, and Esc exits too. No app-drawn chrome, which
would have needed the unrelated NSView.enterFullScreenMode path since
AVPlayerView exposes no programmatic full-screen entry point.

Also correct the doc comment, which asserted AVPlayerView "supplies the native
scrubber, volume, fullscreen, ..." — that false claim is why the gap went
unnoticed. iOS is untouched: AVPlayerViewController already draws its expand
button and already carries the delegate + OrientationGate rotation contract.

Co-Authored-By: Claude Opus 5 <[email protected]>
Review follow-up on the full-screen toggle. When a video plays to the end,
the didPlayToEndTime handler relinquishes the arbiter, which calls
audio.endExternalPlayback(resume:) and restarts the audio queue if it had been
playing. Inline that is fine — the app is visible and the change is legible.
Under AVKit's full-screen window it is not: music starts with no visible UI and
no obvious cause, behind a surface the viewer can't see past.

AVPlayerView offers no way to fix this the way iOS does. It has no method to
enter or exit full screen and no property reporting the state — only the four
AVPlayerViewDelegate notifications — so there is no macOS equivalent of iOS's
exitsFullScreenWhenPlaybackEnds, and a video that ends full screen leaves the
viewer there until they press Esc. What we can control is where the hand-back
happens, so track full-screen state via the delegate and defer it to the exit.

The deferral is skipped if the viewer replayed the video while still full
screen, since that re-claims the arbiter and is playing again; stop() clears
the pending flag so a dismissal can't leave one armed.

Co-Authored-By: Claude Opus 5 <[email protected]>
@tsvb

tsvb commented Aug 10, 2026

Copy link
Copy Markdown
Owner Author

Review follow-up folded in (e39222d)

Addresses finding 1 from review — but not the way the review proposed, because that turned out to be impossible.

The correction

The review suggested exiting full screen on end-of-item, mirroring iOS's exitsFullScreenWhenPlaybackEnds. AVPlayerView can't do that. The full header exposes no method to enter or exit full screen and no property reporting the state — only the four AVPlayerViewDelegate notifications (playerViewWill/DidEnter/ExitFullScreen). There is no macOS equivalent, so a video that ends full screen leaves the viewer there until they press Esc. That half of the finding is unfixable with public API.

What was fixed

The half that actually causes harm. didPlayToEndTimeNotification calls relinquishArbiter()audio.endExternalPlayback(resume:), which restarts the audio queue if it had been playing before the video. Inline that's legible; under a full-screen window it means music starting with no visible UI and no obvious cause.

So the surface now adopts AVPlayerViewDelegate and reports transitions to VideoPlayerService, which defers the hand-back to the full-screen exit:

  • VideoPlayerService.setFullScreen(_:) + isFullScreen / relinquishOnFullScreenExit
  • End-of-item: if isFullScreen { relinquishOnFullScreenExit = true } else { relinquishArbiter() }
  • Deferral is skipped if the viewer replayed the video while still full screen — the rate observer re-claims the arbiter, so relinquishing on exit would kill live playback. Guarded with && !isPlaying.
  • stop() clears the pending flag so a dismissal can't leave one armed.

The coordinator uses @Environment(AppModel.self) rather than a new init parameter, deliberately: VideoDetailView is shared, and the two platforms' surfaces intentionally keep the same VideoPlayerSurface(player:) signature so it stays that way. iOS is still untouched.

Also softened the doc comment per finding 2 — it no longer asserts the shrink-icon and Esc behaviors I couldn't verify.

Verification

  • Both schemes build clean, fresh -derivedDataPath, SWIFT_STRICT_CONCURRENCY=complete. 81/81 tests pass.
  • The deferral state machine is not unit-tested: VideoPlayerService pulls in PlayerService, NugsClient, and VideoProgressStore, and AppleNugsTests is host-free with files compiled in directly — wiring that graph in is a bigger change than the fix.
  • Still needs the live check. New case worth exercising: let a VOD play to the end in full screen with the audio queue previously playing, confirm nothing starts underneath, then exit and confirm audio resumes.

@tsvb
tsvb merged commit e39222d into main Aug 10, 2026
1 check passed
@tsvb
tsvb deleted the macos-video-fullscreen-toggle branch August 10, 2026 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant