Skip to content
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.

Sign up for GitHub

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

Always add a concrete repeat instance to nested repeats #645

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Always add a concrete repeat instance to nested repeats #645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/main/java/org/javarosa/core/model/FormDef.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,8 @@ public void createNewRepeat(FormIndex index) throws InvalidReferenceException {
TreeReference repeatContextRef = getChildInstanceRef(index);
TreeElement template = mainInstance.getTemplate(repeatContextRef);

mainInstance.copyNode(template, repeatContextRef);
//For #4059 fix
mainInstance.copyNode(template.deepCopyForRepeat(), repeatContextRef);

TreeElement newNode = mainInstance.resolveReference(repeatContextRef);
preloadInstance(newNode);
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/org/javarosa/core/model/instance/TreeElement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,18 @@ public TreeElement deepCopy(boolean includeTemplates) {
return newNode;
}

//For #4059 fix
public TreeElement deepCopyForRepeat() {
TreeElement newNode = shallowCopy();

newNode.children.clear();
for (TreeElement child : children) {
child.setMult(TreeReference.DEFAULT_MULTIPLICITY);
newNode.addChild(child.deepCopyForRepeat());
}
return newNode;
}

/* ==== MODEL PROPERTIES ==== */

// factoring inheritance rules
Expand Down
11 changes: 6 additions & 5 deletions src/test/java/org/javarosa/core/model/TriggerableDagTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1831,11 +1831,12 @@ public void addingNestedRepeatInstance_updatesExpressionTriggeredByGenericRef_fo
scenario.createNewRepeat("/data/outer[1]/inner");
scenario.createNewRepeat("/data/outer[1]/inner");

assertThat(scenario.answerOf("/data/outer[0]/inner[0]/count"), is(intAnswer(3)));
assertThat(scenario.answerOf("/data/outer[0]/inner[1]/count"), is(intAnswer(3)));
assertThat(scenario.answerOf("/data/outer[0]/inner[2]/count"), is(intAnswer(3)));
assertThat(scenario.answerOf("/data/outer[1]/inner[0]/count"), is(intAnswer(2)));
assertThat(scenario.answerOf("/data/outer[1]/inner[1]/count"), is(intAnswer(2)));
//For #4059 fix
assertThat(scenario.answerOf("/data/outer[0]/inner[0]/count"), is(intAnswer(4)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 3 inner repeats so the count must reflect that. Now the counts look like they're always off by 1. Is there something I'm missing?

Copy link
Author

@dimwight dimwight Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lognaturel for sparing the time to look at this.
I'm very aware of my limited understanding of how these results come about, but I'm guessing it has something to do with the creation under the updated code of nested repeats that are missing under the current code, which is what the original issue pointed out.

(On re-reading this it looks a bit sarcastic, but I'm genuinely uncertain what's going on.)

Copy link
Author

@dimwight dimwight Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confess to my actual working method, I
- traced the code until it was clear where the original Collect test was failing
- played with the code until the test passed
- looked for JR tests that failed
- played with their code until they in turn passed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, this is thorny stuff! Thanks again for taking a deep dive and I see my message came out more curt than I intended. The limitations of Github communication.

I hadn't carefully read the issue as my nonsense response about pyxform there indicates. I've read it again and now I'm thinking that it may not be a bug at all. I'm still on maternity leave with unpredictable availability but I'll keep this kicking around my brain and will hopefully have some thoughts to share soon.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not strictly a bug, then perhaps a quirk that contravenes the principle of least surprise? @seadowg's point has validity - the current behaviour is arguably inconsistent, even illogical. And my proposed change is so isolated as hardly to create a regression risk.

assertThat(scenario.answerOf("/data/outer[0]/inner[1]/count"), is(intAnswer(4)));
assertThat(scenario.answerOf("/data/outer[0]/inner[2]/count"), is(intAnswer(4)));
assertThat(scenario.answerOf("/data/outer[1]/inner[0]/count"), is(intAnswer(3)));
assertThat(scenario.answerOf("/data/outer[1]/inner[1]/count"), is(intAnswer(3)));
}

@Test
Expand Down
8 changes: 5 additions & 3 deletions src/test/java/org/javarosa/form/api/FormNavigationTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
@RunWith(Parameterized.class)
public class FormNavigationTestCase {

private String formName;
private String[] expectedIndices;
private final String formName;
private final String[] expectedIndices;

@Parameterized.Parameters(name = "{0}")
public static Iterable<Object[]> data() {
Expand All @@ -36,7 +36,9 @@ public static Iterable<Object[]> data() {

ei("twoNestedRepeatGroups.xml",
"-1, ", "0_0, ", "0_0, 0_0, ", "0_0, 0_0, 0, ", "0_0, 0_0, 1, ", "0_0, 0_1, ",
"0_0, 0_1, 0, ", "0_0, 0_1, 1, ", "0_0, 0_2, ", "0_1, ", "-1, "),
"0_0, 0_1, 0, ", "0_0, 0_1, 1, ", "0_0, 0_2, ",
//For #4059 fix
"0_0, 0_2, 0, ", "0_0, 0_2, 1, ", "0_0, 0_3, ", "0_1, ", "-1, "),

ei("simpleFormWithThreeQuestions.xml",
"-1, ", "0, ", "1, ", "2, ", "-1, ")
Expand Down