Backmerge Commcare 2.55 #2940
Backmerge Commcare 2.55 #2940
Conversation
…n-visibility Change progress dialog cancel button visibility state
Migrate to ViewPager2
This reverts commit e92529c.
Update component exported attribute
Minor UI updates
Minor UI updates 2
Minor UI update
📝 WalkthroughWalkthroughThis pull request encompasses a comprehensive set of changes across multiple files in the CommCare Android application, focusing on UI/UX improvements, modernization of components, and security enhancements. The modifications span several key areas including Android SDK version updates, layout restructuring, permission management, and component upgrades. Notable changes include transitioning from Sequence DiagramsequenceDiagram
participant User
participant HomeActivity
participant EntityDetailActivity
participant TabbedDetailView
participant ViewPager2
participant TabLayout
User->>HomeActivity: Open App
HomeActivity->>EntityDetailActivity: Navigate to Details
EntityDetailActivity->>TabbedDetailView: Initialize View
TabbedDetailView->>ViewPager2: Create Adapter
TabbedDetailView->>TabLayout: Configure Tabs
TabLayout-->>ViewPager2: Link Tabs
ViewPager2->>EntityDetailActivity: Display Details
Possibly related PRs
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 3
🧹 Nitpick comments (2)
app/res/values/strings.xml (1)
156-160: LGTM! Well-structured permission strings.The new permission-related strings are well-organized and follow the established pattern for permission resources. The descriptions are clear and effectively communicate the security implications to users.
Consider documenting these permission changes in the developer documentation to ensure proper integration by other apps that may need legitimate access to internal components.
app/src/org/commcare/views/dialogs/StandardAlertDialog.java (1)
101-101: Consider using SP units for better accessibility.Using COMPLEX_UNIT_PX means the text won't scale with system font size settings, which could impact accessibility. Consider using COMPLEX_UNIT_SP instead to respect user's font size preferences.
- negativeButton.setTextSize(TypedValue.COMPLEX_UNIT_PX,this.getDialog().getContext().getResources().getDimensionPixelSize(R.dimen.font_size_medium)); + negativeButton.setTextSize(TypedValue.COMPLEX_UNIT_SP, view.getContext().getResources().getDimension(R.dimen.font_size_medium));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
app/res/drawable-hdpi/icon_chevron_left_attnpos.pngis excluded by!**/*.pngapp/res/drawable-hdpi/icon_chevron_right_attnpos.pngis excluded by!**/*.pngapp/res/drawable-ldpi/icon_chevron_left_attnpos.pngis excluded by!**/*.pngapp/res/drawable-ldpi/icon_chevron_right_attnpos.pngis excluded by!**/*.pngapp/res/drawable-mdpi/icon_chevron_left_attnpos.pngis excluded by!**/*.pngapp/res/drawable-mdpi/icon_chevron_right_attnpos.pngis excluded by!**/*.pngapp/res/drawable-xhdpi/icon_chevron_left_attnpos.pngis excluded by!**/*.pngapp/res/drawable-xhdpi/icon_chevron_right_attnpos.pngis excluded by!**/*.pngapp/res/drawable/icon_chevron_left_attnpos.pngis excluded by!**/*.pngapp/res/drawable/icon_chevron_right_attnpos.pngis excluded by!**/*.png
📒 Files selected for processing (26)
README.md(1 hunks)app/AndroidManifest.xml(7 hunks)app/build.gradle(0 hunks)app/res/drawable/commcare_actionbar_logo.xml(1 hunks)app/res/drawable/rounded_button_shape.xml(1 hunks)app/res/drawable/title_case_tab_vertical.xml(0 hunks)app/res/drawable/title_neutral_tab_vertical.xml(0 hunks)app/res/layout/app_manager.xml(0 hunks)app/res/layout/custom_alert_dialog.xml(1 hunks)app/res/layout/progress_dialog_cancel_button.xml(1 hunks)app/res/layout/prompt_view.xml(1 hunks)app/res/layout/screen_form_entry.xml(1 hunks)app/res/layout/tabbed_detail_view.xml(1 hunks)app/res/values/attrs.xml(1 hunks)app/res/values/dimens.xml(1 hunks)app/res/values/strings.xml(1 hunks)app/res/values/styles.xml(3 hunks)app/src/org/commcare/activities/EntityDetailActivity.java(0 hunks)app/src/org/commcare/activities/EntitySelectActivity.java(0 hunks)app/src/org/commcare/activities/HomeButtons.java(1 hunks)app/src/org/commcare/adapters/EntityDetailPagerAdapter.java(4 hunks)app/src/org/commcare/fragments/BreadcrumbBarFragment.java(0 hunks)app/src/org/commcare/recovery/measures/ExecuteRecoveryMeasuresPresenter.java(1 hunks)app/src/org/commcare/views/ClippingFrame.java(5 hunks)app/src/org/commcare/views/TabbedDetailView.java(4 hunks)app/src/org/commcare/views/dialogs/StandardAlertDialog.java(2 hunks)
💤 Files with no reviewable changes (7)
- app/res/layout/app_manager.xml
- app/res/drawable/title_case_tab_vertical.xml
- app/src/org/commcare/activities/EntityDetailActivity.java
- app/src/org/commcare/activities/EntitySelectActivity.java
- app/src/org/commcare/fragments/BreadcrumbBarFragment.java
- app/res/drawable/title_neutral_tab_vertical.xml
- app/build.gradle
✅ Files skipped from review due to trivial changes (1)
- app/src/org/commcare/recovery/measures/ExecuteRecoveryMeasuresPresenter.java
🔇 Additional comments (24)
app/src/org/commcare/views/dialogs/StandardAlertDialog.java (1)
6-6: LGTM!The TypedValue import is correctly added and properly organized with other imports.
app/res/drawable/commcare_actionbar_logo.xml (1)
2-5: LGTM! Standardizing icon size to Material Design guidelines.The change from 20dp to 24dp aligns with Material Design icon size specifications.
README.md (1)
37-37: Verify compatibility with Android 14 SDK upgrade.The upgrade from Android 12 (API 31) to Android 14 (API 34) is significant. Please ensure:
- All runtime permissions are properly handled
- Background service limitations are addressed
- Foreground service types are correctly specified
✅ Verification successful
Android 14 SDK compatibility verified successfully
The codebase demonstrates proper implementation of Android 14 requirements:
- Foreground service type is correctly declared
- Runtime permissions are properly handled across activities
- Service lifecycle follows recommended patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential Android 14 compatibility issues # Check for foreground service declarations rg "android:foregroundServiceType" -A 2 # Check for permission handling patterns rg "requestPermissions|onRequestPermissionsResult" # Check for background service starts rg "startService|bindService" -A 2Length of output: 3163
app/AndroidManifest.xml (5)
5-5: LGTM! Version change aligns with backmerge purpose.The version change to 2.55.0 is consistent with the backmerge from CommCare 2.55.
74-77: LGTM! Well-structured permission group for internal actions.The new permission group properly encapsulates internal action permissions.
95-101: LGTM! Strong security measure for protecting exposed components.The signature-level permission provides proper protection for components that need to be exported.
362-363: LGTM! Comprehensive security enhancement for exposed components.The changes properly protect multiple components that need to be exported by requiring the signature-level permission. This ensures only the app itself can access these components.
Also applies to: 416-418, 426-428, 522-523
447-448: LGTM! Proper permission protection for logout receiver.The logout receiver is now properly protected with the COMMCARE_LOGOUT permission.
app/res/layout/progress_dialog_cancel_button.xml (2)
16-16: LGTM: Consistent text size standardization.The addition of a standardized text size using
@dimen/font_size_mediumaligns with the broader UI improvements across the application.
18-18: Verify the visibility change impact.Changing the button's visibility from
visibletogonecould affect existing functionality. Since this is a backmerge PR, please confirm:
- Was this visibility change intentional in CommCare 2.55?
- Are there any UI flows that depend on this cancel button being visible by default?
✅ Verification successful
Visibility change is consistent with dialog's default behavior
The visibility change to
gonealigns with CustomProgressDialog's design:
- Progress dialogs are non-cancelable by default
- Cancel button is included in both determinate and indeterminate dialogs
- Button visibility is controlled programmatically when cancellation is explicitly enabled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to this layout to understand usage context echo "Searching for layout references..." rg -l "progress_dialog_cancel_button" --type java --type kotlin # Search for potential programmatic visibility changes echo "Searching for programmatic visibility changes..." rg "progress_dialog_cancel_button.*visibility" --type java --type kotlinLength of output: 324
Script:
#!/bin/bash # Search for layout references with full path echo "Searching for layout references with full path..." rg -l "R.layout.progress_dialog_cancel_button" --type java --type kotlin # Search for layout references with include pattern echo "Searching for include references..." rg -l 'include.*progress_dialog_cancel_button' --type xml # Search for progress dialog related classes echo "Searching for progress dialog implementations..." rg -l "ProgressDialog" --type java --type kotlin # Search for layout inflation patterns echo "Searching for layout inflation..." rg "inflat.*progress_dialog" --type java --type kotlinLength of output: 2304
Script:
#!/bin/bash # Check CustomProgressDialog implementation echo "Examining CustomProgressDialog.java..." rg -A 10 "class CustomProgressDialog" app/src/org/commcare/views/dialogs/CustomProgressDialog.java # Check progress dialog layout files echo "Checking progress dialog layouts..." cat app/res/layout/progress_dialog_determinate.xml cat app/res/layout/progress_dialog_indeterminate.xml # Look for cancel-related functionality echo "Searching for cancel button handling..." rg "setCancelable|setOnCancelListener" app/src/org/commcare/views/dialogs/CustomProgressDialog.javaLength of output: 5645
app/src/org/commcare/views/TabbedDetailView.java (1)
84-84: Verify the lifecycle owner when creatingEntityDetailPagerAdapterWhen instantiating
EntityDetailPagerAdapter, ensure that theLifecycleprovided (mContext.getLifecycle()) is appropriate for fragment management within this adapter. Confirm thatmContext(anAppCompatActivity) correctly manages the lifecycle of the fragments to prevent potential memory leaks or unexpected behavior.app/src/org/commcare/adapters/EntityDetailPagerAdapter.java (2)
24-38: Successful migration toFragmentStateAdapterThe transition from
FragmentStatePagerAdaptertoFragmentStateAdapteris well-implemented. The constructor and methods are updated appropriately, and the use ofFragmentManagerandLifecyclealigns with theFragmentStateAdapterrequirements.
Line range hint
43-59: Ensure consistent fragment creation logic increateFragmentValidate that the logic within the
createFragmentmethod maintains the same behavior as the previousgetItemmethod. Confirm that the selection betweenEntitySubnodeDetailFragmentandEntityDetailFragmentis accurate and that all necessary arguments are properly set.app/src/org/commcare/views/ClippingFrame.java (1)
100-102: VerifyPathclipping functionality across API levelsThe change from using a
Rectto aPathwithaddRoundRectallows for rounded corners in clipping. Ensure thataddRoundRectandclipPathbehave consistently across all supported API levels, particularly on older devices where differences in rendering may occur.Consider testing on devices with different Android versions to confirm that the visuals render as expected without impacting performance.
Also applies to: 115-116
app/src/org/commcare/activities/HomeButtons.java (1)
83-83: LGTM: Color resource update aligns with UI modernizationThe change from
cc_neutral_texttocc_core_textfor the logout button's subtext appears to be part of the broader UI modernization effort.app/res/drawable/rounded_button_shape.xml (1)
2-9: LGTM: Enhanced button styling with elevation effectThe updated drawable implements a subtle elevation effect using a layer-list with 2dp offset, which aligns with Material Design principles while maintaining the existing corner radius functionality.
app/res/layout/tabbed_detail_view.xml (2)
13-27: LGTM: Well-configured Material TabLayout implementationThe TabLayout is properly configured with:
- Scrollable tabs with centered gravity
- Consistent padding and styling
- Proper indicator configuration
- Brand-consistent colors
33-37: LGTM: Upgraded to ViewPager2 with proper layout parametersThe migration from RtlViewPager to ViewPager2 is a good modernization step:
- Improved RTL support
- Better fragment state management
- Proper layout weight for filling available space
app/res/values/attrs.xml (1)
50-50: LGTM: Added flexible corner radius attributeThe new
clip_corner_radiusattribute is properly defined with support for both dimension and reference values, maintaining consistency with existing attribute patterns.app/res/layout/prompt_view.xml (1)
43-43: Verify accessibility impact of reduced text size.The text size reduction from 28sp to 14sp is significant and might affect readability for users with visual impairments.
Run the following script to check if this change aligns with accessibility guidelines and if similar text size reductions have been made across the app:
app/res/values/dimens.xml (1)
19-20: LGTM!The new tab dimensions align with Material Design guidelines for tab layouts.
app/res/layout/custom_alert_dialog.xml (1)
79-82: LGTM!The button spacing and alignment changes are consistent and well-structured, using standard dimension resources.
Also applies to: 86-87, 95-99, 101-102, 109-112, 115-118
app/res/layout/screen_form_entry.xml (1)
135-135: LGTM!The addition of rounded corners using a standard dimension resource enhances visual consistency.
app/res/values/styles.xml (1)
211-212: LGTM!The style changes improve visual hierarchy and consistency:
- Updated row colors enhance visual distinction
- Enhanced button styling with proper elevation and dimensions
- Added tab styling following Material Design guidelines
Also applies to: 314-318, 325-325, 349-351
| Button negativeButton = this.view.findViewById(R.id.negative_button); | ||
| if (usePositiveButtonStyle) { | ||
| negativeButton.setTextAppearance(this.view.getContext(), R.style.Commcare_Button_Primary_Rounded); | ||
| negativeButton.setTextSize(TypedValue.COMPLEX_UNIT_PX,this.getDialog().getContext().getResources().getDimensionPixelSize(R.dimen.font_size_medium)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix code style and potential NPE.
The current implementation has the following issues:
- Missing spaces around operators
- Multiple context/resources calls that could be optimized
- Potential NPE if dialog is null
Consider applying this improvement:
- negativeButton.setTextSize(TypedValue.COMPLEX_UNIT_PX,this.getDialog().getContext().getResources().getDimensionPixelSize(R.dimen.font_size_medium));
+ Context context = view.getContext();
+ if (context != null) {
+ float textSize = context.getResources().getDimensionPixelSize(R.dimen.font_size_medium);
+ negativeButton.setTextSize(TypedValue.COMPLEX_UNIT_PX, textSize);
+ }Committable suggestion skipped: line range outside the PR's diff.
| super(context, attrs); | ||
| if (isInEditMode()) return; | ||
| mContext = (AppCompatActivity)context; | ||
| mContext = (AppCompatActivity) context; |
There was a problem hiding this comment.
Ensure context is an instance of AppCompatActivity before casting
Casting context to AppCompatActivity without checking may lead to a ClassCastException if context is not an instance of AppCompatActivity. Consider verifying the instance before casting or enforcing that this view is only used within an AppCompatActivity.
Apply this diff to add an instance check:
public TabbedDetailView(Context context, AttributeSet attrs) {
super(context, attrs);
if (isInEditMode()) return;
+ if (context instanceof AppCompatActivity) {
mContext = (AppCompatActivity) context;
+ } else {
+ throw new IllegalArgumentException("Context must be an AppCompatActivity");
+ }
loadViewConfig(context, attrs);
}
@SuppressLint("NewApi")
public TabbedDetailView(Context context, AttributeSet attrs, int defStyle) {
super(context, attrs, defStyle);
+ if (context instanceof AppCompatActivity) {
mContext = (AppCompatActivity) context;
+ } else {
+ throw new IllegalArgumentException("Context must be an AppCompatActivity");
+ }
}Also applies to: 64-64
| if (detail.isCompound()) { | ||
| new TabLayoutMediator(mTabLayout, mViewPager, | ||
| (tab, position) -> tab.setText(detail.getDetails()[position].getTitle().getText().evaluate())) | ||
| .attach(); | ||
| } else { | ||
| mTabLayout.setVisibility(GONE); |
There was a problem hiding this comment.
Handle potential NullPointerException when setting tab titles
In the TabLayoutMediator, accessing detail.getDetails()[position].getTitle().getText() may result in a NullPointerException if any of these elements are null. Ensure that all elements are non-null before setting the tab title.
Apply this diff to add null checks:
new TabLayoutMediator(mTabLayout, mViewPager,
- (tab, position) -> tab.setText(detail.getDetails()[position].getTitle().getText().evaluate()))
+ (tab, position) -> {
+ Detail childDetail = detail.getDetails()[position];
+ if (childDetail != null && childDetail.getTitle() != null && childDetail.getTitle().getText() != null) {
+ CharSequence title = childDetail.getTitle().getText().evaluate();
+ tab.setText(title != null ? title : "Tab " + position);
+ } else {
+ tab.setText("Tab " + position);
+ }
+ })📝 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 (detail.isCompound()) { | |
| new TabLayoutMediator(mTabLayout, mViewPager, | |
| (tab, position) -> tab.setText(detail.getDetails()[position].getTitle().getText().evaluate())) | |
| .attach(); | |
| } else { | |
| mTabLayout.setVisibility(GONE); | |
| if (detail.isCompound()) { | |
| new TabLayoutMediator(mTabLayout, mViewPager, | |
| (tab, position) -> { | |
| Detail childDetail = detail.getDetails()[position]; | |
| if (childDetail != null && childDetail.getTitle() != null && childDetail.getTitle().getText() != null) { | |
| CharSequence title = childDetail.getTitle().getText().evaluate(); | |
| tab.setText(title != null ? title : "Tab " + position); | |
| } else { | |
| tab.setText("Tab " + position); | |
| } | |
| }) | |
| .attach(); | |
| } else { | |
| mTabLayout.setVisibility(GONE); |
app/AndroidManifest.xml
Outdated
| xmlns:tools="http://schemas.android.com/tools" | ||
| android:versionCode="106" | ||
| android:versionName="2.56"> | ||
| android:versionName="2.55.0"> |
There was a problem hiding this comment.
this should remain unchanged.
There was a problem hiding this comment.
yeah, I wonder if I'm missing something as this is updated by the mobile script. Let me fix it
avazirna
force-pushed
the
commcare_2.55
branch
from
8459ede to
a4e6eb8
Compare
README.md
Outdated
| - Open Android Studio | ||
| - If this is your first time using Android Studio, click "Config" and setup the Android SDK. | ||
| - Download the Android 12 (API 31) SDK Platform and the Google APIs for 31. | ||
| - Download the Android 14 (API 34) SDK Platform and the Google APIs for 34. |
There was a problem hiding this comment.
this should match the compileSdk i.e. Android 15, api 35
|
@damagatchi retest this please |
Summary
Backmerge CommCare 2.55 into the master branch