[TV] Add a playlist details screen to the TV app - #5667
Conversation
|
Claude finished @sztomek's task in 2m 22s —— View job PR Review — TV Playlist Details ScreenNice, well-factored PR. The I left three inline comments. Summary below. Findings1. Episode-count inconsistency can blank the right pane ( 2. Initial focus on screen open ( 3. Minor — use Nits / observations (non-blocking)
No security concerns. The no-op |
| if (uiState.totalEpisodeCount == 0) { | ||
| NoEpisodes(modifier = Modifier.weight(1f).fillMaxHeight()) | ||
| } else { | ||
| EpisodeList( | ||
| episodes = uiState.episodes, | ||
| playAllFocusRequester = playAllFocusRequester, | ||
| modifier = Modifier.weight(1f), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Inconsistent episode counting can produce a blank right pane.
totalEpisodeCount is playlist.episodes.size (includes PlaylistEpisode.Unavailable), but uiState.episodes is playlist.episodes.toPodcastEpisodes(), which mapNotNulls away every Unavailable entry (see PlaylistEpisode.toPodcastEpisode() returning null).
For a manual playlist whose episodes are all currently unavailable (present in the playlist but no synced PodcastEpisode row):
totalEpisodeCount > 0→ this branch takes theelseand rendersEpisodeList(episodes = emptyList())→ an emptyLazyColumn, so the right pane is blank (no rows, no "No episodes" message).- Meanwhile
PlaylistInfoshowsepisodeSummaryText(episodes)= "0 episodes" and hides thePlay all episodesbutton.
So the header says 0 while the branch logic thinks it's non-empty. The two sides disagree. Consider driving both the empty-branch decision and the summary text off the same collection (either consistently episodes or consistently totalEpisodeCount) so they can't diverge.
| return if (episodes.isEmpty()) { | ||
| countText | ||
| } else { | ||
| val totalDurationMs = episodes.sumOf { episode -> episode.duration * 1000 }.toLong() |
There was a problem hiding this comment.
Minor: BaseEpisode already exposes a durationMs computed property ((duration * 1000.0).toInt()), which TvEpisodeRow uses. For consistency you can write episodes.sumOf { it.durationMs.toLong() } instead of the manual episode.duration * 1000 conversion.
| is TvPlaylistDetailsUiState.Loaded -> { | ||
| val playAllFocusRequester = remember { FocusRequester() } | ||
| Row( | ||
| horizontalArrangement = Arrangement.spacedBy(80.dp), | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .padding(start = 32.dp, top = 16.dp, end = 32.dp), | ||
| ) { | ||
| PlaylistInfo( | ||
| playlist = uiState.playlist, | ||
| episodes = uiState.episodes, | ||
| playAllFocusRequester = playAllFocusRequester, | ||
| modifier = Modifier.width(ArtworkSize), | ||
| ) | ||
| if (uiState.totalEpisodeCount == 0) { | ||
| NoEpisodes(modifier = Modifier.weight(1f).fillMaxHeight()) | ||
| } else { | ||
| EpisodeList( | ||
| episodes = uiState.episodes, | ||
| playAllFocusRequester = playAllFocusRequester, | ||
| modifier = Modifier.weight(1f), | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
On a TV form factor, opening the details screen should land focus on a concrete element so the first d-pad press does something. Here nothing calls playAllFocusRequester.requestFocus() on entry (contrast with TvPlaylistsGrid, which uses focusProperties { onEnter = { ...requestFocus() } }). Have you verified on the emulator that focus reliably lands on Play all episodes (or the first row) when the screen opens? If it depends on the host scaffold, a LaunchedEffect(Unit) { playAllFocusRequester.requestFocus() } would make it deterministic.
Generated by 🚫 Danger |
|
Claude finished @sztomek's task in 1m 23s —— View job PR Review — TV Playlist Details ScreenReviewed against Findings below — one is worth fixing before merge; the rest are minor. 1. Episode-count inconsistency can render a blank right pane —
|
|
Claude finished @sztomek's task in 1m 12s —— View job PR Review — TV Playlist Details Screen (re-review of
|
geekygecko
left a comment
There was a problem hiding this comment.
This looks great!
The one slight difference with the Apple TV version is that the playlist page no longer has the header navigation.
Description
Adds the playlist details screen to the TV app, mirroring the Apple TV implementation: opening a playlist from the Playlists tab shows a left info pane (artwork collage, playlist type, title, "N episodes · total duration", and a
Play all episodesbutton) next to the scrolling episode list. The screen renders inside the app scaffold so the top tab bar stays visible, and back returns to the grid.TvEpisodeRow(artwork, relative date, video indicator, 2-line title, duration/time-left with a progress bar for in-progress and a checkmark for played, archived rows dimmed) andTvArtworkImagecomponents; the header collage reuses the sharedPlaylistArtworkcomposable.TvPlaylistDetailsViewModelobservesPlaylistManager.manualPlaylistFlow/smartPlaylistFlow(assisted-inject with uuid + type, keyed per playlist, replay cache dropped when the screen closes) and resolves to aNotFoundstate that auto-closes the screen when the playlist is deleted while open.BackHandler; pressing LEFT from any episode row focusesPlay all episodes.Play all episodesand episode row clicks are intentionally no-ops — playback wiring comes in follow-up PRs, as do sorting and the archived filter (next two PRs of this stack).Fixes PCDROID-693 https://linear.app/a8c/issue/PCDROID-693/playlist-details.
Designs: Ftk3KwnfqaK4g57yCN63p0-fi-3258_12136.
Stacked on #5658 (
feat/tv-create-playlist) — merge bottom-up.Testing Instructions
./gradlew :tv:installDebug) on an Android TV device or emulator, signed in with playlists synced.Play all episodes.Screenshots or Screencast
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml