feat: implement manual gps coordinate entry screen #219

achou11 · 2024-04-03T20:13:05Z

Closes #164

Mostly ported over from Mapeo Mobile. Had to do a non-trivial amount of refactoring due to different setups for persisted state and differences in hooks though.

Mostly worried about any potential edge cases throughout the app that may be dependent on the manualLocation field of the observation. Tried my best to address the more notable areas (such as in the observation creation screen)

There are also a few small changes that are not strictly related to adding this feature but fix subtle bugs that one could encounter by using this.

Preview:

achou11 · 2024-04-03T20:14:08Z

src/frontend/hooks/persistedState/usePersistedDraftObservation/index.ts

@@ -76,6 +76,7 @@ const draftObservationSlice: StateCreator<DraftObservationSlice> = (
          metadata: {
            ...prevValue.metadata,
            position: props.position,
+            manualLocation: props.manualLocation,


super subtle but found out why updateObservationPosition() didn't update the manualLocation field when initially implementing this 🥲

achou11 · 2024-04-03T20:16:45Z

src/frontend/hooks/persistedState/usePersistedSettings.ts

+  manualCoordinateEntryFormat: 'utm',
  actions: {
    setCoordinateFormat: coordinateFormat => set({coordinateFormat}),
+    setManualCoordinateEntryFormat: coordinateFormat =>
+      set({manualCoordinateEntryFormat: coordinateFormat}),


in Mapeo Mobile, we use to store an individual value in async storage that pertained to the coordinate format used specifically for manual entry. I believe the context for this is that the user's chosen coordinate format setting for displaying is not necessarily the same as for manual input, hence the separation

achou11 · 2024-04-03T20:40:26Z

src/frontend/screens/ManualGpsScreen/index.tsx

+function entryCoordinateFormatSelector(
+  state: Parameters<Parameters<typeof usePersistedSettings>[0]>[0],
+) {
+  return state.manualCoordinateEntryFormat;
+}
+
+function observationValueSelector(
+  state: Parameters<Parameters<typeof usePersistedDraftObservation>[0]>[0],
+) {
+  return state.value;
+}


probably unnecessary but small easy-to-implement optimization when working with zustand selectors

achou11 · 2024-04-03T20:40:44Z

src/frontend/screens/ObservationEdit/LocationView.tsx

          </Text>
-          {!accuracy ? null : (
+          {coordinateInfo.accuracy === undefined ? null : (


subtle bug fix

achou11 · 2024-04-03T20:40:54Z

src/frontend/screens/ObservationEdit/LocationView.tsx


  return (
    <View style={styles.locationContainer}>
-      {!lat || !lon ? (
+      {coordinateInfo.lat === undefined || coordinateInfo.lon === undefined ? (


subtle bug fix

achou11 · 2024-04-03T20:41:21Z

src/frontend/screens/ObservationEdit/SaveButton.tsx

@@ -184,11 +184,12 @@ export const SaveButton = ({
      return;
    }

-    const hasLocation = value.lat && value.lon;
+    const hasLocation = value.lat !== undefined && value.lon !== undefined;


subtle bug fix

achou11 · 2024-04-03T20:42:22Z

src/frontend/screens/ObservationEdit/SaveButton.tsx

+      hasLocation &&
+      (locationSetManually ||
+        isGpsAccurate(value.metadata.position?.coords?.accuracy))


potentially subtle bug fix? noticed that it differs from Mapeo Mobile implementation:

https://github.com/digidem/mapeo-mobile/blob/0c0ebbb9ef2261e21cd1d1c8bd5ab2fe42017ea3/src/frontend/screens/ObservationEdit/SaveButton.js#L85

ErikSin

I know you talked about this before, but the heavy use of useEffect to track the global state is not ideal. I know that you more or less just copied this over for time sake, but perhaps we should create an issue to refactor this in the future.

Otherwise lets write the unit tests before we merge this.

ErikSin · 2024-04-29T18:27:54Z

src/frontend/screens/ManualGpsScreen/DdForm.tsx

+    getInitialCardinality('lat', initialCoordinates),
+  );
+  const [selectedLonCardinality, setSelectedLonCardinality] = React.useState(
+    getInitialCardinality('lon', initialCoordinates),


getInitialCardinality should be curried (read third comment in thread)

oh wow TIL - in this case, i don't need the currying because the intention is to return an initial value (not a function), but will update

addressed via 298c24b

ErikSin · 2024-04-29T18:40:52Z

src/frontend/screens/ManualGpsScreen/index.tsx

+            <Select
+              containerStyles={styles.selectContainer}
+              onChange={value => {
+                if (typeof value === 'number' || !isCoordinateFormat(value)) {


is there a reason for the number check?

hm good question - doesn't seem like it's needed

addressed via 556dabc

ErikSin · 2024-04-29T18:48:12Z

src/frontend/screens/ManualGpsScreen/index.tsx

+  const {setManualCoordinateEntryFormat} = usePersistedSettingsAction();
+  const {updateObservationPosition} = useDraftObservation();
+
+  React.useLayoutEffect(() => {


perhaps a slight and unnecessary optimization, but I think we can use useEffecthere. I don't think we need to block the rendering of the button because it is visually the same(its only functionally different).

addressed via 5b57199

src/frontend/screens/ObservationEdit/useMostAccurateLocationForObservation.ts

ErikSin · 2024-04-29T19:19:28Z

src/frontend/screens/ManualGpsScreen/UtmForm.tsx

it is possible to save when there is no East or North. This is returning a latLon of NaN.

for posterity i saved zone number as 8 and zone letter as 'S'

When trying to save the observation (in the observationEdit Screen), it throws an error.

thanks for catching. was hoping to avoid adding fixes for pre-existing bugs to this PR but was able to fix this pretty easily, so will include the changes for that in this PR

addressed via e79fb89

ErikSin · 2024-04-29T19:27:53Z

src/frontend/screens/ManualGpsScreen/index.tsx

+      if (convertedData.error) {
+        return ToastAndroid.showWithGravity(
+          convertedData.error.message,
+          ToastAndroid.LONG,
+          ToastAndroid.TOP,
+        );
+      }


I think we should also check if lat and lon are valid here. I know that we attempted to do that in each component BUT having one last check will avoid the issue that I found in UTM form

Addressed via e505753

expo · 2024-04-29T22:22:50Z

GitHub Build Trigger Failure

The build trigger failed with the following error:

eas.json not found in the repository.

Please check your GitHub app installation settings and your Expo project's GitHub settings to make sure you've configured everything correctly.

achou11

Addressed feedback and added some unit tests, which mostly test the various functions that are used to determine if the inputs in the various forms are valid or not. Not a full solution to testing this but think it's fine for now, especially if this screen sees a major refactor

achou11 · 2024-04-29T22:24:03Z

src/frontend/screens/ManualGpsScreen/index.tsx

+      if (convertedData.error) {
+        return ToastAndroid.showWithGravity(
+          convertedData.error.message,
+          ToastAndroid.LONG,
+          ToastAndroid.TOP,
+        );
+      }


Addressed via e505753

achou11 · 2024-04-29T22:24:34Z

src/frontend/screens/ManualGpsScreen/UtmForm.tsx

addressed via e79fb89

achou11 · 2024-04-29T22:24:49Z

src/frontend/screens/ManualGpsScreen/index.tsx

+  const {setManualCoordinateEntryFormat} = usePersistedSettingsAction();
+  const {updateObservationPosition} = useDraftObservation();
+
+  React.useLayoutEffect(() => {


addressed via 5b57199

achou11 · 2024-04-29T22:24:57Z

src/frontend/screens/ManualGpsScreen/index.tsx

+            <Select
+              containerStyles={styles.selectContainer}
+              onChange={value => {
+                if (typeof value === 'number' || !isCoordinateFormat(value)) {


addressed via 556dabc

achou11 · 2024-04-29T22:25:07Z

src/frontend/screens/ManualGpsScreen/DdForm.tsx

+    getInitialCardinality('lat', initialCoordinates),
+  );
+  const [selectedLonCardinality, setSelectedLonCardinality] = React.useState(
+    getInitialCardinality('lon', initialCoordinates),


addressed via 298c24b

expo · 2024-04-29T22:27:58Z

GitHub Build Trigger Failure

The build trigger failed with the following error:

eas.json not found in the repository.

Please check your GitHub app installation settings and your Expo project's GitHub settings to make sure you've configured everything correctly.

expo · 2024-04-29T22:41:10Z

GitHub Build Trigger Failure

The build trigger failed with the following error:

eas.json not found in the repository.

Please check your GitHub app installation settings and your Expo project's GitHub settings to make sure you've configured everything correctly.

achou11 requested a review from ErikSin April 3, 2024 20:13

achou11 force-pushed the 164/manual-gps-screen branch 4 times, most recently from 6e09e5c to d9f15ff Compare April 3, 2024 20:38

achou11 commented Apr 3, 2024

View reviewed changes

achou11 force-pushed the 164/manual-gps-screen branch 5 times, most recently from 4103662 to 83e41ca Compare April 16, 2024 20:31

achou11 force-pushed the 164/manual-gps-screen branch 4 times, most recently from c0b2e19 to 8de70c3 Compare April 24, 2024 20:26

feat: implement manual gps coordinate entry screen

78d37df

achou11 force-pushed the 164/manual-gps-screen branch from 8de70c3 to 78d37df Compare April 25, 2024 20:39

ErikSin requested changes Apr 29, 2024

View reviewed changes

achou11 added 8 commits April 29, 2024 15:47

remove unnecessary check

556dabc

update state initializers that use getInitialCardinality()

298c24b

use useEffect instead of useLayoutEffect

5b57199

fix issue with UtmForm when northing/easting not set

e79fb89

add final check for lat+lon before saving to draft observation

e505753

fix bug in dmsValuesAreValid() helper

841b77f

remove unused import

c4c2c67

add unit tests for various helper functions

1f3e48e

achou11 commented Apr 29, 2024

View reviewed changes

Merge branch 'main' into 164/manual-gps-screen

acff686

achou11 requested a review from ErikSin April 29, 2024 22:27

Merge branch 'main' into 164/manual-gps-screen

da0f9db

Merge branch 'main' into 164/manual-gps-screen

f5ce39a

achou11 merged commit 46d3e16 into main May 1, 2024
3 checks passed

achou11 deleted the 164/manual-gps-screen branch May 1, 2024 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement manual gps coordinate entry screen #219

feat: implement manual gps coordinate entry screen #219

achou11 commented Apr 3, 2024 •

edited

Loading

achou11 Apr 3, 2024

achou11 Apr 3, 2024

achou11 Apr 3, 2024

achou11 Apr 3, 2024

achou11 Apr 3, 2024

achou11 Apr 3, 2024

achou11 Apr 3, 2024

ErikSin left a comment

ErikSin Apr 29, 2024

achou11 Apr 29, 2024

achou11 Apr 29, 2024

ErikSin Apr 29, 2024

achou11 Apr 29, 2024

achou11 Apr 29, 2024

ErikSin Apr 29, 2024

achou11 Apr 29, 2024

ErikSin Apr 29, 2024

ErikSin Apr 29, 2024

ErikSin Apr 29, 2024 •

edited

Loading

achou11 Apr 29, 2024

achou11 Apr 29, 2024

ErikSin Apr 29, 2024

achou11 Apr 29, 2024

expo bot commented Apr 29, 2024

achou11 left a comment

achou11 Apr 29, 2024

achou11 Apr 29, 2024

achou11 Apr 29, 2024

achou11 Apr 29, 2024

achou11 Apr 29, 2024

expo bot commented Apr 29, 2024

expo bot commented Apr 29, 2024

feat: implement manual gps coordinate entry screen #219

feat: implement manual gps coordinate entry screen #219

Conversation

achou11 commented Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikSin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikSin Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

expo bot commented Apr 29, 2024

achou11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

expo bot commented Apr 29, 2024

expo bot commented Apr 29, 2024

achou11 commented Apr 3, 2024 •

edited

Loading

ErikSin Apr 29, 2024 •

edited

Loading