feat(#249): Adds default geopoint question type #300
Conversation
|
| this.rows = Number(element.getAttribute('rows')); // ToDo: Make parser function, validate NaN | ||
| this.accuracyThreshold = Number(element.getAttribute('accuracyThreshold')); // ToDo: Make parser function, validate NaN | ||
| this.unacceptableAccuracyThreshold = Number(element.getAttribute('unacceptableAccuracyThreshold')); // ToDo: Make parser function, validate NaN |
There was a problem hiding this comment.
Not suggesting a change this early on. Just adding a minor "yes and" on these TODO comments for additional context. There are two gaps here, and they apply in a few other places too:
accuracyThresholdandunacceptableAccuracyThresholdare specified as decimals. We'll want to make sure that whatever parsing we do accounts for that (including potential float rounding errors).rowsdoesn't specify its type explicitly, but I think it's safe to assume it should be an integer. Same thing about parsing the type, different type to account for.
I wish we'd been tracking these cases sooner, because like I said there are others!
There was a problem hiding this comment.
There are two gaps here, and they apply in a few other places too:
@eyelidlessness Do you mean in the project in general or in this new PR code?
| @@ -11,10 +12,18 @@ export class InputControlDefinition extends ControlDefinition<'input'> { | |||
|
|
|||
| readonly type = 'input'; | |||
| readonly appearances: InputAppearanceDefinition; | |||
| readonly rows: number | null; | |||
There was a problem hiding this comment.
The rows implementation will be completed in this issue: #301
This reverts commit f447f13.
latin-panda
force-pushed
the
adds-default-geopoint-question-type
branch
from
414d3ee to
001fdf8
Compare
| // ToDo: if one is missing, it still need to know the position of the others. | ||
| // ToDo: Change the value type to object? or default to negative / zero number? | ||
| props.node.setValue([latitude ?? 0, longitude ?? 0, altitude ?? 0, accuracy ?? 0].join(' ')); |
There was a problem hiding this comment.
@eyelidlessness I'd like to know your opinion.
First question:
The order of these values in the string is essential for locating them correctly, avoiding confusion between altitude and accuracy.
I noticed that altitude isn't always available (e.g., when testing with WiFi on a desktop). To preserve the position, what do you think about setting a zero or null default value?
The other option is to assume that if only three values come in the string, the third one is accuracy and that altitude wasn't provided 🙁 (which needs to be explained in the documentation for anyone making a client that connects with the engine)
Second question:
We need to create a new codec that builds on the one in XPath you just merged and call it in getSharedValueCodec, correct? It looks like ValueTypePlaceholderCodec doesn’t handle this automatically and is just a placeholder.
There was a problem hiding this comment.
First question.....
All the JR imported tests in the scenario suite use a string with the four values (latitude, longitude, altitude, accuracy), and zero is used.
Maybe zero is the way to go 🤔; I haven't found anything explicit about this in the documentation of XForms
There was a problem hiding this comment.
I think it might be worth taking a step back. The intent of the engine/client interface—in this case InputNode—is to provide a representation of an <input> (in ODK XForms terms) which is most useful for clients, e.g. to provide a UI. The intent of ValueCodec is to decode readable values in that representation (i.e. InputNode.currentState.value), and to encode that representation of a value back to a "node value" (for submission, as serialized XML; for evaluation in XPath expressions).
It's generally up to us what that representation will be. So far, we've usually determined that by choosing a platform type with the closest semantics to the ODK XForms type: e.g. int <-> bigint, decimal <-> number.
There's an added wrinkle in this case, because there are currently two representations of the same type (geopoint) of value:
- In
@getodk/xpath, decoding ageopointnode value as an object which is pretty much a subset of the web standardGeolocationCoordinates - In
@getodk/xforms-engine, encoding ageopointnode value from a GeoJSON Point (see also the semi-official type definitions)
If we're going to share the encode/decode logic for these existing cases, it seems pretty likely (unless we have a good reason otherwise) that we're going to want a representation of the value which is either:
- One of the two we already use
- A combined interface which is a superset of both
- Some other representation entirely???
Option 1 pretty much designs itself:
import { Point as GeoJSONPoint } from 'geojson';
interface GeopointRuntimeValue {
readonly latitude: number;
readonly longitude: number;
readonly altitude: number; // Or `number | null`?
readonly accuracy: number; // Or `number | null`?
}
type GeopointInputValue =
| GepointRuntimeValue
| GeoJSONPoint;
class GeopointValueCodec extends ValueCodec<GepointRuntimeValue, GeopointInputValue> {
/**
* @todo Implementation will:
*
* 1. Decode `GeopointRuntimeValue`, pretty much reusing the logic in `@getodk/xpath`
* 2. Encode `GeopointInputValue` by detecting whether input is a `GeoJSON` point or a `GeopointRuntimeValue`, and then:
* - if the former, reuse logic for GeoJSON external secondary instances
* - if the latter, invert the decode logic
*/
}Coming back to the question of what to do when altitude isn't available, we can make it nullable (like it is in the web standard GeolocationCoordinates), or default to 0 ... in the runtime representation (GeopointRuntimeValue). There's trade-offs either way: nullable is closer to the platform interface, but we can't distinguish a node value's 0 as missing or sea level. In either case, we have to encode a missing value as 0 (as far as I know, the altitude substring isn't optional).
Option 2 (combined superset interface) would make decode a little more complex (assigning the same sub-values each in two places), and simplify the encode logic (only need to reuse logic from GeoJSON external secondary instances).
Option 3 (a completely new representation)... I'd be open to it, but I would want to justify it by referencing something that's successful in projects with strong geo background. But my gut tells me that most representations of a geopoint are going to look very similar to one of the representations we already use.
In any case, I'd like to see if we can settle this aspect of the design before going too much further with the client integration. Whatever the representation we end up with, I don't think that we want the client to be responsible for decoding or encoding node values, since that's an engine responsibility everywhere else.
There was a problem hiding this comment.
Thank you for the chat earlier! This is a quick summary of the plan:
First round:
Build a normal geopoint's engine codec, in which runtime value representation is the shape of GeolocationCoordinates
Second round:
Evaluate the codec and see how to fit GeoJSON; maybe external secondary serialization to use the same encoding logic.
Third round:
Evaluate sharing between xpath and xform-engine packages.
After evaluating the second or third, we might decide that these are out of scope and create GH issues for those.
There was a problem hiding this comment.
Work in progress
| @@ -0,0 +1,25 @@ | |||
| export const parseToInteger = (value: string | null): number | null => { | |||
There was a problem hiding this comment.
Another possible place for this is in packages/xforms-engine/src/lib
…when readonly mode, and has white background
| @@ -61,6 +98,7 @@ export class InputControl<V extends ValueType = ValueType> | |||
| // InputNode | |||
| readonly nodeType = 'input'; | |||
| readonly appearances: InputNodeAppearances; | |||
| readonly nodeOptions: InputNodeOptions<V>; | |||
There was a problem hiding this comment.
I haven't decided yet if I'll add this to BaseNode in this PR on the following one. If it's >20 file changes, I'll open a separate PR
|
@eyelidlessness, if you have time, could you please review this draft? I still have the following pending items that I'll be working on in the meantime:
|
WIP
Closes #249
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
The test plan is here
Test videos:
[To be uploaded soon]
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?
This work should support the default behavior of Geopoint input and no regression are expected.
Do we need any specific form for testing your changes? If so, please attach one.
The test forms are at the end of this comment
What's changed