Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,14 @@
<string name="tv_row_because_you_liked">Because you liked Podcast</string>
<string name="tv_home_keep_listening">Keep Listening</string>
<string name="tv_search_browse_categories">Browse categories</string>
<string name="tv_sponsored_podcast_section_title">Pocket Casts recommends</string>
<string name="tv_banner_create_account_title">One account. Every screen</string>
<string name="tv_banner_create_account_subtitle">Your follows and progress exactly where you left them</string>
<string name="tv_banner_create_account_action_title">Create a free account</string>
<string name="tv_banner_discover_more_title">You haven\'t seen the half of it</string>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tv_search_searching has no references anywhere in the repo (grep -rn "tv_search_searching" only hits this file). Unused strings still get shipped to GlotPress and translated, so it's worth dropping it unless a follow-up PR is about to use it.

<string name="tv_banner_discover_more_subtitle">Your next obsession is in here somewhere</string>
<string name="tv_banner_discover_more_action_title">Discover more shows</string>
<string name="tv_search_searching">Searching…</string>
<string name="tv_search_no_results_title">No results</string>
<string name="tv_search_no_results_subtitle">Try more general or different keywords.</string>
<string name="tv_search_error_subtitle">Check your connection and try again.</string>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,214 @@
package au.com.shiftyjelly.pocketcasts.component

import androidx.annotation.DrawableRes
import androidx.compose.foundation.Image
import androidx.compose.foundation.background
import androidx.compose.foundation.interaction.MutableInteractionSource
import androidx.compose.foundation.interaction.collectIsFocusedAsState
import androidx.compose.foundation.layout.Arrangement
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.fillMaxHeight
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.requiredHeight
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.runtime.Composable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.remember
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
import androidx.compose.ui.draw.clipToBounds
import androidx.compose.ui.graphics.Brush
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.layout.ContentScale
import androidx.compose.ui.res.painterResource
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.text.style.TextOverflow
import androidx.compose.ui.tooling.preview.Devices
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.Dp
import androidx.compose.ui.unit.dp
import androidx.tv.material3.CardDefaults
import androidx.tv.material3.MaterialTheme
import androidx.tv.material3.Text
import au.com.shiftyjelly.pocketcasts.discover.TvDiscoverBanner
import au.com.shiftyjelly.pocketcasts.theme.TvTheme
import au.com.shiftyjelly.pocketcasts.theme.tvColors
import au.com.shiftyjelly.pocketcasts.theme.tvTypography
import au.com.shiftyjelly.pocketcasts.images.R as IR
import au.com.shiftyjelly.pocketcasts.localization.R as LR

@Composable
fun TvBannerRow(
banner: TvDiscoverBanner,
onClick: () -> Unit,
modifier: Modifier = Modifier,
) {
val interactionSource = remember { MutableInteractionSource() }
val isFocused by interactionSource.collectIsFocusedAsState()

TvTile(
onClick = onClick,
shape = CardDefaults.shape(RoundedCornerShape(12.dp)),
scale = CardDefaults.scale(focusedScale = 1.05f),
colors = CardDefaults.colors(
containerColor = Color.Black,
focusedContainerColor = Color.Black,
),
interactionSource = interactionSource,
modifier = modifier
.fillMaxWidth()
.height(132.dp),
) {
Box(modifier = Modifier.fillMaxSize().clipToBounds()) {
BackgroundLift()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This first BackgroundLift() never ends up visible in either branch, so it's a full-size gradient draw per frame for nothing:

  • CreateAccount — the artwork is aligned CenterStart and tv_banner_create_account.webp is lossy VP8 with no alpha channel (fully opaque), drawn ~548dp wide (240dp × 2796/1224) on a ~860dp banner ≈ 63% of the width. The lift only spans 0f → 0.55f, so it's entirely behind opaque pixels.
  • DiscoverMore — the mask below paints opaque black from 0f to 0.82f, which covers the whole 0f → 0.55f lift, and then BackgroundLift() is drawn again on top at line 92 (that one is the visible one).

So the effect only comes from line 92. Either drop line 69, or if it was meant to lift the area under a semi-transparent artwork, it needs to move above the Image for the CreateAccount case.

Related: CreateAccount has hasArtworkMask = false, so text legibility on the left→right transition depends entirely on the fade baked into the asset (which is what the re-export in 6c27451 did). Worth a comment on hasArtworkMask saying so — otherwise a future asset re-crop silently puts title text over the collage again.

Image(
painter = painterResource(banner.artwork()),
contentDescription = null,
contentScale = ContentScale.FillHeight,
alignment = Alignment.CenterEnd,
modifier = Modifier
.align(Alignment.CenterEnd)
.requiredHeight(banner.artworkHeight),
)
if (banner.hasArtworkMask) {
// Opaque black over the text side so the bright collage only shows on the end edge.
Box(
modifier = Modifier
.fillMaxSize()
.background(
Brush.horizontalGradient(
0f to Color.Black,
0.82f to Color.Black,
1f to Color.Transparent,
),
),
)
BackgroundLift()
}
Comment on lines +68 to +93

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Moving the artwork onto black fixed the case I flagged last round — with the image at CenterEnd this first BackgroundLift() is now genuinely visible in the CreateAccount branch (the artwork covers roughly 0.60f → 1f of an ~864dp banner, and the lift is transparent past 0.55f).

It's still redundant in the DiscoverMore branch though: the mask below paints opaque black across 0f → 0.82f, which fully covers the 0f → 0.55f lift, and then line 92 repaints it on top. So that branch does two full-size gradient draws per frame where one suffices. Hoisting a single call below the if gives the same result in both branches — for CreateAccount the lift would then paint over the artwork, but it's already fully transparent by the point the artwork starts:

Suggested change
Box(modifier = Modifier.fillMaxSize().clipToBounds()) {
BackgroundLift()
Image(
painter = painterResource(banner.artwork()),
contentDescription = null,
contentScale = ContentScale.FillHeight,
alignment = Alignment.CenterEnd,
modifier = Modifier
.align(Alignment.CenterEnd)
.requiredHeight(banner.artworkHeight),
)
if (banner.hasArtworkMask) {
// Opaque black over the text side so the bright collage only shows on the end edge.
Box(
modifier = Modifier
.fillMaxSize()
.background(
Brush.horizontalGradient(
0f to Color.Black,
0.82f to Color.Black,
1f to Color.Transparent,
),
),
)
BackgroundLift()
}
Box(modifier = Modifier.fillMaxSize().clipToBounds()) {
Image(
painter = painterResource(banner.artwork()),
contentDescription = null,
contentScale = ContentScale.FillHeight,
alignment = Alignment.CenterEnd,
modifier = Modifier
.align(Alignment.CenterEnd)
.requiredHeight(banner.artworkHeight),
)
if (banner.hasArtworkMask) {
// Opaque black over the text side so the bright collage only shows on the end edge.
Box(
modifier = Modifier
.fillMaxSize()
.background(
Brush.horizontalGradient(
0f to Color.Black,
0.82f to Color.Black,
1f to Color.Transparent,
),
),
)
}
BackgroundLift()

Separately, worth a one-line comment on hasArtworkMask (line 179) noting that CreateAccount needs no mask because its artwork is composited on opaque black — otherwise a future re-export with a transparent or light background silently puts the title text back over the mockup, which is the overlap @geekygecko caught. Same for the hardcoded Color.Black container at lines 60-61: it's the one TV surface bypassing tvColors, and the reason (both assets are baked on black) isn't obvious from the code.

Row(
verticalAlignment = Alignment.CenterVertically,
horizontalArrangement = Arrangement.spacedBy(48.dp),
modifier = Modifier
.align(Alignment.CenterStart)
.fillMaxHeight()
.fillMaxWidth(banner.contentWidthFraction)
.padding(horizontal = 48.dp),
) {
BannerActionPill(banner, isFocused)
BannerText(banner, modifier = Modifier.weight(1f, fill = false))
}
Comment on lines +94 to +105

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Arrangement.spacedBy(48.dp) replacing the fixed Spacer is a real improvement — the 48dp now comes off the top before distribution. But the weighting is still one-sided: BannerText is weighted, BannerActionPill isn't, and in a Row un-weighted children are measured first against the full remaining width. So a long localised CTA can still take the whole 0.68f/0.82f and leave the text Column at ~0dp. The pill's Text (line 153) also has no maxLines, so instead of yielding it wraps to two or three lines and grows the pill vertically inside the 132dp card.

Weighting both bounds it either way, e.g. Modifier.weight(1f, fill = false) on the pill Box and Modifier.weight(2f) on the text — the asymmetric weights keep the text the bigger share while capping the pill at a third, since fill = false means the leftover isn't redistributed. maxLines = 1 + TextOverflow.Ellipsis on the pill's Text would make the failure mode graceful rather than a reflowing pill.

"Create a free account" is the risky one — de/fr/ru commonly run 30-40% longer.

}
}
}
Comment thread
sztomek marked this conversation as resolved.

@Composable
private fun BackgroundLift() {
Box(
modifier = Modifier
.fillMaxSize()
.background(
Brush.horizontalGradient(
0f to MaterialTheme.tvColors.backgroundActive20,
0.55f to Color.Transparent,
),
),
)
}

@Composable
private fun BannerText(banner: TvDiscoverBanner, modifier: Modifier = Modifier) {
Column(verticalArrangement = Arrangement.Center, modifier = modifier) {
Text(
text = banner.title(),
style = MaterialTheme.tvTypography.callout,
color = MaterialTheme.tvColors.textPrimary,
maxLines = 1,
overflow = TextOverflow.Ellipsis,
)
Text(
text = banner.subtitle(),
style = MaterialTheme.tvTypography.body,
color = MaterialTheme.tvColors.textSecondary,
maxLines = 2,
overflow = TextOverflow.Ellipsis,
)
}
}

@Composable
private fun BannerActionPill(banner: TvDiscoverBanner, isFocused: Boolean) {
Box(
contentAlignment = Alignment.Center,
modifier = Modifier
.clip(RoundedCornerShape(percent = 50))
.background(if (isFocused) MaterialTheme.tvColors.backgroundActive else MaterialTheme.tvColors.backgroundActive20)
.padding(horizontal = 24.dp, vertical = 12.dp),
) {
Text(
text = banner.actionTitle(),
style = MaterialTheme.tvTypography.caption1,
color = if (isFocused) MaterialTheme.tvColors.textPrimaryActive else MaterialTheme.tvColors.backgroundActive,
)
}
}

@DrawableRes
private fun TvDiscoverBanner.artwork(): Int = when (this) {
TvDiscoverBanner.CreateAccount -> IR.drawable.tv_banner_create_account

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The tv_banner_create_account.webp re-exported in ce416b5 is horizontally mirrored. Everything inside the phone mockup reads backwards:

  • the screen labels — "Good Times", "Play all", "Smart Rules", "Search", "24 episodes · 19h 48m", and the status bar 10:41 / LTE
  • the play glyph in the "Play all" button points left ()
  • the podcast covers in the grid (GOOD HANG, Bald & Beautiful, Dish, The Book Review, Waitrose) are all flipped
  • the phone's volume/action buttons sit on the right-hand edge instead of the left

It isn't the renderer: tv_banner_discover_more.webp in the same folder reads correctly. Given the commit moved the artwork to the end edge and the file grew 29,858 → 84,958 bytes, my guess is the frame was flipped to get the phone onto the right rather than re-composed.

At requiredHeight(150.dp) the artwork draws ~343dp wide (2796×1224 → 2.28:1), of which the phone is roughly the right 155dp — small, but the reversed "Play all" and the backwards cover art are visible at TV viewing distance on a 1080p panel. Worth re-exporting with the mockup composed on the right rather than mirrored.

TvDiscoverBanner.DiscoverMore -> IR.drawable.tv_banner_discover_more
}

private val TvDiscoverBanner.artworkHeight: Dp
get() = when (this) {
TvDiscoverBanner.CreateAccount -> 150.dp
TvDiscoverBanner.DiscoverMore -> 170.dp
}

private val TvDiscoverBanner.contentWidthFraction: Float
get() = when (this) {
TvDiscoverBanner.CreateAccount -> 0.68f
TvDiscoverBanner.DiscoverMore -> 0.82f
}

private val TvDiscoverBanner.hasArtworkMask: Boolean
get() = this == TvDiscoverBanner.DiscoverMore

@Composable
private fun TvDiscoverBanner.title(): String = when (this) {
TvDiscoverBanner.CreateAccount -> stringResource(LR.string.tv_banner_create_account_title)
TvDiscoverBanner.DiscoverMore -> stringResource(LR.string.tv_banner_discover_more_title)
}

@Composable
private fun TvDiscoverBanner.subtitle(): String = when (this) {
TvDiscoverBanner.CreateAccount -> stringResource(LR.string.tv_banner_create_account_subtitle)
TvDiscoverBanner.DiscoverMore -> stringResource(LR.string.tv_banner_discover_more_subtitle)
}

@Composable
private fun TvDiscoverBanner.actionTitle(): String = when (this) {
TvDiscoverBanner.CreateAccount -> stringResource(LR.string.tv_banner_create_account_action_title)
TvDiscoverBanner.DiscoverMore -> stringResource(LR.string.tv_banner_discover_more_action_title)
}

@Preview(device = Devices.TV_1080p)
@Composable
private fun TvBannerRowPreview() {
TvTheme {
Column(
verticalArrangement = Arrangement.spacedBy(24.dp),
modifier = Modifier
.background(MaterialTheme.tvColors.backgroundSunken)
.padding(48.dp),
) {
TvBannerRow(banner = TvDiscoverBanner.CreateAccount, onClick = {})
TvBannerRow(banner = TvDiscoverBanner.DiscoverMore, onClick = {})
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,25 @@ package au.com.shiftyjelly.pocketcasts.component

import androidx.compose.foundation.background
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.aspectRatio
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.width
import androidx.compose.runtime.Composable
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.layout.ContentScale
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.tooling.preview.Devices
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import androidx.tv.material3.MaterialTheme
import androidx.tv.material3.Text
import au.com.shiftyjelly.pocketcasts.theme.TvTheme
import au.com.shiftyjelly.pocketcasts.theme.tvColors
import au.com.shiftyjelly.pocketcasts.theme.tvTypography
import coil3.compose.AsyncImage
import au.com.shiftyjelly.pocketcasts.localization.R as LR

@Composable
fun TvPodcastTile(
Expand All @@ -22,17 +29,37 @@ fun TvPodcastTile(
onClick: () -> Unit,
modifier: Modifier = Modifier,
imageModifier: Modifier = Modifier,
isSponsored: Boolean = false,
) {
TvTile(
onClick = onClick,
modifier = modifier,
) {
AsyncImage(
model = artworkUrl,
contentDescription = podcastTitle,
contentScale = ContentScale.Crop,
modifier = imageModifier.aspectRatio(1f),
)
if (isSponsored) {
Column(horizontalAlignment = Alignment.CenterHorizontally) {
AsyncImage(
model = artworkUrl,
contentDescription = podcastTitle,
contentScale = ContentScale.Crop,
modifier = imageModifier
.padding(horizontal = 18.dp, vertical = 9.dp)
.aspectRatio(1f),
)
Text(
text = stringResource(LR.string.sponsored),
style = MaterialTheme.tvTypography.caption2,
color = MaterialTheme.tvColors.textSecondary,
modifier = Modifier.padding(bottom = 9.dp),
)
}
Comment thread
sztomek marked this conversation as resolved.
} else {
AsyncImage(
model = artworkUrl,
contentDescription = podcastTitle,
contentScale = ContentScale.Crop,
modifier = imageModifier.aspectRatio(1f),
)
}
}
}

Expand Down
Loading