Add `allow-mock-accuracy` for non-default `geopoint` and `geoshape`/`geotrace` #4857

seadowg · 2021-10-13T14:37:31Z

Closes #4768
~~Blocked by #4842~~

Every kind of geo/location widget should now support allow-mock-accuracy. Again, the refactor here (moving the geo code to geo) is nice and gradual so going through commit by commit is a good idea.

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

New tests.

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

This takes the same approach as the last PR so not a lot to discuss here. This time most of the work is focused on moving GeoPointMapActivity and then adding support for the allow-mock-accuracy param in the same way we did for GeoPointActivity.

It's feeling to me like there's probably a maps module to be extracted at some point that contains the MapFragment, MapPoint and the providers/implementations (or possibly multiple modules for different implementations). That feels like an extra step we could take next time we're playing around in this area however.

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?

Good to check the map appearances of geopoint with the different map providers - Mapbox should require special attention because it uses a different code path for grabbing the device's location. geoshape and geotrace need checked as well.

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

The form used in the previous PR is a good starting point. Testing the specific support in this PR can be done by adding the map appearances to the form.

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

collect_app/src/main/java/org/odk/collect/android/activities/FormMapActivity.java

grzesiek2010 · 2021-10-19T14:50:53Z

geo/src/main/java/org/odk/collect/geo/GeoPointMapActivity.java

@@ -70,8 +68,16 @@
    public static final String LOCATION_STATUS_VISIBILITY_KEY = "location_status_visibility";
    public static final String LOCATION_INFO_VISIBILITY_KEY = "location_info_visibility";

+    public static final String EXTRA_LOCATION = "gp";


Why you didn't move this one to geo/Constants.kt too like EXTRA_READ_ONLY and EXTRA_DRAGGABLE_ONLY?

This one is only relevant to GeoPointMapActivity. I tried to keep it so Constants was only defining the shared ones.

geo/src/main/java/org/odk/collect/geo/GeoPointMapActivity.java

grzesiek2010 · 2021-10-19T15:24:28Z

collect_app/src/main/AndroidManifest.xml

-            tools:replace="android:theme"/>
-        <activity android:name=".activities.GeoPointMapActivity" />
-        <activity android:name=".activities.GeoPolyActivity" />
+            tools:replace="android:theme" />


What is it for?

It's need so that the build tools now which android:theme to use. Otherwise, there is a conflict between the geo and the collect_app manifests.

geo/src/test/java/org/odk/collect/geo/GeoPolyActivityTest.java

seadowg · 2021-10-20T09:32:50Z

geo/src/main/java/org/odk/collect/geo/GeoActivityUtils.kt

+internal object GeoActivityUtils {
+
+    @JvmStatic
+    fun requireLocationPermissions(activity: Activity) {


I don't like that we check this in the Activity and in ActivityGeoDataRequester. However, I don't want to get into untangling the interaction with WaitingForDataRegistry here to make sure that it's safe to just check this in the Activity.

seadowg · 2021-10-20T10:43:51Z

collect_app/src/main/java/org/odk/collect/android/activities/FormEntryActivity.java

+    /**
+     * Should use {@link org.odk.collect.android.externaldata.ExternalAppsUtils} instead
+     */
+    @Deprecated


I didn't want to add the work/risk of replacing current usages of ANSWER_KEY in this PR but also wanted to make it clear we have a better option now.

…ferent classes

These could eventually move to their own `maps` module but moving the map fragment implementations would be very cumbersome and is probably worth doing a single dedicated piece of work.

…ment with faked location client

We'll need to handle Mapbox in a different way as it doesn't use the same location abstraction

kkrawczyk123 · 2021-10-21T11:09:09Z

@seadowg I can see a problem with the display of Google Maps when using all Geo widgets (point, trace, shape) on all of my devices. It loads properly for display points on map. Issue does not occur for Mapbox and OSM. See gif:

The issue does not occur on master.

seadowg · 2021-10-21T11:31:47Z

@kkrawczyk123 I'll look into that

For some reason this needs to be a singleton or Google Maps ends up just being blank. Not sure why yet.

seadowg · 2021-10-21T13:12:22Z

@kkrawczyk123 fixed!

@grzesiek2010 it looks like the MapProvider needs to be a singleton. I've noted this on the class now to hopefully avoid the same mistake being made again, but you might want to take a look and see if there's anything you think we need to address there.

kkrawczyk123 · 2021-10-21T15:48:53Z

@seadowg I have tried that form https://test.getodk.cloud/#/projects/308/forms/external_device_geopoint/versions with Fake GPS app and my understanding is that fake location should be used only in the first question where mock accuracy=true but I can see that it is used also in the second question where mock accuracy=false. Is it an issue or did I misunderstand something?

seadowg · 2021-10-21T18:57:36Z

@kkrawczyk123 the fake location will still be used, but its accuracy should be zero when allow-mock-accuracy is false (or absent). Is that not what you're seeing? The idea is just to remove accuracy from fake locations but allow people that have to use fake locations to be able to keep it if they trust it.

kkrawczyk123 · 2021-10-22T07:24:36Z

@seadowg I can see accuracy 0 for both when allow-mock-accuracy is true and false:

I assumed that there should be a difference but maybe I am wrong.

seadowg · 2021-10-22T08:59:14Z

@kkrawczyk123 is this for all the geo widgets or just one of them? I'll check this out my side as well as it sounds like something it wrong.

seadowg · 2021-10-22T09:24:34Z

@kkrawczyk123 I'm seeing the correct behaviour using mock the GPS Connector app in simulation mode for geopoint (and maps appearances) and geotrace - I get points with 0 accuracy when allow-mock-accuracy is false and non zero when it's true.

Are you potentially testing with manual placement (moving the map to somewhere and then placing a point)? This always results in zero accuracy as the location doesn't come from gps.

kkrawczyk123 · 2021-10-22T09:59:42Z

@seadowg I agree that accuracy difference is visible between both questions for geopoint with map view, geotrace and geoshape widgets but my question was about geopoint without map view - exact form that was attached in PR.

seadowg · 2021-10-22T10:05:59Z

@kkrawczyk123 yeah I'm using that as well and still getting accuracy in the "Location (allows mock)" question. Is that happening every time? And it doesn't happen for the other geo widgets? What app are you using to fake the location? I'm wondering if the app is maybe giving you zero accuracy - I just got a 0.01m out of the simulation in the one I was using.

kkrawczyk123 · 2021-10-22T10:14:37Z

@seadowg I can see accuracy 0 for both questions. It is happening every time for me. @mmarciniak90 Are you seeing the same thing on your devices? I am using the same app "Fake GPS" for that widget and others and there is a difference as I mentioned above it works for widgets with map view. I have set accuracy in the app to be different than 0 - I have 1 on one device and 20 on another and the same results - accuracy 0 for questions with and without allow-mock accuracy when filling a form.

seadowg · 2021-10-22T10:15:49Z

@kkrawczyk123 could link me to the exact app?

kkrawczyk123 · 2021-10-22T10:16:31Z

sure, https://play.google.com/store/apps/details?id=com.lexa.fakegps&hl=en&gl=US

seadowg · 2021-10-22T10:22:30Z

@kkrawczyk123 I get accuracy with that app. Changing its "Accuracy" setting is reflected in Collect as well (although never exactly for some reason, but that could be the app).

mmarciniak90 · 2021-10-22T10:24:35Z

Are you seeing the same thing on your devices?

Unfortunately, I have the same result. I also use the same app to generate fake GPS.

kkrawczyk123 · 2021-10-22T10:25:26Z

@seadowg That's weird. What app have you been using before? We could try with it

seadowg · 2021-10-22T15:36:04Z

Ok turns out I'd got myself confused about the release state of the allow-mock-accuracy changes in pyxform. It's out on https://getodk.org/xlsform/ but not released for Central yet. My bad! I've shared a form created with the newest pyxform with QA for them to test with.

kkrawczyk123 · 2021-10-22T16:12:23Z

Finally tested with success!
Verified on Androids 5.1 and 11.

Verified cases:

form from https://test.getodk.cloud/#/projects/306/forms/external_device_geopoint
Fake GPS app
GPS Connector app

regression checks on:

Geo widgets with/without map
GeoTrace, GeoShape widgets
placement by tapping, manual mode, automatic mode
display submissions on map
submissions with different statuses
Google maps
OSM
Mapbox
offline layers
light mode, dark mode
Audit location in the background
screen rotation
minimize app

mmarciniak90 · 2021-10-22T16:15:04Z

Tested with success on Android 8.1, 9.0, 10.0

seadowg added the blocked label Oct 13, 2021

seadowg force-pushed the external-gps-2 branch from cfecc58 to 7cbad4b Compare October 14, 2021 10:37

seadowg changed the title ~~Add allow-mock-accuracy for geopoint with maps and placement-map appearances~~ Add allow-mock-accuracy for non-default geopoint and geoshape/geotrace Oct 14, 2021

seadowg force-pushed the external-gps-2 branch 2 times, most recently from eea0b73 to 1d0afa7 Compare October 19, 2021 09:27

seadowg removed the blocked label Oct 19, 2021

seadowg marked this pull request as ready for review October 19, 2021 10:08

seadowg requested a review from grzesiek2010 October 19, 2021 10:08

grzesiek2010 requested changes Oct 19, 2021

View reviewed changes

seadowg commented Oct 20, 2021

View reviewed changes

seadowg added 19 commits October 20, 2021 12:07

Push previousState down to subclasses as it's use is differnet in dif…

2ae0ef9

…ferent classes

Remove extra permission check from geo activities

58cab49

Remove unneeded super class

1278d90

Separate GeoPointMapActivity from CollectAbstractActivity

51fe955

Decouple GeoPointMapActivity from MapPreferencesFragment

98ed778

Extract interface for creating map fragments

0931da8

Move maps interfaces to geo module

9066129

These could eventually move to their own `maps` module but moving the map fragment implementations would be very cumbersome and is probably worth doing a single dedicated piece of work.

Move activity extras constants to geo package

394e48c

Move GeoPointMapActivity to geo package

01b9714

Fix test imports

bd5b19b

use fake MapFragment to test GeoPointMapActivity instead of real frag…

2a99e73

…ment with faked location client

Move to using ActivityScenario in GeoPointMapActivityTest

9882f67

Move GeoPointMapActivityTest to geo module

76a28b1

Add pass-through setter for retain mock accuracy

f0eade7

We'll need to handle Mapbox in a different way as it doesn't use the same location abstraction

Support retaining mock accuracy in GeoPointMapActivity

c0eac92

Use same location sanitizer code for mapbox as everything else

83d136c

Support retaining mock accuracy in MapboxMapFragment

542949f

Add note to dagger setup

5aa42ef

Remove dependency on CollectAbstractActivity from GeoPolyActivity

b9f73da

grzesiek2010 approved these changes Oct 20, 2021

View reviewed changes

grzesiek2010 added the needs testing label Oct 20, 2021

seadowg added 4 commits October 21, 2021 13:24

Use singelton MapProvider instead of new one each time

0004a36

For some reason this needs to be a singleton or Google Maps ends up just being blank. Not sure why yet.

Don't inject dependency module dependencies in application object

4019f19

Add note about MapProvider needing to be a singleton

3cc60f1

Optimize imports

f0298d7

seadowg force-pushed the external-gps-2 branch from 257b258 to f0298d7 Compare October 22, 2021 15:36

mmarciniak90 added behavior verified and removed needs testing labels Oct 22, 2021

grzesiek2010 merged commit 2aa071a into getodk:master Oct 22, 2021

seadowg deleted the external-gps-2 branch October 22, 2021 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `allow-mock-accuracy` for non-default `geopoint` and `geoshape`/`geotrace` #4857

Add `allow-mock-accuracy` for non-default `geopoint` and `geoshape`/`geotrace` #4857

seadowg commented Oct 13, 2021 •

edited

Loading

grzesiek2010 Oct 19, 2021

seadowg Oct 19, 2021

grzesiek2010 Oct 19, 2021

seadowg Oct 19, 2021

seadowg Oct 20, 2021

seadowg Oct 20, 2021

kkrawczyk123 commented Oct 21, 2021

seadowg commented Oct 21, 2021 •

edited

Loading

seadowg commented Oct 21, 2021 •

edited

Loading

kkrawczyk123 commented Oct 21, 2021

seadowg commented Oct 21, 2021 •

edited

Loading

kkrawczyk123 commented Oct 22, 2021

seadowg commented Oct 22, 2021

seadowg commented Oct 22, 2021

kkrawczyk123 commented Oct 22, 2021

seadowg commented Oct 22, 2021

kkrawczyk123 commented Oct 22, 2021

seadowg commented Oct 22, 2021

kkrawczyk123 commented Oct 22, 2021

seadowg commented Oct 22, 2021

mmarciniak90 commented Oct 22, 2021

kkrawczyk123 commented Oct 22, 2021

seadowg commented Oct 22, 2021

kkrawczyk123 commented Oct 22, 2021

mmarciniak90 commented Oct 22, 2021

Add allow-mock-accuracy for non-default geopoint and geoshape/geotrace #4857

Add allow-mock-accuracy for non-default geopoint and geoshape/geotrace #4857

Conversation

seadowg commented Oct 13, 2021 • edited Loading

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.

Before submitting this PR, please make sure you have:

grzesiek2010 Oct 19, 2021

Choose a reason for hiding this comment

seadowg Oct 19, 2021

Choose a reason for hiding this comment

grzesiek2010 Oct 19, 2021

Choose a reason for hiding this comment

seadowg Oct 19, 2021

Choose a reason for hiding this comment

seadowg Oct 20, 2021

Choose a reason for hiding this comment

seadowg Oct 20, 2021

Choose a reason for hiding this comment

kkrawczyk123 commented Oct 21, 2021

seadowg commented Oct 21, 2021 • edited Loading

seadowg commented Oct 21, 2021 • edited Loading

kkrawczyk123 commented Oct 21, 2021

seadowg commented Oct 21, 2021 • edited Loading

kkrawczyk123 commented Oct 22, 2021

seadowg commented Oct 22, 2021

seadowg commented Oct 22, 2021

kkrawczyk123 commented Oct 22, 2021

seadowg commented Oct 22, 2021

kkrawczyk123 commented Oct 22, 2021

seadowg commented Oct 22, 2021

kkrawczyk123 commented Oct 22, 2021

seadowg commented Oct 22, 2021

mmarciniak90 commented Oct 22, 2021

kkrawczyk123 commented Oct 22, 2021

seadowg commented Oct 22, 2021

kkrawczyk123 commented Oct 22, 2021

mmarciniak90 commented Oct 22, 2021

Add `allow-mock-accuracy` for non-default `geopoint` and `geoshape`/`geotrace` #4857

Add `allow-mock-accuracy` for non-default `geopoint` and `geoshape`/`geotrace` #4857

seadowg commented Oct 13, 2021 •

edited

Loading

seadowg commented Oct 21, 2021 •

edited

Loading

seadowg commented Oct 21, 2021 •

edited

Loading

seadowg commented Oct 21, 2021 •

edited

Loading