getodk / xforms-spec

The XForms-derived specification used in the ODK ecosystem. If you are interested in building a tool that is compliant with the forms rendered by ODK tools, this is the place to start. ✨⚒✨
https://getodk.github.io/xforms-spec/
30 stars 26 forks source link

Add multi-argument distance #311

Closed lognaturel closed 3 months ago

lognaturel commented 3 months ago

Adds a multi-argument variant to distance. This makes it easier to compute the distance between multiple points within a form's primary instance. Currently to achieve this you'd have to introduce a calculate which concatenates those points together and then call the distance function with a reference to that calculate as the argument.

It also makes it possible to filter a nodeset based on distance as described in this forum post. This is not currently possible because the single-argument variant that exists today only accepts a nodeset.

I also considered accepting a literal string which would meet the requirement but it feels less intuitive and convenient than accepting a variable number of arguments.

I can't actually come up with a usage for more than two arguments so another option would be to only allow two. But I don't think we lose anything by adding flexibility, especially since we already compute distance between more than two points with the other input types.

I considered adding this variant to area as well but I'd rather wait until we have a known use for it.

@eyelidlessness @seadowg does this seem like an ok spec addition? I think it's unlikely to be controversial so I'm inclined to Make It So without much discussion if it seems reasonable to you. I've also shared on the forum.

lognaturel commented 3 months ago

maybe adds a bit too

Can you elaborate, I'm not seeing it!

(geopoint|string) arg* still allows for variadic node-set arguments, so long as each node-set resolves to a single geopoint value

Yes, exactly.

Does that sound reasonably close to the intent?

Yes! I think the single-arg case may need a separate branch for when the single argument is a nodeset of geopoints (there won't be semicolons) but that's an implementation detail.

I kind of have to just trust that our published rendering will format this appropriately

I have run it and verified that it does.

lognaturel commented 3 months ago

From @tiritea at https://github.com/getodk/javarosa/pull/761#issuecomment-2109078282 :

it actually seems more intuitive to me that the (singular) argument to the existing XPath distance() function should be able to support either a node/nodeset (for example, containing a geotrace value), or a literal string of the same. Many functions behave this way...

I don’t disagree if our target audience were developers. For a form designer writing a form I think having to concat two values that are points with a semicolon in between feels weird and surprising — I have these two locations, let me do something with them! A lot of folks who use points don’t know about traces or shapes.

That said, I imagine most folks will use documentation and examples to use this functionality so I don’t feel too strongly either way. Also happy to do both.

tiritea commented 3 months ago

A special flavor of distance() which supports two geopoint arguments is probably useful enough, and minimally supplemental - and not to mention more user-friendly - to warrant it. And it is easy enough to readily identify the new 'flavor' (for implementation purposes) from simply counting the number of arguments.

But that's independent of a desire to support string literals/expressions, to bring the function more in line with some its its compatriots... :-)

tiritea commented 3 months ago

For a form designer writing a form I think having to concat two values that are points with a semicolon in between feels weird and surprising

Somewhat an aside, but it may be worth noting that it still remains an option - especially for things that are potentially 'unintuitive' - that they can ultimately be accommodated by either by XForms/XPath, or just by XLSForm semantic sugar of existing function. Full disclosure: I tend to lean towards not implementing new XForm/XPath functions - for things that can, or perhaps should, be implemented with (minor additional?) functionality, and let XLSForm semantic sugar worry about making it the most user-friendly.

Just another (unsolicited) $0.02 :-) lol

lognaturel commented 3 months ago

A special flavor of distance() which supports two geopoint arguments is probably useful enough, and minimally supplemental - and not to mention more user-friendly - to warrant it. And it is easy enough to readily identify the new 'flavor' (for implementation purposes) from simply counting the number of arguments.

👍 And do you feel strongly about two geopoint arguments as opposed to N?

that's independent of a desire to support string literals/expressions, to bring the function more in line with some its its compatriots...

That's a good point and I was surprised in the forum thread that brought this up that a literal string didn't work. I believe that the way the spec is as-published suggests both literal values and references to them should be supported. Does that sound right? If so, we can go ahead and make the necessary changes to the implementations that need them.

they can ultimately be accommodated by either by XForms/XPath, or just by XLSForm semantic sugar of existing function

Yes, that's a good point and I'll think about where we can codify that practice. In this case, I agree with your earlier point that this is minimally supplemental and therefore attractive. I also think that XLSForm sugar in this case would have a high probability of leading to really confusing errors for users like an error related to a concat function call they didn't make.

Thanks!