Merge layout navigations in dv connect initial #3104
Conversation
…ying light patterns.
This is not valid for Android 14+, where the concept of dismissing notification is not supported except for a few use cases: https://developer.android.com/about/versions/14/behavior-changes-all#non-dismissable-notifications
…ts visibility controlled by code at https://github.com/dimagi/commcare-android/blob/a8818b2ac2e3e7abb14bd92cf2dcaa83ea8b5c09/app/src/org/commcare/activities/StandardHomeActivityUIController.java#L176 https://github.com/dimagi/commcare-android/blob/a8818b2ac2e3e7abb14bd92cf2dcaa83ea8b5c09/app/src/org/commcare/activities/StandardHomeActivityUIController.java#L87
Add audio recording foreground service
Improve image downscaling using progressive steps to reduce artifacts.
-updated formStorageTest file with latest Xpath functions
…i/commcare-android into review/layouts_navigations
Backmerge Commcare 2.56.1
Review for layouts, navigations, views, and some minor strings
|
Closing this PR as some mistake in merge |
📝 WalkthroughWalkthroughThis set of changes introduces a new Android foreground service, Sequence Diagram(s)sequenceDiagram
participant User
participant RecordingFragment
participant AudioRecordingService
participant AudioRecordingHelper
participant NotificationManager
User->>RecordingFragment: Initiate audio recording
RecordingFragment->>AudioRecordingService: Bind and start service (with filename)
AudioRecordingService->>AudioRecordingHelper: setupRecorder(filename)
AudioRecordingHelper-->>AudioRecordingService: MediaRecorder instance
AudioRecordingService->>AudioRecordingService: Start recording
AudioRecordingService->>NotificationManager: Show "Recording in progress" notification
RecordingFragment-->>User: Update UI (recording started)
User->>RecordingFragment: Pause/Resume/Stop recording
RecordingFragment->>AudioRecordingService: pauseRecording()/resumeRecording()/stopRecording()
AudioRecordingService->>NotificationManager: Update notification status
AudioRecordingService-->>RecordingFragment: Confirm action
RecordingFragment-->>User: Update UI (recording state)
Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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: 17
🔭 Outside diff range comments (1)
app/src/org/commcare/views/connect/ConnectEditText.java (1)
191-202: 🛠️ Refactor suggestionDrawable-end touch handling is still LTR-only and may NPE
You correctly switched the start drawable lookup to
getCompoundDrawablesRelative()[0]with a null check, but the end path still usesgetCompoundDrawables()[2]without a null check.
- RTL builds will not detect touches on the end icon.
- If the drawable is unexpectedly null an NPE is thrown.
- if (event.getX() >= (getWidth() - getPaddingRight() - getCompoundDrawables()[2].getBounds().width())) { + Drawable end = getCompoundDrawablesRelative()[2]; + if (end != null && + event.getX() >= (getWidth() - getPaddingRight() - end.getBounds().width())) {
🧹 Nitpick comments (14)
app/res/layout/fragment_connect_delivery_progress.xml (1)
79-79: Avoid hard-coded button text colors—use theme/style
Both MaterialButtons now setandroid:textColor="@color/blue". For consistency, prefer using your app’s themed color resource (e.g.,@color/connect_blue_color) or a style attribute (?attr/colorPrimary) instead of a generic@color/blue.Also applies to: 90-90
app/res/layout/fragment_connect_delivery_details.xml (1)
206-206: Leverage MaterialButton’s Icon APIYou replaced
android:drawableRightwithandroid:drawableEndon aMaterialButton, but the recommended approach is to useapp:icon(andapp:iconGravity="end") so thatapp:iconTintis applied correctly. For example:- android:drawableEnd="@drawable/ic_connect_arrow_forward_20px" - app:iconTint="@color/cc_brand_color" + app:icon="@drawable/ic_connect_arrow_forward_20px" + app:iconGravity="end" + app:iconTint="@color/cc_brand_color"app/res/layout/view_job_card.xml (2)
73-75: Ensure RTL compatibility fortv_job_timealignment
Usingandroid:gravity="right"works in LTR locales, but for full RTL support preferandroid:gravity="end"so alignment flips automatically in RTL languages.
86-91: Ensure RTL compatibility forconnect_job_end_datealignment
Similarly, replaceandroid:gravity="right"withandroid:gravity="end"to respect RTL layout directions without hard-coding “right.”app/assets/locales/android_translatable_strings.txt (2)
45-45: Consider updating the installation source mention
The message refers to the “Android market,” which is outdated. It’s preferable to reference the “Google Play Store” to reflect current terminology.
804-804: Refine Wi-Fi Direct no-forms message
The phrase “for Submitting; did you mean to Send forms?” uses inconsistent capitalization. Consider normalizing to sentence case and clarifying the action name.app/instrumentation-tests/src/org/commcare/utils/InstrumentationUtility.kt (1)
29-32:repeatedlyUntilimport is unused
ViewActions.repeatedlyUntilis imported but never referenced, leading to an IDE/Lint warning.
Remove the import or, even better, use it inlogout()(see next comment).app/src/org/commcare/views/widgets/AudioRecordingHelper.java (1)
56-81: Codec scanning could be cached
isHeAacEncoderSupported()iterates over all codecs every timesetupRecorder()is called.
If multiple recordings are expected in one session, cache the result in a static field to avoid repeated expensive scans.private static Boolean heAacSupportedCache = null; ... boolean isHeAacSupported() { if (heAacSupportedCache != null) { return heAacSupportedCache; } heAacSupportedCache = computeSupport(); return heAacSupportedCache; }app/src/org/commcare/views/widgets/RecordingFragment.java (1)
200-227: Potential memory leak:ServiceConnectionkept as anonymous inner instance
audioRecordingServiceConnectionholds an implicit reference to the fragment. Because it’s stored in the Service binder it can out-live the fragment ifunbindService()is forgotten or fails.
Given you already unbind inonDestroyView(), consider:
- Making the connection a static inner class holding a
WeakReferenceto the fragment, or- Nulling the
audioRecordingServiceConnectionfield afterunbindAudioRecordingService().This will guard against edge-cases such as configuration changes while the bind call is still in flight.
app/src/org/commcare/views/widgets/AudioRecordingService.java (5)
19-19: Fix typo in import statementThere is a typo in the import statement:
CommCareNoficationManagershould beCommCareNotificationManager.-import org.commcare.CommCareNoficationManager; +import org.commcare.CommCareNotificationManager;
74-76: Use Intent constants instead of hardcoded stringsReplace hardcoded strings with the corresponding Intent constants for better code maintainability and to avoid potential typos.
Intent activityToLaunch = new Intent(this, DispatchActivity.class); -activityToLaunch.setAction("android.intent.action.MAIN"); -activityToLaunch.addCategory("android.intent.category.LAUNCHER"); +activityToLaunch.setAction(Intent.ACTION_MAIN); +activityToLaunch.addCategory(Intent.CATEGORY_LAUNCHER);
84-84: Update notification channel reference after fixing importAfter fixing the import for
CommCareNotificationManager, make sure to update this reference as well.-return new NotificationCompat.Builder(this, CommCareNoficationManager.NOTIFICATION_CHANNEL_USER_SESSION_ID) +return new NotificationCompat.Builder(this, CommCareNotificationManager.NOTIFICATION_CHANNEL_USER_SESSION_ID)
31-36: Consider making fields final where appropriateConsider making the binder and helper fields final for better code safety and to communicate your intent that these references shouldn't change.
public class AudioRecordingService extends Service { private MediaRecorder recorder; - private final IBinder binder = new AudioRecorderBinder(); + private final IBinder binder = new AudioRecorderBinder(); public static final String RECORDING_FILENAME_EXTRA_KEY = "recording-filename-extra-key"; private NotificationManager notificationManager; - private AudioRecordingHelper audioRecordingHelper = new AudioRecordingHelper(); + private final AudioRecordingHelper audioRecordingHelper = new AudioRecordingHelper();
24-30: Enhance documentation with exception handling detailsThe JavaDoc comment should mention that the service might throw exceptions during recording operations and how clients should handle them.
/** * A foreground service intended to be bound to the RecordingFragment for managing audio recording * operations. Due to its persistent notification, the system treats it with higher importance, reducing the * likelihood of interruptions during recordings. * + * Note: The service methods do not throw exceptions, but clients should handle potential recording + * failures by implementing appropriate error handling strategies. + * * @author avazirna **/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
app/AndroidManifest.xml(2 hunks)app/assets/locales/android_translatable_strings.txt(8 hunks)app/instrumentation-tests/src/org/commcare/androidTests/DemoUserTest.kt(2 hunks)app/instrumentation-tests/src/org/commcare/androidTests/LogSubmissionTest.kt(1 hunks)app/instrumentation-tests/src/org/commcare/androidTests/LoginTest.kt(1 hunks)app/instrumentation-tests/src/org/commcare/androidTests/MenuTests.kt(1 hunks)app/instrumentation-tests/src/org/commcare/utils/InstrumentationUtility.kt(4 hunks)app/res/layout-land/home_screen.xml(4 hunks)app/res/layout/activity_connect_messaging.xml(1 hunks)app/res/layout/connect_delivery_item.xml(2 hunks)app/res/layout/connect_delivery_progress_item.xml(3 hunks)app/res/layout/dialog_payment_confirmation.xml(1 hunks)app/res/layout/fragment_channel_consent_bottom_sheet.xml(1 hunks)app/res/layout/fragment_connect_delivery_details.xml(6 hunks)app/res/layout/fragment_connect_delivery_list.xml(3 hunks)app/res/layout/fragment_connect_delivery_progress.xml(2 hunks)app/res/layout/fragment_connect_downloading.xml(1 hunks)app/res/layout/fragment_connect_jobs_list.xml(0 hunks)app/res/layout/fragment_connect_message.xml(2 hunks)app/res/layout/fragment_connect_results_summary_list.xml(2 hunks)app/res/layout/home_screen.xml(4 hunks)app/res/layout/item_channel.xml(2 hunks)app/res/layout/item_chat_right_view.xml(0 hunks)app/res/layout/item_login_connect_home_corrupt_apps.xml(0 hunks)app/res/layout/view_job_card.xml(3 hunks)app/res/layout/view_progress_job_card.xml(2 hunks)app/res/navigation/nav_graph_connect.xml(1 hunks)app/res/values-fr/strings.xml(2 hunks)app/res/values-pt/strings.xml(2 hunks)app/res/values-sw/strings.xml(3 hunks)app/res/values-ti/strings.xml(1 hunks)app/res/values/strings.xml(2 hunks)app/src/org/commcare/utils/FileUtil.java(6 hunks)app/src/org/commcare/views/ConnectPhoneInputView.java(0 hunks)app/src/org/commcare/views/connect/CircleProgressBar.java(0 hunks)app/src/org/commcare/views/connect/ConnectEditText.java(5 hunks)app/src/org/commcare/views/connect/LinearProgressBar.java(4 hunks)app/src/org/commcare/views/widgets/AudioRecordingHelper.java(1 hunks)app/src/org/commcare/views/widgets/AudioRecordingService.java(1 hunks)app/src/org/commcare/views/widgets/RecordingFragment.java(11 hunks)app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java(1 hunks)
💤 Files with no reviewable changes (5)
- app/res/layout/item_chat_right_view.xml
- app/res/layout/item_login_connect_home_corrupt_apps.xml
- app/res/layout/fragment_connect_jobs_list.xml
- app/src/org/commcare/views/connect/CircleProgressBar.java
- app/src/org/commcare/views/ConnectPhoneInputView.java
🧰 Additional context used
🧠 Learnings (2)
app/res/layout/activity_connect_messaging.xml (1)
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/layout/activity_connect_messaging.xml:0-0
Timestamp: 2025-05-09T13:45:18.661Z
Learning: In Android layouts, use `layout_constraintStart_toStartOf` and `layout_constraintEnd_toEndOf` instead of the deprecated `layout_constraintLeft_toLeftOf` and `layout_constraintRight_toRightOf` to ensure proper RTL (Right-to-Left) layout support.
app/res/navigation/nav_graph_connect.xml (1)
Learnt from: Jignesh-dimagi
PR: dimagi/commcare-android#3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
🔇 Additional comments (64)
app/res/layout/fragment_channel_consent_bottom_sheet.xml (1)
27-27: Good accessibility improvement.Adding
android:importantForAccessibility="no"to this decorative icon ensures screen readers won't focus on it, improving the experience for users with accessibility needs. This complements the existingandroid:contentDescription="@null"attribute.app/res/layout/dialog_payment_confirmation.xml (2)
12-12: Good layout optimization.Changing the layout height from "match_parent" to "wrap_content" makes the dialog size appropriate to its content rather than filling the entire screen, which improves the user experience for this dialog.
20-21: Good accessibility improvement.Adding both
android:contentDescription="@null"andandroid:importantForAccessibility="no"to this status icon properly excludes it from screen reader focus, improving accessibility for users with visual impairments.app/src/org/commcare/views/connect/LinearProgressBar.java (4)
6-7: LGTM - Required imports for gradient functionality.These imports support the new gradient coloring feature being added to the progress bar.
Also applies to: 9-9
29-29: LGTM - Changed variable type for direct color access.Modified from a resource ID to a direct color integer for more efficient rendering.
48-50: LGTM - Improved color initialization.Using
ContextCompat.getColor()is the recommended way to retrieve colors from resources while maintaining compatibility across API levels.
74-82: Well-implemented gradient support.The gradient implementation correctly:
- Checks for gradient mode and sufficient colors
- Creates a linear gradient shader in the correct direction
- Applies the shader to the paint only when needed
- Clears the shader when not in gradient mode
This provides a nice visual enhancement option for progress bars.
app/res/layout/item_channel.xml (3)
37-39: Good accessibility improvement.Adding
android:contentDescription="@null"andandroid:importantForAccessibility="no"to this decorative icon ensures screen readers won't focus on it, improving accessibility.
44-45: Improved layout constraints for the channel name.Changing from a fixed width to a constraint-based width (
0dp) with proper end constraint and margin creates a more responsive layout that adapts to different screen sizes and text lengths.Also applies to: 52-54
62-63: Refined alignment for channel description.The adjusted margins and constraints properly position the description text relative to the channel name, creating a more consistent and visually pleasing layout.
Also applies to: 66-67, 69-70
app/instrumentation-tests/src/org/commcare/androidTests/MenuTests.kt (1)
40-40: Good naming consistency improvementChanging "Wifi Direct" to "Wi-Fi Direct" properly follows standard capitalization and hyphenation conventions for Wi-Fi terminology, improving consistency throughout the application.
app/res/layout/fragment_connect_downloading.xml (1)
23-24: Good accessibility improvementAdding
android:contentDescription="@null"andandroid:importantForAccessibility="no"correctly marks this decorative image as non-relevant for screen readers, improving the accessibility experience for users with visual impairments.app/instrumentation-tests/src/org/commcare/androidTests/LogSubmissionTest.kt (1)
64-65: Good UI test reliability improvementAdding an explicit keyboard dismissal step immediately after clicking "Report Problem" helps prevent potential UI interference issues during test execution, improving test stability and reliability.
app/instrumentation-tests/src/org/commcare/androidTests/DemoUserTest.kt (4)
24-25: Good UI structure alignmentUpdating from
R.id.home_gridview_buttonstoR.id.nsv_home_screencorrectly targets the new nested scroll view component for swipe operations, aligning with UI layout changes.
29-30: Good UI structure alignmentUpdating from
R.id.home_gridview_buttonstoR.id.nsv_home_screencorrectly targets the new nested scroll view component for swipe operations, aligning with UI layout changes.
57-58: Good UI structure alignmentUpdating from
R.id.home_gridview_buttonstoR.id.nsv_home_screencorrectly targets the new nested scroll view component for swipe operations, aligning with UI layout changes.
62-63: Good UI structure alignmentUpdating from
R.id.home_gridview_buttonstoR.id.nsv_home_screencorrectly targets the new nested scroll view component for swipe operations, aligning with UI layout changes.app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java (1)
393-395: Added new XPath function classes for form serialization compatibility.These new classes for geospatial calculations (
XPathClosestPointOnPolygonFuncandXPathIsPointInsidePolygonFunc) are properly added to the compatibility list with appropriate version annotation.app/instrumentation-tests/src/org/commcare/androidTests/LoginTest.kt (1)
74-74: Updated swipe target for improved test reliability.The test now correctly targets the NestedScrollView (
nsv_home_screen) for the swipe action rather than the grid buttons. This change aligns with similar updates in other instrumentation tests.app/res/values-ti/strings.xml (1)
30-30: Fixed extraneous quotation mark in Tigrinya string resource.Removed the unnecessary trailing double quote from the
back_to_managerstring value, improving string resource consistency.app/res/navigation/nav_graph_connect.xml (1)
62-62: Fixed typo in fragment label.Corrected the spelling of "progress" in the fragment label attribute.
app/res/layout/view_progress_job_card.xml (2)
7-7: Change CardView height to wrap_content
Adjusting the rootCardView’slayout_heighttowrap_contentensures the card sizes dynamically to its content, preventing unnecessary empty space.
73-75: Aligntv_job_timeusing proper constraints
Addinglayout_marginStart="5dp",android:gravity="end", andapp:layout_constraintStart_toEndOf="@+id/tv_view_more"right-aligns the time text between the “view more” label and parent. Verify that whentv_view_moreis GONE the constraint still resolves as expected (it should anchor to the invisible view’s virtual bounds).app/res/layout/connect_delivery_item.xml (2)
23-23: Exclude decorative icon from accessibility focus
MarkingimgDeliveryStatuswithandroid:importantForAccessibility="no"correctly removes a non-informative element from the accessibility traversal, improving screen-reader experience.
70-71: Add missing start/top constraints to status TextView
Introducingapp:layout_constraintStart_toStartOf="parent"andapp:layout_constraintTop_toTopOf="parent"fordelivery_item_statusresolves the previous missing-constraint warnings and guarantees consistent positioning.app/res/layout/activity_connect_messaging.xml (1)
17-18: Use RTL-aware constraints instead of deprecated left/right
Replacinglayout_constraintLeft_toLeftOf/Right_toRightOfwithconstraintStart_toStartOfandconstraintEnd_toEndOfimproves right-to-left language support and aligns with best practices.app/res/layout/fragment_connect_results_summary_list.xml (2)
30-30: Enable weight-based sizing for first CardView
Switchinglayout_widthto0dpallows thelayout_weight="1"setting in the horizontalLinearLayoutto distribute space evenly.
84-84: Enable weight-based sizing for second CardView
Similarly, settinglayout_widthto0dpensures both CardViews share available horizontal space per their weights.app/res/layout/fragment_connect_delivery_details.xml (2)
58-58: Consistent Accessibility Exclusion for Decorative IconsYou’ve correctly marked each decorative ImageView with
android:importantForAccessibility="no"(andcontentDescription="@null") to keep screen readers from announcing them. This is the right approach for purely decorative icons.Also applies to: 82-82, 106-106, 128-128
170-170: Use RTL‐aware Margin for SpacingAdding
android:layout_marginEnd="2dp"ensures proper spacing before the download button in both LTR and RTL layouts. Good use ofmarginEndover the deprecatedmarginRight.app/res/layout/fragment_connect_message.xml (2)
44-44: Localize the Hint TextReplacing the hardcoded hint with
@string/connect_send_message_hintensures this UI text is translatable. Great catch.
67-67: Add Content Description for AccessibilityAdding
android:contentDescription="@string/connect_send_message_btn_desc"makes the send button accessible to screen readers. Perfect improvement.app/res/values-pt/strings.xml (1)
42-42: Fix Unmatched Quotes inback_to_managerThe extraneous quotes have been removed, correcting the resource. This aligns with other localization fixes.
app/res/layout/connect_delivery_progress_item.xml (3)
46-46: Enable Weight Distribution by Setting Width to 0dpChanging
android:layout_widthto0dpis necessary when usinglayout_weightso both child LinearLayouts share space correctly.
102-103: Apply Weight and Wrap Content for Second SectionYou’ve set
android:layout_width="0dp"andandroid:layout_height="wrap_content"alongsidelayout_weight="1", ensuring proportional width and correct height. Well done.
122-122: Nested Layout Height AdjustmentSwitching the nested vertical
LinearLayoutfrommatch_parenttowrap_contentprevents it from unnecessarily filling vertical space.app/res/values-sw/strings.xml (1)
42-42: Correctedback_to_managerTranslationRemoved extraneous quotes to match other locales. Good alignment with default and Portuguese files.
app/res/layout/view_job_card.xml (1)
42-42: Adjusttv_view_morewidth for correct content sizing
Changing thetv_view_morewidth from0dp(match-constraints) towrap_contentensures the label only occupies the space it needs, preventing it from stretching across the row and enabling adjacent views to be positioned relative to its end.app/res/layout-land/home_screen.xml (4)
6-6: Expand root layout to full width
Updating theLinearLayout'slayout_widthtomatch_parentensures the landscape layout spans the entire screen, improving consistency and using the available horizontal space.
85-85: Add ID for scrollable container
Assigning@+id/nsv_home_screento theNestedScrollViewstandardizes the scrollable view reference, aligning with instrumentation tests and facilitating UI interactions.
99-99: Hide connect message CardView by default
Settingandroid:visibility="gone"oncvConnectMessagematches the portrait layout behavior and prevents this alert from appearing until explicitly shown.
134-134: Hide included job card layout by default
Applyingandroid:visibility="gone"on theview_progress_job_cardinclude ensures this card remains hidden on initial render, consistent with the updated UI flow.app/res/values-fr/strings.xml (2)
203-203: New completion message is clear
Theconnect_learn_finishedstring is well-phrased, correctly escaped, and uses French typographic rules for colons.
205-205: New failure notification is clear
Theconnect_learn_failedstring appropriately informs users of a failed assessment and includes score details.app/assets/locales/android_translatable_strings.txt (6)
83-83: Add Wi-Fi Direct menu option
Introducinghome.menu.wifi.directaligns with the UI update to support Wi-Fi Direct functionality and matches the newnsv_home_screeninteractions.
123-123: Handle missing sync URL gracefully
Thesync.fail.empty.urlstring clearly communicates that the Sync URL isn’t set, guiding users to contact support.
413-413: Clarify no active connections action
Thenotification.sync.connections.actionstring gives actionable guidance on enabling network connectivity.
472-474: Add audio recording notification strings
The new keys (recording.notification.title,.in.progress,.paused) back theAudioRecordingServicenotifications, providing clear UX feedback during recording operations.
524-525: Handle empty login URL errors
Addinglogin.attempt.fail.empty.url.titleand.detailimproves error reporting when the login endpoint is not configured.
806-806: Clarify discovery failure guidance
Thewifi.direct.discovery.failed.genericstring succinctly instructs users to toggle Wi-Fi; this is clear and appropriate.app/res/layout/home_screen.xml (4)
85-85: Assign ID to NestedScrollView
Adding@+id/nsv_home_screento theNestedScrollViewenables reliable UI interactions and aligns with updated instrumentation tests.
99-102: Default hide connect message and adjust positioning
HidingcvConnectMessageby default, applyingtranslationYinstead of negative margins, and usingmarginStart/marginEndmakes the alert hidden initially and positions it correctly when shown.
134-134: Hide progress job card by default
Settingandroid:visibility="gone"on the includedview_progress_job_cardprevents it from appearing until explicitly made visible, matching the updated UI flow.
145-145: Limit RecyclerView height to content
Changing theRecyclerView's height frommatch_parenttowrap_contentallows it to size to its content and improves nested scrolling behavior.app/AndroidManifest.xml (2)
39-39: Appropriate permission added for audio recording foreground serviceThe addition of the
FOREGROUND_SERVICE_MICROPHONEpermission is correct and necessary when implementing a foreground service that uses the microphone.
339-345: Properly configured audio recording foreground serviceThe new
AudioRecordingServiceis correctly declared with appropriate attributes:
enabled="true"ensures the service can be startedexported="false"prevents access from external applicationsstopWithTask="true"ensures proper cleanup when the app is terminatedforegroundServiceType="microphone"correctly categorizes this as a microphone service, which aligns with Android's permission modelThis follows best practices for implementing recording functionality using a foreground service.
app/res/values/strings.xml (2)
53-53: Fixed string resource by removing extraneous quotesCorrected the
back_to_managerstring resource.
551-552: Added message input accessibility stringsAdded appropriate strings for messaging input field hint and send button accessibility description, which improves the user experience and accessibility of the messaging interface.
app/res/layout/fragment_connect_delivery_list.xml (3)
32-32: Improved view ID naming to match view typeRenamed ID from
llFilterAlltocvFilterAllto correctly reflect that it's a CardView and not a LinearLayout. All constraint references to this ID are also properly updated.Also applies to: 77-77
36-37: Enhanced accessibility with proper interactive attributesAdded
clickableandfocusableattributes to ensure the CardView properly handles user interaction. The null content description helps screen readers skip decorative elements.Also applies to: 40-40
51-51: Fixed incorrect padding unitChanged padding from
16spto16dp. This is an important fix sincespshould only be used for text sizes, whiledpis the correct unit for layout dimensions.app/src/org/commcare/utils/FileUtil.java (1)
772-782: Updated bitmap scaling to use progressive downscalingThe existing
getBitmapScaledByMaxDimenmethod now uses the new step-wise approach instead of direct scaling, which will provide better quality results.app/instrumentation-tests/src/org/commcare/utils/InstrumentationUtility.kt (1)
317-320:enterTextno longer clears focus before replacing textThe new chain
(click(), replaceText(text), closeSoftKeyboard())is concise, but in crowded forms anEditTextmight already contain the cursor elsewhere. Consider keeping an explicitclearText()(or ensurereplaceTextsuffices in all test scenarios) to avoid residual chars when the field already has content.app/src/org/commcare/views/widgets/RecordingFragment.java (1)
253-257:resumeRecording()beforestopRecording()may throw when service has already stopped
audioRecordingService.resumeRecording()is called unconditionally ifinPausedState, but if the service has been killed (e.g. by system under memory pressure) this will NPE.
Add a null/active check:if (inPausedState && audioRecordingService != null && audioRecordingService.isRecorderActive()) { audioRecordingService.resumeRecording(); }
| <string name="save_all_answers">Salvar Formulário</string> | ||
| <string name="save_data_message">Por favor, salvar Formulário para manter as respostas.</string> | ||
| <string name="save_enter_data_description">Você está no fim de \%s\"."</string> | ||
| <string name="save_enter_data_description">Você está no fim de "\%s\".</string> |
There was a problem hiding this comment.
Incorrect Escaping of Quotes and Placeholder
The string uses backslashes and an unindexed %s. In XML element text you don’t need backslashes, and best practice is to use indexed placeholders and " for quotes. For example:
- <string name="save_enter_data_description">Você está no fim de "\%s\".</string>
+ <string name="save_enter_data_description">Você está no fim de "%1$s".</string>📝 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.
| <string name="save_enter_data_description">Você está no fim de "\%s\".</string> | |
| <string name="save_enter_data_description">Você está no fim de "%1$s".</string> |
🤖 Prompt for AI Agents (early access)
In app/res/values-pt/strings.xml at line 191, remove the backslashes before the
quotes and replace the quotes with " entities. Also, change the %s
placeholder to an indexed format like %1$s for best practice. The corrected
string should use " for quotes and %1$s for the placeholder without any
backslashes.
| <string name="capture_video">Rekodi Video</string> | ||
| <string name="change_language">Badilisha Lugha</string> | ||
| <string name="clearanswer_confirm">Ondoa jibu kwa \%s\"?"</string> | ||
| <string name="clearanswer_confirm">Ondoa jibu kwa \"%s\"?</string> |
There was a problem hiding this comment.
Mis-escaped Quotes in clearanswer_confirm
Backslashes in the element text will appear literally. Use " and an indexed placeholder instead. For example:
- <string name="clearanswer_confirm">Ondoa jibu kwa \"%s\"?</string>
+ <string name="clearanswer_confirm">Ondoa jibu kwa "%1$s"?</string>📝 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.
| <string name="clearanswer_confirm">Ondoa jibu kwa \"%s\"?</string> | |
| <string name="clearanswer_confirm">Ondoa jibu kwa "%1$s"?</string> |
🤖 Prompt for AI Agents (early access)
In app/res/values-sw/strings.xml at line 102, the string clearanswer_confirm
uses backslashes to escape quotes, which will render literally. Replace the
backslash-escaped quotes with " entities and use an indexed placeholder for
the string substitution. For example, change the text to use "%1$s"
instead of \"%s\" to properly escape quotes and support formatting.
| <string name="enter_data">Jaza Fomu tupu</string> | ||
| <string name="enter_data_button">Jaza Fomu tupu</string> | ||
| <string name="enter_data_description">Uko mwanzoni mwa \%s\". Telezesha kidole skrini kama inavyoonyeshwa hapa chini ili kuanza."</string> | ||
| <string name="enter_data_description">Uko mwanzoni mwa \"%s\". Telezesha kidole skrini kama inavyoonyeshwa hapa chini ili kuanza.</string> |
There was a problem hiding this comment.
Mis-escaped Quotes in enter_data_description
Same issue here with backslashes. Use entity references and indexed placeholders:
- <string name="enter_data_description">Uko mwanzoni mwa \"%s\". Telezesha kidole skrini kama inavyoonyeshwa hapa chini ili kuanza.</string>
+ <string name="enter_data_description">Uko mwanzoni mwa "%1$s". Telezesha kidole skrini kama inavyoonyeshwa hapa chini ili kuanza.</string>🤖 Prompt for AI Agents (early access)
In app/res/values-sw/strings.xml at line 126, the string value for
enter_data_description incorrectly uses backslashes to escape quotes. Replace
the backslashes with proper XML entity references for quotes (e.g., ") and
use indexed placeholders (e.g., %1$s) instead of unescaped %s to ensure correct
formatting and escaping in the string resource.
| <string name="download">Obtenez celle sélectionnée</string> | ||
| <string name="download_forms_result">Resultat du téléchargement</string> | ||
| <string name="downloading_data">Connection au serveur…</string> | ||
| <string name="downloading_data">Connexion au serveur…</string> |
There was a problem hiding this comment.
Verify translation accuracy for downloading_data
The string Connexion au serveur… reads as “Connecting to the server…” rather than “Downloading data…”. Please confirm that this matches the intended English meaning; if it should indicate downloading, consider “Téléchargement des données…”.
🤖 Prompt for AI Agents (early access)
In app/res/values-fr/strings.xml at line 44, the French translation for
"downloading_data" currently reads "Connexion au serveur…", which means
"Connecting to the server…". Verify if the intended meaning is "Downloading
data…". If so, update the string to "Téléchargement des données…" to accurately
reflect the English source.
| wifi.direct.change.state.title=Send, Receive, Submit | ||
| wifi.direct.change.state.text=Do you want to send, receive, or submit forms? | ||
| wifi.direct.wifi.direct.off=WiFi Direct is Off - Please turn it on to use this feature | ||
| wifi.direct.wifi.direct.off=Wi-Fi Direct is Off - Please turn it on to use this feature |
There was a problem hiding this comment.
Review inconsistent key naming for Wi-Fi Direct
The key wifi.direct.wifi.direct.off redundantly repeats wifi.direct. Consider renaming to wifi.direct.off or similar to maintain a consistent key hierarchy.
🤖 Prompt for AI Agents (early access)
In app/assets/locales/android_translatable_strings.txt at line 792, the key name
wifi.direct.wifi.direct.off redundantly repeats wifi.direct. Rename the key to
wifi.direct.off to maintain a consistent and concise key hierarchy for Wi-Fi
Direct related strings.
| @Override | ||
| public void onDestroyView() { | ||
| super.onDestroyView(); | ||
| unbindAudioRecordingService(); | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { | ||
| unregisterAudioRecordingConfigurationChangeCallback(); | ||
| } | ||
| } | ||
|
|
||
| private void unbindAudioRecordingService() { | ||
| if (audioRecordingServiceBounded) { | ||
| requireActivity().unbindService(audioRecordingServiceConnection); | ||
| requireActivity() | ||
| .stopService(new Intent(requireActivity(), AudioRecordingService.class)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure service stopped even when recording never started
unbindAudioRecordingService() only executes when audioRecordingServiceBounded is true.
If the user cancels the dialog before onServiceConnected fires the service remains in the foreground.
Consider also calling requireActivity().stopService(...) outside the if to guarantee cleanup.
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/RecordingFragment.java around lines 469 to
484, the current unbindAudioRecordingService method only stops the
AudioRecordingService if audioRecordingServiceBounded is true, which can leave
the service running if the user cancels before onServiceConnected. To fix this,
move the requireActivity().stopService(...) call outside and after the if block
so the service is always stopped regardless of the binding state, ensuring
proper cleanup.
| @RequiresApi(api = Build.VERSION_CODES.N) | ||
| public void resumeRecording() { | ||
| recorder.resume(); | ||
| notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID, | ||
| createNotification(true)); | ||
| } |
There was a problem hiding this comment.
Add null check before resuming recording
Similar to pauseRecording, the resumeRecording method should check if the recorder is null before attempting to resume it.
@RequiresApi(api = Build.VERSION_CODES.N)
public void resumeRecording() {
+ if (recorder == null) {
+ return;
+ }
recorder.resume();
notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID,
createNotification(true));
}📝 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.
| @RequiresApi(api = Build.VERSION_CODES.N) | |
| public void resumeRecording() { | |
| recorder.resume(); | |
| notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID, | |
| createNotification(true)); | |
| } | |
| @RequiresApi(api = Build.VERSION_CODES.N) | |
| public void resumeRecording() { | |
| if (recorder == null) { | |
| return; | |
| } | |
| recorder.resume(); | |
| notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID, | |
| createNotification(true)); | |
| } |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java around lines
129 to 134, the resumeRecording method calls recorder.resume() without checking
if recorder is null. Add a null check for recorder before calling resume() to
prevent potential NullPointerException, similar to the pauseRecording method.
| @Override | ||
| public int onStartCommand(Intent intent, int flags, int startId) { | ||
| String fileName = intent.getExtras().getString(RECORDING_FILENAME_EXTRA_KEY); | ||
| if (recorder == null) { | ||
| recorder = audioRecordingHelper.setupRecorder(fileName); | ||
| } | ||
| recorder.start(); | ||
| return START_NOT_STICKY; | ||
| } |
There was a problem hiding this comment.
Add null check for intent extras to prevent NullPointerException
The onStartCommand method doesn't check if intent.getExtras() is null before accessing it, which could lead to a NullPointerException if the service is started without extras.
@Override
public int onStartCommand(Intent intent, int flags, int startId) {
- String fileName = intent.getExtras().getString(RECORDING_FILENAME_EXTRA_KEY);
+ String fileName = null;
+ if (intent != null && intent.getExtras() != null) {
+ fileName = intent.getExtras().getString(RECORDING_FILENAME_EXTRA_KEY);
+ }
+
+ if (fileName == null) {
+ stopSelf();
+ return START_NOT_STICKY;
+ }
+
if (recorder == null) {
recorder = audioRecordingHelper.setupRecorder(fileName);
}
recorder.start();
return START_NOT_STICKY;
}🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java around lines 50
to 58, add a null check for intent.getExtras() before accessing
getString(RECORDING_FILENAME_EXTRA_KEY) to prevent a NullPointerException.
Modify the code to first verify that intent and intent.getExtras() are not null,
and only then retrieve the fileName from the extras. Handle the case where
extras are null appropriately, such as by logging an error or returning early.
| @RequiresApi(api = Build.VERSION_CODES.N) | ||
| public void pauseRecording() { | ||
| recorder.pause(); | ||
| notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID, | ||
| createNotification(false)); | ||
| } |
There was a problem hiding this comment.
Add null check before pausing recording
The pauseRecording method should check if the recorder is null or not active before attempting to pause it.
@RequiresApi(api = Build.VERSION_CODES.N)
public void pauseRecording() {
+ if (recorder == null) {
+ return;
+ }
recorder.pause();
notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID,
createNotification(false));
}📝 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.
| @RequiresApi(api = Build.VERSION_CODES.N) | |
| public void pauseRecording() { | |
| recorder.pause(); | |
| notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID, | |
| createNotification(false)); | |
| } | |
| @RequiresApi(api = Build.VERSION_CODES.N) | |
| public void pauseRecording() { | |
| if (recorder == null) { | |
| return; | |
| } | |
| recorder.pause(); | |
| notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID, | |
| createNotification(false)); | |
| } |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java around lines
122 to 127, the pauseRecording method calls recorder.pause() without checking if
recorder is null or currently active. Add a null check to ensure recorder is not
null and verify it is in a state that can be paused before calling pause() to
prevent potential NullPointerException or illegal state errors.
| public void stopRecording() { | ||
| recorder.stop(); | ||
| } |
There was a problem hiding this comment.
Add null check before stopping recording
The stopRecording method should check if the recorder is null before attempting to stop it.
public void stopRecording() {
+ if (recorder == null) {
+ return;
+ }
recorder.stop();
}📝 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.
| public void stopRecording() { | |
| recorder.stop(); | |
| } | |
| public void stopRecording() { | |
| if (recorder == null) { | |
| return; | |
| } | |
| recorder.stop(); | |
| } |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java around lines
136 to 138, the stopRecording method calls recorder.stop() without checking if
recorder is null. Add a null check to ensure recorder is not null before calling
stop() to prevent potential NullPointerException.
@Jignesh-dimagi Can you explain, this seems alright to me. |
|
@shubham1g5 Please use #3105 |
Technical Summary
Changes for https://dimagi.atlassian.net/browse/CCCT-1087 already merged in master, now rebasing master into dv/connect_initial
Labels and Review