Merge Google and OSM GeoPoint activities into one activity using the common MapFragment interface. #2776

zestyping · 2019-01-10T04:35:45Z

Overview

This pull request merges GeoPointMapActivity (which was Google-specific) and GeoPointOsmMapActivity (which is OSM-specific) into a single activity, GeoPointMapActivity, using the common MapFragment interface added in #2465. The MapFragment interface is extended to support point markers, which are used by the activity.

Guidance to reviewers

This PR will probably be easiest to review by going through the commits one by one; most of the commits are straightforward. The biggest change is in c69bf44, which migrates the new activity to use MapFragment instead of GoogleMap. Because MapFragment is a higher-level interface, some of the logic and state in the GeoPointMapActivity got simplified away. If anything in there is confusing, please feel free to reach out to me with any questions—I'm also happy to talk it over with you. Sometimes a short conversation can really help speed up a review.

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

I've manually tested, in both Google and OSM modes:

Zooming to the phone's GPS location
Zooming to an existing point
Allowing the first GPS fix to determine the point
Clearing the point with the "trash" button
Placing the point by long-pressing, and confirming that the viewport doesn't move
Moving the point by dragging, and confirming that the viewport shifts to center on the new point
Placing the point with the "add marker" button, and confirming that the viewport zooms to the point
Saving the point
Launching with a saved point and confirming that the point cannot be dragged
Confirming that dragging and long-pressing is allowed only in "placement-map" mode, not in "map" mode

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

There is currently a great deal of code duplicated between GeoPointMapActivity and GeoPointOsmMapActivity, including some vestigial code, member fields that are trivial or unused, and duplicated layouts. This solution eliminates most of that duplication.

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 only user-visible behaviour changes should be:

Tapping on the marker in OSM used to pop up an empty text bubble. It doesn't do that anymore.
When the activity is launched with a previously saved point, marker dragging and long-press are initially disabled; the user must click the "trash" button to discard the point before adding/long-pressing/dragging a new marker. Previously, the instructions "Long press to place mark to tap add marker button" would be displayed even though the user cannot do these things; now the instructions are not displayed.

Do we need any specific form for testing your changes? If so, please attach one.

Any form that uses the GeoPoint widget with "placement-map" or "map" appearance will do. Here's the one I've been using:
geo-widgets.zip

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

No.

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 · 2019-01-10T04:50:03Z

Rebased onto master.

…Poly to PolyFeature.

…old and new GeoPoint activity.

zestyping · 2019-01-10T05:06:04Z

@opendatakit-bot label "needs-testing"

getodk-bot · 2019-01-10T05:06:05Z

ERROR: Label "needs-testing" does not exist and was thus not added to this pull request.

zestyping · 2019-01-10T05:06:30Z

@opendatakit-bot label "needs testing"

zestyping · 2019-01-10T05:08:56Z

The CircleCI failure looks to me like an intermittent network error:

Could not GET 'https://jcenter.bintray.com/net/sourceforge/saxon/saxon/9.1.0.8/saxon-9.1.0.8.pom'. Received status code 502 from server: Bad Gateway

Is there a way to re-run the checks?

grzesiek2010 · 2019-01-15T10:35:32Z

collect_app/src/main/java/org/odk/collect/android/map/OsmMapFragment.java

+        double sd = 0;
+        try {
+            sd = Double.parseDouble(marker.getSubDescription());
+        } catch (NumberFormatException e) { /* ignore */ }


I think logging at least Timber.w would be good here? We usually don't ignore exceptions.

Okay! I also fixed a similar ignored exception in GoogleMapFragment.

grzesiek2010 · 2019-01-15T10:39:30Z

collect_app/src/main/java/org/odk/collect/android/map/GoogleMapFragment.java

+
+    @Override public @Nullable MapPoint getMarkerPoint(int featureId) {
+        MapFeature feature = features.get(featureId);
+        if (feature instanceof MarkerFeature) {


Don't you like ternary operators? I would prefer it here?

Sure! Sometimes I use them; in this case it didn't occur to me. Fixed.

grzesiek2010 · 2019-01-15T10:40:50Z

collect_app/src/main/java/org/odk/collect/android/map/OsmMapFragment.java

+
+    @Override public @Nullable MapPoint getMarkerPoint(int featureId) {
+        MapFeature feature = features.get(featureId);
+        if (feature instanceof MarkerFeature) {


The same like above, ternary operator would be good here.

grzesiek2010 · 2019-01-15T10:49:18Z

@zestyping

I found only some minor things to improve so I'm going to add needs testing label. Please let me know once you fix them.

EDIT: since #2797 is based on this branch let's test only that pr.

…pen).

zestyping · 2019-01-15T21:46:42Z

Thanks for the review, @grzesiek2010! I've fixed the things you pointed out.

zestyping · 2019-01-15T21:46:47Z

@kkrawczyk123 @mmarciniak90 Is it helpful to have the old activities still accessible for comparison testing (with the ability to switch between new and old using the volume buttons), or can I remove them so that this PR is in its final state ready for merging?

…corded point and not the GPS location).

zestyping · 2019-01-16T10:24:48Z

@grzesiek2010 Hey, I discovered two bugs—so sorry I missed these before your review!

If there's an existing point, the activity was only using the latitude and longitude and ignoring the other two fields. So simply opening and then re-saving a point would lose information.
The previous activity's behaviour disables long-press when the activity starts with an existing point. The new activity didn't match this behaviour.

I've now fixed these in the recent commits. Could you have another quick look?

…zoom button.

zestyping · 2019-01-17T08:57:10Z

Found and fixed two more small issues. This'll be the last, I hope!

The onStop() method turns off GPS tracking while the activity is in the background, so there needs to be an onStart() method that reactivates the GPS.
The MapHelper helper field in GeoPointMapActivity shadows the helper field in the base class, BaseGeoMapActivity, and BaseGeoMapActivity.onSaveInstanceState crashes when helper is null.

zestyping · 2019-02-12T07:38:52Z

This pull request is now obsolete; it has been rolled into #2865.

Roll up PRs #2776, #2797, #2806, #2838, #2839, #2855

zestyping force-pushed the ping/geopoint-with-mapfragment branch from 254d2e9 to 915d30a Compare January 10, 2019 04:43

Make a backup copy of GeoPointMapActivity, as GeoPointGoogleMapActivity.

8493cd0

zestyping force-pushed the ping/geopoint-with-mapfragment branch from 915d30a to e2d17f9 Compare January 10, 2019 04:45

zestyping force-pushed the ping/geopoint-with-mapfragment branch from a1d0bb7 to c674fe9 Compare January 10, 2019 04:54

zestyping added 7 commits January 9, 2019 20:56

Add "animate" parameter to all map viewport movement methods.

c609aba

Rename MapFragment.getPointsOfPoly to getPolyPoints; rename Draggable…

f1aee90

…Poly to PolyFeature.

Add MapFragment support for marker (single-point) features.

ac7c445

Add MapFragment support for a drag-end listener.

568823a

Add MapFragment support for getting the location provider.

627a9df

Migrate GeoPointMapActivity to use the MapFragment interface.

c69bf44

Allow manual testers to use the volume buttons to switch between the …

c02a7ec

…old and new GeoPoint activity.

zestyping force-pushed the ping/geopoint-with-mapfragment branch from c674fe9 to 090bcff Compare January 10, 2019 04:58

getodk-bot added the needs testing label Jan 10, 2019

yanokwa removed the needs testing label Jan 10, 2019

zestyping force-pushed the ping/geopoint-with-mapfragment branch from 090bcff to 121257f Compare January 14, 2019 11:20

Fix GeoPointGoogleMapActivityTest.

2ca7710

zestyping force-pushed the ping/geopoint-with-mapfragment branch from 121257f to 2ca7710 Compare January 14, 2019 20:10

Fix SpotBugs complaint.

3b89c93

zestyping mentioned this pull request Jan 14, 2019

Improve Geo{Point,Trace,Shape}Activity tests by adding a FakeLocationClient #2797

Closed

3 tasks

yanokwa added this to the v1.19 milestone Jan 14, 2019

grzesiek2010 reviewed Jan 15, 2019

View reviewed changes

grzesiek2010 added the needs testing label Jan 15, 2019

grzesiek2010 removed the needs testing label Jan 15, 2019

zestyping added 2 commits January 15, 2019 13:43

Log warnings if Marker objects contain invalid data (should never hap…

c0b6c30

…pen).

Use ternary operators as suggested in code review.

305d5c3

Rename locationFromIntent to pointFromIntent (it is related to the re…

330a306

…corded point and not the GPS location).

zestyping added 3 commits January 16, 2019 02:28

Preserve the incoming point's alt and sd fields.

6eeb00a

Rename enableShowLocation to enableZoomButton because it enables the …

93f30fd

…zoom button.

Disable long-pressing while point is initially set from the intent.

ee12532

zestyping force-pushed the ping/geopoint-with-mapfragment branch from 8cbb742 to ee12532 Compare January 16, 2019 10:31

zestyping mentioned this pull request Jan 17, 2019

Refactoring Geo-based Activities #507

Closed

zestyping added 3 commits January 17, 2019 00:48

Resume GPS tracking after GeoPointMapActivity is stopped and restarted.

29d96fb

Prevent NPE on activity stop in BaseGeoMapActivity.

ac36f6f

Remove unused errorDialog field.

a792aa1

yanokwa modified the milestones: v1.19, v1.20 Jan 22, 2019

Remove old GeoPointGoogleMapActivity and GeoPointOsmMapActivity.

4fc6968

zestyping mentioned this pull request Feb 12, 2019

Roll up PRs #2776, #2797, #2806, #2838, #2839, #2855 #2865

Merged

3 tasks

zestyping closed this Feb 12, 2019

zestyping mentioned this pull request Feb 12, 2019

Grey bar is not visible in GeoPoint widget when user opens widget again with saved location #2857

Closed

yanokwa added a commit that referenced this pull request Feb 18, 2019

Merge pull request #2865 from zestyping/ping/feb-12-rollup

c6e7872

Roll up PRs #2776, #2797, #2806, #2838, #2839, #2855

lognaturel mentioned this pull request Jun 18, 2020

Geo: Google Map and OSM use different location providers #2997

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge Google and OSM GeoPoint activities into one activity using the common MapFragment interface. #2776

Merge Google and OSM GeoPoint activities into one activity using the common MapFragment interface. #2776

zestyping commented Jan 10, 2019 •

edited

Loading

zestyping commented Jan 10, 2019

zestyping commented Jan 10, 2019

getodk-bot commented Jan 10, 2019

zestyping commented Jan 10, 2019

zestyping commented Jan 10, 2019

grzesiek2010 Jan 15, 2019 •

edited

Loading

zestyping Jan 15, 2019

grzesiek2010 Jan 15, 2019 •

edited

Loading

zestyping Jan 15, 2019

grzesiek2010 Jan 15, 2019

grzesiek2010 commented Jan 15, 2019 •

edited

Loading

zestyping commented Jan 15, 2019

zestyping commented Jan 15, 2019

zestyping commented Jan 16, 2019 •

edited

Loading

zestyping commented Jan 17, 2019 •

edited

Loading

zestyping commented Feb 12, 2019

Merge Google and OSM GeoPoint activities into one activity using the common MapFragment interface. #2776

Merge Google and OSM GeoPoint activities into one activity using the common MapFragment interface. #2776

Conversation

zestyping commented Jan 10, 2019 • edited Loading

Overview

Guidance to reviewers

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

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

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?

Do we need any specific form for testing your changes? If so, please attach one.

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

Before submitting this PR, please make sure you have:

zestyping commented Jan 10, 2019

zestyping commented Jan 10, 2019

getodk-bot commented Jan 10, 2019

zestyping commented Jan 10, 2019

zestyping commented Jan 10, 2019

grzesiek2010 Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

zestyping Jan 15, 2019

Choose a reason for hiding this comment

grzesiek2010 Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

zestyping Jan 15, 2019

Choose a reason for hiding this comment

grzesiek2010 Jan 15, 2019

Choose a reason for hiding this comment

grzesiek2010 commented Jan 15, 2019 • edited Loading

zestyping commented Jan 15, 2019

zestyping commented Jan 15, 2019

zestyping commented Jan 16, 2019 • edited Loading

zestyping commented Jan 17, 2019 • edited Loading

zestyping commented Feb 12, 2019

zestyping commented Jan 10, 2019 •

edited

Loading

grzesiek2010 Jan 15, 2019 •

edited

Loading

grzesiek2010 Jan 15, 2019 •

edited

Loading

grzesiek2010 commented Jan 15, 2019 •

edited

Loading

zestyping commented Jan 16, 2019 •

edited

Loading

zestyping commented Jan 17, 2019 •

edited

Loading