Add support for a custom Map marker for the Case List

[shubham1g5]

Summary

Applies a custom map marker on the "Map" Case list view when a detail field with template image is defined for the case list when the property cc-enable-custom-map-marker is set to yes in app settings.

Product Description

Image with default icon:

Image with a custom icon applied:

PR Checklist

• If I think the PR is high risk, "High Risk" label is set
• I have confidence that this PR will not introduce a regression for the reasons below
• Do we need to enhance manual QA test coverage ? If yes, "QA Note" label is set correctly
• Does the PR introduce any major changes worth communicating ? If yes, "Release Note" label is set and a "Release Note" is specified in PR description.

Automated test coverage

None

Safety story

Tested locally for scenarios when an icon is defined for the case list and when it's not.

QA Note: View on Map case list view now supports a custom map marker and an appropriate test case should be added to verify it.

Release Note: Custom map marker support for Case list's "View on Map" feature.

Best reviewed commit by commit.

[shubham1g5]

Open Question: Think we should put this bheaviour behind a custom setting in order to not change the behaviour for existing projects using a icon in their case list and "View on Map" case list functionality.

@ctsims Can you advise on this ?

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

app/src/org/commcare/gis/EntityMapActivity.java (2)

134-142: Add error handling and make icon size configurable.

While the implementation is correct, consider these improvements:

1. Add error handling for image loading failures
2. Make the icon size configurable

+ private static final int DEFAULT_MARKER_SIZE = 120;

 private BitmapDescriptor getEntityIcon(Entity<TreeReference> entity) {
     if (imageFieldIndex != -1) {
         String jrUri = String.valueOf(entity.getData()[imageFieldIndex]);
+        if (jrUri == null || jrUri.isEmpty()) {
+            return null;
+        }
         try {
-            Bitmap bitmap = MediaUtil.inflateDisplayImage(this, jrUri, 120, 120, true);
+            Bitmap bitmap = MediaUtil.inflateDisplayImage(this, jrUri, 
+                DEFAULT_MARKER_SIZE, DEFAULT_MARKER_SIZE, true);
             if (bitmap != null) {
                 return BitmapDescriptorFactory.fromBitmap(bitmap);
             }
+        } catch (Exception e) {
+            Log.e(TAG, "Error loading marker icon: " + jrUri, e);
         }
     }
     return null;
 }

---

55-57: Consider making custom markers configurable.

As discussed in the PR description, consider adding a custom setting to control this feature. This would prevent unexpected changes for existing projects that use icons in their case list.

Suggested approaches:

1. Add a feature flag in the app settings
2. Add a configuration in the suite's detail definition
3. Add a global configuration through CommCare's app settings

Would you like me to provide a detailed implementation for any of these approaches?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a85740 and d543c9b.

📒 Files selected for processing (1)

• app/src/org/commcare/gis/EntityMapActivity.java (6 hunks)

🔇 Additional comments (5)

app/src/org/commcare/gis/EntityMapActivity.java (5)

3-4: LGTM! Clean imports and field declaration.

The new imports and field declaration are well-organized and properly initialized.

Also applies to: 8-8, 16-17, 29-29, 31-31, 55-57

---

97-105: LGTM! Clean implementation of image field detection.

The method efficiently finds the image field index with early termination.

---

118-119: LGTM! Clean marker setup with custom icon.

The marker creation properly includes both title and custom icon.

---

157-164: Add null check for map instance.

The location permission handling is correct, but there's a potential NPE if mMap is null. Please refer to the previous review comment for the suggested fix.

---

84-84: Consider handling dynamic field changes.

The imageFieldIndex should be re-evaluated if the detail fields change during runtime.

Run this script to check if detail fields can change during runtime:

[OrangeAndGreen]

The code changes look correct to me, although I'm not sure how to test this functionality as I'm not familiar with the mapping activity. Is there a test app that I could use to test the functionality?

[shubham1g5]

Is there a test app that I could use to test the functionality?

I used this app to do the same, you can navigate as Start -> Case List -> Followup to land on the case list and then click on "View on Map" under the three dots settings menu. The particular setting is visible only when a Display property with format address is present in the case list "Display Properties` configuration.

[OrangeAndGreen]

Found one little issue but otherwise the code looks good.

[OrangeAndGreen]

I think the or-clause needs to be put in parentheses so it calculates before the and-clause, i.e.
if (mMap != null && (FINE || COARSE))

I hit a crash when testing where mMap was null but the if-block executed anyway