Fix Mapbox SDK dependency issue #2918
Conversation
This is needed by Realm Android Library 4.1.1 which is a dependency of Mapbox SDK
📝 WalkthroughWalkthroughThe changes in this pull request involve updates across several files, primarily focusing on the Android layout and resource files. In the Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant AndroidManifest
participant BuildGradle
participant Layouts
participant Drawables
User->>App: Launch Application
App->>AndroidManifest: Load Manifest
AndroidManifest-->>App: Apply Permissions and Activities
App->>BuildGradle: Load Dependencies
BuildGradle-->>App: Include Relinker Library
App->>Layouts: Load UI Layouts
Layouts-->>App: Render UI with MaterialButtons
App->>Drawables: Load Visual Elements
Drawables-->>App: Apply Styles and Colors
User->>App: Interact with UI
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (33)
app/res/layout/recording_fragment.xml (1)
Line range hint
71-84: LGTM with suggestions for accessibility improvementsThe MaterialButton implementation follows Material Design guidelines with proper spacing and layout attributes. However, consider adding the following accessibility improvements:
<com.google.android.material.button.MaterialButton android:id="@+id/action_button" android:layout_width="wrap_content" android:layout_height="wrap_content" android:padding="4dp" android:layout_marginStart="15dp" android:layout_marginLeft="15dp" android:layout_marginEnd="15dp" android:layout_marginRight="15dp" android:layout_marginBottom="15dp" android:layout_marginTop="15dp" android:gravity="center" tools:text="Finish" - android:visibility="invisible" /> + android:visibility="invisible" + android:contentDescription="@string/finish_recording" + android:minHeight="48dp" />app/res/layout/multiple_apps_limit_view.xml (1)
Line range hint
17-25: Enhance maintainability and accessibility of the MaterialButton.While the migration to MaterialButton is good, consider these improvements:
- Move the hardcoded text to string resources
- Add content description for accessibility
- Consider applying a consistent style across all MaterialButtons
Apply this diff:
<com.google.android.material.button.MaterialButton android:layout_width="wrap_content" android:layout_height="wrap_content" android:id="@+id/back_to_manager_button" android:layout_marginBottom="@dimen/standard_spacer_large" - android:text="Go to App Manager" + android:text="@string/go_to_app_manager" + android:contentDescription="@string/go_to_app_manager_description" android:textSize="19sp" android:layout_alignParentBottom="true" - android:layout_centerHorizontal="true"/> + android:layout_centerHorizontal="true" + style="@style/Widget.App.MaterialButton"/>app/res/layout/recording.xml (1)
Line range hint
1-31: Consider adding state handling for recording in progressThe layout includes a ProgressBar but could benefit from proper state management for the buttons:
<com.google.android.material.button.MaterialButton android:layout_width="wrap_content" android:layout_height="wrap_content" - android:id="@+id/startrecording" + android:id="@+id/startrecording" + android:enabled="true" ... /> <ProgressBar android:layout_width="wrap_content" android:layout_height="wrap_content" android:visibility="gone" android:indeterminate="true" android:id="@+id/recordingprogress" android:background="@color/blue"/> <com.google.android.material.button.MaterialButton android:layout_width="wrap_content" android:layout_height="wrap_content" - android:id="@+id/stoprecording" + android:id="@+id/stoprecording" + android:enabled="false" ... />Also, consider adding a recording duration TextView between the buttons to show the elapsed time.
app/res/layout/list_gregorian_widget.xml (3)
54-56: Consider consolidating button styles into a custom style resource.The day increment/decrement buttons mix direct attribute settings with a base style. To improve maintainability and consistency, consider extracting these common attributes into a custom style that extends
CommCare.Button.Default.Create a new style in
styles.xml:<style name="CommCare.Button.DatePicker" parent="CommCare.Button.Default"> <item name="android:textColor">@color/white</item> <item name="android:textSize">@dimen/text_largest</item> <item name="android:backgroundTint">@color/cc_brand_color</item> <item name="android:elevation">@dimen/button_elevation</item> </style> <style name="CommCare.Button.DatePicker.Top"> <item name="android:background">@drawable/date_button_top</item> </style> <style name="CommCare.Button.DatePicker.Bottom"> <item name="android:background">@drawable/date_button_bottom</item> </style>Then simplify the button declarations:
- style="@style/CommCare.Button.Default" - android:background="@drawable/date_button_top" - android:backgroundTint="@color/cc_brand_color" - android:textColor="@color/white" + style="@style/CommCare.Button.DatePicker.Top"Also applies to: 78-81
102-104: Maintain consistent attribute ordering across similar elements.The month increment/decrement buttons have the same attributes but in different orders. Consider maintaining a consistent attribute ordering pattern across all similar elements for better maintainability.
Example consistent ordering:
styleandroid:backgroundandroid:backgroundTintandroid:textColorandroid:elevationAlso applies to: 121-125
Line range hint
1-201: Add content descriptions for better accessibility.The layout is well-structured, but missing content descriptions for interactive elements could impact accessibility for users with screen readers.
Add content descriptions to all buttons and image buttons:
<Button android:id="@+id/dayupbtn" + android:contentDescription="@string/increment_day" ... /> <Button android:id="@+id/daydownbtn" + android:contentDescription="@string/decrement_day" ... /> <ImageButton android:id="@+id/clear_all" + android:contentDescription="@string/clear_date_selection" ... /> <ImageButton android:id="@+id/open_calendar_bottom" + android:contentDescription="@string/open_calendar" ... />app/res/drawable/startup_url.xml (1)
4-5: Remove unnecessary decimal points in viewport dimensionsThe
.0and.5decimal points in the viewport dimensions are unnecessary since integers would work just as well here. Consider simplifying them:- android:viewportWidth="300.0" - android:viewportHeight="232.5" + android:viewportWidth="300" + android:viewportHeight="233"app/res/layout/blank_missing_multimedia_layout.xml (2)
Line range hint
11-14: Replace deprecated pixel units with density-independent units.The layout uses deprecated
pxunits which can cause inconsistent rendering across different screen densities. Consider usingdp(density-independent pixels) instead.- android:paddingLeft="20px" - android:layout_marginRight="20px" - android:paddingTop="20px" - android:paddingBottom="0px" + android:paddingLeft="10dp" + android:layout_marginRight="10dp" + android:paddingTop="10dp" + android:paddingBottom="0dp" - android:layout_marginEnd="20px" - android:paddingStart="20px" + android:layout_marginEnd="10dp" + android:paddingStart="10dp"Also applies to: 21-22
Line range hint
1-7: Consider migrating to ConstraintLayout.While RelativeLayout works, ConstraintLayout would provide better performance and easier maintenance for complex layouts. This is a good-to-have improvement for future updates.
app/res/layout/http_request_layout.xml (1)
41-48: Migration to MaterialButton looks good but needs refinements.The switch to MaterialButton aligns with Material Design best practices. However, consider these improvements:
<com.google.android.material.button.MaterialButton android:id="@+id/request_button" - android:layout_margin="10dp" + android:layout_margin="@dimen/content_min_margin" android:layout_width="wrap_content" android:layout_gravity="center" android:layout_height="wrap_content" android:paddingBottom="@dimen/content_start" + android:paddingStart="@dimen/content_start" + android:paddingEnd="@dimen/content_start" android:paddingTop="@dimen/content_start" + android:contentDescription="@string/request_button_description" + android:text="@string/request_button_text" />Changes suggested:
- Use dimension resource for margin to maintain consistency
- Add missing horizontal padding
- Add content description for accessibility
- Add button text
app/res/layout/custom_alert_dialog.xml (1)
74-85: Consider optimizing button attributes for better maintainability.While the MaterialButton implementation is correct, consider these improvements:
- Remove redundant
marginRightsincemarginEndserves the same purpose in RTL-aware layouts.- Group related attributes together (layout params, margins, styling).
Apply this diff to optimize the attributes:
<com.google.android.material.button.MaterialButton android:id="@+id/positive_button" android:layout_width="wrap_content" android:layout_height="@dimen/min_button_height" android:layout_alignParentEnd="true" - android:layout_alignParentRight="true" android:layout_marginEnd="@dimen/standard_spacer" - android:layout_marginRight="@dimen/standard_spacer" android:layout_marginStart="@dimen/standard_spacer" - android:layout_marginLeft="@dimen/standard_spacer" android:padding="@dimen/standard_spacer" android:visibility="gone" app:strokeColor="@color/cc_brand_color"/>app/res/layout/draw_layout.xml (2)
Line range hint
23-27: Add required MaterialButton attributesThe MaterialButton is missing essential attributes:
android:textorandroid:contentDescriptionfor accessibilitystyleattribute to maintain consistent appearanceandroid:onClickor click handlingApply these changes:
<com.google.android.material.button.MaterialButton android:id="@+id/btnFinishDraw" android:layout_width="wrap_content" android:enabled="false" - android:layout_height="wrap_content"/> + android:layout_height="wrap_content" + android:text="@string/finish_drawing" + android:contentDescription="@string/finish_drawing_description" + style="@style/Widget.MaterialComponents.Button" + android:onClick="onFinishDrawing"/>
35-39: Consider using layout_marginEnd instead of horizontal marginSince this button is in a LinearLayout and the parent already has horizontal margins, consider:
- Using only
layout_marginEndfor spacing between buttons- Moving margin handling to the LinearLayout level
<com.google.android.material.button.MaterialButton android:id="@+id/btnResetDraw" android:layout_width="wrap_content" - android:layout_marginHorizontal="16dp" + android:layout_marginEnd="16dp" android:layout_height="wrap_content"/>app/res/layout/screen_form_entry.xml (2)
240-249: Consider separating UI changes from SDK dependency fixWhile the MaterialButton implementation follows proper Material Design guidelines, this UI change appears unrelated to the PR's primary objective of fixing Mapbox SDK dependency issues. Mixing UI changes with critical SDK fixes could:
- Make it harder to isolate and revert changes if issues arise
- Increase the risk of introducing UI regressions
Consider:
- Moving these UI changes to a separate PR focused on Material Design updates
- If keeping these changes, ensure thorough UI testing across different screen sizes and orientations
Line range hint
1-258: Review PR scope and separation of concernsThis file contains UI modernization changes that appear unrelated to the PR's stated objective of fixing Mapbox SDK dependency issues. While the changes themselves are technically sound, mixing UI updates with critical SDK fixes complicates:
- Code review
- Testing
- Potential rollback scenarios
- Root cause analysis if issues arise
Recommendation: Consider extracting these UI changes into a separate PR focused on Material Design updates.
app/res/layout/screen_form_dump.xml (2)
18-27: Improve layout attributes consistency and RTL supportThe ImageView attributes can be optimized for better maintainability and RTL support:
- android:paddingLeft="8dp" - android:layout_marginRight="8dp" - android:paddingTop="8dp" - android:paddingBottom="0dp" - android:maxHeight="83dp" - android:maxWidth="442dp" - app:srcCompat="@drawable/commcare_by_dimagi" - android:layout_marginEnd="8dp" - android:paddingStart="8dp"></ImageView> + android:paddingHorizontal="8dp" + android:layout_marginEnd="8dp" + android:paddingTop="8dp" + android:maxHeight="83dp" + android:maxWidth="442dp" + app:srcCompat="@drawable/commcare_by_dimagi" + tools:ignore="ContentDescription" />Changes:
- Consolidated padding using
paddingHorizontal- Removed redundant right/end attributes
- Replaced empty closing tag with self-closing tag
- Added content description ignore for accessibility
Line range hint
48-62: Enhance accessibility and maintainability of MaterialButtonsWhile the migration to MaterialButtons is good, there are several improvements that can be made:
- Add content descriptions for accessibility
- Move text strings to resources
- Consider using styles for consistent theming
<com.google.android.material.button.MaterialButton android:id="@+id/screen_bulk_form_dump" android:layout_width="match_parent" android:layout_margin="10dp" android:layout_height="wrap_content" android:layout_below="@+id/screen_bulk_form_prompt" - android:text="Dump Forms"/> + android:text="@string/dump_forms" + android:contentDescription="@string/dump_forms_description" + style="@style/Widget.App.Button"/> <com.google.android.material.button.MaterialButton android:id="@+id/screen_bulk_form_submit" android:layout_margin="10dp" android:layout_width="match_parent" android:layout_height="wrap_content" android:layout_below="@+id/screen_bulk_form_dump" - android:text="Submit Forms"/> + android:text="@string/submit_forms" + android:contentDescription="@string/submit_forms_description" + style="@style/Widget.App.Button"/>Consider simplifying the layout hierarchy by replacing the RelativeLayout with LinearLayout since the buttons are already in a vertical arrangement. This would improve layout performance and readability.
app/res/drawable/home_logout.xml (1)
2-8: Part of systematic UI standardizationThis file is part of a broader UI standardization effort, changing dimensions to 300x233dp and using
@color/square_button_icon_color. While the changes are well-structured, they should be moved to a dedicated UI improvement PR to:
- Reduce the risk of the critical Mapbox SDK fix
- Allow proper UI/UX review
- Enable focused testing of the icon changes
app/res/drawable/home_incomplete.xml (1)
2-8: Recommend splitting UI standardization workThis file, along with the others, shows a consistent pattern of UI updates:
- Standardizing dimensions to 300x233dp
- Moving to resource-based colors
- Updating vector paths
While these changes improve UI consistency, they increase the scope and testing surface of what should be a focused Mapbox SDK dependency fix. Consider:
- Reverting these UI changes from this PR
- Creating a new PR specifically for UI standardization
- Adding before/after screenshots in the UI-focused PR for easier review
This will help maintain the principle of single responsibility and reduce the risk of the critical SDK fix.
app/AndroidManifest.xml (1)
363-363: Consider adding exported flag to MapboxLocationPickerActivity.For better security, consider explicitly declaring the
exportedattribute.- <activity android:name="org.commcare.gis.MapboxLocationPickerActivity" /> + <activity + android:name="org.commcare.gis.MapboxLocationPickerActivity" + android:exported="false" />app/res/layout/square_button_text.xml (1)
Line range hint
1-1: Major discrepancy between PR objective and changesThe PR's objective is to fix Mapbox SDK dependency issues by including the ReLinker library, but the provided files only contain UI changes. Please:
- Provide the relevant build configuration changes that address the SDK dependency
- Explain why these UI changes are necessary for the SDK fix
- Consider splitting unrelated UI changes into a separate PR
This will help maintain clear traceability between the changes and their intended purpose.
app/res/layout/entity_detail.xml (1)
13-18: Consider using MaterialButton for consistencyWhile the styling changes improve the button's appearance, other files in this PR are using MaterialButton components. For consistency across the UI, consider converting this Button to MaterialButton as well.
- <Button + <com.google.android.material.button.MaterialButton style="@style/ScreenPinnedButton" android:layout_width="fill_parent" android:layout_height="wrap_content" app:backgroundTint="@color/cc_attention_positive_bg" android:textColor="@color/cc_attention_positive_text" android:id="@+id/entity_select_button" android:text="Continue" android:layout_below="@+id/universal_frame_tile"/>app/res/layout/app_manager.xml (1)
Line range hint
1-1: Major discrepancy between PR objectives and actual changesThe PR objectives state this is meant to "Fix Mapbox SDK dependency issue" by including the ReLinker library. However, the changes only show UI updates (Button to MaterialButton conversions) with no modifications related to dependencies, Mapbox SDK, or the ReLinker library. Please clarify this discrepancy or ensure all necessary changes are included in the PR.
app/res/layout/screen_recovery.xml (1)
Line range hint
41-48: Consider applying consistent MaterialButton stylingWhile the transition to MaterialButton is good, consider applying a consistent style across all MaterialButtons in the app for better maintainability.
#!/bin/bash # Search for MaterialButton style patterns across the app rg -A 1 "MaterialButton" --type xmlAlso applies to: 51-59
app/res/layout/screen_multimedia_inflater.xml (1)
Line range hint
18-23: Add content description for accessibilityThe ImageButton is missing a content description, which is required for accessibility.
Add the following attribute to the ImageButton:
<ImageButton android:id="@+id/screen_multimedia_inflater_filefetch" + android:contentDescription="@string/multimedia_file_selector" ... />app/res/layout/single_app_view.xml (2)
Line range hint
39-70: Consider separating UI updates from Mapbox SDK fixesThe MaterialButton conversions in this view appear unrelated to the PR's primary objective of fixing Mapbox SDK dependency issues. While the changes themselves are correct, consider:
- Separating these UI updates into a different PR to maintain focused scope
- Ensuring these changes don't introduce unnecessary complexity to the Mapbox SDK fix
PR scope and description mismatch needs attention
Based on the verification results:
- The only actual change is adding the ReLinker library dependency in build.gradle
- There are no changes to any Mapbox-related files in the GIS directory
- The UI changes in single_app_view.xml are not present in the diff, suggesting they might be from a different PR or branch
The PR description appears to be inaccurate or mixed up with another PR since:
- No Mapbox SDK dependency issues are being addressed
- While ReLinker is added, there are no implementation changes
- The mentioned UI updates are not part of this PR's changes
Please:
- Update the PR description to accurately reflect the ReLinker library addition
- Create a separate PR for UI changes if needed
- If Mapbox SDK fixes are still needed, they should be in their own PR
🔗 Analysis chain
Line range hint
1-1: Clarify relationship between UI changes and Mapbox SDK fixThe PR description mentions fixing Mapbox SDK dependency issues and ReLinker library integration, but the provided files show primarily UI updates (Button → MaterialButton conversions). Please clarify:
- How do these UI changes relate to fixing the Mapbox SDK dependency issues?
- Where are the ReLinker library integration changes?
- Should these UI updates be in a separate PR to maintain clear separation of concerns?
Run this script to locate Mapbox SDK and ReLinker related changes:
Let me verify the relationship between UI changes and the dependency updates by checking for any Mapbox-related UI components.
Let me check the actual changes in this PR by looking at the git diff to understand the full scope of changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Mapbox SDK and ReLinker related changes echo "Searching for Mapbox SDK related files..." rg -l "mapbox|kujaku" --type gradle rg -l "relinker" --type gradle echo "Searching for dependency changes..." git diff HEAD~1 -- "**/build.gradle" "**/build.gradle.kts"Length of output: 856
Script:
#!/bin/bash # Search for Mapbox-related UI components and usage echo "Searching for Mapbox UI components in layout files..." rg -l "mapbox|com\.mapbox" --type xml echo "Searching for Mapbox imports and usage in Java/Kotlin files..." rg -l "import.*mapbox" --type java --type kotlin echo "Checking single_app_view.xml content..." cat app/res/layout/single_app_view.xmlLength of output: 3728
Script:
#!/bin/bash # Get the full diff of changes in this PR git diff HEAD~1 --name-status # Check if there are any changes to Mapbox-related files echo "Checking changes in Mapbox-related files..." git diff HEAD~1 -- app/src/org/commcare/gis/ # Check build.gradle changes in detail echo "Checking detailed build.gradle changes..." git diff HEAD~1 app/build.gradleLength of output: 841
app/res/layout/connection_diagnostic.xml (1)
61-66: Consider Mapbox-specific troubleshooting optionsThe settings and report buttons look good with MaterialButton styling. However, since Mapbox SDK issues are a known problem, consider adding Mapbox-specific troubleshooting options in the settings or reporting flow.
Also applies to: 68-73
app/res/layout/execute_recovery_measures.xml (1)
Line range hint
56-85: Unrelated UI changes in recovery screenMultiple changes from
ButtontoMaterialButtonand layout adjustments appear unrelated to fixing the Mapbox SDK dependency issue. While these UI improvements are valuable:
- They increase the scope of changes in a critical fix
- They make it harder to isolate and test the SDK dependency fix
- They introduce additional risk in a recovery-related screen
Strongly recommend:
- Separating UI changes into a different PR
- Keeping this PR focused on the ReLinker library integration and Mapbox SDK fix
- Adding specific tests for the SDK dependency fix
app/res/layout/scrolling_calendar_widget.xml (1)
Line range hint
137-144: Externalize string and add content descriptionThe button text should use a string resource instead of hardcoded text, and needs accessibility support:
Apply this diff:
<com.google.android.material.button.MaterialButton android:layout_width="wrap_content" android:layout_height="wrap_content" - android:text="Today" + android:text="@string/calendar_today" android:id="@+id/today" android:layout_centerHorizontal="true" android:layout_marginTop="@dimen/standard_spacer" - android:layout_marginBottom="@dimen/standard_spacer"/> + android:layout_marginBottom="@dimen/standard_spacer" + android:contentDescription="@string/calendar_today_description"/>app/res/layout/user_get_available_apps.xml (1)
137-142: Consider adding click feedback and accessibility attributesWhile the MaterialButton implementation is correct, consider enhancing it with:
- Content description for accessibility
- Ripple effect for better touch feedback
<com.google.android.material.button.MaterialButton android:id="@+id/get_apps_button" android:layout_width="wrap_content" android:layout_height="wrap_content" android:text="@string/see_available_apps" android:paddingHorizontal="@dimen/standard_spacer_large" - android:layout_marginTop="@dimen/standard_spacer_double" /> + android:layout_marginTop="@dimen/standard_spacer_double" + android:contentDescription="@string/see_available_apps" + android:stateListAnimator="@null" + app:rippleColor="?attr/colorControlHighlight" />app/res/layout/missing_multimedia_layout.xml (2)
19-22: Convert px units to dp and standardize spacing attributesWhile the image resource update looks good, there are some layout improvements needed:
- android:paddingLeft="20px" - android:layout_marginRight="20px" - android:paddingTop="20px" - android:paddingBottom="0px" - android:maxHeight="135px" - android:maxWidth="663px" - android:paddingStart="20px" - android:layout_marginEnd="20px" + android:padding="16dp" + android:layout_marginHorizontal="16dp" + android:maxHeight="90dp" + android:maxWidth="442dp"Rationale:
- Replace deprecated px units with dp for consistent sizing across different screen densities
- Use consistent spacing with padding/margin
- Simplified horizontal spacing using
layout_marginHorizontal
Line range hint
33-40: Remove redundant layout_gravity attributeThe button upgrade looks good, but there's a minor improvement needed:
android:layout_width="fill_parent" android:layout_height="wrap_content" - android:layout_gravity="bottom" android:text="@string/retry_button_text"The
layout_gravityattribute has no effect in a RelativeLayout andlayout_alignParentBottomalready handles the positioning.
🛑 Comments failed to post (8)
app/res/layout/recording.xml (2)
12-15: 🛠️ Refactor suggestion
Add essential attributes to the start recording button
The MaterialButton is missing several important attributes that would improve usability and accessibility:
<com.google.android.material.button.MaterialButton android:layout_width="wrap_content" android:layout_height="wrap_content" - android:id = "@+id/startrecording"/> + android:id="@+id/startrecording" + android:text="@string/start_recording" + android:contentDescription="@string/start_recording_description" + android:layout_margin="@dimen/button_margin" + style="@style/Widget.MaterialComponents.Button.Icon" + app:icon="@drawable/ic_record"/>Committable suggestion skipped: line range outside the PR's diff.
26-29: 🛠️ Refactor suggestion
Add essential attributes to the stop recording button
Similar to the start button, this MaterialButton needs proper attributes for better user experience:
<com.google.android.material.button.MaterialButton android:layout_width="wrap_content" android:layout_height="wrap_content" - android:id="@+id/stoprecording"/> + android:id="@+id/stoprecording" + android:text="@string/stop_recording" + android:contentDescription="@string/stop_recording_description" + android:layout_margin="@dimen/button_margin" + style="@style/Widget.MaterialComponents.Button.Icon" + app:icon="@drawable/ic_stop"/>Committable suggestion skipped: line range outside the PR's diff.
app/res/layout/universal_date_widget.xml (1)
20-22: 💡 Codebase verification
Style conflict confirmed - backgroundTint will override custom drawable states
The current changes create a styling conflict:
- The existing drawables (
date_button_top.xmlanddate_button_bottom.xml) define custom gradient backgrounds with different states (pressed, focused, default) and rounded corners- Setting
backgroundTint="@color/cc_brand_color"will overlay a solid color on top of these drawables, effectively hiding their gradient and state-based styling- The
CommCare.Button.Defaultstyle explicitly setsbackgroundTint="@null"to allow custom backgrounds, but this is being overriddenRecommendations:
- Remove the
backgroundTintattribute to preserve the existing state-based styling with gradients- If a solid brand color is desired, remove the background drawables instead and keep the backgroundTint
- Consider updating the drawable definitions directly if brand colors should be incorporated into the state-based styling
🔗 Analysis chain
Potential style conflict between background drawable and backgroundTint
Each button has both a background drawable (e.g.,
@drawable/date_button_top) and abackgroundTint. This combination might lead to unexpected visual results as both attributes affect the button's background appearance. Consider either:
- Removing the background drawables if the new styling is intended to replace them
- Removing the backgroundTint if the drawable styling should be preserved
Let's check the drawable definitions:
Also applies to: 24-24, 43-45, 47-47, 58-60, 62-62, 88-90, 92-92, 103-105, 107-107, 133-135, 137-137
Let me check the content of these drawable files to understand their styling and potential conflicts with backgroundTint.
Let me check the CommCare.Button.Default style definition to fully understand the styling conflict.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the content of the button background drawables for drawable in date_button_top date_button_bottom date_edit; do echo "=== $drawable ===" fd -e xml "$drawable" app/res/drawable doneLength of output: 593
Script:
#!/bin/bash # Check the content of the drawable XML files echo "=== date_button_top.xml ===" cat app/res/drawable/date_button_top.xml echo -e "\n=== date_button_bottom.xml ===" cat app/res/drawable/date_button_bottom.xml echo -e "\n=== date_edit.xml ===" cat app/res/drawable/date_edit.xml # Also let's check the CommCare.Button.Default style definition echo -e "\n=== Looking for CommCare.Button.Default style ===" rg -l "CommCare\.Button\.Default" app/res/values/Length of output: 5257
Script:
#!/bin/bash # Check the content of themes.xml for the CommCare.Button.Default style echo "=== CommCare.Button.Default style definition ===" rg -A 10 "CommCare\.Button\.Default" app/res/values/themes.xmlLength of output: 685
app/res/layout/custom_alert_dialog.xml (1)
98-111: 💡 Codebase verification
Remove redundant strokeColor attribute from MaterialButton
The
strokeColorattribute is already defined in theNegativeButtonShapeAppearanceStylestyle, which is referenced byNegativeButtonStyle. This creates unnecessary duplication that could lead to maintenance issues.
- Remove
app:strokeColor="@color/cc_brand_color"fromcustom_alert_dialog.xml- Also clean up the deprecated RTL attributes as suggested
🔗 Analysis chain
Review style implementation and attribute redundancy.
While the MaterialButton implementation is correct, there are a few considerations:
- The
strokeColormight be redundantly defined if it's already part ofNegativeButtonStyle.- The same attribute optimization suggestions from previous buttons apply here.
Let's verify the style definition:
If
strokeColoris defined in the style, apply this diff:<com.google.android.material.button.MaterialButton android:id="@+id/negative_button" style="@style/NegativeButtonStyle" android:layout_width="wrap_content" android:layout_height="@dimen/min_button_height" android:layout_marginEnd="@dimen/standard_spacer" - android:layout_marginRight="@dimen/standard_spacer" android:layout_marginStart="@dimen/standard_spacer" - android:layout_marginLeft="@dimen/standard_spacer" android:layout_toStartOf="@id/neutral_button" - android:layout_toLeftOf="@id/neutral_button" android:padding="@dimen/standard_spacer" android:visibility="gone" - app:strokeColor="@color/cc_brand_color"/>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if strokeColor is defined in the style rg -A 10 "style name=\"NegativeButtonStyle\"" app/res/values/styles.xmlLength of output: 844
app/res/layout/draw_layout.xml (1)
41-44:
⚠️ Potential issueMaintain consistency with MaterialButton attributes
Similar to btnFinishDraw, this button needs essential attributes for proper functionality and accessibility.
Apply these changes:
<com.google.android.material.button.MaterialButton android:id="@+id/btnCancelDraw" android:layout_width="wrap_content" - android:layout_height="wrap_content"/> + android:layout_height="wrap_content" + android:text="@string/cancel_drawing" + android:contentDescription="@string/cancel_drawing_description" + style="@style/Widget.MaterialComponents.Button" + android:onClick="onCancelDrawing"/>Committable suggestion skipped: line range outside the PR's diff.
app/build.gradle (1)
108-108: 💡 Codebase verification
ReLinker dependency added but implementation missing
The ReLinker library has been added as a dependency, but there's no evidence of its actual implementation in the codebase. For proper native library loading with Mapbox SDK, ReLinker needs to be initialized in the Application class.
- Add ReLinker initialization in your Application class
- Example usage:
ReLinker.loadLibrary(this, "your-native-library")🔗 Analysis chain
LGTM: ReLinker dependency added correctly.
The addition of ReLinker library (v1.4.4) is appropriate for handling native library loading for the Mapbox SDK. This addresses the core issue mentioned in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ReLinker usage in the codebase # Check if ReLinker is properly initialized in Application class ast-grep --pattern 'class $_ extends Application { $$$ onCreate() { $$$ ReLinker.$$$ $$$ } $$$ }'Length of output: 119
Script:
#!/bin/bash # Search for any ReLinker usage in the codebase rg -i "ReLinker" -A 3 # Also check for any Application class that might be using different ReLinker initialization pattern ast-grep --pattern 'class $_ extends Application { $$$ onCreate($$$) { $$$ } }'Length of output: 479
app/res/layout/activity_mapbox_location_picker.xml (1)
67-72: 💡 Codebase verification
Apply consistent MaterialButton styling with predefined styles
The cancel button in
activity_mapbox_location_picker.xmllacks a style attribute while other Mapbox-related screens use predefined styles like@style/MapButtonGreen,@style/MapButtonRed, and@style/MapButtonOrangefor their MaterialButtons. Consider applying an appropriate predefined style to maintain consistency.
- In
activity_mapbox_location_picker.xml, add a suitable style attribute to the cancel button (e.g.,style="@style/MapButtonRed"for cancel actions)🔗 Analysis chain
Consider consistent styling across Mapbox screens
The MaterialButton migration looks good. However, ensure this styling is consistent across all Mapbox-related screens for a unified user experience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all layout files containing Mapbox-related views rg -l "KujakuMapView" app/res/layout/Length of output: 175
Script:
#!/bin/bash # Check button styling in Mapbox-related layouts rg -A 5 "MaterialButton|Button" app/res/layout/activity_drawing_boundary.xml app/res/layout/activity_entity_mapbox.xmlLength of output: 2706
app/res/layout-land/activity_crash_warning.xml (1)
96-101: 🛠️ Refactor suggestion
Add missing text and content description attributes
The MaterialButton is missing required attributes:
android:textto display the button labelandroid:contentDescriptionfor accessibility supportApply this diff:
<com.google.android.material.button.MaterialButton android:id="@+id/RestartCommCare" android:layout_width="fill_parent" android:layout_height="wrap_content" android:layout_alignParentBottom="true" - android:layout_margin="@dimen/content_min_margin"/> + android:layout_margin="@dimen/content_min_margin" + android:text="@string/restart_commcare" + android:contentDescription="@string/restart_commcare_description"/>Committable suggestion skipped: line range outside the PR's diff.
|
@damagatchi retest this please |
Summary
After the JCenter sunsetting we had to exclude some group dependencies from Kujaku as they were not resolving, and we added the dependecies
aarsandjarsto the project structure. It seems that we missed one, ReLinker, a native libraries loader used by Mapbox SDK, causing CommCare to crash whenever a feature that uses that SDK is used. This PR adds that to the project.Here's a stackstrace of one of the crashes:
Product Description
PR Checklist
Safety story
I tested this fix locally and the features relying on Mapbox SDK are now working as expected.