-
Notifications
You must be signed in to change notification settings - Fork 19
feat(#507): add setgeopoint action #594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: de7cdbe The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/xforms-engine/src/lib/reactivity/createInstanceValueState.ts
Outdated
Show resolved
Hide resolved
| if (action) { | ||
| registerAction(context, setValue, action); | ||
| if (action.element.nodeName === 'odk:setgeopoint') { | ||
| context.parent.rootDocument.getBackgroundGeopoint()?.then((value: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary instance is the parent, not the context itself :(
|
@garethbowen This code isn't pretty and still experimental, but I wanted to get a version working first and get your thoughts from the engine side. In the screenshot, the xform-engine is working; it sets the value when the event runs, and the value comes from a promise.
|
…et-geometry-function # Conflicts: # README.md # feature-matrix.json
|
Testing is complete, and I need to clarify a few small questions with the team later before moving this PR to |
garethbowen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic. Looks like a nice generalisation of the setvalue code!
I haven't tried running it yet (hopefully will do that later today), but thought I'd get this feedback to you ASAP.
packages/scenario/test/actions-events/set-geopoint-action.test.ts
Outdated
Show resolved
Hide resolved
| const { latitude, longitude, accuracy } = position.coords; | ||
| const altitude = position.coords.altitude ?? 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can do this...
| const { latitude, longitude, accuracy } = position.coords; | |
| const altitude = position.coords.altitude ?? 0; | |
| const { latitude, longitude, accuracy, altitude=0 } = position.coords; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't catch the case when altitude is null (mdn ref)
packages/xforms-engine/src/lib/reactivity/createInstanceValueState.ts
Outdated
Show resolved
Hide resolved
| private watcherId: number | null = null; | ||
| private activePromise: Promise<string> | null = null; | ||
| private bestPoint: GeolocationPosition | undefined = undefined; | ||
| private options: PositionOptions = { enableHighAccuracy: true, maximumAge: 0 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find anywhere maximumAge is used - should this be removed or implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the audit attributes ? I didn't consider them as part of the scope. I think we can implement that in a separate PR
I've removed maximumAge because it's zero by default anyway.
| this.complete(resolve, reject, 'No geolocation readings received within the time window.'); | ||
| }, timeoutSeconds * 1000); | ||
| }); | ||
| return this.activePromise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running this in the browser there's an imperfect behaviour if you happen to submit the form in under 20 seconds then the geopoint value is empty. I guess not many forms will be that simple but still, it's a shame that if the browser has returned a perfectly good point, that we throw it away. I think an improved version would set the value every time the watchPosition returns a better accuracy point so we get the best possible data when the submit button is pressed.
This could also lead to interesting custom validations, for example, requiring a certain level of accuracy.
The reason I think it's worth looking into this now is because it will require an API change because Promises can't be resolved twice. Instead it would follow the event emitter pattern, just like watchPosition does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 20s timeout is implemented on the client side, so the client can also resolve that promise immediately to assign the point before submission.
I remember we talked about using emitters, but having the engine subscribe to client-side emitters feels a bit backwards. It could lead to a deep callback chain. Promises are much cleaner for this 'one-and-done' data flow.
The challenge is just catching those last few promises or reactive updates (Solid.js) that fire right before submission. I've added a helper that just flushes the event loop to make sure the engine has fully 'digested' all the reactivity before we capture the payload. It’s tiny and feels like a good safety net, guaranteeing we’re submitting the most accurate state possible. The engine might eventually provide this helper utility as "settle()", but for now, adding it on the client side (lib/async/event-loop.ts) is a non-invasive way to avoid race conditions.
This is one of rare cases I've seen this kind of helpers make sense :)
|
I just realized I missed questions in this PR descriptions, my apologies! We've discussed but I want to make sure the answers are captured here. High-level, the location capture triggered by events other than form load are only relevant for mobile usage, not for desktop. When a form is being filled on a mobile device/laptop, those events can be used to ensure that background location capture is tied to a specific moment in time like after the person filling out the form has confirmed who lives in a certain structure.
Here's a sample form demonstrating the intended usage: The idea is that each repeat can represent a record and the location of the thing represented by that record can be captured in the background without user intervention. For usage like this, we'd expect the data collector to be moving so it's important that a new location capture session starts when the event is triggered.
The intended usage of In general, I would expect the client implementation of the action not to care what event triggers it. However, there are some considerations around what should happen if the same action is triggered again. For example,
Collect does the first: https://github.com/getodk/collect/blob/d574284887015412eb48e9be58f591e19491c3fe/collect_app/src/main/java/org/odk/collect/android/location/client/MaxAccuracyWithinTimeoutLocationClientWrapper.java#L50 I don't think it's terribly important to align on this because it is an edge case. Hopefully all this aligns with what you're already thinking. Happy to talk through more of the intended behavior if helpful. I'm sure you've already looked at this but also cross-linking the Collect implementation for reference:
|
|
@garethbowen This is ready for review! |

Closes #507
ODK XForms spec: https://getodk.github.io/xforms-spec/#events
User-facing docs: start-geopoint (odk:setgeopoint with odk-instance-first-load)
These questions were already answered by the team; see the summary here.
Does it need to show an indicator just like Collect does? (I think we decided not add it, but double-checking)
The
odk-new-repeatis fired when a new repeat instance is created. At that moment, geolocation is requested once. Subsequent repeat instances reuse this same geolocation reading, so all instances end up with the same location value. That's expected, correct?The
background-geopointis anodk:setgeopointwithxforms-value-changed, that, as per the specs:3.1 The location is requested once, and subsequent value changes reuse the same geolocation reading (or empty string if no permissions given) for all questions. Is that correct? This is a lighter and less complex option than requesting the location every time a question value changes.
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
Displays error message when it cannot access the geolocation
Displays multiple messages in the top error banner
Tests all events except new repeat instance
test-all-events.mp4
Tests new repeat instance event
test-setgeopoint.mp4
Why is this the best possible solution? Were any other approaches considered?
Reuses the existing action structure, sets the value without re-evaluating the expression (since it comes from the geolocation provider), and reuses geopoint codecs for validation.
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?
Existing features will continue to work as usual with no changes.
Forms with setgeopoint will now request geolocation in the background, which may affect low-end phones by increasing resource usage (battery, CPU), as expected. The geolocation request stops after 20 seconds, in accordance with the specs.
Do we need any specific form for testing your changes? If so, please attach one.
There are 3 forms in the common package to test the feature. I also have a form that includes all events in one form:
Form: setgeopoint with events
What's changed