diff --git a/app/build.gradle.kts b/app/build.gradle.kts index e51ed9d..90e28dc 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -28,8 +28,8 @@ android { applicationId = "io.github.stozo04.openloop" minSdk = 26 targetSdk = 36 - versionCode = 27 - versionName = "1.0.27" + versionCode = 28 + versionName = "1.0.28" testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" vectorDrawables { diff --git a/app/src/main/java/io/github/stozo04/openloop/MainActivity.kt b/app/src/main/java/io/github/stozo04/openloop/MainActivity.kt index fe11fdd..7b40c31 100644 --- a/app/src/main/java/io/github/stozo04/openloop/MainActivity.kt +++ b/app/src/main/java/io/github/stozo04/openloop/MainActivity.kt @@ -16,6 +16,7 @@ import androidx.activity.result.PickVisualMediaRequest import androidx.activity.result.contract.ActivityResultContracts import androidx.activity.viewModels import androidx.annotation.OptIn +import androidx.annotation.StringRes import androidx.compose.foundation.Image import androidx.compose.foundation.background import androidx.compose.foundation.border @@ -211,6 +212,8 @@ class MainActivity : ComponentActivity() { // Friendly "That clip's a bit long" dialog (slice 07); held in the ViewModel so it // survives Activity recreation after the Photo Picker returns. val showTooLongDialog by viewModel.showImportTooLongDialog.collectAsStateWithLifecycle() + // Its "a bit short" sibling (issue #95 follow-up) — same recreation rationale. + val showTooShortDialog by viewModel.showImportTooShortDialog.collectAsStateWithLifecycle() // Hoisted out of the (non-composable) collect lambda below — stringResource can only // be read in a composable scope. @@ -221,6 +224,7 @@ class MainActivity : ComponentActivity() { val reversePreviewForwardMessage = stringResource(R.string.snackbar_reverse_preview_forward) val reversePreviewReportAction = stringResource(R.string.snackbar_reverse_preview_report_action) val importFailedMessage = stringResource(R.string.snackbar_import_failed) + val captureTooShortMessage = stringResource(R.string.snackbar_capture_too_short) val undoAction = stringResource(R.string.undo) // The "N loops deleted" plural is count-dependent, so we capture resources here (in a // composable scope) and resolve the quantity string inside the collect lambda below. @@ -301,6 +305,15 @@ class MainActivity : ComponentActivity() { // [OpenLoopViewModel.showImportTooLongDialog] so it survives Activity // recreation after the Photo Picker closes. BoomerangEvent.ImportTooLong -> Unit + // Picked clip was too short (issue #95 follow-up): same dialog-over- + // event pattern, driven by [OpenLoopViewModel.showImportTooShortDialog]. + BoomerangEvent.ImportTooShort -> Unit + // Capture stopped before the minimum loopable length (issue #95 + // follow-up): the ViewModel already discarded the scratch and returned + // to the viewfinder — nudge instead of failing silently. + BoomerangEvent.CaptureTooShort -> snackbarHostState.showSnackbar( + message = captureTooShortMessage, + ) // Loops marked for deletion (Issue #35): show an Undo snackbar. The real // file delete is deferred — Undo restores the tiles, any other dismissal // (timeout, swipe, or a superseding delete) commits the delete to disk. @@ -367,6 +380,10 @@ class MainActivity : ComponentActivity() { if (showTooLongDialog) { ImportTooLongDialog(onDismiss = { viewModel.dismissImportTooLongDialog() }) } + // Friendly "too short" guidance over the gallery (issue #95 follow-up). + if (showTooShortDialog) { + ImportTooShortDialog(onDismiss = { viewModel.dismissImportTooShortDialog() }) + } } } } @@ -788,13 +805,43 @@ fun PermissionExplanationScreen( /** * Friendly "That clip's a bit long" dialog shown when an imported library video exceeds the 30 s - * limit (slice 07). Hand-rolled in the app's neon aesthetic (matching [PermissionExplanationScreen] - * and the gallery overlay) rather than a stock Material3 `AlertDialog`, so it reads as warm guidance, - * not a system error. Acknowledgment-only — the user is already back on the gallery and nothing was + * limit (slice 07). Acknowledgment-only — the user is already back on the gallery and nothing was * copied; the single "Got it" button just dismisses. */ @Composable fun ImportTooLongDialog(onDismiss: () -> Unit) { + ImportClipLengthDialog( + titleRes = R.string.import_too_long_title, + bodyRes = R.string.import_too_long_body, + onDismiss = onDismiss, + ) +} + +/** + * The "a bit short" sibling: shown when a picked clip is under the minimum loopable window + * ([io.github.stozo04.openloop.ui.OpenLoopViewModel.MIN_TRIM_DURATION] — issue #95 follow-up). + * Same acknowledgment-only contract as [ImportTooLongDialog]. + */ +@Composable +fun ImportTooShortDialog(onDismiss: () -> Unit) { + ImportClipLengthDialog( + titleRes = R.string.import_too_short_title, + bodyRes = R.string.import_too_short_body, + onDismiss = onDismiss, + ) +} + +/** + * Shared friendly clip-length guidance dialog (too long / too short). Hand-rolled in the app's neon + * aesthetic (matching [PermissionExplanationScreen] and the gallery overlay) rather than a stock + * Material3 `AlertDialog`, so it reads as warm guidance, not a system error. + */ +@Composable +private fun ImportClipLengthDialog( + @StringRes titleRes: Int, + @StringRes bodyRes: Int, + onDismiss: () -> Unit, +) { Dialog(onDismissRequest = onDismiss) { Column( modifier = Modifier @@ -824,7 +871,7 @@ fun ImportTooLongDialog(onDismiss: () -> Unit) { Spacer(modifier = Modifier.height(20.dp)) Text( - text = stringResource(R.string.import_too_long_title), + text = stringResource(titleRes), style = MaterialTheme.typography.headlineSmall, color = Color.White, textAlign = TextAlign.Center @@ -833,7 +880,7 @@ fun ImportTooLongDialog(onDismiss: () -> Unit) { Spacer(modifier = Modifier.height(12.dp)) Text( - text = stringResource(R.string.import_too_long_body), + text = stringResource(bodyRes), style = MaterialTheme.typography.bodyMedium, color = Color.White.copy(alpha = 0.7f), textAlign = TextAlign.Center, @@ -850,7 +897,7 @@ fun ImportTooLongDialog(onDismiss: () -> Unit) { .height(48.dp) ) { Text( - text = stringResource(R.string.import_too_long_button), + text = stringResource(R.string.import_length_dialog_button), style = MaterialTheme.typography.labelLarge, color = LimeInk ) diff --git a/app/src/main/java/io/github/stozo04/openloop/ui/OpenLoopViewModel.kt b/app/src/main/java/io/github/stozo04/openloop/ui/OpenLoopViewModel.kt index 069b88b..fb9cc68 100644 --- a/app/src/main/java/io/github/stozo04/openloop/ui/OpenLoopViewModel.kt +++ b/app/src/main/java/io/github/stozo04/openloop/ui/OpenLoopViewModel.kt @@ -91,6 +91,23 @@ sealed interface BoomerangEvent { */ object ImportTooLong : BoomerangEvent + /** + * A picked library video was shorter than the minimum loopable window + * ([OpenLoopViewModel.MIN_TRIM_DURATION]) — issue #95 follow-up. Drives the friendly + * "That clip's a bit short" dialog; nothing is left in flight (rejected pre-copy, or the + * scratch copy was discarded). + */ + object ImportTooShort : BoomerangEvent + + /** + * A camera capture finalized shorter than the minimum loopable window, or with no encoded + * frames at all (`ERROR_NO_VALID_DATA`, a tap-and-release) — issue #95 follow-up. The scratch + * is discarded and the user is back on the viewfinder; drives a "record a little longer" + * snackbar instead of a silent return. (The shutter is tap-to-start / tap-to-stop, so the + * copy says "record longer", not "hold".) + */ + object CaptureTooShort : BoomerangEvent + /** * Importing a picked library video failed for a non-length reason — unreadable/revoked URI, an * unreadable duration, or a copy I/O error (slice 07). Drives a "Couldn't import that video." @@ -322,6 +339,19 @@ class OpenLoopViewModel( _showImportTooLongDialog.value = false } + /** + * Friendly "That clip's a bit short" dialog (issue #95 follow-up). Same [StateFlow]-not-event + * rationale as [showImportTooLongDialog]: it must survive Activity recreation after the Photo + * Picker returns. + */ + private val _showImportTooShortDialog = MutableStateFlow(false) + val showImportTooShortDialog: StateFlow = _showImportTooShortDialog.asStateFlow() + + /** Dismisses the import-too-short guidance dialog after the user taps "Got it". */ + fun dismissImportTooShortDialog() { + _showImportTooShortDialog.value = false + } + /** * One-shot signal for MainActivity to request [android.Manifest.permission.POST_NOTIFICATIONS] * on first Save (API 33+). Activity checks grant state before showing the system dialog. @@ -375,6 +405,13 @@ class OpenLoopViewModel( videoStorage.discardScratch(scratch) activeScratch = null _uiState.value = OpenLoopUiState.ReadyToCapture + // ERROR_NO_VALID_DATA = the stop landed before a single frame was + // encoded (a tap-and-release). To the user that's "too short", not a + // device failure — say so instead of returning silently (issue #95 + // follow-up). Other error codes keep the log-only behavior. + if (event.error == VideoRecordEvent.Finalize.ERROR_NO_VALID_DATA) { + viewModelScope.launch { _events.send(BoomerangEvent.CaptureTooShort) } + } } else { // Auto-route straight to the Trim screen (no preview landing pad). // The scratch stays in cache until the user saves (promote→raw) or discards. @@ -383,6 +420,22 @@ class OpenLoopViewModel( // repo) before routing — never block the main thread (ANDROID_STANDARDS §9). viewModelScope.launch { val durationMs = videoStorage.durationOf(outputFile) + // A clip below the minimum trim window would land on the Trim + // screen with pinned handles and a dead SAVE (Lesson 025) — a + // silent dead-end. Discard it and tell the user to hold longer + // instead (issue #95 follow-up). Also swallows an unreadable + // (<= 0) duration, which could never be trimmed either. + if (durationMs < MIN_TRIM_DURATION.inWholeMilliseconds) { + Log.w( + "OpenLoopViewModel", + "Capture finalized below the ${MIN_TRIM_DURATION.inWholeMilliseconds}ms minimum (${durationMs}ms); discarding", + ) + videoStorage.discardScratch(scratch) + activeScratch = null + _events.send(BoomerangEvent.CaptureTooShort) + _uiState.value = OpenLoopUiState.ReadyToCapture + return@launch + } Log.d("OpenLoopViewModel", "Capture finalized (${durationMs}ms): ${outputFile.absolutePath}") resetEditorTabForNewClip() _editorState.value = TrimState( @@ -513,10 +566,11 @@ class OpenLoopViewModel( /** * Result of the Android Photo Picker (launched `VideoOnly` from the gallery). [uri] is the picked * video, or `null` if the user backed out. On a valid pick we probe the duration *before* copying - * (so a >30 s clip is rejected with a friendly dialog without ever being copied), then copy the - * bytes into a fresh scratch file and enter the existing [OpenLoopUiState.Trim] flow exactly as a - * fresh capture would — the imported clip is just "a scratch that came from the picker." Any I/O - * or unreadable-duration failure routes back to the gallery with a snackbar; never a crash. + * (so a >30 s clip — or one under [MIN_TRIM_DURATION], issue #95 follow-up — is rejected with a + * friendly dialog without ever being copied), then copy the bytes into a fresh scratch file and + * enter the existing [OpenLoopUiState.Trim] flow exactly as a fresh capture would — the imported + * clip is just "a scratch that came from the picker." Any I/O or unreadable-duration failure + * routes back to the gallery with a snackbar; never a crash. */ fun onVideoPicked(uri: Uri?) { if (uri == null) return // user backed out of the picker @@ -526,6 +580,10 @@ class OpenLoopViewModel( when { // Unreadable duration → we can't enforce the ≤30 s rule, so don't import it. durationMs <= 0L -> failImport() + // Below the minimum trim window the editor would open with pinned handles and a + // dead SAVE (Lesson 025) — reject with friendly guidance instead, before any copy + // (issue #95 follow-up). + durationMs < MIN_TRIM_DURATION.inWholeMilliseconds -> warnTooShort() // Enforce the dialog's advertised "up to 30 s" cap LENIENTLY: the small grace // (IMPORT_DURATION_GRACE_MS) accepts a clip the user thinks is "30 s" but whose // container duration reads 30.2–30.5 s. The grace only ever makes us *more* permissive @@ -546,6 +604,12 @@ class OpenLoopViewModel( failImport() return@launch } + dur < MIN_TRIM_DURATION.inWholeMilliseconds -> { + // Pre-copy probe can over-read; the local copy is authoritative. + videoStorage.discardScratch(scratch) + warnTooShort() + return@launch + } exceedsImportDurationLimit(dur) -> { // Pre-copy probe can under-read; enforce the cap again on the local copy. videoStorage.discardScratch(scratch) @@ -589,6 +653,16 @@ class OpenLoopViewModel( _uiState.value = OpenLoopUiState.Gallery } + /** + * Picked clip is shorter than [MIN_TRIM_DURATION]: friendly dialog + back to the gallery + * (nothing left in flight — issue #95 follow-up). + */ + private suspend fun warnTooShort() { + _showImportTooShortDialog.value = true + _events.send(BoomerangEvent.ImportTooShort) + _uiState.value = OpenLoopUiState.Gallery + } + /** True when [durationMs] is past the advertised "up to 30 s" import cap (including grace). */ private fun exceedsImportDurationLimit(durationMs: Long): Boolean = durationMs > (IMPORT_MAX_DURATION + IMPORT_DURATION_GRACE).inWholeMilliseconds diff --git a/app/src/main/java/io/github/stozo04/openloop/ui/components/TrimFilmstripControls.kt b/app/src/main/java/io/github/stozo04/openloop/ui/components/TrimFilmstripControls.kt index 1015820..29c025c 100644 --- a/app/src/main/java/io/github/stozo04/openloop/ui/components/TrimFilmstripControls.kt +++ b/app/src/main/java/io/github/stozo04/openloop/ui/components/TrimFilmstripControls.kt @@ -391,7 +391,7 @@ private fun FilmstripTrimSelector( valueMs = startMs, rangeMs = 0f..durationMs.toFloat(), onSetValueMs = { target -> - val clamped = target.coerceIn(0L, endMs - minGapMs) + val clamped = clampTrimStartMs(target, endMs, minGapMs) onStartDrag(clamped) onDragEnd() }, @@ -407,7 +407,7 @@ private fun FilmstripTrimSelector( valueMs = endMs, rangeMs = 0f..durationMs.toFloat(), onSetValueMs = { target -> - val clamped = target.coerceIn(startMs + minGapMs, durationMs) + val clamped = clampTrimEndMs(target, startMs, durationMs, minGapMs) onEndDrag(clamped) onDragEnd() }, @@ -462,14 +462,14 @@ private fun FilmstripTrimSelector( val targetMs = dragAnchorMs + pxToMs(change.position.x - dragAnchorPx) when (dragging) { TrimDragTarget.START -> { - val clamped = targetMs.coerceIn(0L, curEndMs - minGapMs) + val clamped = clampTrimStartMs(targetMs, curEndMs, minGapMs) if (clamped != curStartMs) { haptics.performHapticFeedback(HapticFeedbackType.TextHandleMove) startDrag(clamped) } } TrimDragTarget.END -> { - val clamped = targetMs.coerceIn(curStartMs + minGapMs, durationMs) + val clamped = clampTrimEndMs(targetMs, curStartMs, durationMs, minGapMs) if (clamped != curEndMs) { haptics.performHapticFeedback(HapticFeedbackType.TextHandleMove) endDrag(clamped) diff --git a/app/src/main/java/io/github/stozo04/openloop/ui/components/TrimHandleMath.kt b/app/src/main/java/io/github/stozo04/openloop/ui/components/TrimHandleMath.kt new file mode 100644 index 0000000..9dd6f34 --- /dev/null +++ b/app/src/main/java/io/github/stozo04/openloop/ui/components/TrimHandleMath.kt @@ -0,0 +1,24 @@ +package io.github.stozo04.openloop.ui.components + +/** + * Pure clamp math for the trim handles, kept free of Compose/Android imports so it is JVM-testable + * ([io.github.stozo04.openloop.ui.components.TrimHandleMathTest]) — same split as + * `media/BoomerangSequence.kt`. + * + * Kotlin's `coerceIn(min, max)` throws `IllegalArgumentException` when `max < min`, and both trim + * bounds are derived from runtime state: a clip shorter than the minimum trim window (e.g. a 335 ms + * capture vs the 400 ms gap) inverts the range — Crashlytics 7169b499 / issue #95, Lesson 025. Each + * derived bound is therefore clamped to keep the range valid; on a too-short clip both handles pin + * (start at 0, end at the clip duration), matching the disabled NEXT button on the Trim screen. + */ + +/** START handle: clamp [targetMs] into `[0, endMs - minGapMs]`, flooring the upper bound at 0. */ +internal fun clampTrimStartMs(targetMs: Long, endMs: Long, minGapMs: Long): Long = + targetMs.coerceIn(0L, (endMs - minGapMs).coerceAtLeast(0L)) + +/** + * END handle: clamp [targetMs] into `[startMs + minGapMs, durationMs]`, capping the lower bound at + * [durationMs]. + */ +internal fun clampTrimEndMs(targetMs: Long, startMs: Long, durationMs: Long, minGapMs: Long): Long = + targetMs.coerceIn((startMs + minGapMs).coerceAtMost(durationMs), durationMs) diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 113b744..3a84072 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -46,5 +46,10 @@ Couldn\'t import that video. That clip\'s a bit long OpenLoop makes loops from videos up to 30 seconds. Pick a shorter clip and we\'ll loop it. - Got it + Got it + + + That clip\'s a bit short + OpenLoop needs at least half a second of video to make a loop. Pick a longer clip and we\'ll loop it. + That was quick! Record a little longer to make a loop. diff --git a/app/src/test/java/io/github/stozo04/openloop/ui/OpenLoopViewModelTest.kt b/app/src/test/java/io/github/stozo04/openloop/ui/OpenLoopViewModelTest.kt index 3bc8256..dfdbfe1 100644 --- a/app/src/test/java/io/github/stozo04/openloop/ui/OpenLoopViewModelTest.kt +++ b/app/src/test/java/io/github/stozo04/openloop/ui/OpenLoopViewModelTest.kt @@ -339,6 +339,7 @@ class OpenLoopViewModelTest { every { Log.d(any(), any()) } returns 0 every { Log.e(any(), any()) } returns 0 every { Log.e(any(), any(), any()) } returns 0 + every { Log.w(any(), any()) } returns 0 // Default: onboarding NOT completed (first-time user) fakePreferencesRepository = FakeUserPreferencesRepository(initialOnboardingCompleted = false) @@ -651,6 +652,54 @@ class OpenLoopViewModelTest { assertTrue(fakeVideoStorage.saved.isEmpty()) } + @Test + fun `finalize below the minimum loop length discards the scratch and reports CaptureTooShort`() = + runTest(mainDispatcherRule.testDispatcher) { + viewModel.onPermissionsChecked(true) // ReadyToCapture + fakeVideoStorage.fixedDurationMs = 335L // the issue #95 clip — under MIN_TRIM_DURATION + val events = mutableListOf() + val job = backgroundScope.launch { viewModel.events.toList(events) } + + val slot = slot<(VideoRecordEvent) -> Unit>() + every { cameraManager.startRecording(any(), capture(slot)) } returns fakeRecording + viewModel.startBurstCapture(cameraManager) + val finalizeEvent = mockk(relaxed = true) + every { finalizeEvent.hasError() } returns false + slot.captured.invoke(finalizeEvent) + advanceUntilIdle() + + // Never lands on Trim: back to the camera with the scratch discarded and a nudge sent. + assertEquals(OpenLoopUiState.ReadyToCapture, viewModel.uiState.value) + assertTrue(events.contains(BoomerangEvent.CaptureTooShort)) + assertEquals(1, fakeVideoStorage.discardedScratches.size) + assertNull(viewModel.editorState.value) + job.cancel() + } + + @Test + fun `finalize with ERROR_NO_VALID_DATA reports CaptureTooShort instead of a silent return`() = + runTest(mainDispatcherRule.testDispatcher) { + viewModel.onPermissionsChecked(true) // ReadyToCapture + val events = mutableListOf() + val job = backgroundScope.launch { viewModel.events.toList(events) } + + val slot = slot<(VideoRecordEvent) -> Unit>() + every { cameraManager.startRecording(any(), capture(slot)) } returns fakeRecording + viewModel.startBurstCapture(cameraManager) + // A tap-and-release: CameraX finalizes with no encoded frames at all. + val finalizeEvent = mockk(relaxed = true) + every { finalizeEvent.hasError() } returns true + every { finalizeEvent.error } returns VideoRecordEvent.Finalize.ERROR_NO_VALID_DATA + slot.captured.invoke(finalizeEvent) + advanceUntilIdle() + + assertEquals(OpenLoopUiState.ReadyToCapture, viewModel.uiState.value) + assertTrue(events.contains(BoomerangEvent.CaptureTooShort)) + assertEquals(1, fakeVideoStorage.discardedScratches.size) + assertNull(viewModel.editorState.value) + job.cancel() + } + @Test fun `finalize error discards the scratch and returns to ReadyToCapture`() { viewModel.onPermissionsChecked(true) // ReadyToCapture @@ -1562,6 +1611,61 @@ class OpenLoopViewModelTest { job.cancel() } + @Test + fun `onVideoPicked under the minimum loop length warns and copies nothing`() = + runTest(mainDispatcherRule.testDispatcher) { + fakeVideoImporter.probeMs = 335L // the issue #95 clip — under MIN_TRIM_DURATION + val events = mutableListOf() + val job = backgroundScope.launch { viewModel.events.toList(events) } + + viewModel.onVideoPicked(fakeUri) + advanceUntilIdle() + + assertEquals(OpenLoopUiState.Gallery, viewModel.uiState.value) + assertTrue(events.contains(BoomerangEvent.ImportTooShort)) + assertTrue(viewModel.showImportTooShortDialog.value) + // Caught before any copy or scratch mint. + assertEquals(0, fakeVideoImporter.importCallCount) + assertEquals(0, fakeVideoStorage.createScratchCount) + assertNull(viewModel.editorState.value) + job.cancel() + } + + @Test + fun `onVideoPicked when post-copy duration is under the minimum warns and discards the scratch`() = + runTest(mainDispatcherRule.testDispatcher) { + // Pre-copy probe over-reads; post-copy durationOf is authoritative. + fakeVideoImporter.probeMs = 800L + fakeVideoStorage.fixedDurationMs = 335L + val events = mutableListOf() + val job = backgroundScope.launch { viewModel.events.toList(events) } + + viewModel.onVideoPicked(fakeUri) + advanceUntilIdle() + + assertEquals(OpenLoopUiState.Gallery, viewModel.uiState.value) + assertTrue(events.contains(BoomerangEvent.ImportTooShort)) + assertTrue(viewModel.showImportTooShortDialog.value) + assertEquals(1, fakeVideoImporter.importCallCount) + assertEquals(1, fakeVideoStorage.discardedScratches.size) + assertNull(viewModel.editorState.value) + job.cancel() + } + + @Test + fun `onVideoPicked exactly at the minimum loop length is accepted`() = + runTest(mainDispatcherRule.testDispatcher) { + // The gate is strictly-below: a clip of exactly MIN_TRIM_DURATION is loopable. + fakeVideoImporter.probeMs = 400L + fakeVideoStorage.fixedDurationMs = 400L + + viewModel.onVideoPicked(fakeUri) + advanceUntilIdle() + + assertTrue(viewModel.uiState.value is OpenLoopUiState.Trim) + assertEquals(1, fakeVideoImporter.importCallCount) + } + @Test fun `onVideoPicked with unreadable duration fails to import without copying`() = runTest(mainDispatcherRule.testDispatcher) { diff --git a/app/src/test/java/io/github/stozo04/openloop/ui/components/TrimHandleMathTest.kt b/app/src/test/java/io/github/stozo04/openloop/ui/components/TrimHandleMathTest.kt new file mode 100644 index 0000000..75c44cb --- /dev/null +++ b/app/src/test/java/io/github/stozo04/openloop/ui/components/TrimHandleMathTest.kt @@ -0,0 +1,130 @@ +package io.github.stozo04.openloop.ui.components + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * JVM tests for the trim-handle clamp math extracted from `FilmstripTrimSelector`. + * + * Regression coverage for Crashlytics 7169b499 / issue #95 (Lesson 025): a 335 ms clip on the + * Trim screen inverted the `coerceIn` range (`maximum 335 is less than minimum 400`) and crashed + * on the first handle drag or accessibility `setProgress`. The clamp must never throw, for any + * combination of clip duration and handle positions. + */ +class TrimHandleMathTest { + + private companion object { + /** OpenLoopViewModel.MIN_TRIM_DURATION at the time of the crash. */ + const val MIN_GAP_MS = 400L + + /** The exact clip duration from the Crashlytics report. */ + const val CRASH_DURATION_MS = 335L + } + + // ── Regression: clip shorter than the minimum trim window (the issue #95 crash) ── + + @Test + fun `end handle on 335ms clip does not throw and pins to clip duration`() { + // Was coerceIn(0 + 400, 335) -> IllegalArgumentException before the guard. + val clamped = clampTrimEndMs( + targetMs = 200L, + startMs = 0L, + durationMs = CRASH_DURATION_MS, + minGapMs = MIN_GAP_MS, + ) + assertEquals(CRASH_DURATION_MS, clamped) + } + + @Test + fun `start handle on 335ms clip does not throw and pins to zero`() { + // Was coerceIn(0, 335 - 400) -> inverted range before the guard. + val clamped = clampTrimStartMs( + targetMs = 200L, + endMs = CRASH_DURATION_MS, + minGapMs = MIN_GAP_MS, + ) + assertEquals(0L, clamped) + } + + @Test + fun `short clip handles are immovable regardless of drag target`() { + for (targetMs in longArrayOf(Long.MIN_VALUE, -1L, 0L, 100L, 335L, 400L, Long.MAX_VALUE)) { + assertEquals(0L, clampTrimStartMs(targetMs, endMs = CRASH_DURATION_MS, minGapMs = MIN_GAP_MS)) + assertEquals( + CRASH_DURATION_MS, + clampTrimEndMs(targetMs, startMs = 0L, durationMs = CRASH_DURATION_MS, minGapMs = MIN_GAP_MS), + ) + } + } + + // ── Normal clips: original clamp behavior is preserved ── + + @Test + fun `start handle clamps to min gap below the end handle`() { + val clamped = clampTrimStartMs(targetMs = 4_900L, endMs = 5_000L, minGapMs = MIN_GAP_MS) + assertEquals(4_600L, clamped) + } + + @Test + fun `start handle clamps negative drag to zero`() { + val clamped = clampTrimStartMs(targetMs = -250L, endMs = 5_000L, minGapMs = MIN_GAP_MS) + assertEquals(0L, clamped) + } + + @Test + fun `start handle passes through an in-range target`() { + val clamped = clampTrimStartMs(targetMs = 1_200L, endMs = 5_000L, minGapMs = MIN_GAP_MS) + assertEquals(1_200L, clamped) + } + + @Test + fun `end handle clamps to min gap above the start handle`() { + val clamped = clampTrimEndMs(targetMs = 2_100L, startMs = 2_000L, durationMs = 10_000L, minGapMs = MIN_GAP_MS) + assertEquals(2_400L, clamped) + } + + @Test + fun `end handle clamps overshoot to clip duration`() { + val clamped = clampTrimEndMs(targetMs = 12_000L, startMs = 0L, durationMs = 10_000L, minGapMs = MIN_GAP_MS) + assertEquals(10_000L, clamped) + } + + @Test + fun `end handle passes through an in-range target`() { + val clamped = clampTrimEndMs(targetMs = 7_500L, startMs = 2_000L, durationMs = 10_000L, minGapMs = MIN_GAP_MS) + assertEquals(7_500L, clamped) + } + + // ── Boundary durations ── + + @Test + fun `clip exactly at the minimum trim window pins both handles`() { + assertEquals(0L, clampTrimStartMs(targetMs = 150L, endMs = MIN_GAP_MS, minGapMs = MIN_GAP_MS)) + assertEquals( + MIN_GAP_MS, + clampTrimEndMs(targetMs = 150L, startMs = 0L, durationMs = MIN_GAP_MS, minGapMs = MIN_GAP_MS), + ) + } + + @Test + fun `zero duration clip does not throw`() { + assertEquals(0L, clampTrimStartMs(targetMs = 50L, endMs = 0L, minGapMs = MIN_GAP_MS)) + assertEquals(0L, clampTrimEndMs(targetMs = 50L, startMs = 0L, durationMs = 0L, minGapMs = MIN_GAP_MS)) + } + + @Test + fun `no duration and target combination ever throws or escapes the clip bounds`() { + // Sweep every duration around the minimum window with hostile targets; the clamp must + // stay total (no inverted-range throw) and keep results inside [0, duration]. + val targets = longArrayOf(Long.MIN_VALUE, -1L, 0L, 1L, 399L, 400L, 401L, 30_000L, Long.MAX_VALUE) + for (durationMs in 0L..1_000L step 7) { + for (targetMs in targets) { + val start = clampTrimStartMs(targetMs, endMs = durationMs, minGapMs = MIN_GAP_MS) + val end = clampTrimEndMs(targetMs, startMs = 0L, durationMs = durationMs, minGapMs = MIN_GAP_MS) + assertTrue("start $start out of [0, $durationMs]", start in 0L..durationMs) + assertTrue("end $end out of [0, $durationMs]", end in 0L..durationMs) + } + } + } +} diff --git a/docs/e2e/2026-07-06_104410.md b/docs/e2e/2026-07-06_104410.md new file mode 100644 index 0000000..9166536 --- /dev/null +++ b/docs/e2e/2026-07-06_104410.md @@ -0,0 +1,103 @@ +# OpenLoop E2E run — 2026-07-06 10:44 + +**Build:** versionName 1.0.28 (versionCode 28) · debug APK +**Device:** Pixel 8 AVD (`Pixel_8`) · Android 17 (API 37, 16 KB page `emu64xa16k`) · x86_64 · serial emulator-5556 +**Branch / commit:** `claude/github-issue-95-fix-xv25e3` @ fddfb26 (PR #97 pre-merge gate) +**Driver:** Claude Code `run-e2e` skill +**Logcat:** `C:\Users\gates\AppData\Local\Temp\openloop_e2e\logcat_2026-07-06_102941.txt` + +## Verdict + +**PASS** — the issue-#95 crash scenario (trim-handle drag on a sub-400 ms clip) no longer +crashes: both handles pin and the app stays healthy, and the full capture → trim → speed → +reverse loop → B&W → save → gallery-playback path completed with 0 crashes, 0 ANRs, and +0 reverse timeouts. JVM unit tests (including the new `TrimHandleMathTest`) also passed +(`BUILD SUCCESSFUL`, exit 0). PR #97 is safe to merge from this run's perspective. + +## PR-specific regression check (issue #95 / Crashlytics 7169b499) + +The Crashlytics crash was `coerceIn(400, 335)` — a 335 ms clip reaching the Trim screen with +`MIN_TRIM_DURATION` = 400 ms. Reproduced deliberately: + +| Probe | Setup | Result | +|-------|-------|--------| +| Ultra-short capture (tap-tap, ~170 ms) | shutter start/stop back-to-back | CameraX finalized `ERROR_NO_VALID_DATA` (error 8); ViewModel logged "Video burst recording failed: 8" and returned to viewfinder cleanly. Never reaches Trim on this emulator — no crash surface. | +| **335 ms imported clip** (ffmpeg `testsrc`, 0.333 s — the exact Crashlytics duration) | Gallery → Import → picker → Trim tab shows `00:00.0 — 00:00.3` | **No crash.** START-handle drag (pre-fix: `coerceIn(0, −67)` → throw) pinned at 0. END-handle drag (pre-fix: `coerceIn(400, 333)` → throw) pinned at 333 ms. Window label unchanged, app responsive. Zero `IllegalArgumentException` / `coerce` / `FATAL` lines in logcat. | +| SAVE on the 335 ms clip | tap SAVE while window is sub-minimum | Correct no-op (`trimValid` gate) — stayed on Trim, no render attempted. | + +Gesture mechanics were validated separately on a normal clip (same slow-drag registered and +moved both handles, changing the window `00:00.0–00:03.9 → 00:00.7–00:02.7`), so the pinned +handles on the short clip reflect the fix's clamp math, not a missed drag. + +## Flow coverage + +| Step | Action | Result | Evidence (logcat line / UI text) | +|------|--------|--------|----------------------------------| +| Launch | cold start to viewfinder | ✅ | shutter/Gallery/Flip Camera visible (returning-user path, no onboarding) | +| Record | ~4 s capture | ✅ | Trim tab `00:00.0 — 00:03.9` | +| Trim tab | END drag 3.9→2.7 s, START drag 0→0.7 s | ✅ | `00:00.7 — 00:02.7`; matches `trim=675..2697ms` in reverse log | +| Speed tab | SeekBar track tap 2x → 0.4x | ✅ | "Current speed 0.4x"; output estimate 10.0 s | +| Loop tab | **Reverse loop** (non-default, reverse-needing) | ✅ | `reverse.complete attempts=1` in ~3.7 s; `viewModel.ensureReversed.ok gen=4` (49 samples, 694 KB) | +| Filter tab | B&W | ✅ | B&W selected; no "Preview unavailable" banner | +| Create/Save | render boomerang | ✅ | `BoomerangRenderWorker … Worker result SUCCESS` 10:41:02 (~3 s render); share sheet opened for `boom_1783352459166_from_1783352459026.mp4` | +| View loop | gallery playback | ✅ | fresh `ExoPlayerImpl: Init d83e4ea` 10:42:18 after tapping newest thumbnail; playback overlay ("Close preview" / SEND) shown | + +## Concerning logs + +`scan-logcat.ps1` counts for the full run: + +| Class | Signature | Count | +|-------|-----------|------:| +| CRASH | FATAL EXCEPTION (app crash) | 0 | +| CRASH | ANR (app not responding) | 0 | +| CRASH | process died | 0 | +| CRASH | surface has been released (b09e527) | 0 | +| CRASH | dequeueOutputBuffer native (3a506c4e) | 0 | +| CRASH | MediaCodec CodecException | 0 | +| CRASH | FGS type unknown (API 34 regression) | 0 | +| TIMEOUT | reverse preview timeout (120 s bucket) | 0 | +| TIMEOUT | reverse preview failure (non-fatal) | 0 | +| CHURN | codec reclaim pressure | 554 | +| CHURN | dead-thread handler race | 30 | +| CHURN | codec buffer starvation | 246 | +| CHURN | codec components created (c2.*) | 43 | + +All CRASH and TIMEOUT rows are zero. The reverse pipeline was textbook-healthy +(`settle → pass1.done → pass2.done → complete attempts=1 → ensureReversed.ok`). + +## Findings & issues to research deeper + +No hard findings. Smells: + +1. **Codec churn counts are elevated vs the 2026-06-30 baseline run** (reclaim 554 vs 39, + starvation 246 vs 71, components 43 vs 29; dead-thread 30 vs 24). Attribution: pid-grouping + shows the app process (6017) emitted ~95 % of the reclaim and ~80 % of the starvation lines, + so this is OpenLoop, not picker/system noise. Context: this run drove *far* more + interactions than the baseline (2 rejected captures, an import + short-clip editor session + + discard, a second capture, 4 trim drags, tab switching, reverse, render, gallery playback), + and no crash/timeout/UX degradation was tied to the churn. Advisory only — same + pre-existing smell the 06-30 report flagged (lesson 023 disease fingerprint). Next step if + it ever pairs with a stall: count `Created component [c2.*]` per editor interaction on a + physical device. +2. **`uiauto.ps1` label matching hit the dialog title instead of its button** — tapping + "Discard" matched "Discard this clip?" (title) first, silently dismissing the dialog and + keeping the clip. Driver friction, not an app bug, but worth an exact-match option in the + script if this recurs. + +## What could not be verified + +- **Sub-400 ms clip via the *capture* path** — the emulator's encoder finalizes + `ERROR_NO_VALID_DATA` for stops under ~700 ms, so a 335 ms *captured* clip can't be produced + here; the crash scenario was exercised via the import path instead (which lesson 025 confirms + shares the unfloored `trimEndMs = durationMs` code path). On a physical device a fast + tap-tap may still reach Trim via capture — the JVM `TrimHandleMathTest` covers the math for + both. +- **TalkBack / accessibility `setProgress` call sites** — two of the four fixed call sites are + the a11y actions; not driven in this run (no TalkBack on the AVD). Covered by + `TrimHandleMathTest` at the math layer only. +- Playback of the saved loop confirmed via logcat + overlay UI, not visually (screenshots + rate-limited per skill policy). +- Release build, lint, and instrumented tests were **not** run here — this was the E2E gate + only; PR #97's own CI/DoD evidence covers those. +- Single device (Pixel 8 AVD, API 37). No API-34 FGS lane (change is UI-math only, no + service/media-pipeline surface) and no Samsung/OEM lane this run. diff --git a/docs/e2e/2026-07-06_111500-capture-too-short-snackbar.png b/docs/e2e/2026-07-06_111500-capture-too-short-snackbar.png new file mode 100644 index 0000000..04d25ce Binary files /dev/null and b/docs/e2e/2026-07-06_111500-capture-too-short-snackbar.png differ diff --git a/docs/e2e/2026-07-06_111500-import-too-short-dialog.png b/docs/e2e/2026-07-06_111500-import-too-short-dialog.png new file mode 100644 index 0000000..ef97eb6 Binary files /dev/null and b/docs/e2e/2026-07-06_111500-import-too-short-dialog.png differ diff --git a/docs/e2e/2026-07-06_111500-too-short-ux.md b/docs/e2e/2026-07-06_111500-too-short-ux.md new file mode 100644 index 0000000..e9cb1c9 --- /dev/null +++ b/docs/e2e/2026-07-06_111500-too-short-ux.md @@ -0,0 +1,41 @@ +# OpenLoop verification — "clip too short" UX (issue #95 follow-up) — 2026-07-06 11:15 + +**Build:** versionName 1.0.28 (versionCode 28) · debug APK (branch build with the too-short UX) +**Device:** Pixel 8 AVD (`Pixel_8`) · Android 17 (API 37) · x86_64 · serial emulator-5556 +**Branch / commit:** `claude/github-issue-95-fix-xv25e3` (PR #97, on top of the trim-clamp fix) +**Driver:** Claude Code, `run-e2e` uiautomator tooling +**Logcat:** `%TEMP%\openloop_e2e\logcat_verify_tooshort.txt` (local) + +## What changed + +Instead of silently landing a sub-`MIN_TRIM_DURATION` (400 ms) clip on the Trim screen with +pinned handles and a dead SAVE, the app now tells the user: + +- **Import path** (`onVideoPicked`): a clip under 400 ms is rejected *before any copy* (and + re-checked post-copy) with the friendly **"That clip's a bit short"** dialog — the sibling of + the existing "a bit long" dialog — and the user stays on the Gallery. +- **Capture path** (`Finalize` handler): a capture that finalizes under 400 ms — or with zero + encoded frames (`ERROR_NO_VALID_DATA`, a tap-and-release) — discards the scratch, returns to + the viewfinder, and shows the **"That was quick! Record a little longer to make a loop."** + snackbar instead of a silent return. (Copy corrected post-review: the shutter is + tap-to-start / tap-to-stop, so the first draft's "hold the record button" was wrong.) +- The Lesson-025 clamp math and `trimValid` gate stay as the backstop. + +## Verification (all live on the emulator) + +| Probe | Result | Evidence | +|-------|--------|----------| +| Tap-tap capture (~170 ms, `ERROR_NO_VALID_DATA`) | ✅ snackbar shown on viewfinder, app healthy | [`2026-07-06_111500-capture-too-short-snackbar.png`](2026-07-06_111500-capture-too-short-snackbar.png) + uiautomator dump | +| Import 335 ms clip (the exact issue-#95 duration) | ✅ "That clip's a bit short" dialog; never enters Trim; "Got it" returns to Gallery cleanly | [`2026-07-06_111500-import-too-short-dialog.png`](2026-07-06_111500-import-too-short-dialog.png) + uiautomator dump | +| Regression: normal ~4 s capture | ✅ still auto-routes to Trim (`00:00.0 — 00:03.9`) | uiautomator dump | +| Logcat scan | ✅ no FATAL / ANR / `IllegalArgument` / coerce lines | logcat grep | + +JVM suite: `:app:testDebugUnitTest` green with 5 new tests (pre-copy reject, post-copy reject, +exactly-400 ms accepted, sub-400 ms finalize discard + event, `ERROR_NO_VALID_DATA` event). +`:app:lintDebug` 0 errors (warnings unchanged, none new); `:app:assembleRelease` green. + +## Not verified here + +- Sub-400 ms *capture with valid data* (emulator encoder can't produce one — covered by the + JVM finalize test with `durationOf = 335 ms`). +- The capture-path snackbar copy on a physical device / TalkBack readout. diff --git a/docs/lessons_learned/025-trim-coercein-range-guard-short-clip.md b/docs/lessons_learned/025-trim-coercein-range-guard-short-clip.md new file mode 100644 index 0000000..7cb31af --- /dev/null +++ b/docs/lessons_learned/025-trim-coercein-range-guard-short-clip.md @@ -0,0 +1,64 @@ +# 025 — `coerceIn` with runtime-derived bounds throws on short clips; clamp the bound and JVM-test the math + +## What went wrong + +`FilmstripTrimSelector` in `ui/components/TrimFilmstripControls.kt` clamped a dragged handle +position into the valid trim window with bounds derived from runtime state: + +```kotlin +// END handle drag +val clampedEnd = targetMs.coerceIn(curStartMs + minGapMs, durationMs) +// START handle drag +val clampedStart = targetMs.coerceIn(0L, curEndMs - minGapMs) +``` + +Kotlin's `coerceIn(min, max)` **throws `IllegalArgumentException` when `max < min`** instead of +silently clamping. Nothing floors a clip's duration before it reaches the Trim screen — both the +capture-finalize path and the gallery-import path in `OpenLoopViewModel` set +`trimEndMs = durationMs` as-is — so a tap-and-release capture shorter than +`MIN_TRIM_DURATION` (400 ms) inverted the range on the first handle drag or TalkBack +`setProgress`: `coerceIn(400, 335)`. + +Crashlytics issue `7169b499fee6554684dba69b1ae1a8f0` (first seen v1.0.27): +"Cannot coerce value to an empty range: maximum 335 is less than minimum 400." + +The same expression was duplicated inline at **four** call sites (drag + accessibility, per +handle), none of them reachable by a JVM test while inline in a composable lambda. + +## Pattern + +1. **Never call `coerceIn(min, max)` where either bound is an arithmetic expression over runtime + state without clamping the bound itself first.** Keep the range valid by construction: + + ```kotlin + // START handle: floor the upper bound at 0 + fun clampTrimStartMs(targetMs: Long, endMs: Long, minGapMs: Long): Long = + targetMs.coerceIn(0L, (endMs - minGapMs).coerceAtLeast(0L)) + + // END handle: cap the lower bound at the clip duration + fun clampTrimEndMs(targetMs: Long, startMs: Long, durationMs: Long, minGapMs: Long): Long = + targetMs.coerceIn((startMs + minGapMs).coerceAtMost(durationMs), durationMs) + ``` + + On a sub-400 ms clip both handles pin (start at 0, end at `durationMs`) — correct UX, since the + NEXT button and all editor-advance actions are already disabled by `trimValid` in + `TrimScreen.kt`, and `OpenLoopViewModel.updateTrimWindow` ignores sub-minimum windows anyway. + +2. **Extract that math into a pure, import-free file** (`ui/components/TrimHandleMath.kt`) instead + of fixing it inline in each lambda — the same pure-logic split as `media/BoomerangSequence.kt`. + The regression then locks in via a plain JVM test (`TrimHandleMathTest`), including the exact + crash values (335 ms vs 400 ms) and a duration sweep asserting the clamp is total. + +## Detection checklist + +- Grep for `coerceIn` calls whose arguments contain `+` or `-`: + `rg 'coerceIn\([^)]*[+-]' app/src/main`. Every hit needs either a proof the range can't invert + or a `coerceAtLeast`/`coerceAtMost` on the derived bound. +- Any screen that receives a media duration must be audited with a duration **below** every + minimum constant it uses (and with 0). +- Clamp/geometry math living inline in a composable lambda is untestable on the JVM — extract it. + +## Reference + +- Kotlin stdlib `coerceIn` (throws on empty ranges): https://kotlinlang.org/api/core/kotlin-stdlib/kotlin.ranges/coerce-in.html +- Originating crash: Crashlytics `7169b499fee6554684dba69b1ae1a8f0`, GitHub issue #95 diff --git a/docs/lessons_learned/README.md b/docs/lessons_learned/README.md index a32e099..16eeff8 100644 --- a/docs/lessons_learned/README.md +++ b/docs/lessons_learned/README.md @@ -37,6 +37,7 @@ Each lesson follows the same shape: | 022 | [Release CameraX when the PreviewView leaves composition, not only when the Activity stops](./022-release-camera-when-preview-leaves-composition.md) | Issue #36 | | 023 | [A media pipeline stage must count its own output samples; a zero-frame stage can exit "cleanly"](./023-media-pipeline-stages-must-count-output-samples.md) | PR #62 (S23 zero-frame wedge) | | 024 | [Gate a foreground-service type on the API level that ADDED it, not one below](./024-fgs-type-constant-api-gating.md) | Crashlytics 9663c743 (Galaxy A55 / Android 14) | +| 025 | [`coerceIn` with runtime-derived bounds throws on short clips; clamp the bound and JVM-test the math](./025-trim-coercein-range-guard-short-clip.md) | Crashlytics 7169b499 (Issue #95) | ## Adding a new lesson