14_05 master merge in dv connect initial #3105
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
📝 WalkthroughWalkthroughThis update introduces a new foreground audio recording service to the Android application. The AndroidManifest.xml is updated to request the Sequence Diagram(s)sequenceDiagram
participant User
participant RecordingFragment
participant AudioRecordingService
participant MediaRecorder
participant NotificationManager
User->>RecordingFragment: Initiates audio recording
RecordingFragment->>AudioRecordingService: Binds and starts service with filename
AudioRecordingService->>MediaRecorder: Initializes and starts recording
AudioRecordingService->>NotificationManager: Shows "Recording in progress" notification
User->>RecordingFragment: Pauses/resumes/stops recording
RecordingFragment->>AudioRecordingService: Calls pause/resume/stop methods
AudioRecordingService->>MediaRecorder: Pauses/resumes/stops recording
AudioRecordingService->>NotificationManager: Updates notification state
RecordingFragment->>AudioRecordingService: Unbinds and stops service on destroy
AudioRecordingService->>MediaRecorder: Releases resources
AudioRecordingService->>NotificationManager: Removes notification
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: 8
🔭 Outside diff range comments (3)
app/res/values-pt/strings.xml (1)
101-101:⚠️ Potential issueFix
clearanswer_confirmformattingThe current value contains mismatched escape sequences and quotes (
\%s\"?"). This will cause runtime display issues. It should be:<string name="clearanswer_confirm">Remover a resposta para "%s"?</string>app/res/layout/fragment_connect_delivery_list.xml (2)
84-85:⚠️ Potential issueSame incorrect padding unit exists in other filter layouts.
While the padding unit was fixed for the "All" filter, the same issue exists in other filter layouts which still use
16spinstead of the correct16dpunit.- android:paddingHorizontal="16sp" + android:paddingHorizontal="16dp"
67-78: 🛠️ Refactor suggestionMissing accessibility attributes on other filter CardViews.
Accessibility attributes (clickable, focusable, contentDescription) were added to the "All" filter but not to the other filters, which may create inconsistent touch and accessibility behavior.
Apply the same accessibility attributes to the other filters:
app:cardCornerRadius="20dp" app:cardElevation="0dp" + android:clickable="true" + android:focusable="true" + android:contentDescription="@null" app:layout_constraintBottom_toBottomOf="parent"
🧹 Nitpick comments (16)
app/res/layout/view_progress_job_card.xml (1)
11-14: Optional: Update child ConstraintLayout height.Since the root
CardViewis nowwrap_content, consider changing the innerConstraintLayoutheight frommatch_parenttowrap_contentto avoid measurement ambiguities:- <androidx.constraintlayout.widget.ConstraintLayout - android:layout_width="match_parent" - android:layout_height="match_parent" + <androidx.constraintlayout.widget.ConstraintLayout + android:layout_width="match_parent" + android:layout_height="wrap_content" android:padding="16dp">app/res/layout/view_job_card.xml (2)
73-75: Adjusttv_job_timealignmentAdding
layout_marginStartandlayout_constraintStart_toEndOf="@+id/tv_view_more"positionstv_job_timecorrectly relative to theview_morelabel. For better RTL support, consider usingandroid:textAlignment="viewStart"instead ofandroid:gravity.
86-91: Alignconnect_job_end_datewith start-to-end constraintApplying
android:gravity="right",layout_marginStart, andlayout_constraintStart_toEndOf="@+id/tv_view_more"aligns the end date properly. Similarly, for consistency in RTL layouts, preferandroid:textAlignment="viewEnd"overandroid:gravity.app/res/values-fr/strings.xml (1)
44-44: Updatedownloading_datatranslationChanging to
"Connexion au serveur…"is appropriate, but you may want to match the default English semantics (e.g.,"Téléchargement en cours…"). Please verify with the localization team.app/res/layout/fragment_connect_delivery_list.xml (1)
67-78: Inconsistent ID naming across filter CardViews.The "All" filter was renamed from
llFilterAlltocvFilterAllto match its container type, but other filters (Approved, Rejected, Pending) still use thellprefix which creates inconsistency.Consider renaming other filter IDs similarly for consistency:
- android:id="@+id/llFilterApproved" + android:id="@+id/cvFilterApproved"And updating their references throughout the layout.
app/src/org/commcare/views/connect/LinearProgressBar.java (2)
91-95: Method setGradientColors lacks input validation.The method accepts an array of colors without validating that it contains at least two colors, which is required for creating a gradient.
Consider adding validation to ensure the gradient colors array has at least two colors:
public void setGradientColors(int[] colors) { + if (colors == null || colors.length < 2) { + throw new IllegalArgumentException("Gradient requires at least two colors"); + } this.gradientColors = colors; this.isGradient = true; invalidate(); }
28-29: Log statement in setProgressColor could be reduced to debug level.The current error log in setProgressColor (line 104) uses Log.e which is typically reserved for actual errors, not informational messages about state changes.
Consider changing to debug level logging:
- Log.e("LinearProgressBar", "Setting progress color to: " + color); + Log.d("LinearProgressBar", "Setting progress color to: " + color);app/src/org/commcare/views/connect/ConnectEditText.java (1)
189-190: Improved null safety and RTL supportUsing
getCompoundDrawablesRelative()instead ofgetCompoundDrawables()provides better RTL layout support. The additional null check before accessing bounds prevents potential NullPointerExceptions when no drawable is set.Consider applying the same null-safety check to the end drawable access on line 199 to maintain consistency:
if (drawableEndVisible && event.getAction() == MotionEvent.ACTION_UP) { - if (event.getX() >= (getWidth() - getPaddingRight() - getCompoundDrawables()[2].getBounds().width())) { + Drawable end = getCompoundDrawablesRelative()[2]; + if (end != null && event.getX() >= (getWidth() - getPaddingRight() - end.getBounds().width())) { if (onDrawableEndClickListener != null) { onDrawableEndClickListener.onDrawableEndClick(); } return true; } }app/instrumentation-tests/src/org/commcare/utils/InstrumentationUtility.kt (1)
29-32: Remove unused import & avoid lint warnings
repeatedlyUntilis imported but never used after the refactor, which will triggerUnusedImportwarnings and can break Kotlindetekt/ktlintbaselines.-import androidx.test.espresso.action.ViewActions.repeatedlyUntilapp/src/org/commcare/utils/FileUtil.java (1)
758-785: Clarify intent & avoid variable reuse ingetBitmapScaledByMaxDimen()Reusing
sideToScaleandotherSideafter they are repurposed for target dimensions is confusing and error-prone. Simple renaming improves readability and prevents accidental misuse:- sideToScale = maxDimen; - otherSide = (int)Math.floor(maxDimen * aspectRatio); + int scaledMajorSide = maxDimen; + int scaledMinorSide = (int) Math.floor(maxDimen * aspectRatio); ... - if (width > height) { - targetWidth = sideToScale; - targetHeight = otherSide; + if (width > height) { + targetWidth = scaledMajorSide; + targetHeight = scaledMinorSide; } else { - targetWidth = otherSide; - targetHeight = sideToScale; + targetWidth = scaledMinorSide; + targetHeight = scaledMajorSide; }Pure refactor—does not change behaviour but makes the scaling maths self-documenting.
app/src/org/commcare/views/widgets/AudioRecordingHelper.java (1)
55-81: Cache codec-support lookup to avoid repeated heavy scans
isHeAacEncoderSupported()walks the entire codec list every timesetupRecorder()is called.
On older devices this is an expensive I/O-heavy reflection operation.- private boolean isHeAacEncoderSupported() { + private static Boolean heAacSupport = null; + + private boolean isHeAacEncoderSupported() { + if (heAacSupport != null) { + return heAacSupport; + } int numCodecs = MediaCodecList.getCodecCount(); ... - return false; + heAacSupport = false; + return heAacSupport; }Caching avoids redundant work during rapid successive recordings.
app/src/org/commcare/views/widgets/RecordingFragment.java (2)
89-92: Rename state flag for clarity
audioRecordingServiceBoundedshould beaudioRecordingServiceBound(orisServiceBound)—“bounded” is grammatically incorrect and may confuse future readers.Refactor variable & all references:
-private boolean audioRecordingServiceBounded = false; +private boolean serviceBound = false;
470-484: Add try/catch aroundunbindService()to avoidIllegalArgumentExceptionIf the service disconnects unexpectedly,
unbindService()can throw.
Wrap the call to keep the fragment teardown resilient:- requireActivity().unbindService(audioRecordingServiceConnection); + try { + requireActivity().unbindService(audioRecordingServiceConnection); + } catch (IllegalArgumentException ignored) { + // Already unbound + }app/src/org/commcare/views/widgets/AudioRecordingService.java (3)
66-71: Consider adding proper error handling in resetRecorderThe current implementation of resetRecorder doesn't handle exceptions that might occur during release of MediaRecorder resources.
private void resetRecorder() { if (recorder != null) { - recorder.release(); - recorder = null; + try { + recorder.release(); + } catch (Exception e) { + // Log error but continue with cleanup + } finally { + recorder = null; + } } }
31-37: Consider adding a callback mechanism for error reportingThe service currently has no way to communicate errors back to the bound client (RecordingFragment). Consider adding callback interfaces or using LocalBroadcastManager to report errors.
public class AudioRecordingService extends Service { private MediaRecorder recorder; 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(); + + // Interface for communicating with bound clients + public interface RecordingCallback { + void onRecordingError(String errorMessage); + void onRecordingStateChanged(boolean isRecording); + } + + private RecordingCallback callback; + + public void setRecordingCallback(RecordingCallback callback) { + this.callback = callback; + }
73-92: Consider adding action buttons to the notificationFor better user experience, consider adding action buttons to the notification (e.g., stop, pause/resume) to allow users to control the recording without returning to the app.
private Notification createNotification(boolean recordingRunning) { Intent activityToLaunch = new Intent(this, DispatchActivity.class); activityToLaunch.setAction("android.intent.action.MAIN"); activityToLaunch.addCategory("android.intent.category.LAUNCHER"); int pendingIntentFlags = PendingIntent.FLAG_UPDATE_CURRENT; if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { pendingIntentFlags = pendingIntentFlags | PendingIntent.FLAG_IMMUTABLE; } PendingIntent pendingIntent = PendingIntent.getActivity(this, 0, activityToLaunch, pendingIntentFlags); + // Create action intents + Intent stopIntent = new Intent(this, AudioRecordingService.class); + stopIntent.setAction("STOP_RECORDING"); + PendingIntent stopPendingIntent = PendingIntent.getService(this, 1, stopIntent, pendingIntentFlags); + + NotificationCompat.Builder builder = return new NotificationCompat.Builder(this, CommCareNoficationManager.NOTIFICATION_CHANNEL_USER_SESSION_ID) .setContentTitle(Localization.get("recording.notification.title")) .setContentText(recordingRunning ? Localization.get("recording.notification.in.progress") : Localization.get("recording.notification.paused")) .setSmallIcon(R.drawable.commcare_actionbar_logo) .setContentIntent(pendingIntent) .setOngoing(true) + .addAction(R.drawable.ic_mic_off, Localization.get("recording.stop"), stopPendingIntent) .build(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
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/fragments/connect/ConnectDeliveryListFragment.java(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/fragment_connect_jobs_list.xml
- app/res/layout/item_login_connect_home_corrupt_apps.xml
- app/src/org/commcare/views/connect/CircleProgressBar.java
- app/res/layout/item_chat_right_view.xml
- app/src/org/commcare/views/ConnectPhoneInputView.java
🧰 Additional context used
🧠 Learnings (2)
app/res/layout/activity_connect_messaging.xml (2)
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.
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 ConstraintLayout, 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 for languages like Arabic, Hebrew, and Persian.
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.
🧬 Code Graph Analysis (1)
app/src/org/commcare/views/widgets/AudioRecordingService.java (2)
app/src/org/commcare/CommCareNoficationManager.java (1)
CommCareNoficationManager(34-213)app/src/org/commcare/views/widgets/RecordingFragment.java (1)
RecordingFragment(59-485)
🔇 Additional comments (72)
app/res/layout/fragment_connect_results_summary_list.xml (2)
31-35: Use weight-based width for responsive layout
Changinglayout_widthto0dpwithlayout_weight="1"correctly enables equal horizontal distribution of the CardView. This aligns with best practices for weight-based LinearLayouts.
85-89: Apply equal weight to all CardViews
The second CardView also useslayout_width="0dp"andlayout_weight="1", ensuring proportional space sharing. This maintains consistency and responsiveness.app/res/layout/fragment_connect_delivery_details.xml (6)
58-58: Exclude decorative icon from accessibility
Markingconnect_delivery_visits_iconasimportantForAccessibility="no"correctly prevents screen readers from announcing this decorative element.
82-82: Skip days icon in accessibility traversal
AddingimportantForAccessibility="no"toconnect_delivery_days_iconaligns with the intent to treat it as a non-essential decorative graphic.
105-105: Remove max-daily icon from accessibility tree
TheimportantForAccessibility="no"flag onconnect_delivery_max_daily_iconensures that assistive tech ignores this decorative element.
128-128: Omit budget icon from accessibility focus
ApplyingimportantForAccessibility="no"toconnect_delivery_budget_iconmaintains consistency across all decorative icons.
170-170: Use RTL-aware margin attribute
Replacing a hardcoded end margin withandroid:layout_marginEnd="2dp"improves layout behavior in right-to-left locales.
206-206: Use drawableEnd for RTL support
Switching fromdrawableRighttodrawableEndensures the button icon placement adapts correctly in RTL layouts.app/res/layout/item_channel.xml (3)
37-38: Exclude decorative channel icon from accessibility
AddingcontentDescription="@null"andimportantForAccessibility="no"toimgChannelprevents screen readers from announcing this decorative element.
44-53: Enable flexible width for channel name
ChangingtvChannelNametolayout_width="0dp"with start/end constraints and addinglayout_marginEndensures it expands or contracts correctly between the icon and timestamp.
59-69: Constrain description under the channel name
AdjustingtvChannelDescriptiontolayout_width="0dp", updating its margin and constraints ensures proper alignment and spacing in both LTR and RTL configurations.app/instrumentation-tests/src/org/commcare/androidTests/MenuTests.kt (1)
40-40: Correct string spelling for Wi-Fi
Updating"Wi-Fi Direct"ensures consistency with the localized UI strings and proper hyphenation.app/res/navigation/nav_graph_connect.xml (1)
62-62: Fix typo in fragment label
Correcting theandroid:labelto"fragment_connect_learning_progress"resolves the typo and maintains consistency in navigation labels.app/res/layout/fragment_connect_downloading.xml (1)
23-24: Good accessibility improvement!Adding
contentDescription="@null"andimportantForAccessibility="no"to this decorative image properly excludes it from screen reader announcements, improving the accessibility experience.app/unit-tests/src/org/commcare/android/tests/processing/FormStorageTest.java (1)
393-395: Added new serializable XPath function classes to registryThe two new polygon-related XPath function classes are properly registered in the serialization history list for version 2.57, ensuring backward compatibility for restored forms.
app/res/layout/fragment_channel_consent_bottom_sheet.xml (1)
27-27: Good accessibility improvement!Adding
importantForAccessibility="no"to this decorative image properly excludes it from screen reader announcements, improving the accessibility experience.app/res/layout/fragment_connect_delivery_progress.xml (1)
79-79: Improved button text contrastExplicitly setting the text color to blue for the "Yes" and "No" buttons provides better visual consistency and improves the user interface.
Also applies to: 90-90
app/instrumentation-tests/src/org/commcare/androidTests/DemoUserTest.kt (1)
24-25: Updated view ID references for UI testsThe test has been properly updated to use the new
nsv_home_screenID for the NestedScrollView instead of the previoushome_gridview_buttons, ensuring tests work correctly with the updated layout.Also applies to: 29-30, 57-59, 62-64
app/res/layout/dialog_payment_confirmation.xml (2)
12-12: Good UI improvementChanging the layout height from
match_parenttowrap_contentensures the dialog sizes dynamically based on content rather than filling the entire screen, providing a better user experience.
20-21: Good accessibility enhancementAdding
android:contentDescription="@null"andandroid:importantForAccessibility="no"properly marks this image as decorative, preventing screen readers from attempting to announce it unnecessarily.app/res/values-ti/strings.xml (1)
30-30: Fixed string resource typoRemoved the extra double quote at the end of the string, which could have caused display or translation issues.
app/instrumentation-tests/src/org/commcare/androidTests/LogSubmissionTest.kt (1)
64-64: Improved test reliabilityAdding an explicit keyboard dismissal step right after clicking the "Report Problem" button ensures the soft keyboard is closed before attempting to enter text, which prevents potential UI interaction issues during automated testing.
app/instrumentation-tests/src/org/commcare/androidTests/LoginTest.kt (1)
74-74: Updated view reference to match UI changesChanged the swipe target from
home_gridview_buttonstonsv_home_screento align with updates in the app's layout structure. This ensures the test interacts with the correct scrollable container.app/res/layout/connect_delivery_item.xml (2)
19-24: Decorative ImageView properly hidden from accessibility.Adding
android:importantForAccessibility="no"to@id/imgDeliveryStatuscorrectly prevents screen readers from announcing a non-informative icon. This aligns with accessibility best practices for decorative images.
65-71: Fix missing constraints for status TextView.Adding
layout_constraintStart_toStartOf="parent"andlayout_constraintTop_toTopOf="parent"ensures thedelivery_item_statusTextView is fully constrained. Good use of RTL-aware attributes for positioning when the view becomes visible.app/src/org/commcare/fragments/connect/ConnectDeliveryListFragment.java (2)
78-84: Align “All” filter CardView ID with layout rename.Updating
view.findViewById(R.id.cvFilterAll)replaces the oldllFilterAllreference to match the renamed CardView in XML. This change correctly synchronizes the code with the layout resource.
110-116: Use new CardView ID in filter switch.Changing the default return from
R.id.llFilterAlltoR.id.cvFilterAllingetFilterId()properly matches the renamed resource and avoids stale ID references.app/res/layout/view_progress_job_card.xml (2)
7-7: Adjust CardView height for dynamic content.Switching the root
CardViewheight frommatch_parenttowrap_contentallows the card to size itself to its content. Ensure this layout is tested in its parent container to confirm the expected height behavior.
73-77: Refine time TextView alignment.Adding
android:layout_marginStart="5dp",android:gravity="end", and constrainingtv_job_timetotv_view_moreimproves spacing and text alignment. This correctly positions the time label in relation to the “View More” TextView.app/res/layout/activity_connect_messaging.xml (1)
17-18: Replace deprecated left/right with start/end for RTL support.Switching to
app:layout_constraintStart_toStartOfandapp:layout_constraintEnd_toEndOfremoves deprecated attributes and enhances RTL layout compatibility. This matches project-wide standards.app/res/layout/fragment_connect_message.xml (2)
44-44: Localize EditText hint with string resource.Replacing the hardcoded hint with
@string/connect_send_message_hintenables proper localization of the messaging UI.
67-67: Improve accessibility of send button.Adding
android:contentDescription="@string/connect_send_message_btn_desc"to@id/imgSendMessageensures screen readers announce the send action. This follows accessibility best practices.app/res/layout/connect_delivery_progress_item.xml (3)
46-49: Use weight-based sizing correctlyChanging
layout_widthto0dpwithlayout_weight="1"ensures the two child LinearLayouts share the available space proportionally. This aligns with best practices forLinearLayout. Keep an eye on totalweightSumand ensure these changes render as expected on all screen sizes.
102-105: Consistent proportional sizing for second sectionApplying
layout_width="0dp"andlayout_weight="1"to the second horizontal LinearLayout mirrors the first section’s sizing behavior, providing balanced layout columns. This is correct and improves responsiveness.
122-124: Fix nested layout height to wrap_contentSwitching the nested vertical
LinearLayoutheight towrap_contentprevents unwanted stretching to the parent’s full height, ensuring the child views only occupy the space they need. This aligns with the intended design.app/res/values-sw/strings.xml (3)
42-42: Remove extraneous quote inback_to_managerThe unintended double quote was removed, correcting the Swahili translation. This fix ensures the string resource compiles and displays correctly.
102-102: Correctclearanswer_confirmplaceholder quotingUpdating to properly enclose the
%splaceholder within quotes fixes the display of dynamic values and removes escape artifacts.
126-126: Fixenter_data_descriptionstring formattingEnsuring the
%splaceholder is correctly wrapped in quotes and any extraneous characters are removed improves clarity and translation accuracy.app/res/values-pt/strings.xml (1)
42-42: Remove extraneous quote inback_to_managerThe stray double quote at the end of the Portuguese translation has been removed, ensuring proper compilation and display.
app/res/layout/view_job_card.xml (1)
42-42: Usewrap_contentfortv_view_morewidthChanging
layout_widthtowrap_contentfortv_view_moreensures it only occupies the space needed by its text, improving layout flexibility.app/res/values-fr/strings.xml (2)
203-203: Refineconnect_learn_finishedphrasingThe updated translation reads clearly and accurately conveys the intended message. No issues found.
205-205: Refineconnect_learn_failedphrasingThe updated failure message is grammatically correct and provides clear instructions to retry. Approved.
app/res/layout/fragment_connect_delivery_list.xml (3)
31-41: Improved accessibility for CardView element.The CardView has been properly updated with accessibility attributes and a more semantically accurate ID that matches its type (cv prefix for CardView). The element is now correctly set as clickable and focusable, which improves touch interaction and accessibility.
51-51: Fixed incorrect unit for padding dimension.The padding unit has been corrected from
16spto16dp, which is the appropriate unit for layout dimensions. Thespunit should only be used for text sizes since it scales with user font preferences.
77-77: Updated constraint reference to match new ID.The constraint reference has been correctly updated to use the new
cvFilterAllID, ensuring layout integrity is maintained.app/src/org/commcare/views/connect/LinearProgressBar.java (4)
6-9: Added required imports for gradient implementation.The imports for LinearGradient and Shader are correctly added to support gradient functionality.
29-29: Updated backgroundColor declaration to properly use int type.Changed backgroundColor field to use the appropriate int type for Android color values.
48-49: Improved background color initialization using ContextCompat.The code now correctly uses ContextCompat.getColor() to retrieve the color resource, which is the recommended way to access color resources in a backward-compatible way.
73-82: Added gradient support for progress bar rendering.The implementation correctly applies a linear gradient shader when enabled and falls back to solid color otherwise. The code includes proper null/length checks and maintains the original behavior when gradients aren't needed.
app/AndroidManifest.xml (3)
39-39: Added required permission for microphone usage in foreground service.The FOREGROUND_SERVICE_MICROPHONE permission is correctly added, which is required for Android 12+ (API 33+) to use the microphone in a foreground service.
339-345: Properly declared new AudioRecordingService with appropriate attributes.The service declaration includes all necessary attributes:
- enabled="true" - Service can be instantiated by the system
- exported="false" - Service is not available to other apps (security best practice)
- stopWithTask="true" - Service stops when the app is removed from recent tasks
- foregroundServiceType="microphone" - Service specifically uses microphone in foreground
These settings align with best practices for foreground services in Android.
339-345:Details
✅ Verification successful
Verify implementation of AudioRecordingService.
The manifest declares a new service but doesn't provide information about its implementation. Ensure the service properly implements foreground service requirements, including displaying a persistent notification.
🏁 Script executed:
#!/bin/bash # Check for AudioRecordingService implementation and notification creation echo "Checking AudioRecordingService implementation..." rg -A 5 -B 5 "class AudioRecordingService" --type java echo "Checking for notification creation in AudioRecordingService..." rg "NotificationCompat.Builder" --type java | grep -i "audio"Length of output: 1787
🏁 Script executed:
#!/bin/bash echo "Searching for startForeground usage in AudioRecordingService..." rg -n "startForeground" app/src/org/commcare/views/widgets/AudioRecordingService.java echo "Searching for onStartCommand implementation in AudioRecordingService..." rg -n -A3 "onStartCommand" app/src/org/commcare/views/widgets/AudioRecordingService.javaLength of output: 939
AudioRecordingService correctly implements foreground service requirements
The service invokes
startForeground(…)inonStartCommand()using a notification built viaNotificationCompat.Builder(see lines 43–46) and supplies it with the required notification channel (CommCareNotificationManager.NOTIFICATION_CHANNEL_USER_SESSION_ID). No further changes are needed here.app/res/values/strings.xml (3)
53-53: Fixed formatting in back_to_manager string.Removed extraneous quotation marks that were incorrectly included in the string value.
551-552: Added strings to support messaging UI with proper accessibility.New strings for message input hint and send button description will improve user experience and accessibility in the messaging UI.
551-552:Details
✅ Verification successful
Verify usage of new messaging strings in layouts.
The new strings are likely intended for use in a messaging interface, but their usage location is not shown in the current PR.
🏁 Script executed:
#!/bin/bash # Search for usage of the new messaging strings in layout files echo "Searching for usage of connect_send_message_hint..." rg "connect_send_message_hint" --type xml echo "Searching for usage of connect_send_message_btn_desc..." rg "connect_send_message_btn_desc" --type xmlLength of output: 755
New messaging strings properly implemented in layouts
Verified that both
connect_send_message_hintandconnect_send_message_btn_descare referenced inapp/res/layout/fragment_connect_message.xml:
- android:hint="@string/connect_send_message_hint"
- android:contentDescription="@string/connect_send_message_btn_desc"
No further changes needed.
app/res/layout/home_screen.xml (4)
84-85: Good addition of ID to the NestedScrollViewAdding the ID
nsv_home_screento the NestedScrollView is a good practice for easier reference in code and for instrumentation tests. This change aligns with updates in the test files to target this view for swipe gestures instead of thehome_gridview_buttons.
99-102: Improved layout attributes for RTL supportThe changes from
android:layout_marginLeft/Righttoandroid:layout_marginStart/Endimprove RTL language support. Also, replacing negative top margin withtranslationYis a better practice for positioning elements.
134-134: Default visibility set to goneSetting the JobCard to be hidden by default is appropriate based on the PR context. Just ensure the visibility is properly controlled in the relevant Java/Kotlin code.
144-145: Fixed RecyclerView heightChanging the RecyclerView height from
match_parenttowrap_contentis appropriate for a scrollable container. This prevents layout issues where the RecyclerView might extend beyond its parent's bounds.app/res/layout-land/home_screen.xml (3)
5-6: Improved root layout widthChanging the LinearLayout width from
wrap_contenttomatch_parentensures the layout fills the available screen width in landscape orientation, providing a more consistent user experience.
84-85: Added consistent ID for instrumentation testsAdding the ID
nsv_home_screenensures consistency with the portrait layout and supports the updated instrumentation tests that now reference this view for scroll actions.
99-99: Consistent visibility settings across orientationsSetting the same default visibility (
gone) for the connect message and job card views in both portrait and landscape layouts ensures consistent behavior across orientations.Also applies to: 134-134
app/assets/locales/android_translatable_strings.txt (2)
45-45: Improved text conventions and standardizationThe changes standardize terminology with proper capitalization and hyphenation (e.g., "Android Market" instead of "android market", "Wi-Fi" instead of "Wifi"). This improves text consistency throughout the application.
Also applies to: 83-84, 123-123, 413-413, 792-794
472-474: New strings to support audio recording functionalityThese new notification strings support the newly added audio recording service functionality. The strings provide appropriate user feedback for recording states (in progress and paused).
app/src/org/commcare/views/connect/ConnectEditText.java (3)
95-95: Improved initialization methodThe changes to the initialization method provide better organization by centralizing hint attribute setting and ensuring explicit text size initialization in the fallback case.
Also applies to: 121-121
135-136: Enhanced hint text size handlingThe improved
setHintTextSizemethod now forces a hint rebuild by callingsetHint(getHint())before setting the text size, ensuring the changes are properly applied.
148-148: Consistent method usageUsing
setHintTextSizeinstead of directly callingsetTextSizeprovides better encapsulation and ensures that the specialized hint size handling is consistently applied.app/instrumentation-tests/src/org/commcare/utils/InstrumentationUtility.kt (1)
318-320: Good simplification ofenterText()Using
replaceTextautomatically clears previous content, and chainingcloseSoftKeyboard()eliminates the extra explicit call in the test body.
No further action required.app/src/org/commcare/views/widgets/RecordingFragment.java (1)
176-198: 🛠️ Refactor suggestionAvoid double-starting the service
startForegroundService()(orstartService()on < O) is immediately followed bybindService().
SincebindService(..., BIND_AUTO_CREATE)itself starts the service if not running, the explicitstartForegroundService()is usually redundant and risks twoonStartCommand()invocations, complicating state handling insideAudioRecordingService.- if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { - requireActivity().startForegroundService(serviceIntent); - } else { - requireActivity().startService(serviceIntent); - } - requireActivity().bindService(serviceIntent, getAudioRecordingServiceConnection(), - Context.BIND_AUTO_CREATE); + // Binding with BIND_AUTO_CREATE guarantees the service is started if needed + requireActivity().bindService( + serviceIntent, + getAudioRecordingServiceConnection(), + Context.BIND_AUTO_CREATE + );The service itself can promote to foreground in its
onCreate()/onStartCommand().Likely an incorrect or invalid review comment.
app/src/org/commcare/views/widgets/AudioRecordingService.java (3)
1-31: Well-structured service class with good documentationThe class is well-documented and clearly explains its purpose as a foreground service for audio recording, which will help maintainability. The import statements are appropriately organized.
122-127: 🛠️ Refactor suggestionAdd error handling for recorder operations
MediaRecorder operations can throw runtime exceptions in various scenarios (e.g., if the file can't be created, if the device runs out of storage, or if called in an invalid state). Consider adding try-catch blocks around these operations.
@RequiresApi(api = Build.VERSION_CODES.N) public void pauseRecording() { - recorder.pause(); - notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID, - createNotification(false)); + try { + if (recorder != null) { + recorder.pause(); + notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID, + createNotification(false)); + } + } catch (IllegalStateException e) { + // Handle case where recorder is in invalid state + } } @RequiresApi(api = Build.VERSION_CODES.N) public void resumeRecording() { - recorder.resume(); - notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID, - createNotification(true)); + try { + if (recorder != null) { + recorder.resume(); + notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID, + createNotification(true)); + } + } catch (IllegalStateException e) { + // Handle case where recorder is in invalid state + } } public void stopRecording() { - recorder.stop(); + try { + if (recorder != null) { + recorder.stop(); + } + } catch (IllegalStateException e) { + // Handle case where recorder is in invalid state + } }Also applies to: 129-134, 136-139
⛔ Skipped due to learnings
Learnt from: avazirna PR: dimagi/commcare-android#3057 File: app/src/org/commcare/views/widgets/AudioRecordingService.java:105-138 Timestamp: 2025-05-01T10:50:29.135Z Learning: In the CommCare Android app, IllegalStateException from MediaRecorder operations should not be caught to ensure visibility of recording setup issues through app crashes.
1-139:Details
❓ Verification inconclusive
Verify if onStartCommand implementation matches expected behavior
The service returns START_NOT_STICKY, which means it won't be recreated if killed by the system. Is this the expected behavior for audio recording? If recordings should persist through system-initiated service kills, consider using START_REDELIVER_INTENT instead.
🏁 Script executed:
#!/bin/bash # Check how similar services are implemented in the codebase # Look for other Service implementations and their return values from onStartCommand rg "extends Service" -A 25 | grep -A 5 -B 5 "onStartCommand" # Check for any existing documentation on service restart behavior rg -i "service restart" --glob "*.md"Length of output: 1207
Confirm service restart policy for audio recordings
I didn’t find any other
Serviceimplementations or documentation prescribing a restart policy in the repo. By returningSTART_NOT_STICKY, theAudioRecordingServicewon’t be recreated if the system kills it, which could interrupt in-progress recordings. If you need the recording to resume or the intent to be redelivered after a kill, consider returningSTART_REDELIVER_INTENTinstead.Suggested update in
onStartCommand:@@ public int onStartCommand(Intent intent, int flags, int startId) { - return START_NOT_STICKY; + // Restart the service with the original intent if it’s killed + return START_REDELIVER_INTENT;Please verify this aligns with your requirements and expected user experience.
| <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.
Fix save_enter_data_description quote escaping
The escaped quotes ("\%s\".) are invalid. It should correctly escape or remove them:
<string name="save_enter_data_description">Você está no fim de "%s".</string>🤖 Prompt for AI Agents (early access)
In app/res/values-pt/strings.xml at line 191, the string
save_enter_data_description incorrectly escapes quotes around %s, causing
invalid XML. Fix this by removing the backslashes before the quotes so the
string reads: Você está no fim de "%s".
| onView(withId(R.id.nsv_home_screen)) | ||
| .perform(swipeUp()) | ||
| onView(withId(R.id.nsv_home_screen)) // multiple swipe | ||
| .perform(swipeUp()) // to make `logout` button visible as it may have large scroll | ||
| onView(withId(R.id.nsv_home_screen)) | ||
| .perform(swipeUp()) | ||
| onView(withId(R.id.nsv_home_screen)) | ||
| .perform(swipeUp()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hard-coded multiple swipes with repeatedlyUntil()
Four consecutive swipeUp() calls introduce flakiness (fixed swipe distance vs device size/content height) and require maintenance if the layout changes again.
You already imported repeatedlyUntil; use it to scroll until the “Log out of CommCare” view is visible, then stop.
- onView(withId(R.id.nsv_home_screen))
- .perform(swipeUp())
- onView(withId(R.id.nsv_home_screen)) // multiple swipe
- .perform(swipeUp()) // to make `logout` button visible as it may have large scroll
- onView(withId(R.id.nsv_home_screen))
- .perform(swipeUp())
- onView(withId(R.id.nsv_home_screen))
- .perform(swipeUp())
+ onView(withId(R.id.nsv_home_screen))
+ .perform(
+ repeatedlyUntil(
+ swipeUp(),
+ hasDescendant(withText("Log out of CommCare")),
+ /* maxAttempts = */ 8
+ )
+ )This both reduces test time and shields the test from UI‐layout changes.
📝 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.
| onView(withId(R.id.nsv_home_screen)) | |
| .perform(swipeUp()) | |
| onView(withId(R.id.nsv_home_screen)) // multiple swipe | |
| .perform(swipeUp()) // to make `logout` button visible as it may have large scroll | |
| onView(withId(R.id.nsv_home_screen)) | |
| .perform(swipeUp()) | |
| onView(withId(R.id.nsv_home_screen)) | |
| .perform(swipeUp()) | |
| onView(withId(R.id.nsv_home_screen)) | |
| .perform( | |
| repeatedlyUntil( | |
| swipeUp(), | |
| hasDescendant(withText("Log out of CommCare")), | |
| /* maxAttempts = */ 8 | |
| ) | |
| ) |
🤖 Prompt for AI Agents (early access)
In app/instrumentation-tests/src/org/commcare/utils/InstrumentationUtility.kt
around lines 215 to 222, replace the four consecutive swipeUp() calls on the
same view with a single call to repeatedlyUntil() that performs swipeUp() until
the "Log out of CommCare" view is visible. This will reduce flakiness by
adapting to different screen sizes and content heights, improve test stability,
and reduce maintenance when the UI layout changes.
| /** | ||
| * Progressively scales down a bitmap to avoid moiré patterns by using a step-wise approach. | ||
| * This method first performs progressive halving until the dimensions are close to the target, | ||
| * then completes the scaling with a final resize to exactly match the target dimensions. | ||
| * The step-wise approach reduces aliasing artifacts that would occur with direct scaling, | ||
| * particularly important for images with fine patterns or textures. | ||
| * | ||
| * @param originalBitmap The source bitmap to downscale | ||
| * @param targetWidth The desired width of the resulting bitmap | ||
| * @param targetHeight The desired height of the resulting bitmap | ||
| * @return A downscaled bitmap that matches the target dimensions | ||
| */ | ||
| private static Bitmap stepDownscale(Bitmap originalBitmap, int targetWidth, int targetHeight) { | ||
| Bitmap currentBitmap = originalBitmap; | ||
| int height = originalBitmap.getHeight(); | ||
| int width = originalBitmap.getWidth(); | ||
|
|
||
| // First do progressive halving until we get close | ||
| while (height > targetHeight * 2 && width > targetWidth * 2) { | ||
| height /= 2; | ||
| width /= 2; | ||
|
|
||
| Bitmap tempBitmap = Bitmap.createScaledBitmap(currentBitmap, width, height, true); | ||
|
|
||
| if (currentBitmap != originalBitmap && !currentBitmap.isRecycled()) { | ||
| currentBitmap.recycle(); | ||
| } | ||
|
|
||
| currentBitmap = tempBitmap; | ||
| } | ||
|
|
||
| // Final step to exactly match target dimensions | ||
| if (width != targetWidth || height != targetHeight) { | ||
| Bitmap finalBitmap = Bitmap.createScaledBitmap(currentBitmap, targetWidth, targetHeight, false); | ||
|
|
||
| if (currentBitmap != originalBitmap && !currentBitmap.isRecycled()) { | ||
| currentBitmap.recycle(); | ||
| } | ||
|
|
||
| return finalBitmap; | ||
| } | ||
|
|
||
| return currentBitmap; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Recycle intermediate & original bitmaps to prevent memory leaks
stepDownscale() progressively creates new bitmaps but only recycles the current bitmap when it is not the original. The very first originalBitmap reference is never recycled, leaving native memory pressure on low-RAM devices.
@@
- Bitmap currentBitmap = originalBitmap;
+ Bitmap currentBitmap = originalBitmap;
...
- if (currentBitmap != originalBitmap && !currentBitmap.isRecycled()) {
+ if (currentBitmap != originalBitmap && !currentBitmap.isRecycled()) {
currentBitmap.recycle();
}
...
- if (width != targetWidth || height != targetHeight) {
+ if (width != targetWidth || height != targetHeight) {
Bitmap finalBitmap = Bitmap.createScaledBitmap(currentBitmap, targetWidth, targetHeight, /* filter= */ true);
@@
- return finalBitmap;
+ // Recycle the original bitmap as well once we’ve got the final copy
+ if (originalBitmap != currentBitmap && !originalBitmap.isRecycled()) {
+ originalBitmap.recycle();
+ }
+ return finalBitmap;
}
- return currentBitmap;
+ return currentBitmap; // currentBitmap equals originalBitmap hereAlso consider setting filter=true for the final resize; the quality penalty from a non-filtered final step outweighs the negligible performance gain.
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/utils/FileUtil.java around lines 703 to 746, the
stepDownscale method does not recycle the originalBitmap, causing potential
memory leaks. Modify the code to recycle the originalBitmap after the final
scaled bitmap is created if it is no longer needed. Also, change the final
Bitmap.createScaledBitmap call to use filter=true to improve image quality
during the last resize step.
| try { | ||
| recorder.prepare(); | ||
| Logger.log(LogTypes.TYPE_MEDIA_EVENT, "Preparing recording: " + fileName | ||
| + " | " + (isHeAacSupported ? HEAAC_SAMPLE_RATE : AMRNB_SAMPLE_RATE) | ||
| + " | " + (isHeAacSupported ? MediaRecorder.AudioEncoder.HE_AAC : | ||
| MediaRecorder.AudioEncoder.AMR_NB)); | ||
|
|
||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
Propagate recorder-prep failure instead of returning a half-initialised object
If recorder.prepare() throws, the method logs the stack trace but still returns the recorder. Subsequent start() will fail with an IllegalStateException, but the caller has no clue why.
- } catch (IOException e) {
- e.printStackTrace();
+ } catch (IOException e) {
+ recorder.release();
+ Logger.exception("Failed to prepare MediaRecorder", e);
+ return null; // caller can handle a null and notify the user
}Alternatively, re-throw as a runtime exception so failures surface immediately.
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingHelper.java around lines 41
to 50, the catch block for IOException during recorder.prepare() only logs the
stack trace but still returns the recorder, leading to potential
IllegalStateException later without clear cause. Modify the code to re-throw the
caught IOException as a runtime exception or propagate it so the failure is
immediately visible to the caller instead of returning a half-initialized
recorder.
| if (inPausedState) { | ||
| recorder.resume(); | ||
| audioRecordingService.resumeRecording(); | ||
| } | ||
|
|
||
| recorder.stop(); | ||
| audioRecordingService.stopRecording(); | ||
| toggleRecording.setBackgroundResource(R.drawable.play); |
There was a problem hiding this comment.
Null-safety before interacting with the service
audioRecordingService may be null if the binder died or the connection was never fully established.
Guard the calls to prevent a crash:
- if (inPausedState) {
- audioRecordingService.resumeRecording();
- }
- audioRecordingService.stopRecording();
+ if (audioRecordingService != null) {
+ if (inPausedState) {
+ audioRecordingService.resumeRecording();
+ }
+ audioRecordingService.stopRecording();
+ } else {
+ Logger.log(LogTypes.TYPE_MAINTENANCE, "RecordingFragment: service null on stopRecording()");
+ }📝 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.
| if (inPausedState) { | |
| recorder.resume(); | |
| audioRecordingService.resumeRecording(); | |
| } | |
| recorder.stop(); | |
| audioRecordingService.stopRecording(); | |
| toggleRecording.setBackgroundResource(R.drawable.play); | |
| if (audioRecordingService != null) { | |
| if (inPausedState) { | |
| audioRecordingService.resumeRecording(); | |
| } | |
| audioRecordingService.stopRecording(); | |
| } else { | |
| Logger.log(LogTypes.TYPE_MAINTENANCE, "RecordingFragment: service null on stopRecording()"); | |
| } | |
| toggleRecording.setBackgroundResource(R.drawable.play); |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/RecordingFragment.java around lines 252 to
257, the code calls methods on audioRecordingService without checking if it is
null, which can cause a crash if the service is not connected. Add null checks
before calling audioRecordingService.resumeRecording() and
audioRecordingService.stopRecording() to ensure the service is not null before
interacting with it.
| @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.
🛠️ Refactor suggestion
Add runtime checks for API version
Even though methods are annotated with @RequiresApi, the service should perform runtime checks before executing API-specific code to prevent crashes if called on unsupported devices.
public void pauseRecording() {
+ if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N) {
+ // Handle gracefully for older versions
+ return;
+ }
try {
if (recorder != null) {
recorder.pause();
notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID,
createNotification(false));
}
} catch (IllegalStateException e) {
// Handle case where recorder is in invalid state
}
}
public void resumeRecording() {
+ if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N) {
+ // Handle gracefully for older versions
+ return;
+ }
try {
if (recorder != null) {
recorder.resume();
notificationManager.notify(RecordingFragment.RECORDING_NOTIFICATION_ID,
createNotification(true));
}
} catch (IllegalStateException e) {
// Handle case where recorder is in invalid state
}
}Also applies to: 129-134
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java around lines
122 to 127 and 129 to 134, the pauseRecording method and similar API-specific
code lack runtime checks for the Android API version. To fix this, add explicit
runtime checks using Build.VERSION.SDK_INT to verify the device's API level
before calling methods like recorder.pause(). This ensures the code only
executes on supported devices, preventing crashes on lower API levels.
| if (recorder == null) { | ||
| recorder = audioRecordingHelper.setupRecorder(fileName); | ||
| } | ||
| recorder.start(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling when starting the recorder
The call to recorder.start() can throw an IllegalStateException if the recorder is in an invalid state.
- recorder.start();
+ try {
+ recorder.start();
+ } catch (IllegalStateException e) {
+ resetRecorder();
+ stopSelf();
+ }🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java at line 56, the
call to recorder.start() can throw an IllegalStateException if the recorder is
in an invalid state. Wrap the recorder.start() call in a try-catch block to
catch IllegalStateException and handle it appropriately, such as logging the
error and safely stopping or resetting the recorder to prevent the app from
crashing.
| @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.
🛠️ Refactor suggestion
Add null checks for intent extras to prevent crashes
The code assumes the intent and extras will always be non-null, which could lead to NullPointerException if the service is started incorrectly.
@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;
}📝 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.
| @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; | |
| } | |
| @Override | |
| public int onStartCommand(Intent intent, int flags, int startId) { | |
| 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, the code assumes intent and its extras are non-null, risking
NullPointerException. Add null checks to verify that intent and
intent.getExtras() are not null before accessing the recording filename. If
either is null, handle the case gracefully, such as by logging an error and
stopping the service or returning early to prevent crashes.
Product Description
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-1087 is already merged in master, now rebasing master in dv/connect_initial
Labels and Review