address the adversarial review findings

- restore the library sync control removed when the tab bar was reworked, with a test that clicks it
- lift the search field above the tab bar instead of overlapping it
- give the tabs and settings/sync buttons 48dp touch targets
- drop the duplicated label semantics on tabs
- animate the library bottom inset so the bar does not jump when the mini player enters/exits
- remove imports and a parameter left dead by the header rework
This commit is contained in:
austinried
2026-10-04 00:29:57 +00:00
parent 13568b9bee
commit 10b21303d2
3 changed files with 93 additions and 28 deletions
@@ -4,6 +4,7 @@ import android.net.Uri
import androidx.activity.compose.BackHandler import androidx.activity.compose.BackHandler
import androidx.compose.animation.AnimatedVisibility import androidx.compose.animation.AnimatedVisibility
import androidx.compose.animation.core.animate import androidx.compose.animation.core.animate
import androidx.compose.animation.core.animateDpAsState
import androidx.compose.animation.core.tween import androidx.compose.animation.core.tween
import androidx.compose.animation.expandVertically import androidx.compose.animation.expandVertically
import androidx.compose.animation.fadeIn import androidx.compose.animation.fadeIn
@@ -156,6 +157,11 @@ private fun MainNavigation() {
val navBarInset = with(density) { WindowInsets.navigationBars.getBottom(this).toDp() } val navBarInset = with(density) { WindowInsets.navigationBars.getBottom(this).toDp() }
val keyboardUp = WindowInsets.imeAnimationTarget.getBottom(density) > 0 val keyboardUp = WindowInsets.imeAnimationTarget.getBottom(density) > 0
val showMiniPlayer = playerVisible && !keyboardUp val showMiniPlayer = playerVisible && !keyboardUp
val bottomInset by animateDpAsState(
targetValue = if (showMiniPlayer) 0.dp else navBarInset,
animationSpec = tween(MINI_PLAYER_ANIM_MS),
label = "libraryBottomInset",
)
val artwork = rememberArtworkColors(playbackController.coverArt(playback.item, thumbnail = true)) val artwork = rememberArtworkColors(playbackController.coverArt(playback.item, thumbnail = true))
ArtworkTheme(artwork) { ArtworkTheme(artwork) {
@@ -197,7 +203,7 @@ private fun MainNavigation() {
onArtistClick = { artist -> navController.navigate(Routes.artist(artist.id, artist.coverArt)) }, onArtistClick = { artist -> navController.navigate(Routes.artist(artist.id, artist.coverArt)) },
onPlaylistClick = { playlist -> navController.navigate(Routes.playlist(playlist.id)) }, onPlaylistClick = { playlist -> navController.navigate(Routes.playlist(playlist.id)) },
onOpenSettings = { navController.navigate(Routes.SETTINGS) }, onOpenSettings = { navController.navigate(Routes.SETTINGS) },
bottomInset = if (showMiniPlayer) 0.dp else navBarInset, bottomInset = bottomInset,
) )
} }
composable(Routes.SETTINGS) { composable(Routes.SETTINGS) {
@@ -1,10 +1,5 @@
package com.subtracks.ui.library package com.subtracks.ui.library
import androidx.compose.animation.AnimatedVisibility
import androidx.compose.animation.expandHorizontally
import androidx.compose.animation.fadeIn
import androidx.compose.animation.fadeOut
import androidx.compose.animation.shrinkHorizontally
import androidx.compose.foundation.BorderStroke import androidx.compose.foundation.BorderStroke
import androidx.compose.foundation.background import androidx.compose.foundation.background
import androidx.compose.foundation.clickable import androidx.compose.foundation.clickable
@@ -24,7 +19,6 @@ 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.width import androidx.compose.foundation.layout.width
import androidx.compose.foundation.layout.wrapContentHeight
import androidx.compose.foundation.pager.HorizontalPager import androidx.compose.foundation.pager.HorizontalPager
import androidx.compose.foundation.pager.PagerState import androidx.compose.foundation.pager.PagerState
import androidx.compose.foundation.pager.rememberPagerState import androidx.compose.foundation.pager.rememberPagerState
@@ -60,10 +54,9 @@ import androidx.compose.material3.Surface
import androidx.compose.material3.Text import androidx.compose.material3.Text
import androidx.compose.material3.TextField import androidx.compose.material3.TextField
import androidx.compose.material3.TextFieldDefaults import androidx.compose.material3.TextFieldDefaults
import androidx.compose.material3.TopAppBarDefaults import androidx.compose.material3.minimumInteractiveComponentSize
import androidx.compose.runtime.Composable import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.SideEffect
import androidx.compose.runtime.getValue import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableFloatStateOf import androidx.compose.runtime.mutableFloatStateOf
import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.mutableIntStateOf
@@ -91,9 +84,7 @@ import androidx.compose.ui.graphics.Color
import androidx.compose.ui.graphics.Path import androidx.compose.ui.graphics.Path
import androidx.compose.ui.graphics.drawscope.clipPath import androidx.compose.ui.graphics.drawscope.clipPath
import androidx.compose.ui.graphics.drawscope.clipRect import androidx.compose.ui.graphics.drawscope.clipRect
import androidx.compose.ui.graphics.lerp
import androidx.compose.ui.graphics.vector.ImageVector import androidx.compose.ui.graphics.vector.ImageVector
import androidx.compose.ui.input.nestedscroll.nestedScroll
import androidx.compose.ui.layout.boundsInParent 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
@@ -102,7 +93,6 @@ 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
import androidx.compose.ui.unit.Dp import androidx.compose.ui.unit.Dp
import androidx.compose.ui.unit.IntOffset
import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.dp
import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.lifecycle.compose.collectAsStateWithLifecycle
import androidx.paging.compose.LazyPagingItems import androidx.paging.compose.LazyPagingItems
@@ -437,7 +427,7 @@ fun LibraryScreen(
.align(Alignment.BottomCenter) .align(Alignment.BottomCenter)
.fillMaxWidth() .fillMaxWidth()
.imePadding() .imePadding()
.padding(horizontal = 12.dp, vertical = 8.dp), .padding(start = 12.dp, end = 12.dp, top = 8.dp, bottom = tabBarHeight + 8.dp),
) )
} }
@@ -612,6 +602,8 @@ private fun ListOptionsSheet(
private val FAB_CLEARANCE = 80.dp private val FAB_CLEARANCE = 80.dp
private val SEARCH_BAR_CLEARANCE = 80.dp private val SEARCH_BAR_CLEARANCE = 80.dp
private val TAB_ICON_SIZE = 24.dp
private val TAB_VERTICAL_PADDING = 6.dp
@Composable @Composable
private fun SearchField( private fun SearchField(
@@ -686,6 +678,7 @@ private fun LibraryTabs(
val headerColor = artwork?.let(::playerSurfaceColor) ?: MaterialTheme.colorScheme.background val headerColor = artwork?.let(::playerSurfaceColor) ?: MaterialTheme.colorScheme.background
val indicatorColor = artwork?.scheme?.primary ?: MaterialTheme.colorScheme.onBackground val indicatorColor = artwork?.scheme?.primary ?: MaterialTheme.colorScheme.onBackground
val tabContentHeightPx = with(density) { (TAB_ICON_SIZE + TAB_VERTICAL_PADDING * 2).toPx() }
Box(Modifier.fillMaxWidth()) { Box(Modifier.fillMaxWidth()) {
Box( Box(
@@ -693,16 +686,17 @@ private fun LibraryTabs(
Modifier Modifier
.fillMaxWidth() .fillMaxWidth()
.background(headerColor) .background(headerColor)
.padding(8.dp), .padding(horizontal = 8.dp, vertical = 2.dp),
) { ) {
Box( Box(
modifier = modifier =
Modifier.drawBehind { Modifier.drawBehind {
if (current != null && indicatorLeft != null && indicatorRight != null) { if (current != null && indicatorLeft != null && indicatorRight != null && indicatorRight > indicatorLeft) {
val height = tabContentHeightPx.coerceAtMost(current.height)
drawRoundRect( drawRoundRect(
color = indicatorColor, color = indicatorColor,
topLeft = Offset(indicatorLeft, current.top), topLeft = Offset(indicatorLeft, current.top + (current.height - height) / 2f),
size = Size(indicatorRight - indicatorLeft, current.height), size = Size(indicatorRight - indicatorLeft, height),
cornerRadius = CornerRadius(8.dp.toPx()), cornerRadius = CornerRadius(8.dp.toPx()),
) )
} }
@@ -739,19 +733,18 @@ private fun LibraryTabs(
strokeWidth = 2.dp, strokeWidth = 2.dp,
) )
} }
Box( IconButton(onClick = onSync) {
modifier = Icon(
Modifier imageVector = Icons.Rounded.Sync,
.size(36.dp) contentDescription = "Sync",
.clip(CircleShape) tint = MaterialTheme.colorScheme.onBackground,
.clickable(onClick = onOpenSettings), )
contentAlignment = Alignment.Center, }
) { IconButton(onClick = onOpenSettings) {
Icon( Icon(
imageVector = Icons.Rounded.Settings, imageVector = Icons.Rounded.Settings,
contentDescription = "Settings", contentDescription = "Settings",
tint = MaterialTheme.colorScheme.onBackground, tint = MaterialTheme.colorScheme.onBackground,
modifier = Modifier.size(24.dp),
) )
} }
} }
@@ -781,7 +774,8 @@ private fun TabButton(
modifier modifier
.onGloballyPositioned { tabLeft = it.boundsInParent().left } .onGloballyPositioned { tabLeft = it.boundsInParent().left }
.clickable(onClick = onClick) .clickable(onClick = onClick)
.padding(start = 8.dp, end = 12.dp, top = 6.dp, bottom = 6.dp), .minimumInteractiveComponentSize()
.padding(start = 8.dp, end = 12.dp, top = TAB_VERTICAL_PADDING, bottom = TAB_VERTICAL_PADDING),
) { ) {
if (clipStart != null && clipEnd != null) { if (clipStart != null && clipEnd != null) {
Box( Box(
@@ -827,12 +821,13 @@ private fun TabContent(
imageVector = tab.icon, imageVector = tab.icon,
contentDescription = tab.label, contentDescription = tab.label,
tint = color, tint = color,
modifier = Modifier.size(24.dp), modifier = Modifier.size(TAB_ICON_SIZE),
) )
Box( Box(
modifier = modifier =
Modifier Modifier
.clipToBounds() .clipToBounds()
.clearAndSetSemantics {}
.layout { measurable, _ -> .layout { measurable, _ ->
val placeable = measurable.measure(Constraints()) val placeable = measurable.measure(Constraints())
val width = (placeable.width * progress).roundToInt() val width = (placeable.width * progress).roundToInt()
@@ -0,0 +1,64 @@
package com.subtracks.ui
import androidx.activity.ComponentActivity
import androidx.compose.runtime.remember
import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule
import androidx.compose.ui.test.onNodeWithContentDescription
import androidx.compose.ui.test.performClick
import androidx.paging.PagingData
import androidx.paging.compose.collectAsLazyPagingItems
import androidx.test.ext.junit.runners.AndroidJUnit4
import com.github.takahirom.roborazzi.RobolectricDeviceQualifiers
import com.subtracks.data.model.Album
import com.subtracks.data.model.Artist
import com.subtracks.data.model.Playlist
import com.subtracks.data.model.SongListItem
import com.subtracks.ui.library.LibraryScreen
import com.subtracks.ui.library.LibraryTab
import com.subtracks.ui.theme.SubtracksTheme
import kotlinx.coroutines.flow.flowOf
import org.junit.Assert.assertTrue
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.annotation.Config
import org.robolectric.annotation.GraphicsMode
@RunWith(AndroidJUnit4::class)
@GraphicsMode(GraphicsMode.Mode.NATIVE)
@Config(qualifiers = RobolectricDeviceQualifiers.PixelXL)
class LibraryScreenTest {
@get:Rule
val composeRule = createAndroidComposeRule<ComponentActivity>()
@Test
fun theSyncAndSettingsControlsAreReachable() {
var synced = false
var settingsOpened = false
composeRule.setContent {
SubtracksTheme {
LibraryScreen(
selectedTab = LibraryTab.Albums,
onTabSelected = {},
albums = remember { flowOf(PagingData.empty<Album>()) }.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 = { settingsOpened = true },
)
}
}
composeRule.onNodeWithContentDescription("Sync").performClick()
composeRule.onNodeWithContentDescription("Settings").performClick()
assertTrue("the sync control should trigger a sync", synced)
assertTrue("the settings control should open settings", settingsOpened)
}
}