From 5cbb46ea8449dc394a7c779b8998ca86120add4d Mon Sep 17 00:00:00 2001 From: Alex Hart Date: Thu, 27 Aug 2026 17:02:11 -0300 Subject: [PATCH] Apply various bits of design polish. --- .../signal/camera/hud/StandardCameraHud.kt | 113 ++++++++++++------ .../camera/hud/StandardCameraHudTest.kt | 31 ++++- .../mediasend/screens/edit/AddAMessageRow.kt | 19 +-- .../mediasend/screens/edit/MediaEditScreen.kt | 15 ++- .../mediasend/screens/shared/NextButton.kt | 4 +- .../screens/edit/AddAMessageRowTest.kt | 18 +++ 6 files changed, 146 insertions(+), 54 deletions(-) diff --git a/feature/camera/src/main/java/org/signal/camera/hud/StandardCameraHud.kt b/feature/camera/src/main/java/org/signal/camera/hud/StandardCameraHud.kt index a274638a6f..881fa570da 100644 --- a/feature/camera/src/main/java/org/signal/camera/hud/StandardCameraHud.kt +++ b/feature/camera/src/main/java/org/signal/camera/hud/StandardCameraHud.kt @@ -25,6 +25,7 @@ import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.fillMaxHeight import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.heightIn import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.foundation.shape.CircleShape @@ -92,6 +93,12 @@ private val ZOOM_BAR_SIDE_MARGIN = 16.dp /** How long the time and the paused label take to trade the recording red between them. */ private const val PAUSED_TRANSITION_MS = 200 +/** The close and flash buttons along the top of the window, which the recording pill lines its middle up with. */ +private val TOP_CONTROL_SIZE = 48.dp + +/** How far the top controls sit in from the edges of the window. */ +private val TOP_CONTROL_MARGIN = 16.dp + data class StringResources( @param:StringRes val photoCaptureFailed: Int = 0, @param:StringRes val photoProcessingFailed: Int = 0, @@ -282,8 +289,8 @@ private fun BoxScope.StandardCameraHudContent( onClick = { emitter(StandardCameraHudEvents.CloseClick) }, enabled = !isRecordingHeld, modifier = modifier - .padding(16.dp) - .size(48.dp) + .padding(TOP_CONTROL_MARGIN) + .size(TOP_CONTROL_SIZE) .fadedIn(!isRecordingHeld) .background(colorResource(R.color.CameraHud_control_background), shape = CircleShape) .testTag(TestTags.CAMERA_HUD_CLOSE_BUTTON) @@ -306,7 +313,7 @@ private fun BoxScope.StandardCameraHudContent( enabled = !isRecordingHeld, modifier = Modifier .align(Alignment.TopEnd) - .padding(16.dp) + .padding(TOP_CONTROL_MARGIN) .fadedIn(!isRecordingHeld) .rotate(iconRotation) ) @@ -319,7 +326,7 @@ private fun BoxScope.StandardCameraHudContent( pausedLabel = if (stringResources.recordingPaused != 0) stringResource(stringResources.recordingPaused) else null, modifier = Modifier .align(Alignment.TopCenter) - .padding(top = 16.dp) + .padding(top = TOP_CONTROL_MARGIN) ) } @@ -622,14 +629,12 @@ private fun HorizontalControlBar( .fillMaxWidth() .padding(bottom = 40.dp, start = 40.dp, end = 40.dp) ) { - Box(modifier = Modifier.align(Alignment.CenterEnd).rotate(iconRotation)) { - CameraSwitchButton( - onClick = { emitter(StandardCameraHudEvents.SwitchCamera) }, - stringResources = stringResources, - enabled = captureButtonState != CaptureButtonState.RECORDING_HELD, - modifier = Modifier.fadedIn(captureButtonState != CaptureButtonState.RECORDING_HELD) - ) - } + CameraSwitchCorner( + isRecording = captureButtonState.isRecording, + iconRotation = iconRotation, + stringResources = stringResources, + emitter = emitter + ) Box(modifier = Modifier.align(Alignment.Center).rotate(iconRotation)) { captureSlot(captureButtonState) } @@ -640,6 +645,33 @@ private fun HorizontalControlBar( } } +/** + * The camera switch in its own corner of the bottom bar. The camera cannot be swapped out from under a running + * recording, so the button goes for as long as one runs; it is aligned into the corner rather than laid out beside + * anything, so nothing moves when it does. + */ +@Composable +private fun BoxScope.CameraSwitchCorner( + isRecording: Boolean, + iconRotation: Float, + stringResources: StringResources, + emitter: (StandardCameraHudEvents) -> Unit +) { + AnimatedVisibility( + visible = !isRecording, + enter = fadeIn(), + exit = fadeOut(), + modifier = Modifier.align(Alignment.CenterEnd) + ) { + Box(modifier = Modifier.rotate(iconRotation)) { + CameraSwitchButton( + onClick = { emitter(StandardCameraHudEvents.SwitchCamera) }, + stringResources = stringResources + ) + } + } +} + @Composable private fun VerticalControlBar( flashMode: FlashMode, @@ -667,6 +699,7 @@ private fun VerticalControlBar( flashMode = flashMode, emitter = emitter, stringResources = stringResources, + isRecording = captureButtonState.isRecording, enabled = captureButtonState != CaptureButtonState.RECORDING_HELD, modifier = Modifier.fadedIn(captureButtonState != CaptureButtonState.RECORDING_HELD) ) @@ -690,6 +723,7 @@ private fun FlashAndCameraTogglePill( flashMode: FlashMode, stringResources: StringResources, emitter: (StandardCameraHudEvents) -> Unit, + isRecording: Boolean, enabled: Boolean = true, modifier: Modifier = Modifier ) { @@ -710,16 +744,20 @@ private fun FlashAndCameraTogglePill( ) } - IconButton( - onClick = { emitter(StandardCameraHudEvents.SwitchCamera) }, - enabled = enabled, - modifier = Modifier.testTag(TestTags.CAMERA_HUD_SWITCH_BUTTON) - ) { - Icon( - imageVector = SignalIcons.CameraSwitch.imageVector, - contentDescription = if (stringResources.switchCamera != 0) stringResource(stringResources.switchCamera) else null, - tint = Color.White - ) + // The camera cannot be swapped out from under a running recording, so the pill gives the button up and closes + // around the flash for as long as one runs. + AnimatedVisibility(visible = !isRecording) { + IconButton( + onClick = { emitter(StandardCameraHudEvents.SwitchCamera) }, + enabled = enabled, + modifier = Modifier.testTag(TestTags.CAMERA_HUD_SWITCH_BUTTON) + ) { + Icon( + imageVector = SignalIcons.CameraSwitch.imageVector, + contentDescription = if (stringResources.switchCamera != 0) stringResource(stringResources.switchCamera) else null, + tint = Color.White + ) + } } } } @@ -753,18 +791,25 @@ private fun RecordingDurationDisplay( horizontalAlignment = Alignment.CenterHorizontally, modifier = modifier ) { + // The pill is shorter than the close and flash buttons it sits between, so it is centered in a row of their height + // to put the three of them on one line. Box( - modifier = Modifier - .background(lerp(recordingRed, pausedGray, pausedProgress), shape = CircleShape) - .padding(horizontal = 16.dp, vertical = 4.dp) - .testTag(TestTags.CAMERA_HUD_RECORDING_DURATION) + contentAlignment = Alignment.Center, + modifier = Modifier.heightIn(min = TOP_CONTROL_SIZE) ) { - Text( - text = timeText, - color = Color.White, - fontSize = 18.sp, - fontWeight = FontWeight.Medium - ) + Box( + modifier = Modifier + .background(lerp(recordingRed, pausedGray, pausedProgress), shape = CircleShape) + .padding(horizontal = 16.dp, vertical = 4.dp) + .testTag(TestTags.CAMERA_HUD_RECORDING_DURATION) + ) { + Text( + text = timeText, + color = Color.White, + fontSize = 18.sp, + fontWeight = FontWeight.Medium + ) + } } if (pausedLabel != null && pausedProgress > 0f) { @@ -791,7 +836,6 @@ private fun RecordingDurationDisplay( private fun CameraSwitchButton( onClick: () -> Unit, stringResources: StringResources, - enabled: Boolean = true, modifier: Modifier = Modifier ) { val contentDescription = if (stringResources.switchCamera != 0) { @@ -802,7 +846,6 @@ private fun CameraSwitchButton( IconButton( onClick = onClick, - enabled = enabled, modifier = modifier .size(52.dp) .background(colorResource(R.color.CameraHud_control_background), shape = CircleShape) @@ -829,7 +872,7 @@ private fun FlashToggleButton( onClick = onToggle, enabled = enabled, modifier = modifier - .size(48.dp) + .size(TOP_CONTROL_SIZE) .background(colorResource(R.color.CameraHud_control_background), shape = CircleShape) .testTag(TestTags.CAMERA_HUD_FLASH_BUTTON) ) { diff --git a/feature/camera/src/test/java/org/signal/camera/hud/StandardCameraHudTest.kt b/feature/camera/src/test/java/org/signal/camera/hud/StandardCameraHudTest.kt index 6d27cc96fa..af2664a5be 100644 --- a/feature/camera/src/test/java/org/signal/camera/hud/StandardCameraHudTest.kt +++ b/feature/camera/src/test/java/org/signal/camera/hud/StandardCameraHudTest.kt @@ -91,17 +91,30 @@ class StandardCameraHudTest { composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_CLOSE_BUTTON).assertIsNotEnabled() composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_FLASH_BUTTON).assertIsNotEnabled() - composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_SWITCH_BUTTON).assertIsNotEnabled() } - /** A locked recording leaves the hand free, so nothing has to be taken away. */ + /** A locked recording leaves the hand free, so nothing but the camera switch has to be taken away. */ @Test fun `Given a recording that is locked, when displayed, then the chrome around it can still be used`() { setContent(state = lockedRecording()) composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_CLOSE_BUTTON).assertIsEnabled() composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_FLASH_BUTTON).assertIsEnabled() - composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_SWITCH_BUTTON).assertIsEnabled() + } + + /** The camera cannot be swapped out from under a running recording, however that recording was started. */ + @Test + fun `Given a recording, when displayed, then the camera switch is gone`() { + setContent(state = lockedRecording()) + + composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_SWITCH_BUTTON).assertDoesNotExist() + } + + @Test + fun `Given a recording that is being held, when displayed, then the camera switch is gone`() { + setContent(state = heldRecording()) + + composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_SWITCH_BUTTON).assertDoesNotExist() } @Test @@ -246,7 +259,7 @@ class StandardCameraHudTest { setContent(state = heldRecording()) composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_CLOSE_BUTTON).performClick() - composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_SWITCH_BUTTON).performClick() + composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_FLASH_BUTTON).performClick() assertThat(events).isEmpty() } @@ -332,7 +345,15 @@ class StandardCameraHudTest { setContent(state = heldRecording()) composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_FLASH_BUTTON).assertIsNotEnabled() - composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_SWITCH_BUTTON).assertIsNotEnabled() + } + + /** The pill gives the switch up and closes around the flash, the same as the bottom bar drops it. */ + @Test + @Config(qualifiers = "w840dp-h1000dp") + fun `Given a window too large for the bottom bar, when a recording runs, then the pill has no camera switch`() { + setContent(state = lockedRecording()) + + composeTestRule.onNodeWithTag(TestTags.CAMERA_HUD_SWITCH_BUTTON).assertDoesNotExist() } //endregion diff --git a/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/AddAMessageRow.kt b/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/AddAMessageRow.kt index 4f0febda26..dba1b53dab 100644 --- a/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/AddAMessageRow.kt +++ b/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/AddAMessageRow.kt @@ -11,7 +11,6 @@ import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Row import androidx.compose.foundation.layout.height -import androidx.compose.foundation.layout.heightIn import androidx.compose.foundation.layout.padding import androidx.compose.foundation.shape.RoundedCornerShape import androidx.compose.material3.Icon @@ -39,11 +38,17 @@ import org.signal.mediasend.R import org.signal.mediasend.test.TestTags /** - * Mirrors the legacy send button's size. Has to be stated rather than left to the [IconButtons.IconButton] default of - * 40dp: the default draws the container inside the 48dp of layout that [androidx.compose.material3.minimumInteractiveComponentSize] - * reserves, leaving a button that is 8dp smaller than it looks like it should be and smaller than the row it sits in. + * The circle the arrow is drawn in. [IconButtons.IconButton] keeps the 48dp of layout that + * [androidx.compose.material3.minimumInteractiveComponentSize] reserves around it, so what a finger can hit stays a full + * touch target wide while the button reads as 40dp. */ -private val NextButtonSize = 48.dp +private val NextButtonSize = 40.dp + +/** + * The bar itself, stated rather than left to what is inside it: the icon buttons it carries each reserve a 48dp touch + * target, which would otherwise make the bar 4dp taller than it is meant to be. + */ +private val InputBarHeight = 44.dp /** * Because we need to be able to support stuff like mentions, styled text, and custom emoji, we need to allow @@ -88,7 +93,7 @@ fun AddAMessageRow( modifier = Modifier .background(color = MaterialTheme.colorScheme.surfaceVariant, shape = RoundedCornerShape(24.dp)) .weight(1f) - .heightIn(min = 44.dp) + .height(InputBarHeight) .then( if (viewOnce) { // A view-once send cannot carry a body, so the row becomes a static label rather than an entry point. @@ -123,7 +128,7 @@ fun AddAMessageRow( message?.takeIf { it.isNotBlank() } ?: stringResource(if (isReply) R.string.AddAMessageRow__add_a_reply else R.string.AddAMessageRow__message), Modifier .weight(1f) - .height(44.dp) + .height(InputBarHeight) .padding(end = if (viewOnceAvailable) 0.dp else 16.dp, top = 10.dp, bottom = 10.dp) ) } diff --git a/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/MediaEditScreen.kt b/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/MediaEditScreen.kt index f5fc6be834..00376946ad 100644 --- a/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/MediaEditScreen.kt +++ b/feature/media-send/src/main/java/org/signal/mediasend/screens/edit/MediaEditScreen.kt @@ -558,15 +558,20 @@ private fun MediaToolbar( } MediaEditControl(faded = faded, modifier = modifier) { + val isSmall = rememberWindowBreakpoint() is WindowBreakpoint.Small + + // Beyond a phone the toolbar runs down the window's end edge rather than along the bottom, so it has to be held off + // of that edge whichever kind of media it is built for. + val sideRailPadding = if (isSmall) Modifier else Modifier.padding(end = 24.dp) + when (focusedEditorState) { is EditorState.Image -> { - val breakpoint = rememberWindowBreakpoint() - val modifier = if (breakpoint is WindowBreakpoint.Small) { + val toolbarModifier = if (isSmall) { Modifier .navigationBarsPadding() .padding(horizontal = 16.dp) } else { - Modifier.padding(end = 24.dp) + sideRailPadding } imageController?.let { @@ -575,14 +580,14 @@ private fun MediaToolbar( state = state, editorState = focusedEditorState, onEvent = onEvent, - modifier = modifier + modifier = toolbarModifier .then(if (isTextEditing) Modifier.imePadding() else Modifier), enabled = !faded ) } } - else -> MediaEditorToolbar { + else -> MediaEditorToolbar(modifier = sideRailPadding) { MediaEditorToolbarSharedButtons( state = state, editorState = focusedEditorState, diff --git a/feature/media-send/src/main/java/org/signal/mediasend/screens/shared/NextButton.kt b/feature/media-send/src/main/java/org/signal/mediasend/screens/shared/NextButton.kt index f455826469..e88a3923d6 100644 --- a/feature/media-send/src/main/java/org/signal/mediasend/screens/shared/NextButton.kt +++ b/feature/media-send/src/main/java/org/signal/mediasend/screens/shared/NextButton.kt @@ -98,12 +98,12 @@ internal fun NextButton( .align(Alignment.TopCenter) .heightIn(min = NEXT_COUNT_HEIGHT) .widthIn(min = NEXT_COUNT_HEIGHT) - .background(color = recipientChatColor ?: MaterialTheme.colorScheme.primaryContainer, shape = CircleShape) + .background(color = recipientChatColor ?: MaterialTheme.colorScheme.primary, shape = CircleShape) .padding(horizontal = NEXT_COUNT_HORIZONTAL_PADDING) ) { Text( text = selectedMediaCount.toString(), - color = if (recipientChatColor != null) SignalTheme.colors.colorOnCustom else MaterialTheme.colorScheme.onPrimaryContainer, + color = if (recipientChatColor != null) SignalTheme.colors.colorOnCustom else MaterialTheme.colorScheme.onPrimary, style = MaterialTheme.typography.labelSmall, maxLines = 1, modifier = Modifier.testTag(TestTags.MEDIA_SEND_MEDIA_COUNT) diff --git a/feature/media-send/src/test/java/org/signal/mediasend/screens/edit/AddAMessageRowTest.kt b/feature/media-send/src/test/java/org/signal/mediasend/screens/edit/AddAMessageRowTest.kt index 9acd30f211..6d8470e61f 100644 --- a/feature/media-send/src/test/java/org/signal/mediasend/screens/edit/AddAMessageRowTest.kt +++ b/feature/media-send/src/test/java/org/signal/mediasend/screens/edit/AddAMessageRowTest.kt @@ -6,12 +6,15 @@ package org.signal.mediasend.screens.edit import android.app.Application +import androidx.compose.ui.geometry.Offset +import androidx.compose.ui.test.click import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.test.longClick import androidx.compose.ui.test.onNodeWithTag import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performTouchInput +import androidx.compose.ui.unit.dp import androidx.test.core.app.ApplicationProvider import org.junit.Assert.assertEquals import org.junit.Assert.assertNull @@ -75,6 +78,21 @@ class AddAMessageRowTest { assertEquals(emptyList(), events) } + /** + * The button draws smaller than a finger, so the 48dp of touch target reserved around the circle it draws is the part + * worth pinning down: a tap just off the circle still counts as one. + */ + @Test + fun `when a tap lands off the send button but inside its touch target, then the flow still advances`() { + setContent() + + composeTestRule.onNodeWithTag(TestTags.ADD_A_MESSAGE_NEXT_BUTTON).performTouchInput { + click(Offset(width / 2f, height + 3.dp.toPx())) + } + + assertEquals(1, nextClicks) + } + @Test fun `Given a blank message, when the row is displayed, then the placeholder is shown`() { setContent(message = " ")