GeoTrace: Add a selectable GPS accuracy threshold. #2891

zestyping · 2019-02-19T11:27:25Z

Overview

This PR lets the user specify an accuracy threshold for GPS points. In automatic recording mode, points will only be recorded if they meet the accuracy requirement.

The selection is made in the trace settings dialog. In order to prevent this dialog from getting too unwieldy, the existing interval spinners (number and time unit) are combined into one spinner (time interval). Thus, there is one spinner for the interval and one spinner for the accuracy threshold.

With this change, the user gets feedback on the GPS status at the top of the screen:

Whether the GPS sensor is searching or has acquired a fix
GPS accuracy
Whether the accuracy meets the threshold

And the user gets feedback on the collection status at the bottom of the screen:

Number of points collected so far
Whether recording is active
Recording mode, auto or manual
Automatic recording interval and accuracy threshold, if applicable

Guidance for reviewers

This PR is based on top of #2890. Only the commits after fc28c04 are part of this change.

I recommend reviewing the commits one at a time.

40a27ac brings all the settings into a consistent pattern: each piece of UI state has a corresponding member field (recordingMode, recordingActive, intervalIndex); there are listeners on the relevant UI elements that update these member fields; and the updateUi() method updates all the UI elements from the member fields. This ensures that the member fields are kept in sync with the UI at all times; thus, saving and restoring state is simply a matter of putting the member fields into the instance state bundle. The items in the interval spinner are generated programmatically so that we don't have to do more translations when we change the available options.
691636c replaces magic numbers with meaningful constants.
d886925 introduces filtering by accuracy.
cb88981 adds a spinner for the accuracy threshold, following the same pattern as the one for the interval.
89aedca and 843447b add the lines of text above and below the map to give feedback to the user.
The remaining commits fix linter complaints.

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

I've tested GeoTrace manually in the emulator and on a phone:

Simulating changes in GPS accuracy level
Setting accuracy requirement to None
Setting accuracy requirement to a positive value
Changing the collection interval and confirming it is used correctly
Rotating the screen while automatic collection is occurring and confirming collection continues at the proper interval
Backspacing to delete points and confirming the status text is updated correctly
Clearing the trace and starting over with different settings

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

I had a few options for how to use string resources in the settings dialog. I considered using a string array for all the items in the interval and accuracy spinners, but ultimately decided to use <plural> so that it would be easiest to translate into other languages. Consequently, the items in the spinners are generated programmatically; this makes it easy to reconfigure the spinners without requiring new translations.

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?

This provides new functionality that's very useful for tracing roads, drains, and other linear geographic features.

This PR touches the existing code for selecting the recording interval and changes the way the recording interval is stored, so that's a regression risk. It does greatly simplify how the recording interval is handled, though, which should make the code easier to maintain. The change actually had the side effect of fixing a subtle bug that causes automatic recording to stop when the screen is rotated (because the recording is restarted before the recording settings are restored). Now that the trace settings are handled in a uniform way, this problem has gone away.

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

This can be tested using the GeoTrace widget in "All widgets".

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

getodk/docs#968

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-02-19T11:34:03Z

Here's a screenshot of the updated settings dialog:

And this is the recording screen, showing the GPS feedback and collection feedback:

cooperka · 2019-02-19T22:15:14Z

Tip: use https://github.com/opendatakit/collect/pull/2891/files/fc28c04..67d020b to view just the changes from this PR.

cooperka

Looks good in general, mostly just code style feedback! This does some nice cleanup.

collect_app/src/main/res/layout/geotrace_dialog.xml

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

cooperka · 2019-02-19T22:29:53Z

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

+        );
+        locationStatus.setBackgroundColor(Color.parseColor(
+            location == null ? NO_LOCATION_COLOR :
+                acceptable ? ACCEPTABLE_LOCATION_COLOR : UNACCEPTABLE_LOCATION_COLOR


Same nit about colons

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

collect_app/src/main/res/layout/geotrace_dialog.xml

cooperka · 2019-02-19T22:40:48Z

collect_app/src/main/res/values/strings.xml

+    <string name="geotrace_collection_status_paused">Points recorded: %d</string>
+    <string name="geotrace_collection_status_manual">Points recorded: %d (manual recording)</string>
+    <string name="geotrace_collection_status_auto">Points recorded: %1$d (auto recording, %2$d s)</string>
+    <string name="geotrace_collection_status_auto_accuracy">Points recorded: %1$d (auto recording, %2$d s, %3$d m)</string>


I suggest making these strings more explicit, e.g. auto recording every X s, Y m accuracy. Have you tested what happens if they wrap to two lines in other languages?

Good catch! I fixed the layout to allow for the status lines to have variable height.

I'd like to try to get the English text to fit on one line, so I shortened your suggestion to recording every X sec; it seems like every X sec should be enough to imply that it happens automatically.

I also realized that it's better for the status line to say every 10 min instead of every 600 sec, so I added format strings for that case. As a result, the ? : expression for this string has gotten pretty big—I hope the formatting makes it clear enough?

zestyping · 2019-02-20T02:46:31Z

Thanks for the quick review, @cooperka! I think I've addressed all your suggestions.

cooperka

LGTM, though I didn't have time to test the actual behavior today. Would be great if @grzesiek2010 could weigh in too.

zestyping · 2019-02-20T20:06:53Z

Rebased onto the latest 2890.

To view all the changes from only this PR in one diff (i.e. only 2891, not 2890):
https://github.com/opendatakit/collect/pull/2891/files/fff268d6..11342e3

…gMode.

…odk#2886).

…y of values.

zestyping · 2019-02-20T20:16:25Z

Rebased onto master and resolved a merge conflict in colors.xml.

To confirm that the current change is identical to the one that @cooperka approved (cb9e426, see above) except for the merge in colors.xml, you can compare git diff 50b1d9f cb9e426 to git diff 3752395 44f72cb.

To view all the changes from only this PR in one diff (i.e. only 2891, not 2890):
https://github.com/opendatakit/collect/pull/2891/files/3752395..44f72cb

cooperka · 2019-02-21T01:59:04Z

@opendatakit-bot label "needs testing"

mmarciniak90 · 2019-02-21T15:36:15Z

@zestyping I did not check a lot of cases because these changes are based on #2890 which needs to be fixed. I just want to let you know that the scroll is visible when it is not expected because all buttons are visible

mmarciniak90 · 2019-02-22T09:14:31Z

Tested with success

Tested on Android: 4.2, 4.4, 6.0, 7.0, 8.1

Verified cases:

green/red header
disabled GPS
manual/automatic recording
light/dark theme
~80 points
rotating
Google/OSM Maps
accuracy sets to None

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

yanokwa added this to the v1.21 milestone Feb 19, 2019

cooperka suggested changes Feb 19, 2019

View reviewed changes

cooperka reviewed Feb 19, 2019

View reviewed changes

yanokwa modified the milestones: v1.21, v1.20 Feb 20, 2019

zestyping force-pushed the ping/accuracy-threshold branch from 67d020b to 297c98f Compare February 20, 2019 01:40

cooperka approved these changes Feb 20, 2019

View reviewed changes

yanokwa requested a review from grzesiek2010 February 20, 2019 04:38

zestyping force-pushed the ping/accuracy-threshold branch from cb9e426 to 4699d37 Compare February 20, 2019 20:04

zestyping added 17 commits February 20, 2019 12:13

Geo: prevent a crash on rapid screen rotation (fixes getodk#2856).

953d777

GeoTrace: Consolidate UI button updates into updateUi().

0eada04

GeoTrace: Rename modeActive to recordingActive, traceMode to recordin…

56f3bab

…gMode.

GeoTrace: Simplify away the playCheck and beenPaused logic (fixes get…

cb35aed

…odk#2886).

GeoTrace: Add a backspace button to delete last point.

9e36410

GeoShape: Add a backspace button to delete last point.

4417b82

Move previousState member up into BaseGeoMapActivity.

e7374e2

Use app:srcCompat instead of android:src for button icons.

446a7eb

Replace many backspace PNG files with a single vector file.

7ac3eab

Fix enabling of backspace button in GeoShape.

3752395

GeoTrace: Select recording interval with a single Spinner and an arra…

fdf4337

…y of values.

GeoTrace: Define constants for recording modes.

f79d26a

GeoTrace: Filter points using a fixed accuracy threshold.

150ae6d

GeoTrace: Add a selection spinner for the accuracy threshold.

741ad0c

GeoTrace: Show GPS status above the map.

dfe02e1

GeoTrace: Show the recording mode and collection status.

a2f5f07

Fix PMD complaints.

8b92fb9

zestyping added 10 commits February 20, 2019 12:13

Fix lint complaints.

20d357c

Remove unused resources as required by lint.

9d0a250

Adjust a string to make it friendlier.

a5448c7

Fix XML indentation and formatting.

e326a89

Move color constants into color resources.

d253dc5

Factor out GeoTrace.populateSpinner().

00dd385

Improve code formatting.

02dc25a

Add a little padding in the GeoTrace settings dialog.

dab88dd

GeoTrace: Improve collection status string.

e6c4ac4

Fix inconsistent XML indentation.

44f72cb

zestyping force-pushed the ping/accuracy-threshold branch from 11342e3 to 44f72cb Compare February 20, 2019 20:15

grzesiek2010 approved these changes Feb 20, 2019

View reviewed changes

getodk-bot added the needs testing label Feb 21, 2019

zestyping mentioned this pull request Feb 21, 2019

Feb 18 rollup (rapid rotation fix, button state fix, backspace button) #2890

Merged

3 tasks

yanokwa added the high priority Should be looked at before other PRs/issues label Feb 21, 2019

yanokwa merged commit 7fd83b7 into getodk:master Feb 21, 2019

zestyping mentioned this pull request Feb 21, 2019

Remove unnecessary padding below GeoTrace buttons. #2904

Merged

3 tasks

getodk-bot added behavior verified and removed needs testing labels Feb 22, 2019

yanokwa removed the high priority Should be looked at before other PRs/issues label Feb 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GeoTrace: Add a selectable GPS accuracy threshold. #2891

GeoTrace: Add a selectable GPS accuracy threshold. #2891

zestyping commented Feb 19, 2019 •

edited

Loading

zestyping commented Feb 19, 2019

cooperka commented Feb 19, 2019

cooperka left a comment •

edited

Loading

cooperka Feb 19, 2019

zestyping Feb 20, 2019

cooperka Feb 19, 2019

zestyping Feb 20, 2019 •

edited

Loading

zestyping commented Feb 20, 2019

cooperka left a comment

zestyping commented Feb 20, 2019 •

edited

Loading

zestyping commented Feb 20, 2019 •

edited

Loading

cooperka commented Feb 21, 2019

mmarciniak90 commented Feb 21, 2019

mmarciniak90 commented Feb 22, 2019

GeoTrace: Add a selectable GPS accuracy threshold. #2891

GeoTrace: Add a selectable GPS accuracy threshold. #2891

Conversation

zestyping commented Feb 19, 2019 • edited Loading

Overview

Guidance for 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 Feb 19, 2019

cooperka commented Feb 19, 2019

cooperka left a comment • edited Loading

Choose a reason for hiding this comment

cooperka Feb 19, 2019

Choose a reason for hiding this comment

zestyping Feb 20, 2019

Choose a reason for hiding this comment

cooperka Feb 19, 2019

Choose a reason for hiding this comment

zestyping Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

zestyping commented Feb 20, 2019

cooperka left a comment

Choose a reason for hiding this comment

zestyping commented Feb 20, 2019 • edited Loading

zestyping commented Feb 20, 2019 • edited Loading

cooperka commented Feb 21, 2019

mmarciniak90 commented Feb 21, 2019

mmarciniak90 commented Feb 22, 2019

zestyping commented Feb 19, 2019 •

edited

Loading

cooperka left a comment •

edited

Loading

zestyping Feb 20, 2019 •

edited

Loading

zestyping commented Feb 20, 2019 •

edited

Loading

zestyping commented Feb 20, 2019 •

edited

Loading