CCCT-2598 Add CameraOverlayActivity For Rectangle Reticle Capture#3788
CCCT-2598 Add CameraOverlayActivity For Rectangle Reticle Capture#3788Jignesh-dimagi wants to merge 5 commits into
Conversation
New back-camera activity that draws a static rectangle reticle over the CameraX preview as a framing guide and writes the full-resolution frame to a caller-supplied output URI without cropping. Shutter capture shows in-flight progress and disables re-entry; capture errors are non-fatal and retryable. Also migrates the shared camera error/permission strings from the localization txt file to strings.xml (cc:translatable) and updates MicroImageActivity accordingly. Form-side ImageWidget wiring is deferred to a follow-up ticket. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Suggested Review Order
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3788 +/- ##
============================================
+ Coverage 26.57% 26.67% +0.09%
- Complexity 4581 4610 +29
============================================
Files 971 974 +3
Lines 58018 58094 +76
Branches 6909 6921 +12
============================================
+ Hits 15421 15499 +78
+ Misses 40715 40711 -4
- Partials 1882 1884 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/org/commcare/activities/camera/CameraOverlayActivity.kt (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer an explicit fail-fast exception over
!!for the missing-URI programmer error.
intentOutputUri()!!surfaces a bareNullPointerExceptionwhen the caller forgets to supplyOUTPUT_FILE_URI_EXTRA. Since this is a caller-contract violation (a programmer error), an explicitIllegalStateException/IllegalArgumentExceptionwith an actionable message would make debugging easier than a generic NPE, consistent with the fail-fast style used elsewhere in this repo. Note this would require updating the corresponding test atCameraOverlayActivityTest.ktLines 119-126 to expect the new exception type.♻️ Proposed fix
- private val outputUri: Uri by lazy { intentOutputUri()!! } + private val outputUri: Uri by lazy { + intentOutputUri() ?: error("CameraOverlayActivity requires $OUTPUT_FILE_URI_EXTRA to be set on the launching intent") + }Based on learnings, "treat conditions that indicate a programmer error or broken invariant as exceptions rather than defensive handling... Use clear runtime exceptions (e.g., IllegalStateException, IllegalArgumentException)... with actionable messages for quick debugging."
Also applies to: 138-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/org/commcare/activities/camera/CameraOverlayActivity.kt` at line 30, Replace the `outputUri` lazy initializer in `CameraOverlayActivity` so it fails fast with an explicit runtime exception and actionable message instead of using `intentOutputUri()!!`; this should treat the missing `OUTPUT_FILE_URI_EXTRA` as a caller-contract violation and use a clear `IllegalStateException` or `IllegalArgumentException` in the `outputUri` path. Keep the behavior consistent with the rest of `CameraOverlayActivity` and update the corresponding `CameraOverlayActivityTest` expectation to assert the new exception type/message rather than a generic NPE.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/org/commcare/activities/camera/CameraOverlayActivity.kt`:
- Around line 63-91: The captureImage() flow in CameraOverlayActivity currently
assumes contentResolver.openOutputStream(outputUri) will only return null, but
it can throw FileNotFoundException or SecurityException before
handleCaptureError() is reached. Update captureImage() to guard the
openOutputStream call with exception handling, and route any failure through the
existing handleCaptureError() path before starting ImageCapture.takePicture().
In
`@app/unit-tests/src/org/commcare/activities/camera/CameraOverlayActivityTest.kt`:
- Around line 92-105: The capture flow test in CameraOverlayActivityTest only
checks that ImageCapture.takePicture() is called, but it does not exercise the
ImageCapture.OnImageSavedCallback paths inside captureImage(). Add tests that
invoke the callback used by captureImage() and cover both success and error
handling: on success, verify the output stream is closed, RESULT_OK is set, and
finish() is called; on failure, verify the error is routed through
handleCaptureError(). Use the existing captureImage(), handleCaptureError(), and
ImageCapture callback wiring to locate the behavior.
- Around line 27-34: The test setup in buildActivity() is using a hard-coded
FileProvider URI authority that does not match the authority declared in the
manifest, so the intent may point to an invalid content URI and drive
captureImage() into the error path instead of takePicture(). Update the URI
construction in buildActivity() to use the manifest’s FileProvider authority
(the same one used by CameraOverlayActivity.getIntent and openOutputStream()) so
the test exercises the intended capture flow.
---
Nitpick comments:
In `@app/src/org/commcare/activities/camera/CameraOverlayActivity.kt`:
- Line 30: Replace the `outputUri` lazy initializer in `CameraOverlayActivity`
so it fails fast with an explicit runtime exception and actionable message
instead of using `intentOutputUri()!!`; this should treat the missing
`OUTPUT_FILE_URI_EXTRA` as a caller-contract violation and use a clear
`IllegalStateException` or `IllegalArgumentException` in the `outputUri` path.
Keep the behavior consistent with the rest of `CameraOverlayActivity` and update
the corresponding `CameraOverlayActivityTest` expectation to assert the new
exception type/message rather than a generic NPE.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a92e2d2f-db13-4569-b391-3b0f5d839e9c
📒 Files selected for processing (9)
app/AndroidManifest.xmlapp/assets/locales/android_translatable_strings.txtapp/res/drawable/bg_camera_shutter_button.xmlapp/res/layout/camera_overlay_activity.xmlapp/res/values/strings.xmlapp/src/org/commcare/activities/camera/BaseCameraActivity.ktapp/src/org/commcare/activities/camera/CameraOverlayActivity.ktapp/src/org/commcare/activities/camera/MicroImageActivity.javaapp/unit-tests/src/org/commcare/activities/camera/CameraOverlayActivityTest.kt
💤 Files with no reviewable changes (1)
- app/assets/locales/android_translatable_strings.txt
| internal fun captureImage() { | ||
| if (isCapturing) { | ||
| return | ||
| } | ||
| val capture = imageCapture ?: return | ||
| val outputStream = | ||
| contentResolver.openOutputStream(outputUri) ?: run { | ||
| handleCaptureError("Unable to open output stream for captured image", null) | ||
| return | ||
| } | ||
| handleCapturingState(true) | ||
| val outputOptions = ImageCapture.OutputFileOptions.Builder(outputStream).build() | ||
| capture.takePicture( | ||
| outputOptions, | ||
| ContextCompat.getMainExecutor(this), | ||
| object : ImageCapture.OnImageSavedCallback { | ||
| override fun onImageSaved(outputFileResults: ImageCapture.OutputFileResults) { | ||
| outputStream.closeQuietly() | ||
| setResult(RESULT_OK) | ||
| finish() | ||
| } | ||
|
|
||
| override fun onError(exception: ImageCaptureException) { | ||
| outputStream.closeQuietly() | ||
| handleCaptureError("Failed to capture image", exception) | ||
| } | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does ContentResolver.openOutputStream throw FileNotFoundException or SecurityException on Android?
💡 Result:
Yes, ContentResolver.openOutputStream throws FileNotFoundException, and it can also throw a SecurityException under specific circumstances [1]. According to the official Android documentation and source code, the primary checked exception defined for openOutputStream is FileNotFoundException [2][3][4]. This exception is thrown if the provided URI cannot be opened, which includes scenarios where the data is missing, the file system is read-only, or the underlying ContentProvider denies access [5][2][3]. While the method signature explicitly declares throws FileNotFoundException, a SecurityException may also be thrown at runtime if the calling application lacks the necessary permissions to access the specified URI [1]. Internally, Android's permission enforcement mechanisms (such as enforceFilePermission) check for required read or write access; if these checks fail, it can result in a security-related exception being thrown during the process of resolving the file descriptor [1]. Consequently, while FileNotFoundException is the documented checked exception, applications should be prepared to handle SecurityException as a runtime exception when interacting with content URIs [1].
Citations:
- 1: https://android.googlesource.com/platform/frameworks/base/+/HEAD/core/java/android/content/ContentProvider.java
- 2: https://android.googlesource.com/platform/prebuilts/fullsdk/sources/android-29/+/refs/heads/main/android/content/ContentResolver.java
- 3: https://android.googlesource.com/platform/frameworks/base/+/3dec7d563a2f3e1eb967ce2054a00b6620e3558c/core/java/android/content/ContentResolver.java
- 4: https://www.javadoc.io/static/com.google.android/android/2.1.2/android/content/ContentResolver.html
- 5: https://stackoverflow.com/questions/48560028/contentresolver-openoutputstream-throws-strage-filenotfoundexception
🏁 Script executed:
sed -n '1,220p' app/src/org/commcare/activities/camera/CameraOverlayActivity.ktRepository: dimagi/commcare-android
Length of output: 5516
Guard openOutputStream() before starting capture. contentResolver.openOutputStream(outputUri) can throw FileNotFoundException or SecurityException for content URIs, so this path can still crash before handleCaptureError() runs. Catch the failure and route it through the existing non-fatal error path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/org/commcare/activities/camera/CameraOverlayActivity.kt` around lines
63 - 91, The captureImage() flow in CameraOverlayActivity currently assumes
contentResolver.openOutputStream(outputUri) will only return null, but it can
throw FileNotFoundException or SecurityException before handleCaptureError() is
reached. Update captureImage() to guard the openOutputStream call with exception
handling, and route any failure through the existing handleCaptureError() path
before starting ImageCapture.takePicture().
| private fun buildActivity(): CameraOverlayActivity { | ||
| val intent = | ||
| CameraOverlayActivity.getIntent( | ||
| ApplicationProvider.getApplicationContext(), | ||
| Uri.parse("content://org.commcare.dalvik.fileprovider/external/output.jpg"), | ||
| ) | ||
| return Robolectric.buildActivity(CameraOverlayActivity::class.java, intent).create().get() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether org.commcare.dalvik.fileprovider is declared as a real provider,
# and whether other Robolectric tests rely on openOutputStream with content URIs.
rg -n "org.commcare.dalvik.fileprovider" -C3
rg -n "openOutputStream" --type=kt --type=java -C3Repository: dimagi/commcare-android
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## candidate files\n'
git ls-files | rg 'CameraOverlayActivityTest\.kt|CameraOverlayActivity\.kt|AndroidManifest|fileprovider|FileProvider|ContentProvider|shadow'
printf '\n## search for authority/provider declarations\n'
rg -n --hidden --glob '!**/build/**' --glob '!**/.git/**' \
'org\.commcare\.dalvik\.fileprovider|FileProvider|content://org\.commcare\.dalvik\.fileprovider|openOutputStream\(' \
appRepository: dimagi/commcare-android
Length of output: 2656
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## outline: CameraOverlayActivityTest.kt\n'
ast-grep outline app/unit-tests/src/org/commcare/activities/camera/CameraOverlayActivityTest.kt --view expanded
printf '\n## outline: CameraOverlayActivity.kt\n'
ast-grep outline app/src/main/java/org/commcare/activities/camera/CameraOverlayActivity.kt --view expanded
printf '\n## manifest/provider-related hits\n'
rg -n --hidden --glob '!**/build/**' --glob '!**/.git/**' \
'org\.commcare\.dalvik\.fileprovider|androidx\.core\.content\.FileProvider|<provider|ContentProvider' \
app/src app/unit-testsRepository: dimagi/commcare-android
Length of output: 2567
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## candidate files\n'
git ls-files | rg 'CameraOverlayActivityTest\.kt|CameraOverlayActivity\.kt|AndroidManifest|fileprovider|FileProvider|ContentProvider|shadow'
printf '\n## search for authority/provider declarations\n'
rg -n --hidden --glob '!**/build/**' --glob '!**/.git/**' \
'org\.commcare\.dalvik\.fileprovider|FileProvider|content://org\.commcare\.dalvik\.fileprovider|openOutputStream\(' \
app
printf '\n## outline: CameraOverlayActivityTest.kt\n'
ast-grep outline app/unit-tests/src/org/commcare/activities/camera/CameraOverlayActivityTest.kt --view expanded
printf '\n## outline: CameraOverlayActivity.kt\n'
ast-grep outline app/src/main/java/org/commcare/activities/camera/CameraOverlayActivity.kt --view expanded
printf '\n## manifest/provider-related hits\n'
rg -n --hidden --glob '!**/build/**' --glob '!**/.git/**' \
'org\.commcare\.dalvik\.fileprovider|androidx\.core\.content\.FileProvider|<provider|ContentProvider' \
app/src app/unit-testsRepository: dimagi/commcare-android
Length of output: 5062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test and relevant activity files.
fd 'CameraOverlayActivityTest.kt|CameraOverlayActivity.kt|AndroidManifest.xml' app
# Search for the fake authority and any FileProvider registration.
rg -n --hidden --glob '!**/build/**' --glob '!**/.git/**' \
'org\.commcare\.dalvik\.fileprovider|androidx\.core\.content\.FileProvider|<provider|openOutputStream\(' \
app/src app/unit-testsRepository: dimagi/commcare-android
Length of output: 1293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n app/unit-tests/src/org/commcare/activities/camera/CameraOverlayActivityTest.kt | sed -n '1,180p'
printf '\n---\n'
rg -n --hidden --glob '!**/build/**' --glob '!**/.git/**' \
'registerProvider|registerInputStream|registerOutputStream|setStream|fileprovider|openOutputStream' \
app/unit-tests/srcRepository: dimagi/commcare-android
Length of output: 8401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## app/AndroidManifest.xml around FileProvider\n'
nl -ba app/AndroidManifest.xml | sed -n '220,270p'
printf '\n## CameraOverlayActivity.kt around captureImage\n'
nl -ba app/src/org/commcare/activities/camera/CameraOverlayActivity.kt | sed -n '1,180p'Repository: dimagi/commcare-android
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## search for test-only provider registration helpers\n'
rg -n --hidden --glob '!**/build/**' --glob '!**/.git/**' \
'registerProvider|unregisterProvider|ProviderInfo|shadow.*FileProvider|FileProvider' \
app/unit-tests/src app/src
printf '\n## search for application manifest references to file provider authority\n'
rg -n --hidden --glob '!**/build/**' --glob '!**/.git/**' \
'fileprovider|FileProvider' \
app/AndroidManifest.xml app/CommcareAndroidManifest.xml app/unit-tests/AndroidManifest.xmlRepository: dimagi/commcare-android
Length of output: 1953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## app/AndroidManifest.xml provider block\n'
sed -n '232,255p' app/AndroidManifest.xml
printf '\n## CameraOverlayActivity.kt captureImage block\n'
sed -n '1,140p' app/src/org/commcare/activities/camera/CameraOverlayActivity.ktRepository: dimagi/commcare-android
Length of output: 6334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## fileprovider authority search across manifests and resources\n'
rg -n --hidden --glob '!**/build/**' --glob '!**/.git/**' \
'android:authorities=|fileprovider|FileProvider|paths' \
app/AndroidManifest.xml app/CommcareAndroidManifest.xml app/unit-tests/AndroidManifest.xml app/resRepository: dimagi/commcare-android
Length of output: 929
Use the FileProvider authority from the manifest here
buildActivity() hard-codes content://org.commcare.dalvik.fileprovider/..., but the manifest declares ${applicationId}.external.files.provider. That mismatch can make openOutputStream() fail in captureImage(), so the test may hit the error path instead of takePicture().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/unit-tests/src/org/commcare/activities/camera/CameraOverlayActivityTest.kt`
around lines 27 - 34, The test setup in buildActivity() is using a hard-coded
FileProvider URI authority that does not match the authority declared in the
manifest, so the intent may point to an invalid content URI and drive
captureImage() into the error path instead of takePicture(). Update the URI
construction in buildActivity() to use the manifest’s FileProvider authority
(the same one used by CameraOverlayActivity.getIntent and openOutputStream()) so
the test exercises the intended capture flow.
| @Test | ||
| fun `captureImage enters capturing state and delegates to ImageCapture`() { | ||
| val activity = buildActivity() | ||
| val shutter = activity.findViewById<ImageView>(R.id.camera_shutter_button) | ||
| val progress = activity.findViewById<View>(R.id.capture_progress) | ||
| val imageCapture = mock<ImageCapture>() | ||
| activity.imageCapture = imageCapture | ||
|
|
||
| activity.captureImage() | ||
|
|
||
| verify(imageCapture).takePicture(any(), any(), any()) | ||
| assertFalse("Shutter should be disabled while the capture is in flight", shutter.isEnabled) | ||
| assertEquals(View.VISIBLE, progress.visibility) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing coverage for ImageCapture.OnImageSavedCallback (success/error paths).
Every test mocks ImageCapture and only verifies that takePicture(...) is invoked; the actual onImageSaved/onError callback logic inside captureImage() (closing the stream, setting RESULT_OK + finish() on success, and routing to handleCaptureError on failure) is never exercised. This is the core outcome of the capture flow and currently has no coverage.
🧪 Suggested approach
+ `@Test`
+ fun `captureImage finishes with RESULT_OK when the image is saved`() {
+ val activity = buildActivity()
+ val imageCapture = mock<ImageCapture>()
+ activity.imageCapture = imageCapture
+ val callbackCaptor = argumentCaptor<ImageCapture.OnImageSavedCallback>()
+
+ activity.captureImage()
+ verify(imageCapture).takePicture(any(), any(), callbackCaptor.capture())
+ callbackCaptor.firstValue.onImageSaved(mock())
+
+ assertEquals(Activity.RESULT_OK, shadowOf(activity).resultCode)
+ assertTrue(activity.isFinishing)
+ }
+
+ `@Test`
+ fun `captureImage surfaces a retryable error when ImageCapture reports onError`() {
+ val activity = buildActivity()
+ val imageCapture = mock<ImageCapture>()
+ activity.imageCapture = imageCapture
+ val callbackCaptor = argumentCaptor<ImageCapture.OnImageSavedCallback>()
+
+ activity.captureImage()
+ verify(imageCapture).takePicture(any(), any(), callbackCaptor.capture())
+ callbackCaptor.firstValue.onError(mock())
+
+ assertFalse(activity.isFinishing)
+ }As per coding guidelines, "When adding tests, ensure comprehensive coverage of all public methods in the class."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| fun `captureImage enters capturing state and delegates to ImageCapture`() { | |
| val activity = buildActivity() | |
| val shutter = activity.findViewById<ImageView>(R.id.camera_shutter_button) | |
| val progress = activity.findViewById<View>(R.id.capture_progress) | |
| val imageCapture = mock<ImageCapture>() | |
| activity.imageCapture = imageCapture | |
| activity.captureImage() | |
| verify(imageCapture).takePicture(any(), any(), any()) | |
| assertFalse("Shutter should be disabled while the capture is in flight", shutter.isEnabled) | |
| assertEquals(View.VISIBLE, progress.visibility) | |
| } | |
| `@Test` | |
| fun `captureImage enters capturing state and delegates to ImageCapture`() { | |
| val activity = buildActivity() | |
| val shutter = activity.findViewById<ImageView>(R.id.camera_shutter_button) | |
| val progress = activity.findViewById<View>(R.id.capture_progress) | |
| val imageCapture = mock<ImageCapture>() | |
| activity.imageCapture = imageCapture | |
| activity.captureImage() | |
| verify(imageCapture).takePicture(any(), any(), any()) | |
| assertFalse("Shutter should be disabled while the capture is in flight", shutter.isEnabled) | |
| assertEquals(View.VISIBLE, progress.visibility) | |
| } | |
| `@Test` | |
| fun `captureImage finishes with RESULT_OK when the image is saved`() { | |
| val activity = buildActivity() | |
| val imageCapture = mock<ImageCapture>() | |
| activity.imageCapture = imageCapture | |
| val callbackCaptor = argumentCaptor<ImageCapture.OnImageSavedCallback>() | |
| activity.captureImage() | |
| verify(imageCapture).takePicture(any(), any(), callbackCaptor.capture()) | |
| callbackCaptor.firstValue.onImageSaved(mock()) | |
| assertEquals(Activity.RESULT_OK, shadowOf(activity).resultCode) | |
| assertTrue(activity.isFinishing) | |
| } | |
| `@Test` | |
| fun `captureImage surfaces a retryable error when ImageCapture reports onError`() { | |
| val activity = buildActivity() | |
| val imageCapture = mock<ImageCapture>() | |
| activity.imageCapture = imageCapture | |
| val callbackCaptor = argumentCaptor<ImageCapture.OnImageSavedCallback>() | |
| activity.captureImage() | |
| verify(imageCapture).takePicture(any(), any(), callbackCaptor.capture()) | |
| callbackCaptor.firstValue.onError(mock()) | |
| assertFalse(activity.isFinishing) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/unit-tests/src/org/commcare/activities/camera/CameraOverlayActivityTest.kt`
around lines 92 - 105, The capture flow test in CameraOverlayActivityTest only
checks that ImageCapture.takePicture() is called, but it does not exercise the
ImageCapture.OnImageSavedCallback paths inside captureImage(). Add tests that
invoke the callback used by captureImage() and cover both success and error
handling: on success, verify the output stream is closed, RESULT_OK is set, and
finish() is called; on failure, verify the error is routed through
handleCaptureError(). Use the existing captureImage(), handleCaptureError(), and
ImageCapture callback wiring to locate the behavior.
Source: Coding guidelines
There was a problem hiding this comment.
+1 on verifying the complete flow here , also we should be able to mock the the Image Capture APIs to simulate success vs error here without having to call the handleCaptureError directly.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd CameraOverlayActivity.kt --exec cat -n {}Length of output: 6414
@shubham1g5 Good news — this is achievable. I checked CameraOverlayActivity.captureImage(), and the ImageCapture.OnImageSavedCallback is built as an anonymous object inline and passed directly to capture.takePicture(outputOptions, executor, callback). That means we can capture that exact callback instance with an argumentCaptor<ImageCapture.OnImageSavedCallback>() on the mocked ImageCapture, then invoke onImageSaved(...)/onError(...) on it to simulate CameraX's real callback delivery — without ever calling handleCaptureError(...) directly from the test. This exercises the real internal logic (outputStream.closeQuietly(), setResult(RESULT_OK), finish() on success; handleCaptureError(...) routing on error) end-to-end.
Here's a refined version of the two tests:
`@Test`
fun `captureImage finishes with RESULT_OK when the image is saved`() {
val activity = buildActivity()
val imageCapture = mock<ImageCapture>()
activity.imageCapture = imageCapture
val callbackCaptor = argumentCaptor<ImageCapture.OnImageSavedCallback>()
activity.captureImage()
verify(imageCapture).takePicture(any(), any(), callbackCaptor.capture())
callbackCaptor.firstValue.onImageSaved(mock())
assertEquals(Activity.RESULT_OK, shadowOf(activity).resultCode)
assertTrue(activity.isFinishing)
}
`@Test`
fun `captureImage surfaces a retryable error when ImageCapture reports onError`() {
val activity = buildActivity()
val shutter = activity.findViewById<ImageView>(R.id.camera_shutter_button)
val progress = activity.findViewById<View>(R.id.capture_progress)
val imageCapture = mock<ImageCapture>()
activity.imageCapture = imageCapture
val callbackCaptor = argumentCaptor<ImageCapture.OnImageSavedCallback>()
val exception = mock<ImageCaptureException>()
activity.captureImage()
verify(imageCapture).takePicture(any(), any(), callbackCaptor.capture())
callbackCaptor.firstValue.onError(exception)
assertEquals(
activity.getString(R.string.image_capture_failed),
ShadowToast.getTextOfLatestToast(),
)
assertTrue("Shutter should be re-enabled after an error so capture can be retried", shutter.isEnabled)
assertEquals(View.GONE, progress.visibility)
assertFalse("Capture errors are non-fatal; the activity must stay open", activity.isFinishing)
}Notes:
shadowOf(activity)requiresimport org.robolectric.Shadows.shadowOfandActivityfromandroid.app.Activity.onErrorrequiresImageCaptureException, somock<ImageCaptureException>()(or a relaxed mock) is sufficient since the implementation only logs it viaLogger.exception(logMessage, e).- These two tests cover the missing success/error callback paths flagged earlier, closing the coverage gap for
captureImage()'s core outcome logic.
🧠 Learnings used
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3448
File: app/src/org/commcare/gis/EntityMapUtils.kt:214-234
Timestamp: 2025-12-10T16:39:05.007Z
Learning: In Kotlin files (here: app/src/org/commcare/gis/EntityMapUtils.kt), follow fail-fast guidance: treat conditions that indicate a programmer error or broken invariant as exceptions rather than defensive handling. If a condition represents a bug (e.g., internal API assumptions like array/list size mismatches), crash early to alert developers instead of attempting to recover. Use clear runtime exceptions (e.g., IllegalStateException, IllegalArgumentException) or assertions where appropriate, and avoid swallowing or masking bugs. Ensure invariants are checked and failures are surfaced with actionable messages for quick debugging.
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3606
File: app/src/org/commcare/connect/network/ConnectNetworkHelper.kt:89-110
Timestamp: 2026-03-17T02:15:22.484Z
Learning: In the dimagi/commcare-android repository, do not use CancellableContinuation.tryResume (it's annotated InternalCoroutinesApi). For suspendCancellableCoroutine patterns, resume with continuation.resume(...) or continuation.resumeWithException(...) to resume, and use continuation.invokeOnCancellation { ... } for cleanup when the coroutine is cancelled. Apply this across all Kotlin files (pattern: **/*.kt) to ensure safe coroutine usage and avoid internal API leakage.
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3708
File: app/unit-tests/src/org/commcare/utils/StringUtilsTest.kt:39-39
Timestamp: 2026-05-12T11:43:39.849Z
Learning: When reviewing/adding Kotlin unit tests that validate emails using Android’s `android.util.Patterns.EMAIL_ADDRESS` (e.g., via `isValidEmail` in StringUtils), remember the pattern requires at least one dot-separated label after the `@`. Because the TLD/label group uses a `+` quantifier over `(�.[a-zA-Z0-9][a-zA-Z0-9\-]{0,25})+`, single-label domains with no dot after `@` (e.g., `missingdomain`) are correctly rejected. Assert such inputs with `assertFalse(isValidEmail(...))` rather than expecting them to pass.
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3708
File: app/src/org/commcare/connect/network/base/BaseApiHandler.kt:109-110
Timestamp: 2026-05-12T11:49:28.514Z
Learning: In Kotlin (1.4+), a trailing comma immediately before the semicolon that ends an enum’s constant list is valid syntax. For example, `RATE_LIMITED_ERROR, ;` (and similarly `ENTRY, ;`) should compile and should not be flagged as a syntax error. When reviewing Kotlin enum class bodies, treat `ENTRY, ;` as correct grammar (the `TrailingCommaOnDeclarationSite` style rule may even encourage it) rather than an invalid comma/semicolon placement.
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3708
File: app/src/org/commcare/connect/network/base/BaseApiHandler.kt:109-110
Timestamp: 2026-05-12T11:49:28.514Z
Learning: When reviewing Kotlin code, do not flag a syntax error for enum entries that include a trailing comma immediately before the enum-body semicolon. In Kotlin 1.4+, this grammar is valid (e.g., `RATE_LIMITED_ERROR, ;` compiles). Treat `ENTRY, ;` as correct enum declaration syntax, not a malformed trailing comma.
Learnt from: conroy-ricketts
Repo: dimagi/commcare-android PR: 3738
File: app/src/org/commcare/login/SyncOperations.kt:89-108
Timestamp: 2026-05-26T19:58:51.545Z
Learning: When reviewing Kotlin coroutine code that uses `suspendCancellableCoroutine`, understand that if the parent coroutine job is cancelled, the suspended coroutine is resumed by the coroutines framework with a `CancellationException`. In `invokeOnCancellation { ... }` handlers, treat the block as cleanup-only side effects (for example, cancelling an external task) and do not require a manual `continuation.resume(...)`/`continuation.resumeWithException(...)` call inside `invokeOnCancellation` for cancellation completion. Therefore, do not flag missing manual `continuation.resume(...)` calls specifically within `invokeOnCancellation` as a bug; rely on the coroutine framework’s cancellation handling.
Learnt from: conroy-ricketts
Repo: dimagi/commcare-android PR: 3751
File: app/src/org/commcare/connect/network/connectId/parser/ConfirmBackupCodeResponseParser.java:58-59
Timestamp: 2026-06-05T13:09:49.496Z
Learning: In this repo’s Java/Kotlin code (all files matching `**/*.{kt,java}`), reviewers should not flag in-code comments or request their removal. Developers are expected to add useful in-code comments where necessary, even if those comments are added after an AI agent’s review. Do not treat existing Java/Kotlin comments as a review issue.
There was a problem hiding this comment.
I have added the said tests now.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
|
I like the height of |
|
Also think we need to define different height and width ratios here. The box get too narrow for 0.3. and 0.4, maybe we should define minmum width/height of the box as well so that any aspect ratio doesn't make the box dimension smaller than a min threshold |
| val capture = | ||
| ImageCapture | ||
| .Builder() | ||
| .setCaptureMode(ImageCapture.CAPTURE_MODE_MAXIMIZE_QUALITY) |
There was a problem hiding this comment.
Have we considered using CAPTURE_MODE_MINIMIZE_LATENCY here?
Curious about thoughts on that
There was a problem hiding this comment.
While MINIMIZE_LATENCY ensures the photo is captured and returned faster, it can result in noisier image quality. The core requirement of the MUAC flow is legibility; the saved photo must be clear enough to read the numbers on the measuring tape. Because this is a single, deliberate manual capture rather than a burst-shot scenario, image quality must take priority over shutter speed.
There was a problem hiding this comment.
Got it, thanks for the clarification!
There was a problem hiding this comment.
it can result in noisier image quality
Curious if you are seeing that in practice ? Given we have been using that mode already with Personal ID photo capture and have not heard of this issue.
Also what's the impact on image size here in the 2 modes and do we have more info on what intermal camera mechanics do the 2 mode controls ?
There was a problem hiding this comment.
Because the primary requirement here is maximizing image clarity over other performance metrics, the implementation explicitly uses the CAPTURE_MODE_MAXIMIZE_QUALITY flag and ignores alternative flags.
There was a problem hiding this comment.
do we have more info on what intermal camera mechanics do the 2 mode controls ?
Claude/Gemini: The CAPTURE_MODE_* constants are a CameraX abstraction — they don't apply to the stock camera app at all. The default/system camera app (the one that handles MediaStore.ACTION_IMAGE_CAPTURE) is a separate app (Google Camera, Samsung Camera, etc.) built directly on camera2 + the vendor HAL (Hardware Abstraction Layer). It doesn't consume these CameraX enum values, so there's no CAPTURE_MODE it uses.
Also what's the impact on image size here in the 2 modes
I ran small-scale tests comparing the two modes, focusing on the same object. While my manual framing wasn't identical each time, I observed a 10% to 15% reduction in file size when using MINIMIZE_LATENCY. Theoretically, this mode can produce images 30% to 50% smaller than MAXIMIZE_QUALITY, but actual savings vary by device. Visually, I couldn't detect a quality difference, likely due to testing on a higher-end phone. However, we can conclude that MINIMIZE_LATENCY does consistently result in smaller file sizes.
How the modes handle picture quality (From Claude):
MINIMIZE_LATENCY: 3A (Auto-Focus, Auto-Exposure, and Auto-White-Balance) convergence is not guaranteed. It can capture a frame before these loops finish running. How aggressively it short-circuits 3A depends on the vendor's HAL; some devices still produce sharp frames, while lower-end hardware might hand back soft-focus or underexposed shots.
MAXIMIZE_QUALITY: While not entirely device-independent, it raises the quality floor and reduces variance across different hardware. It forces 3A convergence and requests HIGH_QUALITY processing everywhere, making it less likely to produce a bad frame on an unverified field device.
There is also Zero Shutter Lag (CAPTURE_MODE_ZERO_SHUTTER_LAG) mode: It’s highly restricted (incompatible with flash/ImageAnalysis, falls back to latency mode if unsupported) and meant for action shots. It doesn't add value for our static reticle use case.
If CommCare runs across a wide, heterogeneous, may be low-end devices, MAXIMIZE_QUALITY seems better option. Thoughts?
There was a problem hiding this comment.
, I observed a 10% to 15% reduction in file size when using MINIMIZE_LATENCY.
Seems like the size variance is not a lot even on a higher end device where I will expect it to be more prominent because of higher pixels camera availability, so seems good to opt for MAXIMIZE_QUALITY here.
Good callout, but because the reticle size is dynamic based on screen dimensions, we can't fully guarantee it won't overlap with the shutter button on every device. |
@shubham1g5 @OrangeAndGreen @conroy-ricketts I have added this section directly to the design doc. Please feel free to review it and provide your feedback there. |
| val imageCapture = mock<ImageCapture>() | ||
| activity.imageCapture = imageCapture | ||
|
|
||
| activity.captureImage() |
There was a problem hiding this comment.
we should simulate shutter click instead.
There was a problem hiding this comment.
can we remove the @VisibleForTesting annotations now from the activity
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
| <string name="image_capture_activity_title" cc:translatable="true">Take Photo</string> | ||
| <string name="image_capture_failed" cc:translatable="true">Failed to save photo, please try again</string> | ||
| <string name="camera_start_failed" cc:translatable="true">Camera failed to start</string> | ||
| <string name="camera_permission_denied" cc:translatable="true">Go to settings and enable camera permission to use this feature</string> | ||
| <string name="microimage_face_detection_mode_failed" cc:translatable="true">Face detection mode failed, switching to manual mode</string> | ||
| <string name="microimage_cropping_failed" cc:translatable="true">Image cropping failed, make sure the target is inside the capture area</string> |
There was a problem hiding this comment.
Just to confirm something here - do these still need cc:translatable="true" even though we defined translations for them?
There was a problem hiding this comment.
That's right as these strings are used inside the app so App translations should be able to override and provide translations they want here through HQ App config.
| logErrorAndExit("Error acquiring camera permission", "microimage.camera.permission.denied", null) | ||
| logErrorAndExit( | ||
| "Error acquiring camera permission", | ||
| StringUtils.getStringRobust(this, R.string.camera_permission_denied), |
There was a problem hiding this comment.
Confirming that getStringRobust is still necessary with the new translations that were added?
There was a problem hiding this comment.
yes, as cc:translatable="true" doesn't have any effect otherwise.
| import android.content.res.TypedArray | ||
|
|
||
| /** Returns the float at [index] if the attribute is present, otherwise null. */ | ||
| fun TypedArray.optionalFraction(index: Int): Float? = if (hasValue(index)) getFloat(index, 0f) else null |
There was a problem hiding this comment.
"optionalFraction" seems like an odd name for this, I'm not sure how it relates to fractions. Thinking getOptionalFloat might be better?
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Think this is good to be merged for now and we can make the size changes based on product feedback in another PR if you prefer.
4 participants
CCCT-2598
Technical Summary
Adds
CameraOverlayActivity, a newBaseCameraActivitysubclass (base extracted in CCCT-2596) for the MUAC-style capture flow:ImageCaptureatCAPTURE_MODE_MAXIMIZE_QUALITY; writes the full-resolution frame to a caller-supplied output URI via a content-resolverOutputStream(no crop).RectangleOverlayViewreticle (added in CCCT-2597) as a framing guide only — it is never drawn into the saved image.android_translatable_strings.txttostrings.xml(cc:translatable), collapsingBaseCameraActivity.logErrorAndExitto a single resolved-Stringoverload;MicroImageActivityupdated to match.In accordance with the design document decision to use independent scaling factors for height and width, the implementation has been updated to adapt to this change. The scaling factor includes a minimum threshold to guarantee that the target object is safely captured inside the reticle.
Please find the screenshots attached below. Additionally, since the landscape mode layout isn't complex, I was able to implement it without creating a separate landscape XML layout file.
Height / width adjusted as per comment:
Safety Assurance
Safety story
What gives confidence:
Automated test coverage
CameraOverlayActivityTest(Robolectric, 8 tests): thegetIntentfactory contract, capturing-state UI toggling (shutter enabled/alpha + progress visibility), non-fatal error handling, and thecaptureImagebranches — delegates toImageCapture.takePicture, re-entry guard while a capture is in flight, no-op when the camera isn't ready, and the fail-fast NPE when the launch intent omits the output URI.