address the code review findings

- offer a Sync button on a truly empty library tab so an empty library is not stranded (pull-to-refresh needs scrollable content)
- do not close the search field when it auto-syncs from a tab with an empty term
- make rememberArtworkColors nullable so the no-track fallbacks are live and the idle theme is the monochrome background
- measure the tab overlay content offset so the colour clip accounts for the touch-target centering
- put the pull indicator below the status-bar scrim, and seed the tab bar height estimate to avoid a first-frame inset jump
- collapse the two equal clearance constants, drop the redundant search imePadding, reuse the hoisted artwork for the mini player, and de-duplicate the mini player screenshots
This commit is contained in:
austinried
2026-10-04 00:29:57 +00:00
parent 60acdbeae3
commit 5b88792f72
10 changed files with 134 additions and 91 deletions
@@ -289,7 +289,7 @@ private fun MainNavigation() {
MiniPlayer( MiniPlayer(
state = playback, state = playback,
coverArt = miniArt, coverArt = miniArt,
artwork = rememberArtworkColors(miniArt), artwork = artwork,
onExpand = { settleNowPlaying(true) }, onExpand = { settleNowPlaying(true) },
onPlayPause = playbackController::togglePlayPause, onPlayPause = playbackController::togglePlayPause,
onNext = playbackController::next, onNext = playbackController::next,
@@ -172,17 +172,29 @@ fun LoadingState(modifier: Modifier = Modifier) {
fun EmptyState( fun EmptyState(
text: String, text: String,
modifier: Modifier = Modifier, modifier: Modifier = Modifier,
actionLabel: String? = null,
onAction: (() -> Unit)? = null,
) { ) {
Box( Box(
modifier = modifier.fillMaxSize().padding(32.dp), modifier = modifier.fillMaxSize().padding(32.dp),
contentAlignment = Alignment.Center, contentAlignment = Alignment.Center,
) { ) {
Text( Column(
text = text, horizontalAlignment = Alignment.CenterHorizontally,
style = MaterialTheme.typography.bodyLarge, verticalArrangement = Arrangement.spacedBy(16.dp),
color = MaterialTheme.colorScheme.onSurfaceVariant, ) {
textAlign = TextAlign.Center, Text(
) text = text,
style = MaterialTheme.typography.bodyLarge,
color = MaterialTheme.colorScheme.onSurfaceVariant,
textAlign = TextAlign.Center,
)
if (actionLabel != null && onAction != null) {
Button(onClick = onAction) {
Text(actionLabel)
}
}
}
} }
} }
@@ -44,6 +44,7 @@ fun AlbumsContent(
onClearFilters: () -> Unit = {}, onClearFilters: () -> Unit = {},
resetKey: Any? = null, resetKey: Any? = null,
topInset: Dp = 0.dp, topInset: Dp = 0.dp,
onSync: () -> Unit = {},
modifier: Modifier = Modifier, modifier: Modifier = Modifier,
) { ) {
when { when {
@@ -55,7 +56,12 @@ fun AlbumsContent(
if (filtered) { if (filtered) {
FilteredEmptyState(onClearFilters, modifier) FilteredEmptyState(onClearFilters, modifier)
} else { } else {
EmptyState("No albums yet.\nSync with your server to fill your library.", modifier) EmptyState(
text = "No albums yet.\nSync with your server to fill your library.",
modifier = modifier,
actionLabel = "Sync",
onAction = onSync,
)
} }
} }
@@ -42,6 +42,7 @@ fun ArtistsContent(
onClearFilters: () -> Unit = {}, onClearFilters: () -> Unit = {},
resetKey: Any? = null, resetKey: Any? = null,
topInset: Dp = 0.dp, topInset: Dp = 0.dp,
onSync: () -> Unit = {},
modifier: Modifier = Modifier, modifier: Modifier = Modifier,
) { ) {
when { when {
@@ -53,7 +54,7 @@ fun ArtistsContent(
if (filtered) { if (filtered) {
FilteredEmptyState(onClearFilters, modifier) FilteredEmptyState(onClearFilters, modifier)
} else { } else {
EmptyState("No artists yet.", modifier) EmptyState("No artists yet.", modifier, actionLabel = "Sync", onAction = onSync)
} }
} }
@@ -13,11 +13,11 @@ import androidx.compose.foundation.layout.WindowInsets
import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.imePadding
import androidx.compose.foundation.layout.offset import androidx.compose.foundation.layout.offset
import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.statusBars import androidx.compose.foundation.layout.statusBars
import androidx.compose.foundation.layout.statusBarsPadding
import androidx.compose.foundation.layout.width import androidx.compose.foundation.layout.width
import androidx.compose.foundation.pager.HorizontalPager import androidx.compose.foundation.pager.HorizontalPager
import androidx.compose.foundation.pager.PagerState import androidx.compose.foundation.pager.PagerState
@@ -90,6 +90,7 @@ import androidx.compose.ui.layout.boundsInParent
import androidx.compose.ui.layout.layout import androidx.compose.ui.layout.layout
import androidx.compose.ui.layout.onGloballyPositioned import androidx.compose.ui.layout.onGloballyPositioned
import androidx.compose.ui.layout.onSizeChanged import androidx.compose.ui.layout.onSizeChanged
import androidx.compose.ui.layout.positionInRoot
import androidx.compose.ui.platform.LocalDensity import androidx.compose.ui.platform.LocalDensity
import androidx.compose.ui.semantics.clearAndSetSemantics import androidx.compose.ui.semantics.clearAndSetSemantics
import androidx.compose.ui.unit.Constraints import androidx.compose.ui.unit.Constraints
@@ -302,16 +303,16 @@ fun LibraryScreen(
if (pagerState.currentPage != selectedTab.ordinal) { if (pagerState.currentPage != selectedTab.ordinal) {
pagerState.animateScrollToPage(selectedTab.ordinal) pagerState.animateScrollToPage(selectedTab.ordinal)
} }
searchActive = search.isNotEmpty() if (search.isNotEmpty()) searchActive = true
} }
val density = LocalDensity.current val density = LocalDensity.current
val statusBarTop = with(density) { WindowInsets.statusBars.getTop(this).toDp() } val statusBarTop = with(density) { WindowInsets.statusBars.getTop(this).toDp() }
val filtersActive = listQuery.starred != StarredFilter.Any || search.isNotEmpty() val filtersActive = listQuery.starred != StarredFilter.Any || search.isNotEmpty()
var tabBarHeightPx by remember { mutableFloatStateOf(0f) } var tabBarHeightPx by remember { mutableFloatStateOf(with(density) { (TAB_BAR_CONTENT_HEIGHT + bottomInset).toPx() }) }
val tabBarHeight = with(density) { tabBarHeightPx.toDp() } val tabBarHeight = with(density) { tabBarHeightPx.toDp() }
val listTopInset = statusBarTop val listTopInset = statusBarTop
val listBottomInset = tabBarHeight + if (searchActive) SEARCH_BAR_CLEARANCE else FAB_CLEARANCE val listBottomInset = tabBarHeight + BOTTOM_CLEARANCE
val headerColor = artwork?.let(::playerSurfaceColor) ?: MaterialTheme.colorScheme.background val headerColor = artwork?.let(::playerSurfaceColor) ?: MaterialTheme.colorScheme.background
Box( Box(
@@ -335,7 +336,7 @@ fun LibraryScreen(
PullToRefreshDefaults.Indicator( PullToRefreshDefaults.Indicator(
state = pullToRefreshState, state = pullToRefreshState,
isRefreshing = syncing, isRefreshing = syncing,
modifier = Modifier.align(Alignment.TopCenter), modifier = Modifier.align(Alignment.TopCenter).statusBarsPadding(),
containerColor = headerColor, containerColor = headerColor,
color = artwork?.scheme?.primary ?: MaterialTheme.colorScheme.primary, color = artwork?.scheme?.primary ?: MaterialTheme.colorScheme.primary,
) )
@@ -353,6 +354,7 @@ fun LibraryScreen(
onClearFilters = onClearFilters, onClearFilters = onClearFilters,
resetKey = resetKey, resetKey = resetKey,
topInset = listTopInset, topInset = listTopInset,
onSync = onSync,
) )
} }
@@ -366,6 +368,7 @@ fun LibraryScreen(
onClearFilters = onClearFilters, onClearFilters = onClearFilters,
resetKey = resetKey, resetKey = resetKey,
topInset = listTopInset, topInset = listTopInset,
onSync = onSync,
) )
} }
@@ -380,6 +383,7 @@ fun LibraryScreen(
onClearFilters = onClearFilters, onClearFilters = onClearFilters,
resetKey = resetKey, resetKey = resetKey,
topInset = listTopInset, topInset = listTopInset,
onSync = onSync,
) )
} }
@@ -393,6 +397,7 @@ fun LibraryScreen(
onClearFilters = onClearFilters, onClearFilters = onClearFilters,
resetKey = resetKey, resetKey = resetKey,
topInset = listTopInset, topInset = listTopInset,
onSync = onSync,
) )
} }
} }
@@ -444,7 +449,6 @@ fun LibraryScreen(
Modifier Modifier
.align(Alignment.BottomCenter) .align(Alignment.BottomCenter)
.fillMaxWidth() .fillMaxWidth()
.imePadding()
.padding(horizontal = 12.dp, vertical = 8.dp), .padding(horizontal = 12.dp, vertical = 8.dp),
) )
} }
@@ -618,8 +622,8 @@ private fun ListOptionsSheet(
} }
} }
private val FAB_CLEARANCE = 80.dp private val BOTTOM_CLEARANCE = 80.dp
private val SEARCH_BAR_CLEARANCE = 80.dp private val TAB_BAR_CONTENT_HEIGHT = 52.dp
private val TAB_ICON_SIZE = 24.dp private val TAB_ICON_SIZE = 24.dp
private val TAB_VERTICAL_PADDING = 6.dp private val TAB_VERTICAL_PADDING = 6.dp
@@ -768,47 +772,53 @@ private fun TabButton(
modifier: Modifier = Modifier, modifier: Modifier = Modifier,
) { ) {
val selectedContent = artwork?.scheme?.onPrimary ?: MaterialTheme.colorScheme.background val selectedContent = artwork?.scheme?.onPrimary ?: MaterialTheme.colorScheme.background
val paddingStart = with(LocalDensity.current) { 8.dp.toPx() }
var tabLeft by remember { mutableFloatStateOf(0f) } var tabLeft by remember { mutableFloatStateOf(0f) }
val clipStart = indicatorLeft?.minus(tabLeft + paddingStart) var tabRootX by remember { mutableFloatStateOf(0f) }
val clipEnd = indicatorRight?.minus(tabLeft + paddingStart) var contentRootX by remember { mutableFloatStateOf(0f) }
val contentLeft = tabLeft + (contentRootX - tabRootX)
val clipStart = indicatorLeft?.minus(contentLeft)
val clipEnd = indicatorRight?.minus(contentLeft)
Box( Box(
modifier = modifier =
modifier modifier
.onGloballyPositioned { tabLeft = it.boundsInParent().left } .onGloballyPositioned {
.clickable(onClick = onClick) tabLeft = it.boundsInParent().left
tabRootX = it.positionInRoot().x
}.clickable(onClick = onClick)
.minimumInteractiveComponentSize() .minimumInteractiveComponentSize()
.padding(start = 8.dp, end = 12.dp, top = TAB_VERTICAL_PADDING, bottom = TAB_VERTICAL_PADDING), .padding(start = 8.dp, end = 12.dp, top = TAB_VERTICAL_PADDING, bottom = TAB_VERTICAL_PADDING),
) { ) {
if (clipStart != null && clipEnd != null) { Box(Modifier.onGloballyPositioned { contentRootX = it.positionInRoot().x }) {
Box( if (clipStart != null && clipEnd != null) {
modifier = Box(
Modifier.drawWithContent { modifier =
val hole = Modifier.drawWithContent {
Path().apply { val hole =
addRect(Rect(clipStart, 0f, clipEnd, size.height)) Path().apply {
} addRect(Rect(clipStart, 0f, clipEnd, size.height))
clipPath(hole, clipOp = ClipOp.Difference) { }
this@drawWithContent.drawContent() clipPath(hole, clipOp = ClipOp.Difference) {
}
},
) {
TabContent(tab, progress, MaterialTheme.colorScheme.onBackground)
}
Box(
modifier =
Modifier
.clearAndSetSemantics {}
.drawWithContent {
clipRect(clipStart, 0f, clipEnd, size.height) {
this@drawWithContent.drawContent() this@drawWithContent.drawContent()
} }
}, },
) { ) {
TabContent(tab, progress, selectedContent) TabContent(tab, progress, MaterialTheme.colorScheme.onBackground)
}
Box(
modifier =
Modifier
.clearAndSetSemantics {}
.drawWithContent {
clipRect(clipStart, 0f, clipEnd, size.height) {
this@drawWithContent.drawContent()
}
},
) {
TabContent(tab, progress, selectedContent)
}
} else {
TabContent(tab, progress, MaterialTheme.colorScheme.onBackground)
} }
} else {
TabContent(tab, progress, MaterialTheme.colorScheme.onBackground)
} }
} }
} }
@@ -41,6 +41,7 @@ fun PlaylistsContent(
onClearFilters: () -> Unit = {}, onClearFilters: () -> Unit = {},
resetKey: Any? = null, resetKey: Any? = null,
topInset: Dp = 0.dp, topInset: Dp = 0.dp,
onSync: () -> Unit = {},
modifier: Modifier = Modifier, modifier: Modifier = Modifier,
) { ) {
when { when {
@@ -52,7 +53,7 @@ fun PlaylistsContent(
if (filtered) { if (filtered) {
FilteredEmptyState(onClearFilters, modifier) FilteredEmptyState(onClearFilters, modifier)
} else { } else {
EmptyState("No playlists yet.", modifier) EmptyState("No playlists yet.", modifier, actionLabel = "Sync", onAction = onSync)
} }
} }
@@ -53,6 +53,7 @@ fun SongsContent(
onClearFilters: () -> Unit = {}, onClearFilters: () -> Unit = {},
resetKey: Any? = null, resetKey: Any? = null,
topInset: Dp = 0.dp, topInset: Dp = 0.dp,
onSync: () -> Unit = {},
modifier: Modifier = Modifier, modifier: Modifier = Modifier,
) { ) {
when { when {
@@ -64,7 +65,7 @@ fun SongsContent(
if (filtered) { if (filtered) {
FilteredEmptyState(onClearFilters, modifier) FilteredEmptyState(onClearFilters, modifier)
} else { } else {
EmptyState("No songs yet.", modifier) EmptyState("No songs yet.", modifier, actionLabel = "Sync", onAction = onSync)
} }
} }
@@ -182,12 +182,13 @@ private fun blendHue(
fun rememberArtworkColors( fun rememberArtworkColors(
ref: CoverArtRef?, ref: CoverArtRef?,
durationMillis: Int = ARTWORK_THEME_TRANSITION_MS, durationMillis: Int = ARTWORK_THEME_TRANSITION_MS,
): ArtworkColors { ): ArtworkColors? {
val cached = remember(ref?.cacheKey) { ref?.cacheKey?.let(ArtworkSeedCache::cached) } val cached = remember(ref?.cacheKey) { ref?.cacheKey?.let(ArtworkSeedCache::cached) }
val seeds by rememberArtworkSeed(ref) val seeds by rememberArtworkSeed(ref)
val effective = seeds ?: cached val effective = seeds ?: cached
val target = remember(effective) { effective?.let { (primary, secondary) -> artworkColorsFromSeeds(primary, secondary) } } val target = remember(effective) { effective?.let { (primary, secondary) -> artworkColorsFromSeeds(primary, secondary) } }
return rememberAnimatedArtworkColors(target, durationMillis) val colors = rememberAnimatedArtworkColors(target, durationMillis)
return if (target == null) null else colors
} }
@Composable @Composable
@@ -147,49 +147,18 @@ class AppScreenshotTest {
} }
@Test @Test
fun libraryWithMiniPlayer() { fun libraryWithMiniPlayer() = libraryWithMiniPlayerShot(Color.rgb(120, 80, 200), "library_with_mini_player.png")
composeRule.setContent {
SubtracksTheme {
val artwork = artworkColorsFromSeed(Color.rgb(120, 80, 200))
Column(Modifier.fillMaxSize()) {
LibraryScreen(
selectedTab = LibraryTab.Albums,
onTabSelected = {},
albums = remember { flowOf(PagingData.from(Fixtures.albums)) }.collectAsLazyPagingItems(),
artists = remember { flowOf(PagingData.from(Fixtures.artists)) }.collectAsLazyPagingItems(),
songs = remember { flowOf(PagingData.from(Fixtures.songItems)) }.collectAsLazyPagingItems(),
playlists = remember { flowOf(PagingData.from(Fixtures.playlists)) }.collectAsLazyPagingItems(),
coverArt = { id, _ -> id?.let { CoverArtRef(it, "test:$it") } },
onAlbumClick = {},
onArtistClick = {},
onPlaylistClick = {},
onSongClick = {},
onSync = {},
onOpenSettings = {},
artwork = artwork,
modifier = Modifier.weight(1f),
)
MiniPlayer(
state = Fixtures.playbackState(),
coverArt = CoverArtRef("art-al-kid-a", "test:art-al-kid-a"),
artwork = artwork,
onExpand = {},
onPlayPause = {},
onNext = {},
progressInset = true,
)
}
}
}
awaitTag(ALBUM_COVER_TAG)
composeRule.onRoot().captureRoboImage("src/test/screenshots/library_with_mini_player.png")
}
@Test @Test
fun libraryWithMiniPlayerWarm() { fun libraryWithMiniPlayerWarm() = libraryWithMiniPlayerShot(Color.rgb(222, 150, 30), "library_with_mini_player_warm.png")
private fun libraryWithMiniPlayerShot(
seed: Int,
file: String,
) {
composeRule.setContent { composeRule.setContent {
SubtracksTheme { SubtracksTheme {
val artwork = artworkColorsFromSeed(Color.rgb(222, 150, 30)) val artwork = artworkColorsFromSeed(seed)
Column(Modifier.fillMaxSize()) { Column(Modifier.fillMaxSize()) {
LibraryScreen( LibraryScreen(
selectedTab = LibraryTab.Albums, selectedTab = LibraryTab.Albums,
@@ -221,7 +190,7 @@ class AppScreenshotTest {
} }
} }
awaitTag(ALBUM_COVER_TAG) awaitTag(ALBUM_COVER_TAG)
composeRule.onRoot().captureRoboImage("src/test/screenshots/library_with_mini_player_warm.png") composeRule.onRoot().captureRoboImage("src/test/screenshots/$file")
} }
@Test @Test
@@ -4,11 +4,15 @@ import androidx.activity.ComponentActivity
import androidx.compose.runtime.remember import androidx.compose.runtime.remember
import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule
import androidx.compose.ui.test.onAllNodesWithTag import androidx.compose.ui.test.onAllNodesWithTag
import androidx.compose.ui.test.onAllNodesWithText
import androidx.compose.ui.test.onNodeWithContentDescription import androidx.compose.ui.test.onNodeWithContentDescription
import androidx.compose.ui.test.onNodeWithText
import androidx.compose.ui.test.onRoot import androidx.compose.ui.test.onRoot
import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performClick
import androidx.compose.ui.test.performTouchInput import androidx.compose.ui.test.performTouchInput
import androidx.compose.ui.test.swipeDown import androidx.compose.ui.test.swipeDown
import androidx.paging.LoadState
import androidx.paging.LoadStates
import androidx.paging.PagingData import androidx.paging.PagingData
import androidx.paging.compose.collectAsLazyPagingItems import androidx.paging.compose.collectAsLazyPagingItems
import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.ext.junit.runners.AndroidJUnit4
@@ -72,6 +76,44 @@ class LibraryScreenTest {
assertTrue("the settings control should open settings", settingsOpened) assertTrue("the settings control should open settings", settingsOpened)
} }
@Test
fun anEmptyTabOffersSync() {
var synced = false
composeRule.setContent {
SubtracksTheme {
LibraryScreen(
selectedTab = LibraryTab.Albums,
onTabSelected = {},
albums = remember { flowOf(PagingData.empty<Album>(settledLoadStates)) }.collectAsLazyPagingItems(),
artists = remember { flowOf(PagingData.empty<Artist>()) }.collectAsLazyPagingItems(),
songs = remember { flowOf(PagingData.empty<SongListItem>()) }.collectAsLazyPagingItems(),
playlists = remember { flowOf(PagingData.empty<Playlist>()) }.collectAsLazyPagingItems(),
coverArt = { _, _ -> null },
onAlbumClick = {},
onArtistClick = {},
onPlaylistClick = {},
onSongClick = {},
onSync = { synced = true },
onOpenSettings = {},
)
}
}
composeRule.waitUntil(timeoutMillis = 5_000) {
composeRule.onAllNodesWithText("Sync").fetchSemanticsNodes().isNotEmpty()
}
composeRule.onNodeWithText("Sync").performClick()
assertTrue("an empty library should offer a sync", synced)
}
private val settledLoadStates =
LoadStates(
refresh = LoadState.NotLoading(endOfPaginationReached = true),
prepend = LoadState.NotLoading(endOfPaginationReached = true),
append = LoadState.NotLoading(endOfPaginationReached = true),
)
private fun albums() = private fun albums() =
(1..60).map { index -> (1..60).map { index ->
Album( Album(