Implement Personal ID face capture Page #3116
Conversation
📝 WalkthroughWalkthroughThis change set introduces a new face/photo capture feature for ConnectID account setup in the Android application. It adds a new Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdPhotoCaptureFragment
participant MicroImageActivity
participant ApiConnectId
participant Server
participant LocalDB
User->>PersonalIdPhotoCaptureFragment: Open photo capture screen
PersonalIdPhotoCaptureFragment->>MicroImageActivity: Launch camera activity (intent)
MicroImageActivity->>MicroImageActivity: Initialize camera & face detection/manual mode
MicroImageActivity->>User: Show camera preview UI
User->>MicroImageActivity: Capture photo (auto/manual)
MicroImageActivity->>MicroImageActivity: Process image (crop, scale, compress, encode)
MicroImageActivity-->>PersonalIdPhotoCaptureFragment: Return Base64 image result
PersonalIdPhotoCaptureFragment->>PersonalIdPhotoCaptureFragment: Display photo preview, enable Save
User->>PersonalIdPhotoCaptureFragment: Tap Save Photo
PersonalIdPhotoCaptureFragment->>ApiConnectId: Upload photo (updatePhoto)
ApiConnectId->>Server: Send photo update request
Server-->>ApiConnectId: Respond (success/failure)
ApiConnectId-->>PersonalIdPhotoCaptureFragment: Callback with result
alt Success
PersonalIdPhotoCaptureFragment->>LocalDB: Save photo in user record
PersonalIdPhotoCaptureFragment->>PersonalIdPhotoCaptureFragment: Update UI, navigate to success
else Failure
PersonalIdPhotoCaptureFragment->>PersonalIdPhotoCaptureFragment: Show error, enable retry
end
Suggested labels
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (15)
app/res/drawable/baseline_person_24.xml (1)
1-5: New person icon vector – consider standard sizing
The vector correctly defines a person icon with a 24×24 viewport and white fill. To improve reusability, you may set thewidthandheightto24dp(the standard icon size) and let the hostingImageViewcontrol the rendered size, rather than hardcoding120dp.app/AndroidManifest.xml (1)
583-583: Declare MicroImageActivity – nitpick
The newMicroImageActivityis added without an intent-filter and therefore defaults toexported="false", which is correct for internal use. For explicit clarity on Android 12+, you could addandroid:exported="false".app/res/layout/screen_personalid_photo_capture.xml (1)
50-58: Consider adding a placeholder state or instructions for the photo viewThe ImageView has a person icon as a placeholder, which is good. However, you might consider adding a visual indication (like a camera overlay or frame) to help users understand where their face should be positioned during capture.
app/res/layout/micro_image_widget.xml (1)
1-39: Layout looks good but add contentDescription for better accessibilityThe layout structure is well-organized with appropriate layering of the camera preview, face overlay, and shutter button. The component hierarchy follows best practices for camera interfaces.
The camera shutter button should include a contentDescription attribute for accessibility:
<ImageView android:id="@+id/camera_shutter_button" android:layout_height="wrap_content" android:layout_width="match_parent" android:src="@drawable/camera_shutter" android:background="@color/cc_neutral_bg_tr" android:visibility="gone" + android:contentDescription="@string/take_photo_content_description" android:paddingBottom="15dp"/>You'll need to add the corresponding string resource in strings.xml.
app/src/org/commcare/connect/network/ApiConnectId.java (1)
345-359: LGTM! Well-implemented photo update methodThe new updatePhoto method correctly validates critical parameters with Objects.requireNonNull() and follows the established pattern of other API methods in this class.
Consider adding a validation check for the userName parameter to ensure it's not null before adding it to the params map:
public static void updatePhoto(Context context, String userId, String password, String userName, String photoAsBase64, IApiCallback callback) { Objects.requireNonNull(photoAsBase64); AuthInfo authInfo = new AuthInfo.ProvidedAuth(userId, password, false); String token = HttpUtils.getCredential(authInfo); Objects.requireNonNull(token); HashMap<String, String> params = new HashMap<>(); params.put("photo", photoAsBase64); + if (userName != null) { params.put("name", userName); + } ApiService apiService = ApiClient.getClient().create(ApiService.class); Call<ResponseBody> call = apiService.updateProfile(token, params); callApi(context, call, callback); }app/res/navigation/nav_graph_personalid.xml (1)
154-169: New fragment definition looks good but has inconsistent action namingThe fragment definition is well-structured with appropriate arguments and navigation actions.
The self-navigation action ID (
action_personalid_signup_fragment_self) doesn't follow the naming pattern of the fragment ID (personalid_photo_capture). Consider renaming for consistency:- android:id="@+id/action_personalid_signup_fragment_self" + android:id="@+id/action_personalid_photo_capture_self"app/src/org/commcare/fragments/connectId/PersonalIdPhotoCaptureFragment.java (1)
85-120: Consider adding progress indicator for photo uploadThe upload logic and callback handling look good, but the user might not know an upload is in progress.
Add a progress indicator to give users feedback during the upload process:
private void uploadImage() { + showProgressIndicator(); IApiCallback networkResponseCallback = new IApiCallback() { @Override public void processSuccess(int responseCode, InputStream responseData) { + hideProgressIndicator(); onPhotoUploadSuccess(photoAsBase64); } @Override public void processFailure(int responseCode) { + hideProgressIndicator(); onPhotoUploadFailure(requireContext().getString(R.string.connectid_photo_upload_failure), true); } @Override public void processNetworkFailure() { + hideProgressIndicator(); onPhotoUploadFailure(requireContext().getString(R.string.recovery_network_unavailable), true); } @Override public void processTokenUnavailableError() { + hideProgressIndicator(); onPhotoUploadFailure(requireContext().getString(R.string.recovery_network_token_unavailable), true); } @Override public void processTokenRequestDeniedError() { + hideProgressIndicator(); onPhotoUploadFailure(requireContext().getString(R.string.recovery_network_token_request_rejected), false); } @Override public void processOldApiError() { + hideProgressIndicator(); onPhotoUploadFailure(requireContext().getString(R.string.recovery_network_outdated), false); } }; ApiConnectId.updatePhoto(requireContext(), connectUserRecord.getUserId(), connectUserRecord.getPassword(), connectUserRecord.getName(), photoAsBase64, networkResponseCallback); } +private void showProgressIndicator() { + viewBinding.progressBar.setVisibility(View.VISIBLE); + viewBinding.savePhotoButton.setEnabled(false); + viewBinding.takePhotoButton.setEnabled(false); +} + +private void hideProgressIndicator() { + viewBinding.progressBar.setVisibility(View.GONE); +}Note: This assumes you add a ProgressBar to your layout with ID "progressBar".
app/src/org/commcare/utils/MediaUtil.java (2)
47-48: Typo & naming –IMAGE_QUALIY_REDUCTION_FACTORshould beIMAGE_QUALITY_REDUCTION_FACTORSpelling mistakes in constant names quickly spread through the code-base and make grepping harder. Renaming also clarifies the intent that this constant is about JPEG/WebP quality.
- private static final int IMAGE_QUALIY_REDUCTION_FACTOR = 10; + /** Amount (in %) by which compression quality is reduced each iteration */ + private static final int IMAGE_QUALITY_REDUCTION_STEP = 10;
623-630: Silent failure hides decoding problemsReturning
nullwithout logging loses valuable debugging information.- } catch(Exception e){ - return null; + } catch (Exception e) { + Logger.exception("Failed to decode Base64 bitmap", e); + return null; }app/src/org/commcare/fragments/MicroImageActivity.java (3)
134-142: Face detector recreated for every frame – heavy GC & CPU cost
FaceDetection.getClient()allocates native resources on each call. Create oneFaceDetectorinstance inonCreate()(orstartCamera()) and reuse it, closing it inonDestroy().- FaceDetectorOptions realTimeOpts = new FaceDetectorOptions.Builder() - .setContourMode(FaceDetectorOptions.CONTOUR_MODE_NONE) - .setPerformanceMode(FaceDetectorOptions.PERFORMANCE_MODE_ACCURATE) - .build(); - FaceDetector faceDetector = FaceDetection.getClient(realTimeOpts); + if (faceDetector == null) { return; } // faceDetector initialised once in onCreate
210-214: Toast not localized
handleErrorDuringDetection()shows the string key instead of the translated string.- Toast.makeText(this, "microimage.face.detection.mode.failed", Toast.LENGTH_LONG).show(); + Toast.makeText(this, Localization.get("microimage.face.detection.mode.failed"), Toast.LENGTH_LONG).show();
287-292: Typo & Base64 flag
- Method name
finishWithResul(missing ‘t’) hurts readability.Base64.DEFAULTinserts line breaks – considerNO_WRAPto avoid server-side decode issues.-private void finishWithResul(String finalImageAsBase64) { +private void finishWithResult(String finalImageAsBase64) { Intent result = new Intent(); - result.putExtra(MICRO_IMAGE_BASE_64_RESULT_KEY, finalImageAsBase64); + result.putExtra(MICRO_IMAGE_BASE_64_RESULT_KEY, finalImageAsBase64); setResult(AppCompatActivity.RESULT_OK, result); finish(); }And change the caller.
- finishWithResul(finalImageAsBase64); + finishWithResult(finalImageAsBase64);Also:
- String finalImageAsBase64 = Base64.encodeToString(compressedByteArray, Base64.DEFAULT); + String finalImageAsBase64 = Base64.encodeToString(compressedByteArray, Base64.NO_WRAP);app/src/org/commcare/views/FaceCaptureView.java (3)
38-41: Constants should bestatic final
DEFAULT_IMAGE_WIDTH,DEFAULT_IMAGE_HEIGHT, andVIEW_CAPTURE_AREA_RATIOare immutable and can be shared across instances – declare themstatic finalto clarify intent and enable compiler optimizations.- public static int DEFAULT_IMAGE_WIDTH = 480; - public static int DEFAULT_IMAGE_HEIGHT = 640; - private static float VIEW_CAPTURE_AREA_RATIO = 0.8f; + public static final int DEFAULT_IMAGE_WIDTH = 480; + public static final int DEFAULT_IMAGE_HEIGHT = 640; + private static final float VIEW_CAPTURE_AREA_RATIO = 0.8f;
84-88: Missing fallback forcountdownTextSizeSpIf the attribute is not provided,
countdownTextSizeSpis-1, leading to an invalid text size.- countdownTextSizeSp = typedArr.getDimensionPixelSize(R.styleable.FaceCaptureView_countdown_text_size, -1); + countdownTextSizeSp = typedArr.getDimensionPixelSize( + R.styleable.FaceCaptureView_countdown_text_size, + (int)TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_SP, 40, + getResources().getDisplayMetrics()));
278-287:isFaceInCaptureArea– early return improves readabilityTiny readability suggestion:
- if ((faceViewCoords.left < faceCaptureArea.left) || - (faceViewCoords.top < faceCaptureArea.top) || - (faceViewCoords.right > faceCaptureArea.right) || - (faceViewCoords.bottom > faceCaptureArea.bottom)) { - return false; - } - return true; + return faceViewCoords.left >= faceCaptureArea.left && + faceViewCoords.top >= faceCaptureArea.top && + faceViewCoords.right <= faceCaptureArea.right && + faceViewCoords.bottom <= faceCaptureArea.bottom;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
app/res/drawable-hdpi/camera_shutter.pngis excluded by!**/*.pngapp/res/drawable-ldpi/camera_shutter.pngis excluded by!**/*.pngapp/res/drawable-mdpi/camera_shutter.pngis excluded by!**/*.pngapp/res/drawable-xhdpi/camera_shutter.pngis excluded by!**/*.png
📒 Files selected for processing (20)
app/AndroidManifest.xml(2 hunks)app/assets/locales/android_translatable_strings.txt(1 hunks)app/build.gradle(1 hunks)app/res/drawable/baseline_person_24.xml(1 hunks)app/res/layout/micro_image_widget.xml(1 hunks)app/res/layout/screen_personalid_photo_capture.xml(1 hunks)app/res/navigation/nav_graph_personalid.xml(2 hunks)app/res/values/attrs.xml(1 hunks)app/res/values/colors.xml(1 hunks)app/res/values/strings.xml(2 hunks)app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java(1 hunks)app/src/org/commcare/connect/network/ApiConnectId.java(5 hunks)app/src/org/commcare/fragments/MicroImageActivity.java(1 hunks)app/src/org/commcare/fragments/connectId/PersonalIdBackupCodeFragment.java(1 hunks)app/src/org/commcare/fragments/connectId/PersonalIdPhotoCaptureFragment.java(1 hunks)app/src/org/commcare/utils/AndroidUtil.java(2 hunks)app/src/org/commcare/utils/FileUtil.java(3 hunks)app/src/org/commcare/utils/MediaUtil.java(4 hunks)app/src/org/commcare/views/FaceCaptureView.java(1 hunks)build.gradle(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/org/commcare/fragments/MicroImageActivity.java (4)
app/src/org/commcare/utils/AndroidUtil.java (1)
AndroidUtil(22-72)app/src/org/commcare/utils/FileUtil.java (1)
FileUtil(65-967)app/src/org/commcare/utils/MediaUtil.java (1)
MediaUtil(40-632)app/src/org/commcare/views/FaceCaptureView.java (1)
FaceCaptureView(25-306)
🔇 Additional comments (23)
build.gradle (1)
35-35: New CameraX dependency version added.This addition correctly specifies a central version for CameraX dependencies which will be used across the project, following good dependency management practices.
app/res/values/colors.xml (1)
90-90: New translucent background color for face capture UI.This color resource adds a semi-transparent neutral background color (50% opacity) that will be used in the face capture UI components.
app/res/values/strings.xml (2)
739-739: New string for micro image activity title.Clear and descriptive title for the face capture screen.
918-918: Clear error message for photo upload failures.Good error handling message that informs users to check their internet connection.
app/build.gradle (1)
163-167: Dependencies Added for CameraX and ML Kit Face Detection – LGTM
Adds the required CameraX modules and the ML Kit face-detection dependency to enable the new face-capture page. Ensure that thecameraX_versionvariable is defined in the project’s rootbuild.gradleand remains in sync across modules.app/res/values/attrs.xml (1)
133-138: Declare styleable attributes for FaceCaptureView – LGTM
Introduces the necessary custom attributes (background_color,face_capture_area_delimiter_color,face_marker_color, andcountdown_text_size) for the newFaceCaptureView. This aligns perfectly with the view’s requirements.app/AndroidManifest.xml (1)
140-143: Register ML Kit face-detection dependencies metadata – LGTM
Correctly adds the<meta-data>entry forcom.google.mlkit.vision.DEPENDENCIESwith value"face", enabling the face-detection API at runtime.app/src/org/commcare/utils/AndroidUtil.java (2)
12-14: Good addition of Google Play Services importsThese imports are needed for the new method that checks for Google Play Services availability, which is a good practice before using any Google API features.
69-71: Well-implemented Google Play Services checkThis utility method provides a clean way to check if Google Play Services are available on a device. It follows the recommended pattern from Google's documentation by checking if the availability status equals
ConnectionResult.SUCCESS.This will be useful for conditional face detection functionality, falling back to manual capture when Google services aren't available.
app/src/org/commcare/fragments/connectId/PersonalIdBackupCodeFragment.java (1)
341-343: Updated navigation flow to include photo captureThe navigation flow has been changed to direct users to the new photo capture screen after successful PIN setup, passing the user's name as an argument. This aligns with the PR objective of implementing the Personal ID face capture feature.
app/src/org/commcare/utils/FileUtil.java (3)
26-26: Appropriate import for StringUtilsAdded import for Apache Commons Lang3's StringUtils, which will be used for null/empty checks.
759-759: Increased accessibility of bitmap scaling methodChanged access modifier from private to public for
getBitmapScaledByMaxDimen. This is appropriate as the method now needs to be accessed from other classes to support the new photo capture functionality.
889-889: Improved null/empty checkUpdated to use
StringUtils.isEmpty()which handles both null and empty string cases in a single call, improving code readability.app/res/layout/screen_personalid_photo_capture.xml (2)
1-88: Well-structured photo capture layoutThe layout for the Personal ID photo capture screen follows good design practices:
- Clear visual hierarchy with appropriate spacing
- Proper use of Material Design components
- Accessible elements with content descriptions
- Proper state management (e.g., save button initially disabled)
- Organized UI components with clear purpose and intuitive flow
Nice work on creating a clean, user-friendly interface for the photo capture functionality.
75-84: Ensure consistent error handlingThe error TextView is properly styled with red text. Make sure the fragment properly manages its visibility (initially invisible, only shown when needed) and that error messages are descriptive enough to help users resolve issues.
app/src/org/commcare/connect/network/ApiConnectId.java (3)
37-37: LGTM! Added import for Objects utility classClean import addition for the Objects class needed for null checking operations.
218-220: LGTM! Method visibility and signature changes look goodChanged callApi from package-private to private and simplified the Callback generic type parameter.
465-465: LGTM! Method rename for clarityRenaming handleApiError to logNetworkError better reflects the method's purpose.
app/res/navigation/nav_graph_personalid.xml (1)
110-113: LGTM! New navigation action from backup code to photo captureThe navigation action is correctly defined to move the user from the backup code screen to the new photo capture screen.
app/src/org/commcare/fragments/connectId/PersonalIdPhotoCaptureFragment.java (4)
1-56: LGTM! Good implementation of fragment setup and initializationThe class structure, imports, field declarations, and initialization code are well-organized. The fragment properly sets up the view binding, initializes the user record, and configures the UI.
58-70: LGTM! Well-configured result launcher for photo captureThe activity result launcher is correctly set up to handle the photo capture result, process the returned Base64 image, and update the UI accordingly.
166-172: LGTM! Properly implemented photo capture intentThe fragment correctly sets up the intent for MicroImageActivity with appropriate size constraints.
174-183: LGTM! Clear navigation to success screenThe success navigation is well-implemented with all necessary parameters for the message display fragment.
app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java
Show resolved
Hide resolved
app/src/org/commcare/fragments/connectId/PersonalIdPhotoCaptureFragment.java
Show resolved
Hide resolved
| private void finalizeImageCapture(Rect faceArea) { | ||
| Bitmap croppedBitmap = null; | ||
| Bitmap scaledBitmap = null; | ||
| try { | ||
| int paddingXPx = 10; | ||
| int paddingYPy = 50; | ||
| Rect safeFaceArea = new Rect( | ||
| Math.max(0, faceArea.left - paddingXPx), | ||
| Math.max(0, faceArea.top - paddingYPy), | ||
| Math.min(inputImage.getWidth(), faceArea.right + paddingXPx), | ||
| Math.min(inputImage.getHeight(), faceArea.bottom + paddingYPy) | ||
| ); | ||
| croppedBitmap = MediaUtil.cropImage(inputImage, safeFaceArea); | ||
| scaledBitmap = FileUtil.getBitmapScaledByMaxDimen(croppedBitmap, getMaxDimensionSize()); | ||
| if (scaledBitmap == null) { | ||
| scaledBitmap = croppedBitmap; | ||
| } | ||
| byte[] compressedByteArray = MediaUtil.compressBitmapToTargetSize(scaledBitmap, getMaxImageSize()); | ||
| String finalImageAsBase64 = Base64.encodeToString(compressedByteArray, Base64.DEFAULT); | ||
| finishWithResul(finalImageAsBase64); | ||
| } catch (Exception e) { | ||
| logErrorAndExit(e.getMessage(), "microimage.cropping.failed", e.getCause()); | ||
| } finally { | ||
| recycleBitmap(croppedBitmap); | ||
| recycleBitmap(scaledBitmap); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Heavy image processing on UI thread can jank or ANR
Cropping, scaling and compressing bitmaps are CPU-intensive and occasionally hit the heap. Moving this block to a background executor keeps the UI responsive, especially on low-end devices.
Recommend wrapping finalizeImageCapture() in Executors.newSingleThreadExecutor().submit(() -> …) and posting finishWithResult() back to the main thread.
🤖 Prompt for AI Agents
In app/src/org/commcare/fragments/MicroImageActivity.java between lines 251 and
277, the finalizeImageCapture method performs heavy image processing on the UI
thread, which can cause UI jank or ANR. To fix this, wrap the entire image
processing logic inside a background executor using
Executors.newSingleThreadExecutor().submit(() -> { ... }), and ensure that the
call to finishWithResult() is posted back to the main thread using a handler or
runOnUiThread. This will keep the UI responsive by offloading CPU-intensive
tasks to a background thread.
There was a problem hiding this comment.
Goos suggestion but purposefully ignoring for now to keep things simple here, will revisit if it becomes an issue in future.
There was a problem hiding this comment.
@shubham1g5 Understood! Keeping the implementation simpler is a reasonable approach. If performance issues arise in the future, the background thread solution will be there when you need it.
🐰✌️
✏️ Learnings added
Learnt from: shubham1g5
PR: dimagi/commcare-android#3116
File: app/src/org/commcare/fragments/MicroImageActivity.java:0-0
Timestamp: 2025-05-19T16:02:03.715Z
Learning: In MicroImageActivity.java, image processing operations (cropping, scaling, compressing) in the finalizeImageCapture() method are deliberately performed on the UI thread to keep the implementation simple, with plans to move to a background thread if performance issues arise.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
shubham1g5
force-pushed
the
connectFaceCapture
branch
from
176c8c6 to
2cec482
Compare
shubham1g5
requested review from
Jignesh-dimagi,
OrangeAndGreen,
avazirna and
pm-dimagi
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
A nit or two but otherwise all looks good
| try { | ||
| inputImage = ImageConvertUtils.getInstance().convertToUpRightBitmap(image); | ||
| } catch (MlKitException e) { | ||
| Logger.exception("Error during face detection ", e); |
There was a problem hiding this comment.
Call handleErrorDuringDetection here instead?
| } | ||
|
|
||
| public static boolean isGooglePlayServicesAvailable(Context ctx) { | ||
| return GoogleApiAvailability.getInstance().isGooglePlayServicesAvailable(ctx) == ConnectionResult.SUCCESS; |
There was a problem hiding this comment.
Noting I think Play Services is going to be a requirement for PersonalID, since it has to be enabled to do the Play Integrity check. We may want to incorporate this function earlier in the Device Configuration workflow (like on the first page where user enters their phone number).
There was a problem hiding this comment.
noted, I kept that fallback more around so that we can use the MicroImageActivity in other places in CommCare side if needed.
|
@shubham1g5 camera_shutter.png missing for xxhdpi and xxxhdpi |
| } | ||
|
|
||
| private void initTakePhotoLauncher() { | ||
| takePhotoLauncher = registerForActivityResult( |
There was a problem hiding this comment.
@shubham1g5 It is better to define as a private member/variable instead inside the method.
There was a problem hiding this comment.
that seems more of a stylistic preference to me. Are there any advantages of doing that one way vs the other ?
There was a problem hiding this comment.
@shubham1g5 When orientation changes, sometimes function which is instantiating this launcher might not got called so Google prefer as private member which ensures it's 100% availability.
There was a problem hiding this comment.
I see but why would I want to initialize it if my view is not getting generated itself as part of onCreateView and given it's a UI action that triggers this launcher ?
There was a problem hiding this comment.
Yeah, you are correct here, this is more applied when such launchers are getting created with button click. Just trying to see if we follow same through out our code. You can take your call here.
| } | ||
|
|
||
| private void setUpUi() { | ||
| viewBinding.title.setText(requireContext().getString(R.string.connectid_photo_capture_title, getUserName())); |
There was a problem hiding this comment.
requireContext() not required
| if (byteArray.length > targetSize) { | ||
| throw new ImageSizeTooLargeException("Could not compress image to target size"); | ||
| } | ||
| Logger.log(LogTypes.TYPE_MEDIA_EVENT, "Micro image compressed successfully. Number of cycles: " + numCompressionCycles); |
There was a problem hiding this comment.
Do we need to log this?
There was a problem hiding this comment.
think it would be useful to know from logs, how many cycles this process takes on average. That can in future help us to decide what factor to reduce the quality by in each iteration.
| takePhotoLauncher = registerForActivityResult( | ||
| new ActivityResultContracts.StartActivityForResult(), | ||
| result -> { | ||
| if (result.getResultCode() == Activity.RESULT_OK && result.getData() != null) { |
There was a problem hiding this comment.
Should we also check result.getData().hasStringExtra(MicroImageActivity.MICRO_IMAGE_BASE_64_RESULT_KEY)
There was a problem hiding this comment.
Think it's ok to crash if not as that's a clear programming error.
|
|
||
| <LinearLayout | ||
| android:layout_width="match_parent" | ||
| android:layout_height="fill_parent" |
There was a problem hiding this comment.
Please make it match_parent as fill_parent is depreciated
|
|
||
| <TextView | ||
| android:id="@+id/errorTextView" | ||
| tools:text="Error uploading photo" |
There was a problem hiding this comment.
Please remove hard coded text, also visibility should be gone by default?
There was a problem hiding this comment.
good catch on visibility.
tools:text is stripped down in the final apk and is there only for debugging layouts.
| <fragment | ||
| android:id="@+id/personalid_photo_capture" | ||
| android:name="org.commcare.fragments.personalId.PersonalIdPhotoCaptureFragment" | ||
| android:label="fragment_connectid_photo_capture"> |
There was a problem hiding this comment.
fragment_connectid_photo_capture -> fragment_personalid_photo_capture
|
|
||
| public static void updatePhoto(Context context, String userId, String password, String userName, | ||
| String photoAsBase64, IApiCallback callback) { | ||
| Objects.requireNonNull(photoAsBase64); |
There was a problem hiding this comment.
Requires Objects.requireNonNull(username)?
| cameraView = findViewById(R.id.view_finder); | ||
| cameraShutterButton = findViewById(R.id.camera_shutter_button); | ||
|
|
||
| ActionBar actionBar = getSupportActionBar(); |
There was a problem hiding this comment.
We also need action bar back button for user to cancel and navigate back.
| faceCaptureView.setCaptureMode(FaceCaptureView.CaptureMode.ManualMode); | ||
| try { | ||
| startCamera(); | ||
| } catch (ExecutionException | InterruptedException e) { |
There was a problem hiding this comment.
We can remove this try catch as startCamera() method already has try catch for catching the same
|
@shubham1g5 Overall functionality wise, it looks really good as per the video you have posted. |
|
@damagatchi retest this please |
Product Description
https://dimagi.atlassian.net/browse/CCCT-1142
Example UX:
WhatsApp.Video.2025-05-19.at.20.33.31.mp4
Technical Summary
Implements a new page on PersonalID on top of the MicroImageActiity work detailed in the PR
Safety Assurance
Personal ID only changes that doesn't affect CommCare
QA Plan
Will be QA'ed as part of release process
Labels and Review