From 10b21303d25ae9b6994b304ecc830fad928e95b4 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Fri, 25 Sep 2026 07:33:55 +0000 Subject: [PATCH] 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 --- .../main/kotlin/com/subtracks/ui/AppRoot.kt | 8 ++- .../com/subtracks/ui/library/LibraryScreen.kt | 49 +++++++------- .../com/subtracks/ui/LibraryScreenTest.kt | 64 +++++++++++++++++++ 3 files changed, 93 insertions(+), 28 deletions(-) create mode 100644 app/src/test/kotlin/com/subtracks/ui/LibraryScreenTest.kt diff --git a/app/src/main/kotlin/com/subtracks/ui/AppRoot.kt b/app/src/main/kotlin/com/subtracks/ui/AppRoot.kt index b35a8f1f..ebd8a51e 100644 --- a/app/src/main/kotlin/com/subtracks/ui/AppRoot.kt +++ b/app/src/main/kotlin/com/subtracks/ui/AppRoot.kt @@ -4,6 +4,7 @@ import android.net.Uri import androidx.activity.compose.BackHandler import androidx.compose.animation.AnimatedVisibility import androidx.compose.animation.core.animate +import androidx.compose.animation.core.animateDpAsState import androidx.compose.animation.core.tween import androidx.compose.animation.expandVertically import androidx.compose.animation.fadeIn @@ -156,6 +157,11 @@ private fun MainNavigation() { val navBarInset = with(density) { WindowInsets.navigationBars.getBottom(this).toDp() } val keyboardUp = WindowInsets.imeAnimationTarget.getBottom(density) > 0 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)) ArtworkTheme(artwork) { @@ -197,7 +203,7 @@ private fun MainNavigation() { onArtistClick = { artist -> navController.navigate(Routes.artist(artist.id, artist.coverArt)) }, onPlaylistClick = { playlist -> navController.navigate(Routes.playlist(playlist.id)) }, onOpenSettings = { navController.navigate(Routes.SETTINGS) }, - bottomInset = if (showMiniPlayer) 0.dp else navBarInset, + bottomInset = bottomInset, ) } composable(Routes.SETTINGS) { diff --git a/app/src/main/kotlin/com/subtracks/ui/library/LibraryScreen.kt b/app/src/main/kotlin/com/subtracks/ui/library/LibraryScreen.kt index 54deae78..a502644e 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/LibraryScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/LibraryScreen.kt @@ -1,10 +1,5 @@ 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.background 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.statusBars import androidx.compose.foundation.layout.width -import androidx.compose.foundation.layout.wrapContentHeight import androidx.compose.foundation.pager.HorizontalPager import androidx.compose.foundation.pager.PagerState import androidx.compose.foundation.pager.rememberPagerState @@ -60,10 +54,9 @@ import androidx.compose.material3.Surface import androidx.compose.material3.Text import androidx.compose.material3.TextField import androidx.compose.material3.TextFieldDefaults -import androidx.compose.material3.TopAppBarDefaults +import androidx.compose.material3.minimumInteractiveComponentSize import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect -import androidx.compose.runtime.SideEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableFloatStateOf 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.drawscope.clipPath import androidx.compose.ui.graphics.drawscope.clipRect -import androidx.compose.ui.graphics.lerp 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.layout 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.unit.Constraints import androidx.compose.ui.unit.Dp -import androidx.compose.ui.unit.IntOffset import androidx.compose.ui.unit.dp import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.paging.compose.LazyPagingItems @@ -437,7 +427,7 @@ fun LibraryScreen( .align(Alignment.BottomCenter) .fillMaxWidth() .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 SEARCH_BAR_CLEARANCE = 80.dp +private val TAB_ICON_SIZE = 24.dp +private val TAB_VERTICAL_PADDING = 6.dp @Composable private fun SearchField( @@ -686,6 +678,7 @@ private fun LibraryTabs( val headerColor = artwork?.let(::playerSurfaceColor) ?: MaterialTheme.colorScheme.background val indicatorColor = artwork?.scheme?.primary ?: MaterialTheme.colorScheme.onBackground + val tabContentHeightPx = with(density) { (TAB_ICON_SIZE + TAB_VERTICAL_PADDING * 2).toPx() } Box(Modifier.fillMaxWidth()) { Box( @@ -693,16 +686,17 @@ private fun LibraryTabs( Modifier .fillMaxWidth() .background(headerColor) - .padding(8.dp), + .padding(horizontal = 8.dp, vertical = 2.dp), ) { Box( modifier = 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( color = indicatorColor, - topLeft = Offset(indicatorLeft, current.top), - size = Size(indicatorRight - indicatorLeft, current.height), + topLeft = Offset(indicatorLeft, current.top + (current.height - height) / 2f), + size = Size(indicatorRight - indicatorLeft, height), cornerRadius = CornerRadius(8.dp.toPx()), ) } @@ -739,19 +733,18 @@ private fun LibraryTabs( strokeWidth = 2.dp, ) } - Box( - modifier = - Modifier - .size(36.dp) - .clip(CircleShape) - .clickable(onClick = onOpenSettings), - contentAlignment = Alignment.Center, - ) { + IconButton(onClick = onSync) { + Icon( + imageVector = Icons.Rounded.Sync, + contentDescription = "Sync", + tint = MaterialTheme.colorScheme.onBackground, + ) + } + IconButton(onClick = onOpenSettings) { Icon( imageVector = Icons.Rounded.Settings, contentDescription = "Settings", tint = MaterialTheme.colorScheme.onBackground, - modifier = Modifier.size(24.dp), ) } } @@ -781,7 +774,8 @@ private fun TabButton( modifier .onGloballyPositioned { tabLeft = it.boundsInParent().left } .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) { Box( @@ -827,12 +821,13 @@ private fun TabContent( imageVector = tab.icon, contentDescription = tab.label, tint = color, - modifier = Modifier.size(24.dp), + modifier = Modifier.size(TAB_ICON_SIZE), ) Box( modifier = Modifier .clipToBounds() + .clearAndSetSemantics {} .layout { measurable, _ -> val placeable = measurable.measure(Constraints()) val width = (placeable.width * progress).roundToInt() diff --git a/app/src/test/kotlin/com/subtracks/ui/LibraryScreenTest.kt b/app/src/test/kotlin/com/subtracks/ui/LibraryScreenTest.kt new file mode 100644 index 00000000..ce8ce4ab --- /dev/null +++ b/app/src/test/kotlin/com/subtracks/ui/LibraryScreenTest.kt @@ -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() + + @Test + fun theSyncAndSettingsControlsAreReachable() { + var synced = false + var settingsOpened = false + composeRule.setContent { + SubtracksTheme { + LibraryScreen( + selectedTab = LibraryTab.Albums, + onTabSelected = {}, + albums = remember { flowOf(PagingData.empty()) }.collectAsLazyPagingItems(), + artists = remember { flowOf(PagingData.empty()) }.collectAsLazyPagingItems(), + songs = remember { flowOf(PagingData.empty()) }.collectAsLazyPagingItems(), + playlists = remember { flowOf(PagingData.empty()) }.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) + } +}