From b019efbef91214468afe49708d49f74611ab2fad Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Thu, 15 Oct 2026 12:25:34 +0000 Subject: [PATCH] now playing: stop clipping the cover shadow The cover strip clipped to the 24dp content inset. A landscape cover fills that inset, so its drop shadow was cut off on the left and right and only the top and bottom survived, which reads as a stray band hugging the artwork. A tile that fills the strip's height had the same problem vertically. Drop the clip and space the tiles by TILE_GAP instead. The shadow then renders whole on all four sides while the swipe target and the artwork keep the size and the content margins they had, and the spacing leaves a gap between the tiles while dragging between tracks. It is also what keeps the neighbours off-screen now that the strip is unclipped. Refs: #173 --- .../subtracks/ui/playback/NowPlayingScreen.kt | 15 ++-- .../com/subtracks/ui/AppScreenshotTest.kt | 79 +++++++++---------- .../ui/playback/NowPlayingTapsTest.kt | 24 ++++++ 3 files changed, 73 insertions(+), 45 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt b/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt index 267a18e8..290bcf41 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt @@ -58,7 +58,6 @@ import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier -import androidx.compose.ui.draw.clipToBounds import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.graphicsLayer import androidx.compose.ui.layout.onSizeChanged @@ -111,6 +110,10 @@ private const val SWIPE_SETTLE_MS = 180 private const val SWIPE_EDGE_RESISTANCE = 0.3f private const val HAND_BACK_TIMEOUT_MS = 750L +// The strip is unclipped, so this spacing is what keeps the neighbours off-screen, and it has to +// clear the content inset plus a tile's shadow. It doubles as the gap between tiles while dragging. +private val TILE_GAP = 40.dp + data class NowPlayingArt( val id: String?, val ref: CoverArtRef?, @@ -329,6 +332,7 @@ fun NowPlayingScreen( val strip = frozenStrip ?: liveStrip val canGoNext = strip.getOrNull(2) != null val canGoPrevious = strip.getOrNull(0) != null + val stridePx = coverWidthPx + with(LocalDensity.current) { TILE_GAP.toPx() } fun animateSwipe(target: Float) { settleJob?.cancel() @@ -340,7 +344,7 @@ fun NowPlayingScreen( fun commitSwipe(direction: Int) { frozenStrip = rotateStrip(strip, direction) - swipeOffsetX += direction * coverWidthPx + swipeOffsetX += direction * stridePx if (direction > 0) onNext() else onSkipPrevious() animateSwipe(0f) } @@ -410,13 +414,14 @@ fun NowPlayingScreen( .padding(horizontal = 24.dp), horizontalAlignment = Alignment.CenterHorizontally, ) { + // Deliberately unclipped: a tile that fills the width would lose its drop + // shadow. TILE_GAP keeps the neighbours off-screen instead. Box( modifier = Modifier .weight(1f) .fillMaxWidth() .padding(top = 2.dp) - .clipToBounds() .onSizeChanged { coverWidthPx = it.width.toFloat() } .draggable( orientation = Orientation.Horizontal, @@ -428,7 +433,7 @@ fun NowPlayingScreen( delta < 0 && !canGoNext -> delta * SWIPE_EDGE_RESISTANCE else -> delta } - swipeOffsetX = (swipeOffsetX + resisted).coerceIn(-coverWidthPx, coverWidthPx) + swipeOffsetX = (swipeOffsetX + resisted).coerceIn(-stridePx, stridePx) }, onDragStarted = { dragging = true @@ -484,7 +489,7 @@ fun NowPlayingScreen( Modifier .fillMaxSize() .graphicsLayer { - translationX = (index - 1) * coverWidthPx + swipeOffsetX + translationX = (index - 1) * stridePx + swipeOffsetX }.then( when (index) { 0 -> Modifier.testTag(NOW_PLAYING_PREVIOUS_TAG) diff --git a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt index 658ed8d9..c6635480 100644 --- a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt @@ -444,38 +444,31 @@ class AppScreenshotTest { } @Test - fun nowPlaying() { - composeRule.setContent { - SubtracksTheme { - NowPlayingScreen( - state = Fixtures.playbackState(), - positionMs = 62_000, - title = "Kid A", - coverArt = CoverArtRef("art-al-kid-a", "test:art-al-kid-a"), - artwork = artworkColorsFromSeed(Color.rgb(120, 80, 200)), - onBack = {}, - onQueue = {}, - onPlayPause = {}, - onNext = {}, - onPrevious = {}, - onSeek = {}, - modifier = Modifier.fillMaxSize(), - ) - } - } - awaitText("Everything In Its Right Place") - composeRule.onRoot().captureRoboImage("src/test/screenshots/now_playing.png") - } + fun nowPlaying() = nowPlayingShot(Fixtures.playbackState(), "art-al-kid-a", "now_playing.png") @Test - fun nowPlayingModes() { + fun nowPlayingModes() = + nowPlayingShot( + Fixtures.playbackState().copy(shuffle = true, repeat = RepeatMode.One), + "art-al-kid-a", + "now_playing_modes.png", + ) + + @Test + fun nowPlayingNonSquare() = nowPlayingShot(Fixtures.playbackState(), "art-nonsquare", "now_playing_nonsquare.png") + + private fun nowPlayingShot( + state: PlaybackState, + coverArtId: String, + file: String, + ) { composeRule.setContent { SubtracksTheme { NowPlayingScreen( - state = Fixtures.playbackState().copy(shuffle = true, repeat = RepeatMode.One), + state = state, positionMs = 62_000, title = "Kid A", - coverArt = CoverArtRef("art-al-kid-a", "test:art-al-kid-a"), + coverArt = CoverArtRef(coverArtId, "test:$coverArtId"), artwork = artworkColorsFromSeed(Color.rgb(120, 80, 200)), onBack = {}, onQueue = {}, @@ -488,7 +481,7 @@ class AppScreenshotTest { } } awaitText("Everything In Its Right Place") - composeRule.onRoot().captureRoboImage("src/test/screenshots/now_playing_modes.png") + composeRule.onRoot().captureRoboImage("src/test/screenshots/$file") } @Test @@ -725,6 +718,7 @@ private object Fixtures { fun artEngine(): FakeImageLoaderEngine { val builder = FakeImageLoaderEngine.Builder() + builder.intercept("art-nonsquare", art("art-nonsquare", ArtKind.Album, width = 640, height = 256)) albums.mapNotNull { it.coverArt }.forEach { builder.intercept(it, art(it, ArtKind.Album)) } playlists.mapNotNull { it.coverArt }.forEach { builder.intercept(it, art(it, ArtKind.Playlist)) } artists.mapNotNull { it.coverArt }.forEach { builder.intercept(it, art(it, ArtKind.Artist)) } @@ -734,9 +728,14 @@ private object Fixtures { private fun art( key: String, kind: ArtKind, + width: Int = 256, + height: Int = 256, ): Image { - val size = 256 - val f = size.toFloat() + val w = width.toFloat() + val h = height.toFloat() + val cx = w / 2f + val cy = h / 2f + val span = maxOf(w, h) val random = Random(key.hashCode()) val base = random.nextFloat() * 360f @@ -752,21 +751,21 @@ private object Fixtures { listOf(color(0f), color(170f + random.nextFloat() * 30f)) } - val bitmap = Bitmap.createBitmap(size, size, Bitmap.Config.ARGB_8888) + val bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888) val canvas = Canvas(bitmap) val paint = Paint(Paint.ANTI_ALIAS_FLAG) canvas.drawColor(palette[0]) when (kind) { ArtKind.Album -> { - val stripe = f / 9f + val stripe = w / 9f canvas.save() - canvas.rotate(-30f, f / 2f, f / 2f) - var x = -f + canvas.rotate(-30f, cx, cy) + var x = -span var index = 1 - while (x < f * 2f) { + while (x < span * 2f) { paint.color = palette[index % palette.size] - canvas.drawRect(x, -f, x + stripe * 0.6f, f * 2f, paint) + canvas.drawRect(x, -span, x + stripe * 0.6f, span * 2f, paint) x += stripe index++ } @@ -774,12 +773,12 @@ private object Fixtures { } ArtKind.Playlist -> { - val step = f / 4f + val step = w / 4f var row = 1 var y = step / 2f - while (y < f) { + while (y < h) { var x = step / 2f - while (x < f) { + while (x < w) { paint.color = palette[row % palette.size] canvas.drawCircle(x, y, step * 0.3f, paint) x += step @@ -791,12 +790,12 @@ private object Fixtures { ArtKind.Artist -> { paint.style = Paint.Style.STROKE - paint.strokeWidth = f / 13f - var radius = f * 0.5f + paint.strokeWidth = span / 13f + var radius = span * 0.5f var index = 1 while (radius > 0f) { paint.color = palette[index % palette.size] - canvas.drawCircle(f / 2f, f / 2f, radius, paint) + canvas.drawCircle(cx, cy, radius, paint) radius -= paint.strokeWidth index++ } diff --git a/app/src/test/kotlin/com/subtracks/ui/playback/NowPlayingTapsTest.kt b/app/src/test/kotlin/com/subtracks/ui/playback/NowPlayingTapsTest.kt index 6c92da7c..10dfafdd 100644 --- a/app/src/test/kotlin/com/subtracks/ui/playback/NowPlayingTapsTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/playback/NowPlayingTapsTest.kt @@ -4,6 +4,7 @@ import androidx.activity.ComponentActivity import androidx.compose.ui.geometry.Offset import androidx.compose.ui.test.assertHeightIsAtLeast import androidx.compose.ui.test.assertHeightIsEqualTo +import androidx.compose.ui.test.assertIsNotDisplayed import androidx.compose.ui.test.assertIsNotEnabled import androidx.compose.ui.test.assertWidthIsAtLeast import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule @@ -285,6 +286,29 @@ class NowPlayingTapsTest { composeRule.onNodeWithTag(NOW_PLAYING_NEXT_TAG, useUnmergedTree = true).assertDoesNotExist() } + @Test + fun theSwipeTargetStaysInsideTheContentMargins() { + render() + + val target = composeRule.onNodeWithTag(NOW_PLAYING_COVER_TAG).fetchSemanticsNode().boundsInRoot + val screen = composeRule.onRoot().fetchSemanticsNode().boundsInRoot + + assertEquals( + "the swipe target must not reach into the screen margins", + screen.width - with(composeRule.density) { 48.dp.toPx() }, + target.width, + ) + } + + @Test + fun theNeighboursStayHiddenAtRest() { + render(previousArt = adjacentArt("s-previous"), nextArt = adjacentArt("s-next")) + + // TILE_GAP is what keeps them off-screen now that the strip is unclipped. + composeRule.onNodeWithTag(NOW_PLAYING_PREVIOUS_TAG, useUnmergedTree = true).assertIsNotDisplayed() + composeRule.onNodeWithTag(NOW_PLAYING_NEXT_TAG, useUnmergedTree = true).assertIsNotDisplayed() + } + @Test fun aShortSlowDragOnTheCoverDoesNotSkip() { var next = false