From 3d69317a48eb884527eaa7a8bc3d1f422005b5d8 Mon Sep 17 00:00:00 2001 From: Alex Hart Date: Mon, 17 Aug 2026 10:31:57 -0300 Subject: [PATCH] Fix the media-send thumbnail row's delete affordance landing on the wrong image mid-reorder. --- .../mediasend/screens/edit/ThumbnailRow.kt | 24 ++- .../org/signal/mediasend/test/TestTags.kt | 5 + .../screens/edit/ThumbnailRowTest.kt | 151 ++++++++++++++++++ 3 files changed, 176 insertions(+), 4 deletions(-) create mode 100644 feature/media-send/src/test/java/org/signal/mediasend/screens/edit/ThumbnailRowTest.kt diff --git a/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/ThumbnailRow.kt b/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/ThumbnailRow.kt index fb2a9c5a52..b17c9b99a3 100644 --- a/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/ThumbnailRow.kt +++ b/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/ThumbnailRow.kt @@ -36,6 +36,7 @@ import androidx.compose.ui.draw.clip import androidx.compose.ui.graphics.Color import androidx.compose.ui.platform.LocalDensity import androidx.compose.ui.platform.LocalInspectionMode +import androidx.compose.ui.platform.testTag import androidx.compose.ui.unit.Dp import androidx.compose.ui.unit.dp import androidx.core.net.toUri @@ -53,6 +54,7 @@ import org.signal.core.ui.compose.list.reorderableList import org.signal.core.util.ContentTypeUtil import org.signal.glide.compose.GlideImage import org.signal.mediasend.screens.MediaSendMetrics +import org.signal.mediasend.test.TestTags import kotlin.math.abs import kotlin.math.floor import kotlin.math.roundToInt @@ -96,6 +98,15 @@ internal fun ThumbnailRow( ) val isReordering = reorderableListState.draggingItemIndex != null + // A thumbnail's page has to be looked up by identity rather than taken from its slot in the row: for the length of a + // drag the row renders the order that drag has built up while the pager is still on the pre-drag one, so the two + // disagree about what any given index refers to. Everything keyed off the pager - the delete affordance and the + // fish-eye - has to follow the media rather than the index, or it lands on whatever the drag has shuffled into the + // pager's slot and only corrects itself once the new order arrives. + val pageIndices = remember(selectedMedia) { + selectedMedia.withIndex().associate { (index, media) -> media.uri to index } + } + val draggableState = rememberDraggableState { delta -> val scaledDelta = delta * (pagerPageSize.toFloat() / itemStride) pagerState.dispatchRawDelta(-scaledDelta) @@ -170,10 +181,12 @@ internal fun ThumbnailRow( modifier = if (enabled) Modifier.reorderableList(reorderableListState) else Modifier ) { itemsIndexed(reorderBuffer.items, key = { _, media -> media.uri }) { index, media -> - val padding by remember(index) { + val pageIndex = pageIndices[media.uri] ?: index + + val padding by remember(pageIndex) { derivedStateOf { val currentPosition = pagerState.currentPage + pagerState.currentPageOffsetFraction - val distanceFromCenter = abs(index - currentPosition).coerceIn(0f, 1f) + val distanceFromCenter = abs(pageIndex - currentPosition).coerceIn(0f, 1f) lerp(MAX_PADDING, MIN_PADDING, distanceFromCenter) } } @@ -184,13 +197,14 @@ internal fun ThumbnailRow( modifier = Modifier.clip(MediaSendMetrics.SelectedMediaPreviewShape) ) { DeleteBox( - enabled = pagerState.currentPage == index + enabled = pagerState.currentPage == pageIndex, + testTag = TestTags.thumbnailRowDeleteIcon(media.uri.toString()) ) { Thumbnail( media = media, modifier = Modifier .padding(horizontal = padding) - .clickable(enabled = enabled) { onThumbnailClick(index) } + .clickable(enabled = enabled) { onThumbnailClick(pageIndex) } ) } } @@ -206,6 +220,7 @@ private fun lerp(start: Dp, stop: Dp, fraction: Float): Dp { @Composable private fun DeleteBox( enabled: Boolean, + testTag: String? = null, content: @Composable () -> Unit ) { Box { @@ -221,6 +236,7 @@ private fun DeleteBox( .size(MediaSendMetrics.SelectedMediaPreviewSize) .padding(10.dp) .align(Alignment.Center) + .then(if (testTag != null) Modifier.testTag(testTag) else Modifier) ) } } diff --git a/feature/media-send/src/main/java/org/signal/mediasend/test/TestTags.kt b/feature/media-send/src/main/java/org/signal/mediasend/test/TestTags.kt index 6b6c1b16d4..e97ed38c8b 100644 --- a/feature/media-send/src/main/java/org/signal/mediasend/test/TestTags.kt +++ b/feature/media-send/src/main/java/org/signal/mediasend/test/TestTags.kt @@ -19,6 +19,11 @@ object TestTags { const val MEDIA_EDITOR_TOOLBAR_ADD_MEDIA_BUTTON = "media_editor_toolbar_add_media_button" const val MEDIA_EDITOR_TOOLBAR_MUTE_BUTTON = "media_editor_toolbar_mute_button" + // Media Edit Screen + + /** Tag for the delete affordance the thumbnail row puts on the media at [uri]. */ + fun thumbnailRowDeleteIcon(uri: String): String = "thumbnail_row_delete_icon_$uri" + // Media Capture Screen const val MEDIA_CAPTURE_SCREEN = "media_capture_screen" const val MEDIA_CAPTURE_CAMERA_TOGGLE = "media_capture_camera_toggle" diff --git a/feature/media-send/src/test/java/org/signal/mediasend/screens/edit/ThumbnailRowTest.kt b/feature/media-send/src/test/java/org/signal/mediasend/screens/edit/ThumbnailRowTest.kt new file mode 100644 index 0000000000..eb0de66340 --- /dev/null +++ b/feature/media-send/src/test/java/org/signal/mediasend/screens/edit/ThumbnailRowTest.kt @@ -0,0 +1,151 @@ +/* + * Copyright 2026 Signal Messenger, LLC + * SPDX-License-Identifier: AGPL-3.0-only + */ + +package org.signal.mediasend.screens.edit + +import android.app.Application +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.size +import androidx.compose.foundation.pager.rememberPagerState +import androidx.compose.runtime.CompositionLocalProvider +import androidx.compose.ui.Modifier +import androidx.compose.ui.geometry.Offset +import androidx.compose.ui.platform.LocalInspectionMode +import androidx.compose.ui.platform.testTag +import androidx.compose.ui.test.hasClickAction +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.performTouchInput +import androidx.compose.ui.unit.dp +import androidx.core.net.toUri +import androidx.test.core.app.ApplicationProvider +import org.junit.Assert.assertEquals +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import org.signal.core.models.media.Media +import org.signal.core.ui.CoreUiDependenciesRule +import org.signal.core.ui.compose.theme.SignalTheme +import org.signal.mediasend.test.TestTags + +/** + * Covers the delete affordance staying on the media the pager is showing for the length of a reorder. The row renders + * the order the drag has built up while the pager stays on the pre-drag one until the drop lands in state, so whether + * the two still agree about which thumbnail is the focused one is not something either of them can be asked alone. + */ +@RunWith(RobolectricTestRunner::class) +@Config(application = Application::class, qualifiers = "w400dp-h800dp") +class ThumbnailRowTest { + + @get:Rule + val composeTestRule = createComposeRule() + + @get:Rule + val coreUiDependenciesRule = CoreUiDependenciesRule(ApplicationProvider.getApplicationContext()) + + private val reorders = mutableListOf>() + + @Test + fun `Given the focused thumbnail is dragged past its neighbour, when it has not been dropped, then the delete affordance is still on it`() { + setContent() + + composeTestRule.onNodeWithTag(HOST).performTouchInput { + down(thumbnailCenter(0)) + advanceEventTime(LONG_PRESS_MS) + moveTo(thumbnailCenter(1) + Offset(overshootPx, 0f)) + } + composeTestRule.waitForIdle() + + composeTestRule.onNodeWithTag(deleteIconTagFor(0)).assertExists() + composeTestRule.onNodeWithTag(deleteIconTagFor(1)).assertDoesNotExist() + } + + /** Guards the test above from passing on a drag that never swapped anything. */ + @Test + fun `Given the focused thumbnail is dragged past its neighbour, when it is dropped, then the two have traded places`() { + setContent() + + composeTestRule.onNodeWithTag(HOST).performTouchInput { + down(thumbnailCenter(0)) + advanceEventTime(LONG_PRESS_MS) + moveTo(thumbnailCenter(1) + Offset(overshootPx, 0f)) + up() + } + composeTestRule.waitForIdle() + + assertEquals(listOf(0 to 1), reorders) + } + + /** + * Read off the laid-out thumbnails rather than derived from their size, so the gesture lands where they actually are + * once the row's fish-eye padding has had its say. + */ + private fun thumbnailCenter(index: Int): Offset { + val thumbnail = composeTestRule.onAllNodes(hasClickAction())[index].fetchSemanticsNode() + + return Offset( + thumbnail.positionInRoot.x + thumbnail.size.width / 2f, + thumbnail.positionInRoot.y + thumbnail.size.height / 2f + ) + } + + /** Enough past the neighbour's center to be clear of the midpoint a swap is decided on. */ + private val overshootPx: Float + get() = with(composeTestRule.density) { OVERSHOOT.toPx() } + + private fun deleteIconTagFor(index: Int): String = TestTags.thumbnailRowDeleteIcon(MEDIA[index].uri.toString()) + + /** + * Inspected rather than loaded: a thumbnail emits nothing at all until Glide has something to put in it, and there is + * nothing to load here, so the row would otherwise lay out as a strip of empty slots with no geometry for a drag to + * work against. + */ + private fun setContent() { + composeTestRule.setContent { + SignalTheme { + CompositionLocalProvider(LocalInspectionMode provides true) { + Box(modifier = Modifier.size(ROW_WIDTH.dp, ROW_HEIGHT.dp).testTag(HOST)) { + ThumbnailRow( + selectedMedia = MEDIA, + pagerState = rememberPagerState(initialPage = 0, pageCount = { MEDIA.size }), + onReorder = { fromIndex, toIndex -> reorders += fromIndex to toIndex } + ) + } + } + } + } + + composeTestRule.waitForIdle() + } + + private companion object { + private const val HOST = "thumbnail_row_host" + private const val LONG_PRESS_MS = 600L + private const val ROW_WIDTH = 400f + private const val ROW_HEIGHT = 100f + + private val OVERSHOOT = 10.dp + + private val MEDIA: List = (0 until 3).map { index -> + Media( + uri = "content://media/$index".toUri(), + contentType = "image/jpeg", + date = index.toLong(), + width = 100, + height = 100, + size = 1024, + duration = 0, + isBorderless = false, + isVideoGif = false, + bucketId = "bucket", + caption = null, + transformProperties = null, + fileName = "media_$index.jpg" + ) + } + } +}