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..655ec11 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,9 @@ private fun FilmstripTrimSelector( valueMs = startMs, rangeMs = 0f..durationMs.toFloat(), onSetValueMs = { target -> - val clamped = target.coerceIn(0L, endMs - minGapMs) + // Guard: if the clip is shorter than minGapMs the range would be inverted. + val maxMs = (endMs - minGapMs).coerceAtLeast(0L) + val clamped = target.coerceIn(0L, maxMs) onStartDrag(clamped) onDragEnd() }, @@ -407,7 +409,9 @@ private fun FilmstripTrimSelector( valueMs = endMs, rangeMs = 0f..durationMs.toFloat(), onSetValueMs = { target -> - val clamped = target.coerceIn(startMs + minGapMs, durationMs) + // Guard: if the clip is shorter than minGapMs the range would be inverted. + val minMs = (startMs + minGapMs).coerceAtMost(durationMs) + val clamped = target.coerceIn(minMs, durationMs) onEndDrag(clamped) onDragEnd() }, @@ -462,14 +466,18 @@ private fun FilmstripTrimSelector( val targetMs = dragAnchorMs + pxToMs(change.position.x - dragAnchorPx) when (dragging) { TrimDragTarget.START -> { - val clamped = targetMs.coerceIn(0L, curEndMs - minGapMs) + // Guard: clip shorter than minGapMs would invert the range. + val maxMs = (curEndMs - minGapMs).coerceAtLeast(0L) + val clamped = targetMs.coerceIn(0L, maxMs) if (clamped != curStartMs) { haptics.performHapticFeedback(HapticFeedbackType.TextHandleMove) startDrag(clamped) } } TrimDragTarget.END -> { - val clamped = targetMs.coerceIn(curStartMs + minGapMs, durationMs) + // Guard: clip shorter than minGapMs would invert the range. + val minMs = (curStartMs + minGapMs).coerceAtMost(durationMs) + val clamped = targetMs.coerceIn(minMs, durationMs) if (clamped != curEndMs) { haptics.performHapticFeedback(HapticFeedbackType.TextHandleMove) endDrag(clamped) 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..4eb99d5 --- /dev/null +++ b/docs/lessons_learned/025-trim-coercein-range-guard-short-clip.md @@ -0,0 +1,43 @@ +# 025 — Guard `coerceIn` bounds before calling when range can be inverted by a short clip + +## What went wrong + +`FilmstripTrimSelector` in `TrimFilmstripControls.kt` called `coerceIn` to clamp a drag position into the valid trim range: + +```kotlin +// END handle drag +val clamped = targetMs.coerceIn(curStartMs + minGapMs, durationMs) +// START handle drag +val clamped = targetMs.coerceIn(0L, curEndMs - minGapMs) +``` + +When a clip shorter than `MIN_TRIM_DURATION` (400 ms) reached the Trim screen — e.g., a 335 ms recording — the minimum bound exceeded the maximum: `coerceIn(400, 335)`. Kotlin's `Long.coerceIn` throws an `IllegalArgumentException` on an inverted range instead of silently clamping. + +Crashlytics issue `7169b499fee6554684dba69b1ae1a8f0` (v1.0.27): "Cannot coerce value to an empty range: maximum 335 is less than minimum 400." + +## Pattern + +Before calling `coerceIn(min, max)` where either bound is derived from a user-controlled value (not a compile-time constant), clamp the bound itself to keep the range valid: + +```kotlin +// END handle: ensure min ≤ durationMs +val minMs = (curStartMs + minGapMs).coerceAtMost(durationMs) +val clamped = targetMs.coerceIn(minMs, durationMs) + +// START handle: ensure max ≥ 0 +val maxMs = (curEndMs - minGapMs).coerceAtLeast(0L) +val clamped = targetMs.coerceIn(0L, maxMs) +``` + +With a sub-400 ms clip both handles become immovable (start pinned to 0, end pinned to `durationMs`), which is the correct UX — the NEXT button is already disabled by `trimValid` in `TrimScreen.kt`. + +## Detection checklist + +- Grep for `coerceIn` where either argument is an arithmetic expression: `\.coerceIn\([^,)]*[+\-][^,)]*,` or `, .*[+\-].*\)`. +- Any call of the form `coerceIn(A + K, B)` or `coerceIn(A, B - K)` needs a pre-clamp when `A`, `B`, or `K` come from runtime state. + +## Reference + +- Kotlin stdlib `Long.coerceIn`: https://kotlinlang.org/api/core/kotlin-stdlib/kotlin.ranges/coerce-in.html +- Originating crash: Crashlytics issue `7169b499fee6554684dba69b1ae1a8f0`, first seen v1.0.27 +- Fix: PR resolving GitHub issue #95 diff --git a/docs/lessons_learned/README.md b/docs/lessons_learned/README.md index a32e099..bf48945 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 | [Guard `coerceIn` bounds before calling when range can be inverted by a short clip](./025-trim-coercein-range-guard-short-clip.md) | Crashlytics 7169b499 (Issue #95) | ## Adding a new lesson