Skip to content

Lay the player out side by side in landscape - #1639

Merged
GianniCarlo merged 1 commit into
developfrom
feat/player-landscape
Oct 12, 2026
Merged

GianniCarlo merged 1 commit into
developfrom
feat/player-landscape

Conversation

@GianniCarlo

Copy link
Copy Markdown
Collaborator

Problem

In landscape the player kept its portrait layout squeezed into a short screen. On an iPhone 17 Pro the artwork shrank from 330 pt to 92 pt (130 pt on a Pro Max), a book without artwork showed its title and author cut to "Lon…" / "Unk" with the AirPlay button over them, and swipe-to-close needed about two thirds of the screen's height.

Changes

  • PlayerView: on a short screen (verticalSizeClass == .compact: every iPhone in landscape) the artwork sits on the left and the chapter row, progress and playback controls on the right, starting at the artwork's top. iPad and portrait keep the one-column layout.
    • The switch is an AnyLayout (HStackLayout / VStackLayout) around the existing artwork and controls, so rotating moves the views instead of rebuilding them.
    • The artwork is the largest square that fits above the button row, 40 pt from the controls; the button row stays across the bottom, 16 pt below the artwork.
    • Each half is its own accessibility container, so VoiceOver reads the artwork's elements before the controls instead of mixing them by position.
    • Swipe-to-close needs half the distance side by side (the screen is about half as tall). The drag counts half the finger's travel (the view follows the finger), so that's about 132 pt of travel, roughly the share of the screen it takes in portrait.
  • MediaActionRow: an optional buttonSpacing. Side by side the five buttons stay together in the middle with 24 pt gaps instead of spreading across the full width. In the right column alone they didn't fit: a running sleep countdown was cut to "5…" at 1×, and gone with 1.25×.
  • ArtworkView: an alignment for the square when it's narrower than its space (trailing side by side, so it sits next to the controls).
  • NavigationRowView: keepsTitleHeight. Side by side the title no longer gets a fixed 56 pt row (the row is the chevrons' 48 pt), so a one-line title doesn't start further below the artwork's top.
  • PlayerJumpView: the frame is 56 pt tall, the icon's height; at 48 the icon spilled 4 pt past its frame. Portrait doesn't move: the row sits between flexible spaces.

Testing

  • Simulators (Xcode 27.1): iPhone 17 Pro, iPhone 17 Pro Max, iPhone SE (3rd generation) on iOS 27.0; iPad Air 11" on iOS 27.0.
    • Portrait: every player element's frame matches develop on the iPhone 17 Pro Max (hierarchy diff).
    • Landscape, both sides: artwork 310 pt on the Pro Max, 265 pt on the SE; nothing clipped, cut or overlapping.
    • Longest labels (1.25× and a one-hour sleep countdown): the row shows "1.25×" and the countdown in full on the Pro, Pro Max and SE.
    • iPad: one column in both orientations, frames unchanged.
    • VoiceOver order (landscape): artwork, then chapter row and controls, then the button row.
    • iPhone 17 Pro: artwork 272 pt, 40 pt from the controls; the custom sleep timer popover stayed open through a rotation and re-anchored to the moon; the speed sheet stayed open; a 100 pt downward drag snaps back, 170 pt closes the player.
  • Device: iPhone 17, iterated on the layout in both orientations.

Not covered: iPhone Duo. Its simulator never rotated the closed (outer) display, so the side-by-side layout there is untested; Apple documents the outer display as rotating like any iPhone.

Known and left as is (also on develop): on the iPhone SE, which has no home indicator, the button row sits on the screen's bottom edge.

In landscape the player squeezed its portrait layout into a short screen:
the artwork shrank to under a third of its size, a book without artwork had
its title and author cut, and swipe-to-close needed most of the screen.

On a short screen (vertical size class compact, every iPhone in landscape)
the artwork now sits on the left, as large as fits above the button row,
with the chapter row, progress and playback controls on the right. The
button row stays across the bottom, its buttons kept together in the middle.
An AnyLayout switches between the two, so rotating moves the views instead
of rebuilding them. Swipe-to-close needs half the distance side by side.
ProgressLabelsSectionView()
StartupPlayerSectionView()
VideoPlaybackSectionView()
// VideoPlaybackSectionView()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 WARN — Commenting out VideoPlaybackSectionView() has nothing to do with the landscape layout, and the PR description doesn't mention it. It hides the "Show Video" and "Picture in Picture" toggles. Anyone who already turned them on can't turn them off, and VideoPiPCoordinator.isEnabled still reads videoPictureInPictureEnabled. It looks like leftover debugging. Restore the line, or if hiding video is intended, remove the feature in a separate PR instead of leaving commented-out code.

@github-actions

Copy link
Copy Markdown

🔴 Claude PR Review — FAIL

The PR lays out the iPhone player side by side in landscape: an AnyLayout HStack/VStack around the artwork and controls, a smaller dismiss threshold, fixed spacing in MediaActionRow, an alignment on ArtworkView, a keepsTitleHeight toggle on NavigationRowView, and a 56 pt PlayerJumpView frame. The layout work looks sound. Concurrency, player and AVAudioSession lifecycle, CoreData and entitlements are untouched; it's view-layer only. Each half gets its own accessibility container, MediaActionRow keeps its labels and values, and the if inside its ForEach builds plain View content, so the iOS 27 builder-conformance crash doesn't apply. Blocking: the diff also removes video playback from the player, which the description never mentions. The isVideoItem → VideoArtworkView branch and the refreshVideoState() call are gone, and the Settings ▸ Video section is commented out. Merging as is ships a regression with dead video code still in place.

Findings: 1 error · 1 warn

Findings not visible inline (no line in this diff, beyond the 25-comment cap, a comment the API refused, on a thread that could not be reopened, or on one a maintainer had the last word on)
  • 🔴 BookPlayer/Player/Views/PlayerView.swift:173 — This silently removes video playback. In the old code the artwork was if viewModel.isVideoItem { VideoArtworkView() } else { ArtworkView(...) }. Now it's always ArtworkView, and viewModel.refreshVideoState() was also dropped from .onAppear (old line 148). The PR description doesn't mention either change.

The effects:

  • PlayerViewModel still publishes isVideoItem, but nothing reads it.
  • VideoArtworkView, the fullscreen presenter and PiP hosting (VideoPlayerSurface(hostsPictureInPicture: true)) are now unreachable.
  • A user who turned on "Show Video" gets only artwork for video chapters, with no PiP and no fullscreen.

Fix: keep the branch inside the new layout { VStack { ... } }:

Group {
  if viewModel.isVideoItem { VideoArtworkView() }
  else { ArtworkView(..., alignment: artworkAlignment) }
}
.simultaneousGesture(...)

Also restore viewModel.refreshVideoState() in .onAppear. VideoArtworkView ends in .frame(maxWidth: .infinity, maxHeight: .infinity) with no alignment, so give it the same alignment so it sits against the controls side by side. If removing video is intentional, put it in its own PR and delete the dead code.

Model claude-opus-5-5 · run log · 1 new · 0 carried over · 0 resolved · advisory (a human should still review). Findings are de-duplicated across pushes; an earlier finding closes only when the verification pass judges it against the current code — fixed, no longer applicable, accepted by a maintainer, or a duplicate of a finding reported on this push.

@GianniCarlo
GianniCarlo merged commit 6479a75 into develop Oct 12, 2026
3 checks passed
@GianniCarlo
GianniCarlo deleted the feat/player-landscape branch October 12, 2026 14:08

This branch was successfully deployed

1 active deployment
reviewer — 1230905b Deployed Oct 12, 2026 by GianniCarlo via review #254
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