Add a new MapActivity that shows saved forms on a map.

[zestyping]

Closes #61

See the design document for background, details on the design of this feature, the supporting reasoning, and the implementation plan.

Related issues: #61, getodk/getodk#511
Forum discussion: Visualize ... the data collected with ODK on a map
Roadmap item: Collect: Map submissions on device

The new form chooser list:

The new MapActivity with custom icons:

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

The user-visible behaviour changes in this PR are:

• Map buttons are shown in the form list for forms with geometry.
• The map button launches a MapActivity that shows a marker on the map for each form instance.
• The map markers are small balloons coloured according to form status.
• The "new instance" button on the map launches the FormEntryActivity.
• When a form is completed, the app returns to the MapActivity with the newly added point.
• Clicking a map marker navigates to viewing/editing the saved form instance.

The affected areas of the codebase are:

• MapFragment and its implementations
• The form listing displayed by FormChooserListActivity and FormListAdapter
• The DiskSyncTask, which processes form definitions on load.
• The SaveToDiskTask, which saves form instances when the user taps "Save".
• There are new columns in the forms table and the instances table, which means the schemas have been incremented by 1 and a data migration will be automatically performed to the new schema when the app is upgraded from a previous version.

What has been done to verify that this works as intended?

I manually tested each of the behaviour changes shown above. In particular:

• I added a few forms that contained geo questions and confirmed that map buttons appeared for these forms in the form list.
• I saved several instances of the "geo widgets" form and confirmed that I could see the points as icons on the map.
• I tried changing the status of some instances by finalizing them and submitting them, and confirmed that the icon colours changed as expected.

Why is this the best possible solution? Were any other approaches considered?

See the design document for the background and rationale.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Yes: getodk/docs#1144

Before submitting this PR, please make sure you have:

• run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[zestyping]

I've made the map markers clickable and customized the icons according to the status of each form instance. (Feels quite satisfying to tap a blue icon, mark finalized, save, and see it turn purple on the map!)

I believe this PR is now feature-complete (as defined by the design document and ready for review!

[zestyping]

Hmm, looks like I can't directly add labels so I can't tag this needs review.

[zestyping]

Okay, as a bonus, I added a "zoom to bounds" button because it was easy and we're pretty sure we'll need it. 😄

[lognaturel]

This is fantastic.

Feels quite satisfying to tap a blue icon, mark finalized, save, and see it turn purple on the map!

The whole thing is super satisfying! 🎉 🗺 Zoom to bounds is a really nice addition. 💯

I'll be diving into the code now but I have a few UI/UX observations:

• I wonder whether the map icon should be black instead of blue. This would match other buttons that represent actions like the search and sort buttons right above and the media buttons while filling the form. @seadowg is the Material Design / UI consistency Tsar now so would be good to get his UI impressions.
• Changing location doesn't get reflected in map. Tap a point to edit, change the location and come back to the map. The location is still the original one.
• Sent and deleted points look the same as sent points. When deleted, instances that have been sent are kept in the db so there's a record of them but their instance XML is deleted. Tapping on deleted point currently opens a new blank form. It should do nothing? Show a toast with the deleted date?
• Point XPath discovery doesn't seem to work from form download (vs. push to sdcard). Probably doesn't matter because it will get revised after Build a FormDef when doing initial form discovery #3452 is merged anyway so this is more of a note not to forget that.

[lognaturel]

Really not much to say here. The commit history is really straightforward to review, thank you.

[zestyping]

I'm travelling to DRC very soon, so @lognaturel is taking this PR forward from here. Thank you so much @lognaturel !

[lognaturel]

You've taken us most of the way to this fantastic feature, @zestyping! Safe travels and may the impact of your work continue to grow. For those who may not know, DRC has an ebola outbreak and @zestyping is going to be iterating on and deploying some software to support frontline workers there (including a bit of ODK!).

This is what the icon looks like in black (CC @seadowg):

I have a slight preference for this. @smoyth, @cooperka, since you also reviewed Material Design guidelines when you reworked this screen and have some interest in this feature, please consider weighing in. The immediate question is whether the icon should be filled blue or black but feel free to make other suggestions.

[cooperka]

Super cool functionality! Latest screenshot LGTM in terms of Material Design. Nice work all.

[seadowg]

Just a few comments on Material Design/Theming things. The feature looks really cool though!

[seadowg]

Are these icons coming from Studio's Vector Assests tool? If so could we leave the names the way they are when they are generated? That way it's easier to work out whether we already have an icon in our resources or not.

[lognaturel]

Going through the Vector Assets tool gives name ic_room_black_24dp no matter the tint. The icon already exists without a tint as ic_place_black and is used for warning dialogs related to location.

For this feature, there are 4 colored variants needed as you can see in getDrawableIdForStatus. Is there a way to do that other than introducing 4 resources?

[lognaturel]

I'll add that the marker gets set by the setMarkerIcon method defined by the MapFragment interface. I believe that to use a single black icon and tint it, we'd have to introduce a new method there and make sure hat all three mapping engines (Mapbox, OSM, Google Maps) all support tinting across all API levels. Does that seem worth it, @seadowg?

[seadowg]

Ah I see because it takes a resource int I see. Yeah right now that does feel like a bigger disruption. For the moment can we just have them named like ic_room_red_24dp, ic_room_blue_24dp etc?

Just a not on tinting though: My impression is that we'd be able to use DrawableCompat.wrap to create "tintable" drawables back to at least API 14. I guess that it might not work with map engines for some reason but I think we could probably just test at our min API level.

[mmarciniak90]

@lognaturel after today's testing, I have a few thoughts

1. Leaving form always moves to the main view but with these changes, we have different behavior.
   I just want to confirm that we want to change this standard like described on PR, that we want to leave it in that way.
2. I also wonder should this map view be available by default to all users.
   Maybe we should think about additional option in settings which turns it on.
3. Leaks appear more frequently
4. User needs to be precise to click on the icon.
   Few times when I wanted to open map, the form opened because I didn't aim at the icon
5. You can say that I'm picky 😄 but the map icon is not centered on all Android versions.
   I see that icon is not in the center on Android 9.0, 6.0, 4.4

[lognaturel]

Leaving form always moves to the main view but with these changes, we have different behavior.

Yes, this is important because users want to be able to perform their data collection in the context of the map view. That is, they may use the map to navigate and to evaluate their progress.

I also wonder should this map view be available by default to all users.

Can you elaborate on your concerns? Do you think it’s distracting, possibly reveals information, something else? Our reasoning is that it’s broadly useful but also clearly a secondary action so not disruptive. We do have to be careful not to allow users to do things they couldn’t do before, though. For that reason, if Edit Saved Form is made unavailable from the admin menu, instances can’t be edited. Instances that are deleted won’t be mapped. I went back and forth on what to do when the View Sent Form button wasn’t available. Ultimately I decided that it’s reasonable to still make sent instances visible on the map because those instances are still on the device. If the issue is data sensitivity, it’s not a bad thing for users to more clearly see that the data is still on the device so that they can turn on delete after send, for example.

I will look into 3-5, thank you!

[mmarciniak90]

@lognaturel I continued testing and here are results:

Can you elaborate on your concerns?

You dispelled my doubts 😄

---

Cases verified with success:

• Map buttons are shown in the form list for forms with geopoint question ✔️
• The map button launches a MapActivity that shows a marker for each form instance ✔️
• The markers are small balloons colored according to form status (blue, purple, red, green) ✔️
• The "new instance" button on the map launches the FormEntryActivity ✔️
• When a form is completed, the app returns to the MapActivity with the newly added point ✔️
• Clicking a map marker navigates to viewing/editing the saved form instance ✔️
• If Edit Saved Form is made unavailable from the admin menu, instances can’t be edited ✔️
• If the instance is sent and deleted (auto-delete or manually) point disappears from the map ✔️

---

Problems with removing response

Use case:

1. User fills a few forms and adds points.
2. User opens the new map view
3. User sees a correct number of points shown on map on new map view.
4. User clicks on point on map and removes response for geopoint widget
5. User saves changes. User is moved to map view
6. User sees that removed point is still marked and number of points shown on map is not updated

Expected result

• Points wich were removed are not visible on map
• Number of points does not count removed responses

---

Use case 2:

1. User fills a few forms and adds points.
2. User takes look at the new map and leaves it
3. User goes to saved form and removes response for geopoint widget
4. User takes a look at the new map
5. User sees that removed point is still marked and number of points shown on map is not updated

Expected result

• Points wich were removed are not visible on map
• Number of points does not count removed responses

---

Problem with moving backward option

Use case 3:

1. Moving backward is unselected
2. User fills a few forms and adds points.
3. User opens a new map view and clicks on point
4. User is moved to empty hierarchy list with a possibility to open General Settings

Expected result:

• Points are not clickable if moving backward is unselected

Moving backward is unselected	Edit Saved Form is unselected

[mmarciniak90]

@opendatakit-bot unlabel "needs testing"

[lognaturel]

Thank you so much for the thorough review, @mmarciniak90!

You dispelled my doubts 😄

Glad to hear it! I do think it's a very valid question and if you come up with any other concerns, please share them.

Todo:

• Investigate leaks
• Make sure the tap surface extends beyond the icon
• Icon is not horizontally centered in button on Android 9.0, 6.0, 4.4
• Removing point response should remove map point
• Moving backwards setting should not result in blank hierarchy - this turned out to be a bug introduced in 0a6bb3d. Now if an instance is opened for editing and moving backwards is disabled, the user should be jumped directly to the last question that was viewed. It should also be able to open for viewing just like if moving backwards is enabled. Does that behavior look right to you, @mmarciniak90? You said the points should not be clickable. I can see an argument for that -- the edit saved form button is generally not available when moving backwards is disabled -- but I don't think that's the behavior that users would want or expect. As long as users can't jump around and edit, I think it's ok. It will be important to verify this with the identify user feature.

The two bugs you found should be fixed. 🙏 I haven't made much progress on the other three smaller issues you pointed out. Given that those three are relatively small, I think we could file them as separate issues? Alternately, we can see what I can get done during my Monday.

For the tap surface, I think that adding an OutlinedButton style will likely address that. Does that sound right, @seadowg? Or maybe the entire vertical space below the button should be for the secondary action?

Another quick thing -- both forms and instance databases have been added to. If you haven't already, please check downgrading.

[seadowg]

@lognaturel I'm guessing the problem is that the whole list item is clickable and that surrounds the button? It looks like the style of the button is all good (Outlined.Icon is correct). What might be a good idea is to use a layout similar to AudioViewImageTextLabel - the right hand buttons live in their own layout (with padding) that is not part of the tappable area for selects etc. The trick is to have the left hand content expand to fill the space if the layout with the buttons (in this case just the map button) is gone so that the whole thing remains clickable when the buttons don't need to be there.

[lognaturel]

@obdulia-losantos It'd be great to get your feedback on this initial version of the feature! This forum post has a screen capture as well as information on how to download a beta with the functionality.

[lognaturel]

9cf5a01 makes all space above and below the map outlined button as well as 16dp to the left and to the right tappable. I'm not proud of the approach but I looked into negative margins and they seem to be tolerated (see e.g. https://stackoverflow.com/a/10673572/137744).

I could not reproduce the horizontal center issue after that change. Perhaps it has been addressed?

The leak that is now happening frequently is the following which I can't figure out:

In org.odk.collect.android:v1.25.0-beta.0-78-g9cf5a01b9:3590.
* androidx.lifecycle.ReportFragment has leaked:
* LocationManager$ListenerTransport.!(this$0)!
* ↳ LocationManager.!(mContext)!
* ↳ ContextImpl.!(mAutofillClient)!
* ↳ FormMapActivity.mFragments
* ↳ FragmentController.mHost
* ↳ Activity$HostCallbacks.mFragmentManager
* ↳ FragmentManagerImpl.mAdded
* ↳ ArrayList.elementData
* ↳ array Object[].[0]
* ↳ ReportFragment

[mmarciniak90]

@lognaturel I focused on upgrade more today and I checked more cases than before.
I did not notice the crash and the update was successful.

I noticed one thing that can be good to improve:
Use case:

1. The old version is installed, eg. 1.24
2. Few forms with Geopoint widget are available
3. User upgrade to a version with these changes
4. Forms available before upgrade do not have a new MapActivity. Only forms downloaded after upgrade have new MapActivity.

Expected result:
Forms available before upgrade have new MapActivity after the upgrade.
User does not need to remove and download again forms to use the new functionality.

[lognaturel]

Forms available before upgrade do not have a new MapActivity. Only forms downloaded after upgrade have new MapActivity.

I agree that this is really annoying. Unfortunately, I think trying to parse existing forms and instances to add the functionality is too risky. This is because form definitions can take a long time to parse depending on their length and the capacity of the device used. There could also potentially be several form definitions. That means we'd have to be sure to provide user feedback that this process is ongoing. We'd probably also want to let the user continue filling out forms. There are then lots of possible failure states to handle and I think it's cleaner to say that only forms downloaded with version 1.25 or later will be mapped. I'm disappointed because it is a big limitation but I don't see a good alternative.

[smoyte]

Is this (need to re-parse forms) something that is likely to come up again in the future? What about adding a manual "re-parse forms" action in the settings. It wouldn't be as smooth as just doing it automatically but for folks who don't have the option to easily re-download their forms, it could help. We could potentially/eventually pair this with a "new release notes" modal that lets users know about new features when they are released. Might that be a good practice anyway?

[lognaturel]

Is this (need to re-parse forms) something that is likely to come up again in the future?

That's a good question and concept, @smoyth. I don't see any immediate other need for parsing existing forms and instances in this way but I can imagine that we'll want to similarly cache other things from the form definition in the future. Do you see it as a must-have for this initial release of the map feature or do you think it's something we could add on if we get feedback that it's essential?

We could potentially/eventually pair this with a "new release notes" modal that lets users know about new features when they are released. Might that be a good practice anyway?

This has come up at various points and I'm not really sure how to reason about it because often the people who need to know about the new features are form designers or trainers rather than end users. Even in the case of a feature like this, it's a bit hard to know what to say because it's highly dependent on the structure of forms that will be put on the device.

[smoyte]

Definitely not a must-have for my purposes. I don't have a good sense of what other users might be needing...

[mmarciniak90]

Tested with success

Verified on Android: 10.0, 9.0, 8.1, 7.0, 6.0, 5.1, 4.4, 4.2

Final test cases:

• upgrade
• Map buttons are shown in the form list for forms with geopoint question
• Map buttons are NOT shown in the form list for forms with GeoPoint in repeat section
• Area near the map icon is clickable
• The markers are small balloons colored according to form status (blue, purple, red, green)
• The map button launches a MapActivity that shows a marker for each form instance
• The "new instance" button on the map launches the FormEntryActivity
• When a form is completed, the app returns to the MapActivity with the newly added point
• When response for GeoPoint is removed, point disappears from the map
• Clicking a map marker navigates to viewing/editing the saved form instance
• If Edit Saved Form is made unavailable from the admin menu, instances can’t be edited
• If the instance is sent and deleted (auto-delete or manually) point disappears from the map
• If Moving backward is unselected from the admin menu, instances can’t be edited

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"