-
Notifications
You must be signed in to change notification settings - Fork 19
feat(#507): add setgeopoint function #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: 91f6799 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 |

Closes #507
ODK XForms spec: https://getodk.github.io/xforms-spec/#events
User-facing docs: start-geopoint (odk:setgeopoint with odk-instance-first-load)
It's WIP - Questions to clarify:
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