Use relative references for repeats only.

[ukanga]

This introduces relative referencing for ${variables} in repeat sections.

Not sure if my interpretation is accurate, I found a super long old thread in ODK forum, not sure if it is relevant still.

I picked the option of dealing with https://opendatakit.github.io/xforms-spec/#a-big-deviation-with-xforms hoping it does address it.

Fix #4

[MartijnR]

OMG, this is exciting! 👍 Maybe we can take out that deviation from the spec some time.

Will check it out!

[MartijnR]

The number of steps down ( current()/../ vs current()/../../../../) should be determined from the context (i.e. question where ref is used). See https://docs.google.com/spreadsheets/d/1DUgtclTL8fBOHS9h88a8eC6x1u5M17g1b7CPJBm7JP4/edit?usp=sharing.

You could decide to strip the current()/ part if the expression is not inside a predicate, e.g. calculate="../crop" instead of calculate="current()/../crop". That would be preferable. See https://docs.google.com/spreadsheets/d/1lTJ9-AnrGDdFK8UzvmSglqFgGjLsdzQzp_XLoYkV__Q/edit?usp=sharing

P.S. Gotta run now, but will markdownify these forms, to facilitate turning them into tests later.

[lognaturel]

As the thread @ukanga links to notes, Javarosa doesn't interpret current() correctly in choice filters. This is the variant of @MartijnR's form above that has the expected behavior in Javarosa (note the wrong name = current()/crop and name = current()/../crop):

<select1 ref="/repeat-relative-current-1/rep/sel_a">
  <label> Verify <output value=" current()/../crop "/> </label>
  <itemset nodeset="instance('crop_list')/root/item[name =  current()/crop ]">
    <value ref="name"/>
    <label ref="jr:itext(itextId)"/>
  </itemset>
</select1>
<group ref="/repeat-relative-current-1/rep/group">
  <select1 ref="/repeat-relative-current-1/rep/group/sel_b">
    <label> Verify <output value=" current()/../../crop "/> </label>
  <itemset nodeset="instance('crop_list')/root/item[name = current()/../crop]">
      <value ref="name"/>
      <label ref="jr:itext(itextId)"/>
    </itemset>
  </select1>
</group>

Perhaps there can be some flags or model version information added in somehow as explored in the thread but I don't think it can be the default behavior for pyxform right now, at least not for choice filters. The problem is that even if we patch JR today (which doesn't sound trivial), all users running older versions of Collect (and there are many), will be unable to use new forms with choice filters. That's what the very long thread was about.

[MartijnR]

Ah, yes, when fixing the current() bug in JR, I understand you may need a way to determine whether an (old) XForm may be relying on a bug-dependent usage of current().

(UPDATE: made my comment less nasty (sorry) and moved useful parts here: getodk/javarosa#293)

[MartijnR]

Test 1:

survey
	type	name	label	choice_filter
	begin repeat	rep
	select_one crop_list	crop	Select
	begin group	group
	select_one crop_list	b	Verify	name = ${crop}
	end group
	end repeat

expected output for b itemset: [ name = current()/../../crop ]

Test 2:

survey
	type	name	label	calculate
	begin repeat	rep
	select_one crop_list	crop	Select
	text	a	Verify	name = ${crop}
	begin group	group
	text	b	Verify	name = ${crop}
	end group
	end repeat

expected output for a bind: calculate="../crop"
expected output for b bind: calculate="../../crop"

[lognaturel]

These tests look great. Would be good to get to a high-level game plan also. Can we do relative paths everywhere other than choice filters for now? Do we wait on the overall change?

[MartijnR]

I'm fine with just introducing them all at once (waiting a bit for the JR folks) or in 2 batches (splitting choice_name and rest). Whatever is easiest in terms of pyxform code changes.

What is important to me is to not lose momentum in getting the code ready to go now that this is hot (after 5 years). Merging can wait.

[ukanga]

Thank you guys for the comments and tests, I will add them in the next iteration on this PR.

[ukanga]

Example of XForm XML with relative paths as generated by this PR. The first two are the examples @MartijnR provided above.

• Choice filter with relative paths
• Calculate with relative paths
• For loop with relative paths
• A more complex repeat structure with relative paths XML.

If you have the chance to test the above XML in ODK Collect or Enketo, let me know what's not working as expected.

[lognaturel]

If you have the chance to test the above XML in ODK Collect or Enketo, let me know what's not working as expected.

Everything works as expected in Collect other than the choice filters. 🎉 I think we will have a fix for JavaRosa soon. I /think/ this is actually ok to merge though I'd like a sanity check on my thinking. The JavaRosa problem is that current() doesn't refer to the right thing in selects. As far as I can tell, this only affects two things: choice filters and the itemset nodeset. It's not very likely users are using relative paths to set the itemset nodeset. One would use that in a scenario like collecting household info in which the enumerator is asked to select the owner of each pet from a dynamically generated list of children:

repeat people
  repeat children
  repeat pets
    select itemset nodeset=current()../../children

It's cool and powerful but there's no XLSForm support for itemsets from arbitrary nodes (#38) and it's pretty esoteric. I've only shown users how to do it with absolute paths but have never seen it come up with relative paths before.

The other case is choice filters. XLSForms with choice filters in repeats fail today because an absolute path doesn't make sense there. Currently in Collect we get an error about needing indexed-repeat. With this change we'd get no error but also no choices shown. Maybe a little harder for users to troubleshoot but nothing too bad. And as soon as they got patched Collect it would Just Work™️.

The change we're going to make in JR would only affect users who are trying to use current() already. This PR doesn't affect them (maybe a good test to add @ukanga? Demonstrate that if a path is already relative this doesn't change anything?).

If someone else who uses Collect and understands the JR issue can run through this reasoning and validate it, I'd be comfortable merging.

[lognaturel]

You could decide to strip the current()/ part if the expression is not inside a predicate, e.g. calculate="../crop" instead of calculate="current()/../crop"

I would be in favor of this. current() is a bit of a strange bird so the more it can be avoided the better.

[MartijnR]

Thanks. I can confirm the forms work in Enketo too. (after correcting calculation in calculate_w_relative_paths.xml to calculate="current()/../crop" and calculate="current()/../../crop ", but that's not a pyxform issue.)

I agree it is best to avoid using current() when it's not required.

[ggalmazor]

Hi! Just wanted to drop by and say that @lognaturel's reasoning looks good to me. I also agree that I would prefer having ../ instead of current() wherever possible.

[ukanga]

Thank you all for your feedback, I will incorporate it and ping you again for another review.

[ukanga]

@MartijnR @lognaturel not sure what was pending on this but I see most of what was on your comments was addressed. Any blocker to getting this merged?

[MartijnR]

We're using a predicate here, and we are changing the context to instance('crop_list'), so we would need current() here: [name = current()/../crop] and [name = current()/../../crop]

[MartijnR]

Thanks! Once current() is back in choice-filters, I'll run the test forms in Enketo for a final/proper check.

[ukanga]

@MartijnR 3c31570 should address it.

[yanokwa]

I got some XForms consulting time with @lognaturel and this PR looks like what she wanted. That is pyxform generates relative paths with . and .. everywhere except in predicates like choice filters. In those cases, current() should be used. My only minor concern is that are there any other places besides choice filters where we could use predicates?

[MartijnR]

Will do a check with some forms today.

My only minor concern is that are there any other places besides choice filters where we could use predicates?

I believe all those would be hand-coded combinations of pure XPath mixed with ${shortcuts} such as a calculate that searches for a value:

calculate = "count(instance('households')/root/item[rooms > ${room_size}])"

Since this is so advanced, I think we can for now require those users to take care of using current() themselves (and not use ${shortcut}s)

[MartijnR]

I confirm that this works great! Thanks!

I noticed that the calculations in Test 2 that I provided (name = ${crop}) make no sense (will always result in false), but that doesn't matter as the test still serves its purpose and the expression is valid.