Define the MapFragment interface; factor its implementation out of both GeoShape activities.

[zestyping]

This pull request factors out a common interface between the Google Map SDK and the OSMDroid Map SDK, moving us close to the goal of having a single GeoShapeActivity that supports both map SDKs.

The new interface is MapFragment.

GeoShapeGoogleMapActivity is now an activity with no GoogleMap dependencies; instead it calls GoogleMapFragment, a Google-specific implementation of MapFragment.

GeoShapeOsmMapActivity is now an activity with no osmdroid dependencies; instead it calls OsmMapFragment, an OSM-specific implementation of MapFragment.

GeoShapeGoogleMapActivity and GeoShapeOsmMapActivity are almost identical; the final step is to make them identical, rename them to GeoShapeActivity, and celebrate!

Before we can do that, we need to verify that the new activities work and have the same behaviour as before. To facilitate comparison testing, the original GeoShapeGoogleMapActivity and GeoShapeOsmMapActivity are still present and available in the app (but renamed as GeoShapeOldGoogleMapActivity and GeoShapeOldOsmMapActivity). In both cases, the old activity is started whenever the ringer is on, and the new activity is started when the ringer is off. That way, you can easily test behaviour in the app, and switch between the two activities to compare them just by pressing the volume buttons to put the phone in silent or ringing mode. The text label at the top of the screen shows you whether you're using the old activity or the new one.

Guidance to reviewers

I recommend starting with MapFragment first, as most of the interesting design decisions are there.

The next step would be looking at how GeoShapeGoogleMapActivity has changed to use MapFragment, and then finally the implementation classes, GoogleMapFragment and OsmMapFragment.

Guidance to testers

This change affects the GeoShape widget (both in Google mode and OSM mode).

To bring up the old activity, make sure your ringer is on (volume up) and then open the GeoShape widget. To bring up the new activity, make sure your ringer is off (volume down until silent), and then open the GeoShape widget. You can use the volume buttons to flip between the old and the new activity to make quick comparisons (close the widget with the "Save" button or the back button, press the volume buttons to turn silent mode on or off, then open the widget again).

What we want to verify: The new activity should behave exactly the same as the old activity, in both Google mode and OSM mode, with only the following intentional changes:

1. With the old activities, if a polygon already exists, you can move existing points but you cannot long-press to add new points. In the new activities, you can long-press to add new points.
2. With the old activities, if a polygon already exists, then the OSM activity opens up initially zoomed in to show the polygon but the Google activity does not. With the new activities, if a polygon already exists, both the OSM activity and the Google activity open up zoomed in to show the polygon.
3. With the old activities, if you start with no points, add two points, tap the save button (which generates an error message), and then add a third point, you get only two sides of a triangle instead of a complete three-sided triangle. This is a bug. With the new activities, this sequence of steps will produce a normal three-sided triangle, as it should.

[lognaturel]

Is it necessary to pass in the context? Can the fragment always call getActivity()?

[zestyping]

Good catch! I've fixed this.

[lognaturel]

@grzesiek2010, it would be great to get your high-level thoughts on this since you've spent the most time in this code recently. Do you see any issues with the MapFragment interface or have any questions about the design decisions?

I had a chance to have a brief voice conversation about this with @zestyping. I asked him about the use of the features map in the MapFragment and featureId in several of the interface methods. I initially thought he was envisioning future functionality where a map could have multiple features but he explained it was more to solve the problem of needing to support multiple feature types (point, trace, shape) and not needing to impose a limit on clients to call addDraggableShape only once. He'll add a little more background on that in comments.

@mmarciniak90, when you have a chance, it would be very helpful to get a comprehensive check on whether the behavior between the new and old shape activities is the same. 🙏

[grzesiek2010]

I think this constructor should be private so that only the CREATOR field can access. Am I right?

[zestyping]

Yes, good idea! Fixed.

[grzesiek2010]

The code looks good I think @mmarciniak90 can perform some tests and you can go ahead.

[kkrawczyk123]

I have already started some testing. It looks good so far - switching between old and new activities works as described and I didn't recognize any regression with geoshape widget and OpenStreetMap SDK. Unfortunately, I encountered the problem with Google maps as they are not visible for me, I can see a Google logo and empty space and buttons. I think that it might be something wrong with my API configuration, I generated API key on google clouds, add in there package name and SHA-1 certificate fingerprint on restriction section. I have also added API key number AndroidManifest.xml file and then build an app. I tried to figure it out but for now, I didn't succeed. @lognaturel, do you have any ideas what I can be possibly missing in the configuration? This is not connected with this pr as I have the same result on the master branch. I am able to publish submission to google sheet so my configuration must be not that bad. Maybe I am not aware of something which I should do to use it. It looks like I am a bit block with testing this pr right now.

[lognaturel]

It sounds like you followed all the instructions linked to from https://github.com/opendatakit/collect#using-apis-for-local-development. I tried to start the process over with a new project to try the full process but it looks like that now requires a billing account? @yanokwa is this something you know about?

I was then able to grab a key I had previously created at https://console.cloud.google.com/google/maps-apis/apis/maps-android-backend.googleapis.com. I first tried it restricted and it doesn't work. Then I went through the restriction process with the SHA1 and it still didn't work. The docs say it could take some time to propagate so I tried again after 10 minutes but still no map.

@zestyping How did you get your key working?

[zestyping]

I've made some more progress and updated this pull request. This now includes a new implementation for the OSM activity as well as the Google Map activity, so we are extremely close to having a single GeoShape activity now.

@mmarciniak90 @kkrawczyk123 Thanks for starting to test this! I'm sorry that I don't know how to solve your API key problem, but to make testing possible, I'm attaching my own build of the APK here. This is a debug build that uses my personal Google Maps API key, so it should work for you. We're ready now for you to test this and verify that:

• The new GeoShape activity in OSM mode behaves the same as the old GeoShape activity in OSM mode (except for the intentional differences noted in the PR description above)
• The new GeoShape activity in Google mode behaves the same as the old GeoShape activity in Google mode (except for the intentional differences noted in the PR description above)
• There aren't any new bugs in the new activities.

You can use the volume buttons to put the phone in silent / non-silent mode to switch between the new / old activities, respectively. The bar on top will tell you which activity you're getting.

Here's that APK:
collect-a1c8ff0-ping-key.zip

GitHub wouldn't let me upload an .apk file, so I've packed it inside a .zip. Open the .zip file and you'll get the .apk.

[kkrawczyk123]

@zestyping @lognaturel I was able to test it with attached apk but I couldn't on my own build. In a meantime, I noticed an issue which occurs on master too so reported it as a separate one in #2492.

I have noticed a few issues connected with the new version of the geoshape widget:

• Zoom to current location on Google maps doesn't work. I can see zoom to the water, but of course I am not working in the middle of the ocean, when I zoom out the map I can see a correct point of my location so it is probably the zoom issue. It failed on all Android version on which I am testing.

• When OSM mode is chosen app crashes after clicking on Start Geoshape button. I also reproduced it on all Androids from 4.1 to 8.1.

• I have noticed behavior difference when do actions like: click on View or Change GeoShape, clear, save and return to the widget. On the old version geoshape results are cleared and you can Start GeoShape again, but on the new version Geoshape records are not cleared. So it should be probably considered which way is better.

• When I already saved a polygon and I want to return to the widget, old version allows me for that. The new version displays dialog "discard changes and return to ODK?" cancel/discard even if there were no changes just a preview - does it make sense to display it in this case?

@opendatakit-bot unlabel "needs testing"

[lognaturel]

Thank you @zestyping for posting the APK and @kkrawczyk123 for your careful testing!

The first two bullets look like bugs in the new implementation.

I have noticed behavior difference when do actions like: click on View or Change GeoShape, clear, save and return to the widget. On the old version geoshape results are cleared and you can Start GeoShape again, but on the new version Geoshape records are not cleared. So it should be probably considered which way is better.

Good catch, this is a subtle difference! I think this comes from a difference in when the constraint that requires 3 points is evaluated. The old version seems to still save the polygon even if it's invalid. The new version does not save an invalid polygon.

Overall, I find the new version more correct. For example, it means you can't save a 2-point polygon whereas in the old version you can. The clearing case is kind of annoying, though. It's true that the user could long-press on the question to clear it (same as with any other question) but that's not so discoverable and not everyone knows about it. I think maybe we should allow a 0-point polygon or polyline, just not a 1-point line or 1 or 2 point polygon. @zestyping, what do you think? 0-point seems like it should be valid (to be clear, this is an issue with the validations as they exist, not with any new code but I think it would be ok to make the change here, ideally in its own commit).

When I already saved a polygon and I want to return to the widget, old version allows me for that. The new version displays dialog "discard changes and return to ODK?" cancel/discard even if there were no changes just a preview - does it make sense to display it in this case?

The old version makes no check before letting the user back out of a shape collection. That's bad because a user could have collected a very careful shape and then lose it all! So I think the check on back button is a very good one. It's true that there are two cases -- one where changes were actually made and one where none were. @zestyping what do you think of adding state to keep track of whether any changes were made and only prompting if there was a change?

[zestyping]

Thank you @kkrawczyk123 for catching these issues!

1. Zoom to current location in Google version: This one is showing inconsistent behaviour on my phone because it seems to have a lot of trouble getting a GPS fix. I'm going to keep working on this when I have a better signal or I can try it on another phone. Stay tuned.

2. Crash on startup in OSM version: This happened when there are zero points in the polygon (because it was trying to close the polygon by copying the first point to the end). So glad you caught this—I've fixed it now. I failed to catch this in my testing because I was always loading the form with an existing polygon, and never saved a cleared polygon because it doesn't let you save a 0-point polygon. And that's a nice segue to the next item...

3. I agree with your suggestion, @lognaturel! I think it makes sense to be able to save a 0-point polygon. I've made this change.

4. Yes, prompting only when the result is actually changed is the right thing to do. In the change I've just pushed, I'm keeping the original string value of the form result, and comparing it to the result that would be saved. I know this seems like more computational work than simply keeping a flag around; the reason I decided to go this way is that (a) the user can move points by dragging, which happens at the MapFragment level without the activity knowing, so maintaining a flag would require adding another callback to notify the activity; and (b) in some sense it's slightly more correct to compare the final result, because if you start with nothing and then add points and then clear them, pressing Back won't actually lose any information and it probably shouldn't prompt you.

The commits I've just pushed include the fixes for items 2, 3, and 4 above, and also complete the merge into a single activity, GeoShapeActivity. So now the original activities are present with their original names (GeoShapeGoogleMapActivity and GeoShapeOsmMapActivity) and the new activity has its new merged name, GeoShapeActivity.

Here's a new APK of this build with my key, in case anyone wants to try it out. Note that the zoom-to-location issue is still not addressed yet, though.
collect-051a7a3-ping-key.zip

[lognaturel]

I'm keeping the original string value of the form result, and comparing it to the result that would be saved

Oh, I just hadn't considered that for some reason. I agree it's better than a flag. 👍

Once you have a chance to look into the zoom issue and fix pmd problems breaking the build, we can do QA and review pass in preparation for merge. 🎉

[zestyping]

With these latest commits, all the various linter complaints are addressed (or suppressed with good reason), and the Google Map zoom issue is fixed.

The cause of the zoom problem was that the activity did an animated setCenter immediately followed by an animated setZoom, and because the first animation was not finished, the second animation simply preserved the current center while changing the zoom. The MapFragment interface now has a method zoomToPoint that adjusts both center and zoom in a single animation.

Note also one small behaviour change: the old Google activity would zoom to the current location at zoom level 17, whereas the old OSM activity would zoom to the current location at zoom level 15. The new activity splits the difference and zooms to the current location at zoom level 16.

This is an APK build with the zoom issue fixed, and with the old activities still available in case we need to do any further comparison testing:
collect-d735a06-ping-key.zip

This is a candidate final APK build with the old activities removed, with the code in a state ready for merging to master:
collect-aaeefed-ping-key.zip

[mmarciniak90]

Hi @zestyping! I looked on your fix.
I compared current behavior with behavior on v1.16.0 or v1.16.4
I like your improvements like:

• adding a new point if a polygon already exists
• activity is opened up and zoomed to a polygon if a polygon already exists,
• three-sided triangle is created if toast was previously visible

Unfortunately, I noticed also a few cases which do not work as expected:

• location point icon is not clickable until GPS is not recognized even if I added a polygon, so I was not able to zoom to added feature
• localization values are less accuracy

v1.16.0	pr/2465

• OSM map shows the whole world two or more times
  

[mmarciniak90]

@opendatakit-bot unlabel "needs testing"

[zestyping]

Thank you very much @mmarciniak90! I will investigate the issues you found.

[zestyping]

Hi, I'm back from Tanzania and have pushed new commits to address the issues that @mmarciniak90 found in her last round of testing. Specifically:

• Location point icon is not clickable: I was only able to replicate this problem by opening the activity with no points in the polygon, and then adding the first point with a long press; in this situation, the zoom-to-location button remains disabled, so I assume it's the case that was tested. I've fixed this so that the button becomes enabled when you add the first point. In the case where the polygon already has points in it, the button is already enabled.

• Coordinate values in result have lower precision: This was actually an intentional change; it seemed pointless to have 14 decimal places on GPS coordinates, because 5 decimal places is sufficient to get to 1-meter precision, and 6 decimal places is sufficient for 10-cm precision. I can revert it if it's important, though to me it makes more sense this way. @lognaturel, what do you think?

• OSM map shows the whole world two or more times: I assume this is only in the case where the view first opens with no points in the polygon. I've made two changes to address this: (a) the map now opens with a more reasonable default view, when no points have been entered; and (b) I've set a zoom limit for the map so you cannot zoom out beyond the point where the whole world fits on the screen.

As usual, I've made an APK that contains my Google API key so you can test these changes. Here it is:
collect-7d0e0ff-ping-key.zip

[zestyping]

My apologies—these changes broke the tests that rely on the GoogleMap. Unfortunately, it looks like Google Play Services is unavailable during Robolectric testing, which means there's no GoogleMap at all during tests. To get around this, I had to stub out more of the implementation during testing. This is kind of messy and I don't like it, but I haven't found another solution; at least these are all checks for map == null, which also insulates against any crash when trying to use a GoogleMapFragment before it's initialized.

Here's the fix, again with a new APK:
collect-666e4ba-ping-key.zip

[lognaturel]

Coordinate values in result have lower precision:

😬I always find these kinds of things tough. I agree with you that all those decimal digits are a bit silly. I tend to be pretty conservative about those kinds of changes now, though, and wouldn't be surprised if some folks somehow rely on the specific number of digits for their analysis. I've been burned by this kind of seemingly trivial change enough times that I would tend to leave it as-is unless there's a strong argument against. If we do make the change, it should at least be consistent between all the geo types.

[mmarciniak90]

@zestyping, thanks for the update.
I do not know what's happen but the app crashes when it find my location when I use OSM.
It causes testing problematic.

@lognaturel, so results should have high precision, as before. Am I right?

@opendatakit-bot unlabel "needs testing"

[lognaturel]

I can also consistently reproduce the crash:

09-10 21:41:10.877 23448-24275/org.odk.collect.android E/AndroidRuntime: FATAL EXCEPTION: Thread-5319
    Process: org.odk.collect.android, PID: 23448
    android.view.ViewRootImpl$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views.
        at android.view.ViewRootImpl.checkThread(ViewRootImpl.java:7665)
        at android.view.ViewRootImpl.invalidateChildInParent(ViewRootImpl.java:1156)
        at android.view.ViewGroup.invalidateChild(ViewGroup.java:5033)
        at android.view.View.invalidateInternal(View.java:13023)
        at android.view.View.invalidate(View.java:12987)
        at android.view.View.setEnabled(View.java:7308)
        at org.odk.collect.android.activities.GeoShapeActivity.onGpsLocationReady(GeoShapeActivity.java:166)
        at org.odk.collect.android.activities.GeoShapeActivity.bridge$lambda$2$GeoShapeActivity(GeoShapeActivity.java)
        at org.odk.collect.android.activities.GeoShapeActivity$$Lambda$8.onReady(Unknown Source)
        at org.odk.collect.android.map.OsmMapFragment.lambda$runOnGpsLocationReady$1$OsmMapFragment(OsmMapFragment.java:181)
        at org.odk.collect.android.map.OsmMapFragment$$Lambda$1.run(Unknown Source)
        at java.lang.Thread.run(Thread.java:818)

@lognaturel, so results should have high precision, as before. Am I right?

Yes but @zestyping will need to build a new APK for that.

[lognaturel]

Had a quick conversation with @zestyping about this and he can reproduce. In looking at this, we realized that the version of osmdroid we're using is very out of date. He is planning on updating that to see if it might address the issue.

[zestyping]

Hi @lognaturel, I investigated more closely and it turns out that it actually was the problem I previously identified—setting the center before setting the zoom level. My error was that I had fixed it in a different branch (the prospective work that I was doing on GeoTrace), and not in this branch. Doing the operations in the other order—setting the zoom level first, then setting the center—fixes the problem.

Since this is a much smaller change than upgrading to the next version of osmdroid, I'm inclined to go with this for this PR. Upgrading osmdroid is a major version number change (5 to 6), and will require changes to how we implement offline map tiles because the new API is incompatible. I haven't evaluated how much of a code change it will be, but at least it will require testing and validation of offline map tiles on OSM and I don't think we want to risk breaking that as part of this change.

My plan is to follow up by rebasing this branch as we discussed previously so it's ready for merging, and then produce another (hopefully final!) APK for testing this PR.

[lognaturel]

My plan is to follow up by rebasing this branch as we discussed previously so it's ready for merging, and then produce another (hopefully final!) APK for testing this PR.

Sounds great!

Upgrading osmdroid is a major version number change (5 to 6), and will require changes to how we implement offline map tiles because the new API is incompatible.

I'm glad we've noticed this now. Would be good to do as part of this overall work but I agree that if we can bump it, that's a good idea.

[zestyping]

@lognaturel @mmarciniak90 Okay, this is fixed, and ready for testing. I have also cleaned up the commits in this branch so that if it passes QA, then it is ready for merging.

This is an APK built from commit 0ad7052 plus my Google API key:
collect-0ad7052-ping-key.zip

[lognaturel]

Looks good and I have marked as needs testing. Please make sure all new files have a license header and that commit message length is capped at 70 (or even better, 50) with enough context to know what the change is about. The details can have any length.

[zestyping]

Added license headers and fixed commit messages. New commit is 6991ba6. (I don't know why Github is displaying it as the second-last commit above. It's a bug in Github; 6991ba6 is definitely the last commit.)

New APK built at 6991ba6 with my Google API key:
THIS ZIP FILE IS BAD DO NOT USE

[kkrawczyk123]

@zestyping inside that zip you attached above there is a build collect-9917329-ping-key.apk from the 5th of September.

[kkrawczyk123]

@zestyping, I have to ask if switching between new and old version of the widget by volume button has been turned off intentionally? as there is no new/old GeoShapeActivity title above map as it used to be before and behaviors are consistent.
The issue with multiplied map on OSM Maps is still visible but it is also visible on the master, so it is not caused by those changes.
I have checked out all issues that me and @mmarciniak90 have mentioned and everything seems to be fixed. I have't noticed any new issues.
cc @lognaturel
@opendatakit-bot unlabel "needs testing"
Verified on Androids: 4.1, 4.2, 4.4, 6.0 and 8.1

[zestyping]

@kkrawczyk123 Sorry! I don't know how that happened. You are correct, the zip file in the comment above contained the wrong APK.

Here is a zip file 6991ba6 containing the latest build, 6991ba6 with my Google API key.
collect-6991ba6-ping-key.zip

Which APK is the one that you tested?

[zestyping]

As of the latest commit, 6991ba6, the volume button feature has been removed intentionally. The old versions of the activities are not present anymore, because this is intended to be ready for production.

[lognaturel]

Let's wait to hear the final word from @kkrawczyk123 but I think she may actually have a gmaps key that she was able to try things out with. @mmarciniak90 and I are the ones who didn't have functional keys.

I finally took the time to review the current API key process and now do have a working key. I've also issued a PR to update the instructions: #2611. @kkrawczyk123, @mmarciniak90, it's an unrestricted key so I can share it with you privately if you can't set up a billing account.

Barring anything unexpected from @kkrawczyk123, I do think it's ready to merge! History is beautiful and the quick spin I took it on didn't show any behavior problems.

[kkrawczyk123]

@lognaturel my key didn't work, so I wanted to use attached apk but it was outdated. I thought "ooops" and I asked @mmarciniak90 if her key worked, and it did so she built an apk for me. I tested built with the newest commits and I also compared to old one - this is why I was asking about volume button as it changed but I didn't find any information about it in comments so I wasn't sure if it was intentional.

[lognaturel]

🎉 🎉🎉🎉🎉

[zestyping]

Wonderful!