From 04bf1bd721cbb9b6434e2c079afccc67c939a8cc Mon Sep 17 00:00:00 2001 From: Guille Date: Mon, 16 Dec 2019 11:43:38 +0100 Subject: [PATCH 01/51] Migrate Triggerable.targets to Set --- .../javarosa/core/model/QuickTriggerable.java | 2 +- .../org/javarosa/core/model/TriggerableDag.java | 12 ++++-------- .../javarosa/core/model/condition/Condition.java | 2 +- .../core/model/condition/Recalculate.java | 2 +- .../core/model/condition/Triggerable.java | 16 +++++++--------- 5 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/QuickTriggerable.java b/src/main/java/org/javarosa/core/model/QuickTriggerable.java index 035f782ee..fddd2a04b 100644 --- a/src/main/java/org/javarosa/core/model/QuickTriggerable.java +++ b/src/main/java/org/javarosa/core/model/QuickTriggerable.java @@ -50,7 +50,7 @@ public List apply(FormInstance mainInstance, EvaluationContext return triggerable.apply(mainInstance, ec, qualified); } - public List getTargets() { + public Set getTargets() { return triggerable.getTargets(); } diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 30fdb248f..b51553dbf 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -182,8 +182,7 @@ private Set initializeTriggerables(FormInstance mainInstance, Set applicable = new HashSet<>(); for (QuickTriggerable qt : triggerablesDAG) { - for (int j = 0; j < qt.getTargets().size(); j++) { - TreeReference target = qt.getTargets().get(j); + for (TreeReference target : qt.getTargets()) { if (genericRoot.isAncestorOf(target, false)) { applicable.add(qt); break; @@ -394,8 +393,7 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev conditionRepeatTargetIndex.clear(); for (QuickTriggerable qt : triggerablesDAG) { if (qt.isCondition()) { - List targets = qt.getTargets(); - for (TreeReference target : targets) { + for (TreeReference target : qt.getTargets()) { if (mainInstance.getTemplate(target) != null) { conditionRepeatTargetIndex.put(target, qt); } @@ -426,8 +424,7 @@ public void throwCyclesInDagException(Collection vertices) { public void fillTriggeredElements(FormInstance mainInstance, EvaluationContext evalContext, QuickTriggerable qt, Set destinationSet, Set newDestinationSet) { - for (int j = 0; j < qt.getTargets().size(); j++) { - TreeReference target = qt.getTargets().get(j); + for (TreeReference target : qt.getTargets()) { Set updatedNodes = new HashSet<>(); updatedNodes.add(target); @@ -622,8 +619,7 @@ public void reportDependencyCycles() { List triggered = triggerIndex.get(trigger); targets.clear(); for (QuickTriggerable qt : triggered) { - for (int j = 0; j < qt.getTargets().size(); j++) { - TreeReference target = qt.getTargets().get(j); + for (TreeReference target : qt.getTargets()) { if (!targets.contains(target)) targets.add(target); } diff --git a/src/main/java/org/javarosa/core/model/condition/Condition.java b/src/main/java/org/javarosa/core/model/condition/Condition.java index 6585a8359..72122c116 100644 --- a/src/main/java/org/javarosa/core/model/condition/Condition.java +++ b/src/main/java/org/javarosa/core/model/condition/Condition.java @@ -42,7 +42,7 @@ public class Condition extends Triggerable { public Condition() { } - protected Condition(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, List targets, Set immediateCascades, ConditionAction trueAction, ConditionAction falseAction) { + protected Condition(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, Set targets, Set immediateCascades, ConditionAction trueAction, ConditionAction falseAction) { super(expr, contextRef, originalContextRef, targets, immediateCascades); this.trueAction = trueAction; this.falseAction = falseAction; diff --git a/src/main/java/org/javarosa/core/model/condition/Recalculate.java b/src/main/java/org/javarosa/core/model/condition/Recalculate.java index 18485d773..2ad47a649 100644 --- a/src/main/java/org/javarosa/core/model/condition/Recalculate.java +++ b/src/main/java/org/javarosa/core/model/condition/Recalculate.java @@ -41,7 +41,7 @@ public Recalculate() { } - protected Recalculate(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, List targets, Set immediateCascades) { + protected Recalculate(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, Set targets, Set immediateCascades) { super(expr, contextRef, originalContextRef, targets, immediateCascades); } diff --git a/src/main/java/org/javarosa/core/model/condition/Triggerable.java b/src/main/java/org/javarosa/core/model/condition/Triggerable.java index a56b18326..c782bfb39 100644 --- a/src/main/java/org/javarosa/core/model/condition/Triggerable.java +++ b/src/main/java/org/javarosa/core/model/condition/Triggerable.java @@ -58,7 +58,7 @@ public abstract class Triggerable implements Externalizable { * References to all of the (non-contextualized) nodes which should be * updated by the result of this triggerable */ - protected List targets; + protected Set targets; /** * Current reference which is the "Basis" of the trigerrables being evaluated. This is the highest @@ -79,7 +79,7 @@ protected Triggerable() { } - protected Triggerable(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, List targets, Set immediateCascades) { + protected Triggerable(XPathConditional expr, TreeReference contextRef, TreeReference originalContextRef, Set targets, Set immediateCascades) { this.expr = expr; this.targets = targets; this.contextRef = contextRef; @@ -88,11 +88,11 @@ protected Triggerable(XPathConditional expr, TreeReference contextRef, TreeRefer } public static Triggerable condition(XPathConditional expr, ConditionAction trueAction, ConditionAction falseAction, TreeReference contextRef) { - return new Condition(expr, contextRef, contextRef, new ArrayList<>(), new HashSet<>(), trueAction, falseAction); + return new Condition(expr, contextRef, contextRef, new HashSet<>(), new HashSet<>(), trueAction, falseAction); } public static Triggerable recalculate(XPathConditional expr, TreeReference contextRef) { - return new Recalculate(expr, contextRef, contextRef, new ArrayList<>(), new HashSet<>()); + return new Recalculate(expr, contextRef, contextRef, new HashSet<>(), new HashSet<>()); } public abstract Object eval(FormInstance instance, EvaluationContext ec); @@ -167,7 +167,7 @@ public final List apply(FormInstance mainInstance, EvaluationC return affectedNodes; } - public List getTargets() { + public Set getTargets() { return targets; } @@ -196,9 +196,7 @@ public IConditionExpr getExpr() { } public void addTarget(TreeReference target) { - if (targets.indexOf(target) == -1) { - targets.add(target); - } + targets.add(target); } @Override @@ -242,7 +240,7 @@ public void readExternal(DataInputStream in, PrototypeFactory pf) throws IOExcep expr = (XPathConditional) ExtUtil.read(in, new ExtWrapTagged(), pf); contextRef = (TreeReference) ExtUtil.read(in, TreeReference.class, pf); originalContextRef = (TreeReference) ExtUtil.read(in, TreeReference.class, pf); - targets = new ArrayList<>((List) ExtUtil.read(in, new ExtWrapList(TreeReference.class), pf)); + targets = new HashSet<>((List) ExtUtil.read(in, new ExtWrapList(TreeReference.class), pf)); } @Override From ed1bae4ac6f64ca3af7c6550bfb75878197afe8a Mon Sep 17 00:00:00 2001 From: Guille Date: Mon, 16 Dec 2019 11:45:28 +0100 Subject: [PATCH 02/51] Migrate TriggerableDag.triggerablesDAG to a LinkedHashSet This removes the need for duplicate checking and makes explicit that this set is dependant on (insertion) order. --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index b51553dbf..8059471fb 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -25,6 +25,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -64,7 +65,7 @@ public interface EventNotifierAccessor { * the list, where tA comes before tB, evaluating tA cannot depend on any * result from evaluating tB. */ - protected final List triggerablesDAG = new ArrayList<>(); + protected final Set triggerablesDAG = new LinkedHashSet<>(); /** * NOT VALID UNTIL finalizeTriggerables() is called!! From beb274b026bc056b5235b1e0c124a59bdca9f4ae Mon Sep 17 00:00:00 2001 From: Guille Date: Mon, 16 Dec 2019 11:46:40 +0100 Subject: [PATCH 03/51] Migrate TriggerableDag.triggerIndex to a map of sets --- .../org/javarosa/core/model/TriggerableDag.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 8059471fb..58dd461f7 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -79,7 +79,7 @@ public interface EventNotifierAccessor { * Maps a tree reference to the set of triggerables that need to be * processed when the value at this reference changes. */ - protected final Map> triggerIndex = new HashMap<>(); + protected final Map> triggerIndex = new HashMap<>(); /** * List of all the triggerables in the form. Unordered. @@ -306,14 +306,12 @@ public final Triggerable addTriggerable(Triggerable t) { Set triggers = t.getTriggers(); for (TreeReference trigger : triggers) { - List triggered = triggerIndex.get(trigger); + Set triggered = triggerIndex.get(trigger); if (triggered == null) { - triggered = new ArrayList<>(); + triggered = new HashSet<>(); triggerIndex.put(trigger.clone(), triggered); } - if (!triggered.contains(qt)) { - triggered.add(qt); - } + triggered.add(qt); } return t; @@ -452,7 +450,7 @@ public void fillTriggeredElements(FormInstance mainInstance, EvaluationContext e // so we'll be more inclusive than needed and see if any // of our triggers are keyed on the predicate-less path // of this ref - List triggered = triggerIndex.get(ref.hasPredicates() ? ref.removePredicates() : ref); + Set triggered = triggerIndex.get(ref.hasPredicates() ? ref.removePredicates() : ref); if (triggered != null) { // If so, walk all of these triggerables that we @@ -590,7 +588,7 @@ private Set triggerTriggerables(FormInstance mainInstance, Eva TreeReference genericRef = ref.genericize(); // get triggerables which are activated by the generic reference - List triggered = triggerIndex.get(genericRef); + Set triggered = triggerIndex.get(genericRef); if (triggered == null) { return alreadyEvaluated; } @@ -617,7 +615,7 @@ public void reportDependencyCycles() { List targets = new ArrayList<>(); for (TreeReference trigger : triggerIndex.keySet()) { vertices.add(trigger); - List triggered = triggerIndex.get(trigger); + Set triggered = triggerIndex.get(trigger); targets.clear(); for (QuickTriggerable qt : triggered) { for (TreeReference target : qt.getTargets()) { From de6efcff18719a7ecfc3a6473ad984c2e06291de Mon Sep 17 00:00:00 2001 From: Guille Date: Mon, 16 Dec 2019 11:47:33 +0100 Subject: [PATCH 04/51] Migrate TriggerableDag.unorderedTriggers to a Set --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 58dd461f7..6a21f7ee0 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -84,7 +84,7 @@ public interface EventNotifierAccessor { /** * List of all the triggerables in the form. Unordered. */ - protected final List unorderedTriggerables = new ArrayList<>(); + protected final Set unorderedTriggerables = new HashSet<>(); protected TriggerableDag(EventNotifierAccessor accessor) { this.accessor = accessor; From 883ec3b3c4d5b68004dd7c7f83b6fdf4e98b3919 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 12:17:25 +0100 Subject: [PATCH 05/51] Migrate to immutable and side-effect free code We need the code to be easier to understand when it comes to recursive algorithms. Mutation of input arguments makes it super hard, and this commit tries to solve that by making those methods return something instead. --- .../javarosa/core/model/TriggerableDag.java | 42 ++++++++++--------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 6a21f7ee0..60e6ae9d9 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -334,11 +334,9 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev List vertices = new ArrayList<>(unorderedTriggerables); triggerablesDAG.clear(); List partialOrdering = new ArrayList<>(); - Set newDestinationSet = new HashSet<>(); for (QuickTriggerable qt : vertices) { Set deps = new HashSet<>(); - newDestinationSet.clear(); - fillTriggeredElements(mainInstance, evalContext, qt, deps, newDestinationSet); + Set newDestinationSet = fillTriggeredElements(mainInstance, evalContext, qt, deps); if (deps.contains(qt)) throwCyclesInDagException(deps); @@ -420,10 +418,10 @@ public void throwCyclesInDagException(Collection vertices) { * Get all of the elements which will need to be evaluated (in order) when * the triggerable is fired. */ - public void fillTriggeredElements(FormInstance mainInstance, EvaluationContext evalContext, QuickTriggerable qt, Set destinationSet, Set newDestinationSet) { + public Set fillTriggeredElements(FormInstance mainInstance, EvaluationContext evalContext, QuickTriggerable qt, Set destinationSet) { + Set dependantTriggerables = new LinkedHashSet<>(); - - for (TreeReference target : qt.getTargets()) { + for (TreeReference target : qt.getTargets()) { Set updatedNodes = new HashSet<>(); updatedNodes.add(target); @@ -432,8 +430,7 @@ public void fillTriggeredElements(FormInstance mainInstance, EvaluationContext e // In that case, we want to add all of those nodes // to the list of updated elements as well. if (qt.isCascadingToChildren()) { - addChildrenOfReference(mainInstance, evalContext, - target, updatedNodes); + updatedNodes.addAll(getChildrenOfReference(mainInstance, evalContext, target)); } // Now go through each of these updated nodes (generally @@ -450,7 +447,7 @@ public void fillTriggeredElements(FormInstance mainInstance, EvaluationContext e // so we'll be more inclusive than needed and see if any // of our triggers are keyed on the predicate-less path // of this ref - Set triggered = triggerIndex.get(ref.hasPredicates() ? ref.removePredicates() : ref); + Set triggered = triggerIndex.get(ref.hasPredicates() ? ref.removePredicates() : ref); if (triggered != null) { // If so, walk all of these triggerables that we @@ -460,12 +457,13 @@ public void fillTriggeredElements(FormInstance mainInstance, EvaluationContext e // there already if (!destinationSet.contains(qu)) { destinationSet.add(qu); - newDestinationSet.add(qu); + dependantTriggerables.add(qu); } } } } } + return dependantTriggerables; } /** @@ -505,41 +503,47 @@ private Set evaluateTriggerables(FormInstance mainInstance, Ev /** * This is a utility method to get all of the references of a node. It can * be replaced when we support dependent XPath Steps (IE: /path/to//) + * + * @return */ - private void addChildrenOfReference(FormInstance mainInstance, EvaluationContext evalContext, TreeReference original, Set toAdd) { + private Set getChildrenOfReference(FormInstance mainInstance, EvaluationContext evalContext, TreeReference original) { // original has already been added to the 'toAdd' list. + Set descendantRefs = new HashSet<>(); TreeElement repeatTemplate = mainInstance.getTemplatePath(original); if (repeatTemplate != null) { for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { TreeElement child = repeatTemplate.getChildAt(i); - toAdd.add(child.getRef().genericize()); - addChildrenOfElement(mainInstance, child, toAdd); + descendantRefs.add(child.getRef().genericize()); + descendantRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); } } else { List refSet = evalContext.expandReference(original); for (TreeReference ref : refSet) { - addChildrenOfElement(mainInstance, evalContext.resolveReference(ref), toAdd); + descendantRefs.addAll(getChildrenRefsOfElement(mainInstance, evalContext.resolveReference(ref))); } } + return descendantRefs; } // Recursive step of utility method - private void addChildrenOfElement(FormInstance mainInstance, AbstractTreeElement el, Set toAdd) { + private Set getChildrenRefsOfElement(FormInstance mainInstance, AbstractTreeElement el) { + Set childrenRefs = new HashSet<>(); TreeElement repeatTemplate = mainInstance.getTemplatePath(el.getRef()); if (repeatTemplate != null) { for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { TreeElement child = repeatTemplate.getChildAt(i); - toAdd.add(child.getRef().genericize()); - addChildrenOfElement(mainInstance, child, toAdd); + childrenRefs.add(child.getRef().genericize()); + childrenRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); } } else { for (int i = 0; i < el.getNumChildren(); ++i) { AbstractTreeElement child = el.getChildAt(i); - toAdd.add(child.getRef().genericize()); - addChildrenOfElement(mainInstance, child, toAdd); + childrenRefs.add(child.getRef().genericize()); + childrenRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); } } + return childrenRefs; } /** From 9b0f5a56ec6e5a4e4aceb7cd22c025d3370aa76e Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 12:22:34 +0100 Subject: [PATCH 06/51] Migrate to immutable and side-effect free code - Phase 2 It turns out that the newDestinationSet variable was redundant and it looks like the real output of the method is the deps set (to be removed in the next commit) --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 60e6ae9d9..5129b49fa 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -335,8 +335,7 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev triggerablesDAG.clear(); List partialOrdering = new ArrayList<>(); for (QuickTriggerable qt : vertices) { - Set deps = new HashSet<>(); - Set newDestinationSet = fillTriggeredElements(mainInstance, evalContext, qt, deps); + Set deps = fillTriggeredElements(mainInstance, evalContext, qt, new HashSet<>()); if (deps.contains(qt)) throwCyclesInDagException(deps); From 4a75184fa4d0dccc24136125dc313a5dd0cfca38 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 12:23:41 +0100 Subject: [PATCH 07/51] Migrate to immutable and side-effect free code - Phase 3 Remove the output arg. The block where elements get added can be further simplified using Map.getOrDefault or the alternative code that is compatible with our target Android API level (next commit) --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 5129b49fa..d03be1c6a 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -335,7 +335,7 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev triggerablesDAG.clear(); List partialOrdering = new ArrayList<>(); for (QuickTriggerable qt : vertices) { - Set deps = fillTriggeredElements(mainInstance, evalContext, qt, new HashSet<>()); + Set deps = fillTriggeredElements(mainInstance, evalContext, qt); if (deps.contains(qt)) throwCyclesInDagException(deps); @@ -417,7 +417,7 @@ public void throwCyclesInDagException(Collection vertices) { * Get all of the elements which will need to be evaluated (in order) when * the triggerable is fired. */ - public Set fillTriggeredElements(FormInstance mainInstance, EvaluationContext evalContext, QuickTriggerable qt, Set destinationSet) { + public Set fillTriggeredElements(FormInstance mainInstance, EvaluationContext evalContext, QuickTriggerable qt) { Set dependantTriggerables = new LinkedHashSet<>(); for (TreeReference target : qt.getTargets()) { @@ -454,8 +454,7 @@ public Set fillTriggeredElements(FormInstance mainInstance, Ev for (QuickTriggerable qu : triggered) { // And add them to the queue if they aren't // there already - if (!destinationSet.contains(qu)) { - destinationSet.add(qu); + if (!dependantTriggerables.contains(qu)) { dependantTriggerables.add(qu); } } From e67174480c1547de2298522a6fb8bfc666bcac1e Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 12:26:11 +0100 Subject: [PATCH 08/51] Migrate to immutable and side-effect free code - Phase 4 Simplify adding elements into the result set --- .../org/javarosa/core/model/TriggerableDag.java | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index d03be1c6a..2abc08721 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -447,18 +447,8 @@ public Set fillTriggeredElements(FormInstance mainInstance, Ev // of our triggers are keyed on the predicate-less path // of this ref Set triggered = triggerIndex.get(ref.hasPredicates() ? ref.removePredicates() : ref); - - if (triggered != null) { - // If so, walk all of these triggerables that we - // found - for (QuickTriggerable qu : triggered) { - // And add them to the queue if they aren't - // there already - if (!dependantTriggerables.contains(qu)) { - dependantTriggerables.add(qu); - } - } - } + if (triggered != null) + dependantTriggerables.addAll(triggered); } } return dependantTriggerables; From acd8d54d95b2315cfc3a10c3114ec11815b94b62 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 12:31:23 +0100 Subject: [PATCH 09/51] Simple rearrange --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 2abc08721..0d6c15e5d 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -327,12 +327,11 @@ public final Triggerable addTriggerable(Triggerable t) { * triggers can't be laid out appropriately */ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext evalContext) throws IllegalStateException { - // + triggerablesDAG.clear(); + // DAGify the triggerables based on dependencies and sort them so that // triggerables come only after the triggerables they depend on - // List vertices = new ArrayList<>(unorderedTriggerables); - triggerablesDAG.clear(); List partialOrdering = new ArrayList<>(); for (QuickTriggerable qt : vertices) { Set deps = fillTriggeredElements(mainInstance, evalContext, qt); From 49a596baed033f2fd53a42931f4126da530578f5 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 12:31:40 +0100 Subject: [PATCH 10/51] Add TODO note --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 0d6c15e5d..10ce87c78 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -329,6 +329,8 @@ public final Triggerable addTriggerable(Triggerable t) { public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext evalContext) throws IllegalStateException { triggerablesDAG.clear(); + // TODO Study how this algorithm ensures that we follow the ancestor >>> descendant direction and if the sorting step is strictly required + // DAGify the triggerables based on dependencies and sort them so that // triggerables come only after the triggerables they depend on List vertices = new ArrayList<>(unorderedTriggerables); From 73e63649d759e210056fc7735271bb4b5946ddc0 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 12:36:37 +0100 Subject: [PATCH 11/51] Improve naming and extract methods to reduce duplication of concepts --- .../javarosa/core/model/TriggerableDag.java | 121 +++++++++--------- 1 file changed, 60 insertions(+), 61 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 10ce87c78..9c29bdfd8 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -334,30 +334,30 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev // DAGify the triggerables based on dependencies and sort them so that // triggerables come only after the triggerables they depend on List vertices = new ArrayList<>(unorderedTriggerables); - List partialOrdering = new ArrayList<>(); - for (QuickTriggerable qt : vertices) { - Set deps = fillTriggeredElements(mainInstance, evalContext, qt); + List edges = new ArrayList<>(); + for (QuickTriggerable triggerable : vertices) { + Set dependantTriggerables = getDependantTriggerables(mainInstance, evalContext, triggerable); - if (deps.contains(qt)) - throwCyclesInDagException(deps); + if (dependantTriggerables.contains(triggerable)) + throwCyclesInDagException(dependantTriggerables); - if (qt.canCascade()) - for (QuickTriggerable qu : deps) { - QuickTriggerable[] edge = {qt, qu}; - partialOrdering.add(edge); + if (triggerable.canCascade()) + for (QuickTriggerable dependantTriggerable : dependantTriggerables) { + QuickTriggerable[] edge = {triggerable, dependantTriggerable}; + edges.add(edge); } - qt.setImmediateCascades(deps); + // TODO Move this from Triggerable to TriggerableDag + triggerable.setImmediateCascades(dependantTriggerables); } List orderedRoots = new ArrayList<>(); - Set roots = new HashSet<>( - vertices.size()); + Set roots = new HashSet<>(vertices.size()); while (vertices.size() > 0) { // determine root nodes roots.clear(); roots.addAll(vertices); - for (QuickTriggerable[] edge : partialOrdering) { + for (QuickTriggerable[] edge : edges) { roots.remove(edge[1]); } @@ -368,6 +368,7 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev // order the root nodes - so the order is fixed orderedRoots.clear(); orderedRoots.addAll(roots); + // TODO Study if insertion order & using LinkedSets would suffice to ensure proper triggerable chaining Collections.sort(orderedRoots, QuickTriggerableComparator.INSTANCE); // remove root nodes and edges originating from them @@ -376,17 +377,16 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev triggerablesDAG.add(root); vertices.remove(root); } - for (int i = partialOrdering.size() - 1; i >= 0; i--) { - QuickTriggerable[] edge = partialOrdering.get(i); + for (int i = edges.size() - 1; i >= 0; i--) { + QuickTriggerable[] edge = edges.get(i); if (roots.contains(edge[0])) - partialOrdering.remove(i); + edges.remove(i); } } // // build the condition index for repeatable nodes // - conditionRepeatTargetIndex.clear(); for (QuickTriggerable qt : triggerablesDAG) { if (qt.isCondition()) { @@ -418,25 +418,25 @@ public void throwCyclesInDagException(Collection vertices) { * Get all of the elements which will need to be evaluated (in order) when * the triggerable is fired. */ - public Set fillTriggeredElements(FormInstance mainInstance, EvaluationContext evalContext, QuickTriggerable qt) { - Set dependantTriggerables = new LinkedHashSet<>(); + public Set getDependantTriggerables(FormInstance mainInstance, EvaluationContext ec, QuickTriggerable triggerable) { + Set allDependantTriggerables = new LinkedHashSet<>(); - for (TreeReference target : qt.getTargets()) { - Set updatedNodes = new HashSet<>(); - updatedNodes.add(target); + for (TreeReference target : triggerable.getTargets()) { + Set targets = new HashSet<>(); + targets.add(target); // For certain types of triggerables, the update will affect // not only the target, but also the children of the target. // In that case, we want to add all of those nodes // to the list of updated elements as well. - if (qt.isCascadingToChildren()) { - updatedNodes.addAll(getChildrenOfReference(mainInstance, evalContext, target)); + if (triggerable.isCascadingToChildren()) { + targets.addAll(getChildrenOfReference(mainInstance, ec, target)); } // Now go through each of these updated nodes (generally // just 1 for a normal calculation, // multiple nodes if there's a relevance cascade. - for (TreeReference ref : updatedNodes) { + for (TreeReference ref : targets) { // Check our index to see if that target is a Trigger // for other conditions // IE: if they are an element of a different calculation @@ -447,12 +447,12 @@ public Set fillTriggeredElements(FormInstance mainInstance, Ev // so we'll be more inclusive than needed and see if any // of our triggers are keyed on the predicate-less path // of this ref - Set triggered = triggerIndex.get(ref.hasPredicates() ? ref.removePredicates() : ref); - if (triggered != null) - dependantTriggerables.addAll(triggered); + Set dependantTriggerables = triggerIndex.get(ref.hasPredicates() ? ref.removePredicates() : ref); + if (dependantTriggerables != null) + allDependantTriggerables.addAll(dependantTriggerables); } } - return dependantTriggerables; + return allDependantTriggerables; } /** @@ -495,42 +495,41 @@ private Set evaluateTriggerables(FormInstance mainInstance, Ev * * @return */ - private Set getChildrenOfReference(FormInstance mainInstance, EvaluationContext evalContext, TreeReference original) { - // original has already been added to the 'toAdd' list. - - Set descendantRefs = new HashSet<>(); - TreeElement repeatTemplate = mainInstance.getTemplatePath(original); - if (repeatTemplate != null) { - for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { - TreeElement child = repeatTemplate.getChildAt(i); - descendantRefs.add(child.getRef().genericize()); - descendantRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); - } - } else { - List refSet = evalContext.expandReference(original); - for (TreeReference ref : refSet) { - descendantRefs.addAll(getChildrenRefsOfElement(mainInstance, evalContext.resolveReference(ref))); - } + private Set getChildrenOfReference(FormInstance mainInstance, EvaluationContext ec, TreeReference ref) { + TreeElement repeatTemplate = mainInstance.getTemplatePath(ref); + if (repeatTemplate != null) + return getChildrenRefsByTemplate(mainInstance, repeatTemplate); + // TODO Write a test that goes through this path and see how and why childrenRef can be different than child.getRef() + return getChildrenByExpandedRef(mainInstance, ec, ref); + } + + private Set getChildrenRefsOfElement(FormInstance mainInstance, AbstractTreeElement element) { + TreeElement repeatTemplate = mainInstance.getTemplatePath(element.getRef()); + if (repeatTemplate != null) + return getChildrenRefsByTemplate(mainInstance, repeatTemplate); + return getChildrenOfElement(mainInstance, element); + } + + private Set getChildrenRefsByTemplate(FormInstance mainInstance, TreeElement repeatTemplate) { + return getChildrenOfElement(mainInstance, repeatTemplate); + } + + private Set getChildrenByExpandedRef(FormInstance mainInstance, EvaluationContext ec, TreeReference initialRef) { + Set descendants = new HashSet<>(); + for (TreeReference childrenRef : ec.expandReference(initialRef)) { + AbstractTreeElement child = ec.resolveReference(childrenRef); + descendants.add(child.getRef().genericize()); + descendants.addAll(getChildrenRefsOfElement(mainInstance, child)); } - return descendantRefs; + return descendants; } - // Recursive step of utility method - private Set getChildrenRefsOfElement(FormInstance mainInstance, AbstractTreeElement el) { + private Set getChildrenOfElement(FormInstance mainInstance, AbstractTreeElement element) { Set childrenRefs = new HashSet<>(); - TreeElement repeatTemplate = mainInstance.getTemplatePath(el.getRef()); - if (repeatTemplate != null) { - for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { - TreeElement child = repeatTemplate.getChildAt(i); - childrenRefs.add(child.getRef().genericize()); - childrenRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); - } - } else { - for (int i = 0; i < el.getNumChildren(); ++i) { - AbstractTreeElement child = el.getChildAt(i); - childrenRefs.add(child.getRef().genericize()); - childrenRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); - } + for (int i = 0; i < element.getNumChildren(); ++i) { + AbstractTreeElement child = element.getChildAt(i); + childrenRefs.add(child.getRef().genericize()); + childrenRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); } return childrenRefs; } From eaa0f8197392b302c26cfa3d469506d461f8e23e Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 12:17:25 +0100 Subject: [PATCH 12/51] Extract inner loop in cascade outside of the main loop Now the algorithm is easier to understand with no overhead (same number of iterations in the inner loop) --- .../javarosa/core/model/TriggerableDag.java | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 9c29bdfd8..a11371717 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -420,9 +420,9 @@ public void throwCyclesInDagException(Collection vertices) { */ public Set getDependantTriggerables(FormInstance mainInstance, EvaluationContext ec, QuickTriggerable triggerable) { Set allDependantTriggerables = new LinkedHashSet<>(); - + Set targets = new HashSet<>(); for (TreeReference target : triggerable.getTargets()) { - Set targets = new HashSet<>(); + targets.add(target); // For certain types of triggerables, the update will affect @@ -432,25 +432,25 @@ public Set getDependantTriggerables(FormInstance mainInstance, if (triggerable.isCascadingToChildren()) { targets.addAll(getChildrenOfReference(mainInstance, ec, target)); } + } - // Now go through each of these updated nodes (generally - // just 1 for a normal calculation, - // multiple nodes if there's a relevance cascade. - for (TreeReference ref : targets) { - // Check our index to see if that target is a Trigger - // for other conditions - // IE: if they are an element of a different calculation - // or relevancy calc - - // We can't make this reference generic before now or - // we'll lose the target information, - // so we'll be more inclusive than needed and see if any - // of our triggers are keyed on the predicate-less path - // of this ref - Set dependantTriggerables = triggerIndex.get(ref.hasPredicates() ? ref.removePredicates() : ref); - if (dependantTriggerables != null) - allDependantTriggerables.addAll(dependantTriggerables); - } + // Now go through each of these updated nodes (generally + // just 1 for a normal calculation, + // multiple nodes if there's a relevance cascade. + for (TreeReference target : targets) { + // Check our index to see if that target is a Trigger + // for other conditions + // IE: if they are an element of a different calculation + // or relevancy calc + + // We can't make this reference generic before now or + // we'll lose the target information, + // so we'll be more inclusive than needed and see if any + // of our triggers are keyed on the predicate-less path + // of this ref + Set dependantTriggerables = triggerIndex.get(target.hasPredicates() ? target.removePredicates() : target); + if (dependantTriggerables != null) + allDependantTriggerables.addAll(dependantTriggerables); } return allDependantTriggerables; } From 1f7d147db14ba9f3bc0ed8438b8549fb2ec50102 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 12:58:01 +0100 Subject: [PATCH 13/51] Inline for more concise code --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index a11371717..ff3907ac6 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -343,8 +343,7 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev if (triggerable.canCascade()) for (QuickTriggerable dependantTriggerable : dependantTriggerables) { - QuickTriggerable[] edge = {triggerable, dependantTriggerable}; - edges.add(edge); + edges.add(new QuickTriggerable[]{triggerable, dependantTriggerable}); } // TODO Move this from Triggerable to TriggerableDag From aaf611435ad6ec0e872635920e020d75bc521abe Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 12:33:21 +0100 Subject: [PATCH 14/51] Separate computing the DAG from assigning it We want to extract some methods to structure the process a bit more --- .../java/org/javarosa/core/model/TriggerableDag.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index ff3907ac6..d812b08ae 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -65,7 +65,7 @@ public interface EventNotifierAccessor { * the list, where tA comes before tB, evaluating tA cannot depend on any * result from evaluating tB. */ - protected final Set triggerablesDAG = new LinkedHashSet<>(); + protected Set triggerablesDAG = new LinkedHashSet<>(); /** * NOT VALID UNTIL finalizeTriggerables() is called!! @@ -327,8 +327,7 @@ public final Triggerable addTriggerable(Triggerable t) { * triggers can't be laid out appropriately */ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext evalContext) throws IllegalStateException { - triggerablesDAG.clear(); - + Set newTriggerablesDAG = new LinkedHashSet<>(); // TODO Study how this algorithm ensures that we follow the ancestor >>> descendant direction and if the sorting step is strictly required // DAGify the triggerables based on dependencies and sort them so that @@ -373,7 +372,7 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev // remove root nodes and edges originating from them // add them to the triggerablesDAG. for (QuickTriggerable root : orderedRoots) { - triggerablesDAG.add(root); + newTriggerablesDAG.add(root); vertices.remove(root); } for (int i = edges.size() - 1; i >= 0; i--) { @@ -383,6 +382,9 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev } } + triggerablesDAG.clear(); + triggerablesDAG = newTriggerablesDAG; + // // build the condition index for repeatable nodes // From 03254208ec2f84a1be437a7e0c4dae859ce17171 Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 12:33:52 +0100 Subject: [PATCH 15/51] Extract method that builds the DAG --- .../javarosa/core/model/TriggerableDag.java | 39 +++++++++++-------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index d812b08ae..a51ca9c43 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -327,6 +327,27 @@ public final Triggerable addTriggerable(Triggerable t) { * triggers can't be laid out appropriately */ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext evalContext) throws IllegalStateException { + Set newTriggerablesDAG = buildDag(mainInstance, evalContext); + + triggerablesDAG.clear(); + triggerablesDAG = newTriggerablesDAG; + + // + // build the condition index for repeatable nodes + // + conditionRepeatTargetIndex.clear(); + for (QuickTriggerable qt : triggerablesDAG) { + if (qt.isCondition()) { + for (TreeReference target : qt.getTargets()) { + if (mainInstance.getTemplate(target) != null) { + conditionRepeatTargetIndex.put(target, qt); + } + } + } + } + } + + public Set buildDag(FormInstance mainInstance, EvaluationContext evalContext) { Set newTriggerablesDAG = new LinkedHashSet<>(); // TODO Study how this algorithm ensures that we follow the ancestor >>> descendant direction and if the sorting step is strictly required @@ -381,23 +402,7 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev edges.remove(i); } } - - triggerablesDAG.clear(); - triggerablesDAG = newTriggerablesDAG; - - // - // build the condition index for repeatable nodes - // - conditionRepeatTargetIndex.clear(); - for (QuickTriggerable qt : triggerablesDAG) { - if (qt.isCondition()) { - for (TreeReference target : qt.getTargets()) { - if (mainInstance.getTemplate(target) != null) { - conditionRepeatTargetIndex.put(target, qt); - } - } - } - } + return newTriggerablesDAG; } public void throwCyclesInDagException(Collection vertices) { From d9961786debd1c1b73aa814663764f5549ddcacd Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 12:34:13 +0100 Subject: [PATCH 16/51] Simplify assigning a new DAG There's no need for clearing the collection if we're reassigning it --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index a51ca9c43..b080638d6 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -327,10 +327,7 @@ public final Triggerable addTriggerable(Triggerable t) { * triggers can't be laid out appropriately */ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext evalContext) throws IllegalStateException { - Set newTriggerablesDAG = buildDag(mainInstance, evalContext); - - triggerablesDAG.clear(); - triggerablesDAG = newTriggerablesDAG; + triggerablesDAG = buildDag(mainInstance, evalContext); // // build the condition index for repeatable nodes From ccc0087f19d9587f71df855235faf0616832d7ba Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 12:39:49 +0100 Subject: [PATCH 17/51] Pull up computing the edges to avoid passing through dependencies As a result, some methods can be made static. Also review visibility and set it to private where possible --- .../javarosa/core/model/TriggerableDag.java | 58 ++++++++++--------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index b080638d6..f8409146a 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -327,11 +327,11 @@ public final Triggerable addTriggerable(Triggerable t) { * triggers can't be laid out appropriately */ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext evalContext) throws IllegalStateException { - triggerablesDAG = buildDag(mainInstance, evalContext); + triggerablesDAG = buildDag( + unorderedTriggerables, + getDagEdges(mainInstance, evalContext, unorderedTriggerables, triggerIndex) + ); - // - // build the condition index for repeatable nodes - // conditionRepeatTargetIndex.clear(); for (QuickTriggerable qt : triggerablesDAG) { if (qt.isCondition()) { @@ -344,28 +344,13 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev } } - public Set buildDag(FormInstance mainInstance, EvaluationContext evalContext) { + public static Set buildDag(Set unorderedTriggerables, List edges) { Set newTriggerablesDAG = new LinkedHashSet<>(); // TODO Study how this algorithm ensures that we follow the ancestor >>> descendant direction and if the sorting step is strictly required // DAGify the triggerables based on dependencies and sort them so that // triggerables come only after the triggerables they depend on List vertices = new ArrayList<>(unorderedTriggerables); - List edges = new ArrayList<>(); - for (QuickTriggerable triggerable : vertices) { - Set dependantTriggerables = getDependantTriggerables(mainInstance, evalContext, triggerable); - - if (dependantTriggerables.contains(triggerable)) - throwCyclesInDagException(dependantTriggerables); - - if (triggerable.canCascade()) - for (QuickTriggerable dependantTriggerable : dependantTriggerables) { - edges.add(new QuickTriggerable[]{triggerable, dependantTriggerable}); - } - - // TODO Move this from Triggerable to TriggerableDag - triggerable.setImmediateCascades(dependantTriggerables); - } List orderedRoots = new ArrayList<>(); Set roots = new HashSet<>(vertices.size()); @@ -402,7 +387,26 @@ public Set buildDag(FormInstance mainInstance, EvaluationConte return newTriggerablesDAG; } - public void throwCyclesInDagException(Collection vertices) { + public static List getDagEdges(FormInstance mainInstance, EvaluationContext evalContext, Collection vertices, Map> triggerIndex) { + List edges = new ArrayList<>(); + for (QuickTriggerable triggerable : vertices) { + Set dependantTriggerables = getDependantTriggerables(mainInstance, evalContext, triggerable, triggerIndex); + + if (dependantTriggerables.contains(triggerable)) + throwCyclesInDagException(dependantTriggerables); + + if (triggerable.canCascade()) + for (QuickTriggerable dependantTriggerable : dependantTriggerables) { + edges.add(new QuickTriggerable[]{triggerable, dependantTriggerable}); + } + + // TODO Move this from Triggerable to TriggerableDag + triggerable.setImmediateCascades(dependantTriggerables); + } + return edges; + } + + public static void throwCyclesInDagException(Collection vertices) { StringBuilder hints = new StringBuilder(); for (QuickTriggerable qt2 : vertices) { for (TreeReference r : qt2.getTargets()) { @@ -421,7 +425,7 @@ public void throwCyclesInDagException(Collection vertices) { * Get all of the elements which will need to be evaluated (in order) when * the triggerable is fired. */ - public Set getDependantTriggerables(FormInstance mainInstance, EvaluationContext ec, QuickTriggerable triggerable) { + public static Set getDependantTriggerables(FormInstance mainInstance, EvaluationContext ec, QuickTriggerable triggerable, Map> triggerIndex) { Set allDependantTriggerables = new LinkedHashSet<>(); Set targets = new HashSet<>(); for (TreeReference target : triggerable.getTargets()) { @@ -498,7 +502,7 @@ private Set evaluateTriggerables(FormInstance mainInstance, Ev * * @return */ - private Set getChildrenOfReference(FormInstance mainInstance, EvaluationContext ec, TreeReference ref) { + private static Set getChildrenOfReference(FormInstance mainInstance, EvaluationContext ec, TreeReference ref) { TreeElement repeatTemplate = mainInstance.getTemplatePath(ref); if (repeatTemplate != null) return getChildrenRefsByTemplate(mainInstance, repeatTemplate); @@ -506,18 +510,18 @@ private Set getChildrenOfReference(FormInstance mainInstance, Eva return getChildrenByExpandedRef(mainInstance, ec, ref); } - private Set getChildrenRefsOfElement(FormInstance mainInstance, AbstractTreeElement element) { + private static Set getChildrenRefsOfElement(FormInstance mainInstance, AbstractTreeElement element) { TreeElement repeatTemplate = mainInstance.getTemplatePath(element.getRef()); if (repeatTemplate != null) return getChildrenRefsByTemplate(mainInstance, repeatTemplate); return getChildrenOfElement(mainInstance, element); } - private Set getChildrenRefsByTemplate(FormInstance mainInstance, TreeElement repeatTemplate) { + private static Set getChildrenRefsByTemplate(FormInstance mainInstance, TreeElement repeatTemplate) { return getChildrenOfElement(mainInstance, repeatTemplate); } - private Set getChildrenByExpandedRef(FormInstance mainInstance, EvaluationContext ec, TreeReference initialRef) { + private static Set getChildrenByExpandedRef(FormInstance mainInstance, EvaluationContext ec, TreeReference initialRef) { Set descendants = new HashSet<>(); for (TreeReference childrenRef : ec.expandReference(initialRef)) { AbstractTreeElement child = ec.resolveReference(childrenRef); @@ -527,7 +531,7 @@ private Set getChildrenByExpandedRef(FormInstance mainInstance, E return descendants; } - private Set getChildrenOfElement(FormInstance mainInstance, AbstractTreeElement element) { + private static Set getChildrenOfElement(FormInstance mainInstance, AbstractTreeElement element) { Set childrenRefs = new HashSet<>(); for (int i = 0; i < element.getNumChildren(); ++i) { AbstractTreeElement child = element.getChildAt(i); From a8b9c7af1b72cd1d2e28582b356ac10017b53480 Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 12:45:26 +0100 Subject: [PATCH 18/51] Renaming and sorting methods and variables for improved reading --- .../javarosa/core/model/TriggerableDag.java | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index f8409146a..de073ed48 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -16,6 +16,8 @@ package org.javarosa.core.model; +import static java.util.Collections.emptySet; + import java.io.DataInputStream; import java.io.DataOutputStream; import java.io.IOException; @@ -65,7 +67,7 @@ public interface EventNotifierAccessor { * the list, where tA comes before tB, evaluating tA cannot depend on any * result from evaluating tB. */ - protected Set triggerablesDAG = new LinkedHashSet<>(); + protected Set triggerablesDAG = emptySet(); /** * NOT VALID UNTIL finalizeTriggerables() is called!! @@ -344,8 +346,27 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev } } + // TODO We can avoid having to resolve dependant refs using the mainInstance by adding descendant refs as targets of relevance triggerables + private static List getDagEdges(FormInstance mainInstance, EvaluationContext evalContext, Set vertices, Map> triggerIndex) { + List edges = new ArrayList<>(); + for (QuickTriggerable vertex : vertices) { + Set dependants = getDependantTriggerables(mainInstance, evalContext, vertex, triggerIndex); + + if (dependants.contains(vertex)) + throwCyclesInDagException(dependants); + + if (vertex.canCascade()) + for (QuickTriggerable dependantVertex : dependants) + edges.add(new QuickTriggerable[]{vertex, dependantVertex}); + + // TODO Move this from Triggerable to TriggerableDag + vertex.setImmediateCascades(dependants); + } + return edges; + } + public static Set buildDag(Set unorderedTriggerables, List edges) { - Set newTriggerablesDAG = new LinkedHashSet<>(); + Set dag = new LinkedHashSet<>(); // TODO Study how this algorithm ensures that we follow the ancestor >>> descendant direction and if the sorting step is strictly required // DAGify the triggerables based on dependencies and sort them so that @@ -375,7 +396,7 @@ public static Set buildDag(Set unorderedTrig // remove root nodes and edges originating from them // add them to the triggerablesDAG. for (QuickTriggerable root : orderedRoots) { - newTriggerablesDAG.add(root); + dag.add(root); vertices.remove(root); } for (int i = edges.size() - 1; i >= 0; i--) { @@ -384,32 +405,13 @@ public static Set buildDag(Set unorderedTrig edges.remove(i); } } - return newTriggerablesDAG; - } - - public static List getDagEdges(FormInstance mainInstance, EvaluationContext evalContext, Collection vertices, Map> triggerIndex) { - List edges = new ArrayList<>(); - for (QuickTriggerable triggerable : vertices) { - Set dependantTriggerables = getDependantTriggerables(mainInstance, evalContext, triggerable, triggerIndex); - - if (dependantTriggerables.contains(triggerable)) - throwCyclesInDagException(dependantTriggerables); - - if (triggerable.canCascade()) - for (QuickTriggerable dependantTriggerable : dependantTriggerables) { - edges.add(new QuickTriggerable[]{triggerable, dependantTriggerable}); - } - - // TODO Move this from Triggerable to TriggerableDag - triggerable.setImmediateCascades(dependantTriggerables); - } - return edges; + return dag; } - public static void throwCyclesInDagException(Collection vertices) { + private static void throwCyclesInDagException(Collection triggerables) { StringBuilder hints = new StringBuilder(); - for (QuickTriggerable qt2 : vertices) { - for (TreeReference r : qt2.getTargets()) { + for (QuickTriggerable qt : triggerables) { + for (TreeReference r : qt.getTargets()) { hints.append("\n").append(r.toString(true)); } } @@ -425,7 +427,7 @@ public static void throwCyclesInDagException(Collection vertic * Get all of the elements which will need to be evaluated (in order) when * the triggerable is fired. */ - public static Set getDependantTriggerables(FormInstance mainInstance, EvaluationContext ec, QuickTriggerable triggerable, Map> triggerIndex) { + private static Set getDependantTriggerables(FormInstance mainInstance, EvaluationContext ec, QuickTriggerable triggerable, Map> triggerIndex) { Set allDependantTriggerables = new LinkedHashSet<>(); Set targets = new HashSet<>(); for (TreeReference target : triggerable.getTargets()) { From 2f6ddf816fd8e28842066d1b866d4602f481ca60 Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 12:52:52 +0100 Subject: [PATCH 19/51] Make the dag building algorithm immutable Now we compute new collections during each loop instead of mutating them --- .../javarosa/core/model/TriggerableDag.java | 48 ++++++++----------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index de073ed48..e078790db 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -365,45 +365,37 @@ private static List getDagEdges(FormInstance mainInstance, E return edges; } - public static Set buildDag(Set unorderedTriggerables, List edges) { + private static Set buildDag(Set vertices, List edges) { Set dag = new LinkedHashSet<>(); // TODO Study how this algorithm ensures that we follow the ancestor >>> descendant direction and if the sorting step is strictly required - // DAGify the triggerables based on dependencies and sort them so that - // triggerables come only after the triggerables they depend on - List vertices = new ArrayList<>(unorderedTriggerables); + Set remainingVertices = new HashSet<>(vertices); + Set remainingEdges = new HashSet<>(edges); + while (remainingVertices.size() > 0) { - List orderedRoots = new ArrayList<>(); - Set roots = new HashSet<>(vertices.size()); - while (vertices.size() > 0) { - // determine root nodes - roots.clear(); - roots.addAll(vertices); - for (QuickTriggerable[] edge : edges) { + Set roots = new HashSet<>(remainingVertices); + for (QuickTriggerable[] edge : remainingEdges) roots.remove(edge[1]); - } - // if no root nodes while graph still has nodes, graph has cycles if (roots.size() == 0) throwCyclesInDagException(vertices); - // order the root nodes - so the order is fixed - orderedRoots.clear(); - orderedRoots.addAll(roots); + List orderedRoots = new ArrayList<>(roots); // TODO Study if insertion order & using LinkedSets would suffice to ensure proper triggerable chaining Collections.sort(orderedRoots, QuickTriggerableComparator.INSTANCE); - - // remove root nodes and edges originating from them - // add them to the triggerablesDAG. - for (QuickTriggerable root : orderedRoots) { - dag.add(root); - vertices.remove(root); - } - for (int i = edges.size() - 1; i >= 0; i--) { - QuickTriggerable[] edge = edges.get(i); - if (roots.contains(edge[0])) - edges.remove(i); - } + dag.addAll(orderedRoots); + + Set newRemainingVertices = new HashSet<>(); + for (QuickTriggerable vertex : vertices) + if (!dag.contains(vertex)) + newRemainingVertices.add(vertex); + remainingVertices = newRemainingVertices; + + Set newRemainingEdges = new HashSet<>(); + for (QuickTriggerable[] edge : remainingEdges) + if (!roots.contains(edge[0])) + newRemainingEdges.add(edge); + remainingEdges = newRemainingEdges; } return dag; } From 04a39f093933a32d47923529d6340d5a7a74320a Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 12:53:47 +0100 Subject: [PATCH 20/51] Remove unnecessary sorting of the nodes added to the DAG --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index e078790db..5ca2eb7bf 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -23,7 +23,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; @@ -367,7 +366,6 @@ private static List getDagEdges(FormInstance mainInstance, E private static Set buildDag(Set vertices, List edges) { Set dag = new LinkedHashSet<>(); - // TODO Study how this algorithm ensures that we follow the ancestor >>> descendant direction and if the sorting step is strictly required Set remainingVertices = new HashSet<>(vertices); Set remainingEdges = new HashSet<>(edges); @@ -381,8 +379,6 @@ private static Set buildDag(Set vertices, Li throwCyclesInDagException(vertices); List orderedRoots = new ArrayList<>(roots); - // TODO Study if insertion order & using LinkedSets would suffice to ensure proper triggerable chaining - Collections.sort(orderedRoots, QuickTriggerableComparator.INSTANCE); dag.addAll(orderedRoots); Set newRemainingVertices = new HashSet<>(); From 6b7b52f4b48afebbbbea02f80f6a4e19642f8244 Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 12:54:06 +0100 Subject: [PATCH 21/51] Simplify by adding the roots collection directly --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 5ca2eb7bf..748d49f87 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -370,7 +370,6 @@ private static Set buildDag(Set vertices, Li Set remainingVertices = new HashSet<>(vertices); Set remainingEdges = new HashSet<>(edges); while (remainingVertices.size() > 0) { - Set roots = new HashSet<>(remainingVertices); for (QuickTriggerable[] edge : remainingEdges) roots.remove(edge[1]); @@ -378,8 +377,7 @@ private static Set buildDag(Set vertices, Li if (roots.size() == 0) throwCyclesInDagException(vertices); - List orderedRoots = new ArrayList<>(roots); - dag.addAll(orderedRoots); + dag.addAll(roots); Set newRemainingVertices = new HashSet<>(); for (QuickTriggerable vertex : vertices) From fc112c73587ba6cf06de552faef96e47463f1cca Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 14:23:41 +0100 Subject: [PATCH 22/51] Make member explicitly mutable This should help avoid misguiding future readers with that 'final' keyword --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 748d49f87..8a569b4fa 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -74,7 +74,7 @@ public interface EventNotifierAccessor { * Associates repeatable nodes with the Condition that determines their * relevance. */ - protected final Map conditionRepeatTargetIndex = new HashMap<>(); + protected Map conditionRepeatTargetIndex = new HashMap<>(); /** * Maps a tree reference to the set of triggerables that need to be @@ -333,16 +333,17 @@ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ev getDagEdges(mainInstance, evalContext, unorderedTriggerables, triggerIndex) ); - conditionRepeatTargetIndex.clear(); + Map newRepeatConditionsPerTarget = new HashMap<>(); for (QuickTriggerable qt : triggerablesDAG) { if (qt.isCondition()) { for (TreeReference target : qt.getTargets()) { if (mainInstance.getTemplate(target) != null) { - conditionRepeatTargetIndex.put(target, qt); + newRepeatConditionsPerTarget.put(target, qt); } } } } + conditionRepeatTargetIndex = newRepeatConditionsPerTarget; } // TODO We can avoid having to resolve dependant refs using the mainInstance by adding descendant refs as targets of relevance triggerables From 6157ffd2f8512942503adc2cb8cfdfeb7d4b1644 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 14:26:45 +0100 Subject: [PATCH 23/51] Improve names and context to understand what finalizing the DAG means --- .../javarosa/core/model/TriggerableDag.java | 38 ++++++++----------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 8a569b4fa..edc7115c4 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -74,7 +74,7 @@ public interface EventNotifierAccessor { * Associates repeatable nodes with the Condition that determines their * relevance. */ - protected Map conditionRepeatTargetIndex = new HashMap<>(); + protected Map repeatConditionsPerTargets = new HashMap<>(); /** * Maps a tree reference to the set of triggerables that need to be @@ -176,7 +176,7 @@ private List evaluateTriggerable(FormInstance mainInstance, Ev } public QuickTriggerable getTriggerableForRepeatGroup(TreeReference repeatRef) { - return conditionRepeatTargetIndex.get(repeatRef.genericize()); + return repeatConditionsPerTargets.get(repeatRef.genericize()); } private Set initializeTriggerables(FormInstance mainInstance, EvaluationContext evalContext, TreeReference rootRef, Set alreadyEvaluated) { @@ -323,27 +323,21 @@ public final Triggerable addTriggerable(Triggerable t) { * Finalize the DAG associated with the form's triggered conditions. This * will create the appropriate ordering and dependencies to ensure the * conditions will be evaluated in the appropriate orders. - * - * @throws IllegalStateException - If the trigger ordering contains an illegal cycle and the - * triggers can't be laid out appropriately */ - public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext evalContext) throws IllegalStateException { - triggerablesDAG = buildDag( - unorderedTriggerables, - getDagEdges(mainInstance, evalContext, unorderedTriggerables, triggerIndex) - ); - - Map newRepeatConditionsPerTarget = new HashMap<>(); - for (QuickTriggerable qt : triggerablesDAG) { - if (qt.isCondition()) { - for (TreeReference target : qt.getTargets()) { - if (mainInstance.getTemplate(target) != null) { - newRepeatConditionsPerTarget.put(target, qt); - } - } - } - } - conditionRepeatTargetIndex = newRepeatConditionsPerTarget; + public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ec) throws IllegalStateException { + List edges = getDagEdges(mainInstance, ec, unorderedTriggerables, triggerIndex); + triggerablesDAG = buildDag(unorderedTriggerables, edges); + repeatConditionsPerTargets = getRepeatConditionsPerTargets(mainInstance, triggerablesDAG); + } + + private static Map getRepeatConditionsPerTargets(FormInstance mainInstance, Set triggerables) { + Map repeatConditionsPerTargets = new HashMap<>(); + for (QuickTriggerable triggerable : triggerables) + if (triggerable.isCondition()) + for (TreeReference target : triggerable.getTargets()) + if (mainInstance.getTemplate(target) != null) + repeatConditionsPerTargets.put(target, triggerable); + return repeatConditionsPerTargets; } // TODO We can avoid having to resolve dependant refs using the mainInstance by adding descendant refs as targets of relevance triggerables From 8eaaff1c0b0c8c40a3043980f81342c4f8671f3d Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 16:27:47 +0100 Subject: [PATCH 24/51] Remove docblock comments These will be replaced in next commits with better naming and reviewed language --- .../javarosa/core/model/TriggerableDag.java | 38 ------------------- 1 file changed, 38 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index edc7115c4..70b4929dd 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -59,32 +59,12 @@ public interface EventNotifierAccessor { protected final EventNotifierAccessor accessor; - /** - * NOT VALID UNTIL finalizeTriggerables() is called!! - *

- * Topologically ordered list, meaning that for any tA and tB in - * the list, where tA comes before tB, evaluating tA cannot depend on any - * result from evaluating tB. - */ protected Set triggerablesDAG = emptySet(); - /** - * NOT VALID UNTIL finalizeTriggerables() is called!! - *

- * Associates repeatable nodes with the Condition that determines their - * relevance. - */ protected Map repeatConditionsPerTargets = new HashMap<>(); - /** - * Maps a tree reference to the set of triggerables that need to be - * processed when the value at this reference changes. - */ protected final Map> triggerIndex = new HashMap<>(); - /** - * List of all the triggerables in the form. Unordered. - */ protected final Set unorderedTriggerables = new HashSet<>(); protected TriggerableDag(EventNotifierAccessor accessor) { @@ -282,25 +262,7 @@ private QuickTriggerable findTriggerable(Triggerable t) { public final Triggerable addTriggerable(Triggerable t) { QuickTriggerable qt = findTriggerable(t); if (qt != null) { - // one node may control access to many nodes; this means many nodes - // effectively have the same condition - // let's identify when conditions are the same, and store and calculate - // it only once - - // nov-2-2011: ctsims - We need to merge the context nodes together - // whenever we do this (finding the highest - // common ground between the two), otherwise we can end up failing to - // trigger when the ignored context - // exists and the used one doesn't - - // TODO It's fishy to mutate the Triggerable here. We might prefer to return a copy of the original Triggerable return qt.changeContextRefToIntersectWithTriggerable(t); - - // note, if the contextRef is unnecessarily deep, the condition will be - // evaluated more times than needed - // perhaps detect when 'identical' condition has a shorter contextRef, - // and use that one instead? - } else { qt = QuickTriggerable.of(t); unorderedTriggerables.add(qt); From bffc1be83c68f0027984e0316f770e0a2725567c Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 16:30:02 +0100 Subject: [PATCH 25/51] Rename members for more clarity --- .../javarosa/core/model/TriggerableDag.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 70b4929dd..5aa002920 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -63,9 +63,9 @@ public interface EventNotifierAccessor { protected Map repeatConditionsPerTargets = new HashMap<>(); - protected final Map> triggerIndex = new HashMap<>(); + protected final Map> triggerablesPerTrigger = new HashMap<>(); - protected final Set unorderedTriggerables = new HashSet<>(); + protected final Set allTriggerables = new HashSet<>(); protected TriggerableDag(EventNotifierAccessor accessor) { this.accessor = accessor; @@ -251,7 +251,7 @@ public void copyItemsetAnswer(FormInstance mainInstance, EvaluationContext evalC } private QuickTriggerable findTriggerable(Triggerable t) { - for (QuickTriggerable qt : unorderedTriggerables) { + for (QuickTriggerable qt : allTriggerables) { if (qt.contains(t)) { return qt; } @@ -265,14 +265,14 @@ public final Triggerable addTriggerable(Triggerable t) { return qt.changeContextRefToIntersectWithTriggerable(t); } else { qt = QuickTriggerable.of(t); - unorderedTriggerables.add(qt); + allTriggerables.add(qt); Set triggers = t.getTriggers(); for (TreeReference trigger : triggers) { - Set triggered = triggerIndex.get(trigger); + Set triggered = triggerablesPerTrigger.get(trigger); if (triggered == null) { triggered = new HashSet<>(); - triggerIndex.put(trigger.clone(), triggered); + triggerablesPerTrigger.put(trigger.clone(), triggered); } triggered.add(qt); } @@ -287,8 +287,8 @@ public final Triggerable addTriggerable(Triggerable t) { * conditions will be evaluated in the appropriate orders. */ public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ec) throws IllegalStateException { - List edges = getDagEdges(mainInstance, ec, unorderedTriggerables, triggerIndex); - triggerablesDAG = buildDag(unorderedTriggerables, edges); + List edges = getDagEdges(mainInstance, ec, allTriggerables, triggerablesPerTrigger); + triggerablesDAG = buildDag(allTriggerables, edges); repeatConditionsPerTargets = getRepeatConditionsPerTargets(mainInstance, triggerablesDAG); } @@ -532,7 +532,7 @@ private Set triggerTriggerables(FormInstance mainInstance, Eva TreeReference genericRef = ref.genericize(); // get triggerables which are activated by the generic reference - Set triggered = triggerIndex.get(genericRef); + Set triggered = triggerablesPerTrigger.get(genericRef); if (triggered == null) { return alreadyEvaluated; } @@ -557,9 +557,9 @@ public void reportDependencyCycles() { //build graph List targets = new ArrayList<>(); - for (TreeReference trigger : triggerIndex.keySet()) { + for (TreeReference trigger : triggerablesPerTrigger.keySet()) { vertices.add(trigger); - Set triggered = triggerIndex.get(trigger); + Set triggered = triggerablesPerTrigger.get(trigger); targets.clear(); for (QuickTriggerable qt : triggered) { for (TreeReference target : qt.getTargets()) { @@ -635,7 +635,7 @@ public static List readExternalTriggerables(DataInputStream dis, Pr private List getConditions() { List conditions = new ArrayList<>(); - for (QuickTriggerable qt : unorderedTriggerables) { + for (QuickTriggerable qt : allTriggerables) { if (qt.isCondition()) { conditions.add((Condition) qt.getTriggerable()); } @@ -645,7 +645,7 @@ private List getConditions() { private List getRecalculates() { List recalculates = new ArrayList<>(); - for (QuickTriggerable qt : unorderedTriggerables) { + for (QuickTriggerable qt : allTriggerables) { if (qt.isRecalculate()) { recalculates.add((Recalculate) qt.getTriggerable()); } From 901156c578781f5669fd22bba8efe8ed65bcee27 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 16:30:17 +0100 Subject: [PATCH 26/51] Reorder members --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 5aa002920..5f068045b 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -59,13 +59,12 @@ public interface EventNotifierAccessor { protected final EventNotifierAccessor accessor; + protected final Set allTriggerables = new HashSet<>(); protected Set triggerablesDAG = emptySet(); protected Map repeatConditionsPerTargets = new HashMap<>(); - protected final Map> triggerablesPerTrigger = new HashMap<>(); - protected final Set allTriggerables = new HashSet<>(); protected TriggerableDag(EventNotifierAccessor accessor) { this.accessor = accessor; From 39d8cd0dfd7d0c4ff13dc9cf1381715064ff42f3 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 16:33:03 +0100 Subject: [PATCH 27/51] Review dag.addTriggerable to provide up-to-date context and detail --- .../javarosa/core/model/QuickTriggerable.java | 6 +-- .../javarosa/core/model/TriggerableDag.java | 49 ++++++++++++------- .../core/model/condition/Triggerable.java | 4 +- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/QuickTriggerable.java b/src/main/java/org/javarosa/core/model/QuickTriggerable.java index fddd2a04b..238178922 100644 --- a/src/main/java/org/javarosa/core/model/QuickTriggerable.java +++ b/src/main/java/org/javarosa/core/model/QuickTriggerable.java @@ -58,10 +58,8 @@ public boolean contains(Triggerable triggerable) { return this.triggerable.equals(triggerable); } - public Triggerable changeContextRefToIntersectWithTriggerable(Triggerable other) { - // TODO It's fishy to mutate the Triggerable here. We might prefer to return a copy of the original Triggerable - triggerable.changeContextRefToIntersectWithTriggerable(other); - return triggerable; + public void intersectContextWith(Triggerable other) { + triggerable.intersectContextWith(other); } public void setImmediateCascades(Set deps) { diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 5f068045b..94735eb96 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -258,26 +258,39 @@ private QuickTriggerable findTriggerable(Triggerable t) { return null; } - public final Triggerable addTriggerable(Triggerable t) { - QuickTriggerable qt = findTriggerable(t); - if (qt != null) { - return qt.changeContextRefToIntersectWithTriggerable(t); - } else { - qt = QuickTriggerable.of(t); - allTriggerables.add(qt); - - Set triggers = t.getTriggers(); - for (TreeReference trigger : triggers) { - Set triggered = triggerablesPerTrigger.get(trigger); - if (triggered == null) { - triggered = new HashSet<>(); - triggerablesPerTrigger.put(trigger.clone(), triggered); - } - triggered.add(qt); - } + /** + * Adds the provided triggerable to the DAG. + *

+ * This method has side-effects: + *

    + *
  • If a similar triggerable has been already added, its context gets intersected with the provided triggerable to cover both using only one entry
    • + *
  • This method builds the index of triggerables per trigger ref
    • + *
+ */ + Triggerable addTriggerable(Triggerable triggerable) { + QuickTriggerable existingQuickTriggerable = findTriggerable(triggerable); + if (existingQuickTriggerable != null) { + // We found a quick triggerable wrapping a triggerable matching the provided one. + // Since Triggerable.equals() doesn't have into account contexts or targets, the + // safest thing to do would be to create a new quick triggerable for the incoming + // triggerable, but we could cover both triggerables if we compute the intersection + // between their respective context and reuse the one we've already stored. + existingQuickTriggerable.intersectContextWith(triggerable); + return existingQuickTriggerable.getTriggerable(); + } + + QuickTriggerable newQuickTriggerable = QuickTriggerable.of(triggerable); + allTriggerables.add(newQuickTriggerable); - return t; + // Build the triggerable per trigger index + Set triggers = triggerable.getTriggers(); + for (TreeReference trigger : triggers) { + if (!triggerablesPerTrigger.containsKey(trigger)) + triggerablesPerTrigger.put(trigger, new HashSet<>()); + triggerablesPerTrigger.get(trigger).add(newQuickTriggerable); } + + return triggerable; } /** diff --git a/src/main/java/org/javarosa/core/model/condition/Triggerable.java b/src/main/java/org/javarosa/core/model/condition/Triggerable.java index c782bfb39..4e7460921 100644 --- a/src/main/java/org/javarosa/core/model/condition/Triggerable.java +++ b/src/main/java/org/javarosa/core/model/condition/Triggerable.java @@ -171,8 +171,8 @@ public Set getTargets() { return targets; } - public void changeContextRefToIntersectWithTriggerable(Triggerable t) { - contextRef = contextRef.intersect(t.contextRef); + public void intersectContextWith(Triggerable other) { + contextRef = contextRef.intersect(other.contextRef); } public TreeReference getContext() { From 0c012b30d2c1504109791663b1eb54a9d7dcd995 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 16:33:48 +0100 Subject: [PATCH 28/51] Add docblock detailing side effects that might not be apparent --- .../java/org/javarosa/core/model/TriggerableDag.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 94735eb96..af52aa0cd 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -315,6 +315,16 @@ private static Map getRepeatConditionsPerTarget } // TODO We can avoid having to resolve dependant refs using the mainInstance by adding descendant refs as targets of relevance triggerables + + /** + * Returns the list of edges in the DAG that can be built from the provided vertices. + *

+ * This method has side-effects: + *

    + *
  • Throws IllegalStateException when cycles are detected, either due to a self-reference or more complex cycle chains
    • + *
  • Builds a cache of immediate cascades of each vertex, meaning that we will remember the dependant vertices without having to traverse the DAG again
    • + *
+ */ private static List getDagEdges(FormInstance mainInstance, EvaluationContext evalContext, Set vertices, Map> triggerIndex) { List edges = new ArrayList<>(); for (QuickTriggerable vertex : vertices) { From ebb51f52c9d84bade73a912cf35f8981ecbed329 Mon Sep 17 00:00:00 2001 From: Guille Date: Tue, 17 Dec 2019 16:35:55 +0100 Subject: [PATCH 29/51] Avoid confusing value reassignation dance --- .../parse/StandardBindAttributesProcessor.java | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/javarosa/xform/parse/StandardBindAttributesProcessor.java b/src/main/java/org/javarosa/xform/parse/StandardBindAttributesProcessor.java index c1316b8b8..7f2a93963 100644 --- a/src/main/java/org/javarosa/xform/parse/StandardBindAttributesProcessor.java +++ b/src/main/java/org/javarosa/xform/parse/StandardBindAttributesProcessor.java @@ -59,9 +59,7 @@ DataBinding createBinding(IXFormParserFunctions parserFunctions, FormDef formDef } else if ("false()".equals(xpathRel)) { binding.relevantAbsolute = false; } else { - Triggerable c = buildCondition(xpathRel, "relevant", ref); - c = formDef.addTriggerable(c); - binding.relevancyCondition = c; + binding.relevancyCondition = formDef.addTriggerable(buildCondition(xpathRel, "relevant", ref)); } } @@ -72,9 +70,7 @@ DataBinding createBinding(IXFormParserFunctions parserFunctions, FormDef formDef } else if ("false()".equals(xpathReq)) { binding.requiredAbsolute = false; } else { - Triggerable c = buildCondition(xpathReq, "required", ref); - c = formDef.addTriggerable(c); - binding.requiredCondition = c; + binding.requiredCondition = formDef.addTriggerable(buildCondition(xpathReq, "required", ref)); } } @@ -85,9 +81,7 @@ DataBinding createBinding(IXFormParserFunctions parserFunctions, FormDef formDef } else if ("false()".equals(xpathRO)) { binding.readonlyAbsolute = false; } else { - Triggerable c = buildCondition(xpathRO, "readonly", ref); - c = formDef.addTriggerable(c); - binding.readonlyCondition = c; + binding.readonlyCondition = formDef.addTriggerable(buildCondition(xpathRO, "readonly", ref)); } } @@ -104,15 +98,13 @@ DataBinding createBinding(IXFormParserFunctions parserFunctions, FormDef formDef final String xpathCalc = element.getAttributeValue(null, "calculate"); if (xpathCalc != null) { - Triggerable r; try { - r = buildCalculate(xpathCalc, ref); + binding.calculate = formDef.addTriggerable(buildCalculate(xpathCalc, ref)); } catch (XPathSyntaxException xpse) { throw new XFormParseException("Invalid calculate for the bind attached to \"" + nodeset + "\" : " + xpse.getMessage() + " in expression " + xpathCalc); } - r = formDef.addTriggerable(r); - binding.calculate = r; + } binding.setPreload(element.getAttributeValue(NAMESPACE_JAVAROSA, "preload")); From 303e9689ef34055847f79d744882b17fac1d57b3 Mon Sep 17 00:00:00 2001 From: Guille Date: Fri, 20 Dec 2019 09:04:43 +0100 Subject: [PATCH 30/51] Remove unused imports --- src/main/java/org/javarosa/core/model/condition/Condition.java | 1 - src/main/java/org/javarosa/core/model/condition/Recalculate.java | 1 - 2 files changed, 2 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/condition/Condition.java b/src/main/java/org/javarosa/core/model/condition/Condition.java index 72122c116..d074a15b4 100644 --- a/src/main/java/org/javarosa/core/model/condition/Condition.java +++ b/src/main/java/org/javarosa/core/model/condition/Condition.java @@ -19,7 +19,6 @@ import java.io.DataInputStream; import java.io.DataOutputStream; import java.io.IOException; -import java.util.List; import java.util.Set; import org.javarosa.core.model.QuickTriggerable; import org.javarosa.core.model.instance.FormInstance; diff --git a/src/main/java/org/javarosa/core/model/condition/Recalculate.java b/src/main/java/org/javarosa/core/model/condition/Recalculate.java index 2ad47a649..8f8c6ca50 100644 --- a/src/main/java/org/javarosa/core/model/condition/Recalculate.java +++ b/src/main/java/org/javarosa/core/model/condition/Recalculate.java @@ -19,7 +19,6 @@ import java.io.DataInputStream; import java.io.DataOutputStream; import java.io.IOException; -import java.util.List; import java.util.Set; import org.javarosa.core.model.QuickTriggerable; import org.javarosa.core.model.data.IAnswerData; From 2bd11f6a78d4b39e4e384a7b6f26d9f408fd6535 Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 13:48:08 +0100 Subject: [PATCH 31/51] Avoid hitting the main instance to get the dependant triggerables Key change in this commit is to add as targets all descendants of a group node with a relevance condition --- .../java/org/javarosa/core/model/FormDef.java | 2 +- .../javarosa/core/model/TriggerableDag.java | 51 ++----- .../xform/parse/FormInstanceParser.java | 139 ++++++++++++------ 3 files changed, 111 insertions(+), 81 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/FormDef.java b/src/main/java/org/javarosa/core/model/FormDef.java index 488dc1919..e5fd76df6 100644 --- a/src/main/java/org/javarosa/core/model/FormDef.java +++ b/src/main/java/org/javarosa/core/model/FormDef.java @@ -687,7 +687,7 @@ public void finalizeTriggerables() throws IllegalStateException { // DAGify the triggerables based on dependencies and sort them so that // triggerables come only after the triggerables they depend on // - dagImpl.finalizeTriggerables(getMainInstance(), getEvaluationContext()); + dagImpl.finalizeTriggerables(getMainInstance()); } /** diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index af52aa0cd..438d14a31 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -298,8 +298,8 @@ Triggerable addTriggerable(Triggerable triggerable) { * will create the appropriate ordering and dependencies to ensure the * conditions will be evaluated in the appropriate orders. */ - public void finalizeTriggerables(FormInstance mainInstance, EvaluationContext ec) throws IllegalStateException { - List edges = getDagEdges(mainInstance, ec, allTriggerables, triggerablesPerTrigger); + public void finalizeTriggerables(FormInstance mainInstance) throws IllegalStateException { + List edges = getDagEdges(allTriggerables, triggerablesPerTrigger); triggerablesDAG = buildDag(allTriggerables, edges); repeatConditionsPerTargets = getRepeatConditionsPerTargets(mainInstance, triggerablesDAG); } @@ -325,10 +325,10 @@ private static Map getRepeatConditionsPerTarget *
  • Builds a cache of immediate cascades of each vertex, meaning that we will remember the dependant vertices without having to traverse the DAG again
    • * */ - private static List getDagEdges(FormInstance mainInstance, EvaluationContext evalContext, Set vertices, Map> triggerIndex) { + private static List getDagEdges(Set vertices, Map> triggerIndex) { List edges = new ArrayList<>(); for (QuickTriggerable vertex : vertices) { - Set dependants = getDependantTriggerables(mainInstance, evalContext, vertex, triggerIndex); + Set dependants = getDependantTriggerables(vertex, triggerIndex); if (dependants.contains(vertex)) throwCyclesInDagException(dependants); @@ -392,41 +392,16 @@ private static void throwCyclesInDagException(Collection trigg * Get all of the elements which will need to be evaluated (in order) when * the triggerable is fired. */ - private static Set getDependantTriggerables(FormInstance mainInstance, EvaluationContext ec, QuickTriggerable triggerable, Map> triggerIndex) { - Set allDependantTriggerables = new LinkedHashSet<>(); - Set targets = new HashSet<>(); - for (TreeReference target : triggerable.getTargets()) { - - targets.add(target); - - // For certain types of triggerables, the update will affect - // not only the target, but also the children of the target. - // In that case, we want to add all of those nodes - // to the list of updated elements as well. - if (triggerable.isCascadingToChildren()) { - targets.addAll(getChildrenOfReference(mainInstance, ec, target)); - } - } - - // Now go through each of these updated nodes (generally - // just 1 for a normal calculation, - // multiple nodes if there's a relevance cascade. - for (TreeReference target : targets) { - // Check our index to see if that target is a Trigger - // for other conditions - // IE: if they are an element of a different calculation - // or relevancy calc - - // We can't make this reference generic before now or - // we'll lose the target information, - // so we'll be more inclusive than needed and see if any - // of our triggers are keyed on the predicate-less path - // of this ref - Set dependantTriggerables = triggerIndex.get(target.hasPredicates() ? target.removePredicates() : target); - if (dependantTriggerables != null) - allDependantTriggerables.addAll(dependantTriggerables); + private static Set getDependantTriggerables(QuickTriggerable triggerable, Map> triggerIndex) { + Set result = new HashSet<>(); + for (TreeReference targetRef : triggerable.getTargets()) { + Set children = triggerIndex.getOrDefault(targetRef, emptySet()); + result.addAll(children); + for (QuickTriggerable child : children) + if (child != triggerable && !result.contains(child)) + result.addAll(getDependantTriggerables(child, triggerIndex)); } - return allDependantTriggerables; + return result; } /** diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java index 5823c3fd0..bca6379d4 100644 --- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java +++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java @@ -1,5 +1,15 @@ package org.javarosa.xform.parse; +import static org.javarosa.xform.parse.XFormParser.buildInstanceStructure; +import static org.javarosa.xform.parse.XFormParser.getVagueLocation; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; import org.javarosa.core.model.Constants; import org.javarosa.core.model.DataBinding; import org.javarosa.core.model.FormDef; @@ -10,6 +20,7 @@ import org.javarosa.core.model.QuestionDef; import org.javarosa.core.model.condition.Constraint; import org.javarosa.core.model.condition.EvaluationContext; +import org.javarosa.core.model.instance.AbstractTreeElement; import org.javarosa.core.model.instance.DataInstance; import org.javarosa.core.model.instance.FormInstance; import org.javarosa.core.model.instance.InvalidReferenceException; @@ -17,18 +28,9 @@ import org.javarosa.core.model.instance.TreeReference; import org.javarosa.xform.util.XFormUtils; import org.kxml2.kdom.Element; - -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.List; -import java.util.Map; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static org.javarosa.xform.parse.XFormParser.buildInstanceStructure; -import static org.javarosa.xform.parse.XFormParser.getVagueLocation; - class FormInstanceParser { private static final Logger logger = LoggerFactory.getLogger(FormInstanceParser.class); @@ -40,7 +42,9 @@ class FormInstanceParser { private final List selectOnes; private final List selectMultis; private final List actionTargets; - /** pseudo-data model tree that describes the repeat structure of the instance; useful during instance processing and validation */ + /** + * pseudo-data model tree that describes the repeat structure of the instance; useful during instance processing and validation + */ private FormInstance repeatTree; FormInstanceParser(FormDef formDef, String defaultNamespace, @@ -59,12 +63,12 @@ class FormInstanceParser { FormInstance parseInstance(Element e, boolean isMainInstance, String name, Map namespacePrefixesByUri) { TreeElement root = buildInstanceStructure(e, null, !isMainInstance ? name : null, e.getNamespace(), - namespacePrefixesByUri, null); + namespacePrefixesByUri, null); FormInstance instanceModel = new FormInstance(root); instanceModel.setName(isMainInstance ? formDef.getTitle() : name); final List usedAtts = Collections.unmodifiableList(Arrays.asList("id", "version", "uiVersion", "name", - "prefix", "delimiter")); + "prefix", "delimiter")); String schema = e.getNamespace(); if (schema != null && schema.length() > 0 && !schema.equals(defaultNamespace)) { @@ -87,7 +91,7 @@ FormInstance parseInstance(Element e, boolean isMainInstance, String name, Map missingTemplates = new ArrayList<>(); @@ -179,7 +185,7 @@ private void verifyBindings(FormInstance instance, String mainInstanceNodeName) verifyItemsetSrcDstCompatibility(instance); } - private void verifyActions (FormInstance instance) { + private void verifyActions(FormInstance instance) { //check the target of actions which are manipulating real values for (TreeReference target : actionTargets) { List nodes = new EvaluationContext(instance).expandReference(target, true); @@ -189,12 +195,12 @@ private void verifyActions (FormInstance instance) { } } - private static void checkDuplicateNodesAreRepeatable (TreeElement node) { + private static void checkDuplicateNodesAreRepeatable(TreeElement node) { int mult = node.getMult(); if (mult > 0) { //repeated node if (!node.isRepeatable()) { logger.warn("repeated nodes [{}] detected that have no repeat binding " + - "in the form; DO NOT bind questions to these nodes or their children!", + "in the form; DO NOT bind questions to these nodes or their children!", node.getName()); //we could do a more comprehensive safety check in the future } @@ -205,8 +211,10 @@ private static void checkDuplicateNodesAreRepeatable (TreeElement node) { } } - /** Checks repeat sets for homogeneity */ - private void checkHomogeneity (FormInstance instance) { + /** + * Checks repeat sets for homogeneity + */ + private void checkHomogeneity(FormInstance instance) { for (TreeReference ref : getRepeatableRefs()) { TreeElement template = null; for (TreeReference nref : new EvaluationContext(instance).expandReference(ref)) { @@ -224,7 +232,7 @@ private void checkHomogeneity (FormInstance instance) { } } - private void verifyControlBindings (IFormElement fe, FormInstance instance, List errors) { //throws XmlPullParserException { + private void verifyControlBindings(IFormElement fe, FormInstance instance, List errors) { //throws XmlPullParserException { if (fe.getChildren() == null) return; @@ -235,7 +243,7 @@ private void verifyControlBindings (IFormElement fe, FormInstance instance, List if (child instanceof GroupDef) { ref = child.getBind(); - type = (((GroupDef)child).getRepeat() ? "Repeat" : "Group"); + type = (((GroupDef) child).getRepeat() ? "Repeat" : "Group"); } else if (child instanceof QuestionDef) { ref = child.getBind(); type = "Question"; @@ -259,13 +267,13 @@ private void verifyControlBindings (IFormElement fe, FormInstance instance, List } } - private void verifyRepeatMemberBindings (IFormElement fe, FormInstance instance, GroupDef parentRepeat) { + private void verifyRepeatMemberBindings(IFormElement fe, FormInstance instance, GroupDef parentRepeat) { if (fe.getChildren() == null) return; for (int i = 0; i < fe.getChildren().size(); i++) { IFormElement child = fe.getChildren().get(i); - boolean isRepeat = (child instanceof GroupDef && ((GroupDef)child).getRepeat()); + boolean isRepeat = (child instanceof GroupDef && ((GroupDef) child).getRepeat()); //get bindings of current node and nearest enclosing repeat TreeReference repeatBind = (parentRepeat == null ? TreeReference.rootRef() : FormInstance.unpackReference(parentRepeat.getBind())); @@ -306,11 +314,11 @@ private void verifyRepeatMemberBindings (IFormElement fe, FormInstance instance, } } - verifyRepeatMemberBindings(child, instance, (isRepeat ? (GroupDef)child : parentRepeat)); + verifyRepeatMemberBindings(child, instance, (isRepeat ? (GroupDef) child : parentRepeat)); } } - private void verifyItemsetBindings (FormInstance instance) { + private void verifyItemsetBindings(FormInstance instance) { for (ItemsetBinding itemset : itemsets) { //check proper parent/child relationship if (!itemset.nodesetRef.isAncestorOf(itemset.labelRef, false)) { @@ -350,7 +358,7 @@ else if (itemset.valueRef != null && fi.getTemplatePath(itemset.valueRef) == nul } } - private void verifyItemsetSrcDstCompatibility (FormInstance instance) { + private void verifyItemsetSrcDstCompatibility(FormInstance instance) { for (ItemsetBinding itemset : itemsets) { boolean destRepeatable = (instance.getTemplate(itemset.getDestRef()) != null); if (itemset.copyMode) { @@ -378,13 +386,14 @@ private void verifyItemsetSrcDstCompatibility (FormInstance instance) { } } - private void applyInstanceProperties (FormInstance instance) { + private void applyInstanceProperties(FormInstance instance) { for (DataBinding bind : bindings) { final TreeReference ref = FormInstance.unpackReference(bind.getReference()); - final List nodes = new EvaluationContext(instance).expandReference(ref, true); + EvaluationContext ec = new EvaluationContext(instance); + final List nodes = ec.expandReference(ref, true); if (nodes.size() > 0) { - attachBindGeneral(bind); + attachBindGeneral(bind, instance, ec); } for (TreeReference nref : nodes) { attachBind(instance.resolveReference(nref), bind); @@ -394,11 +403,17 @@ private void applyInstanceProperties (FormInstance instance) { applyControlProperties(instance); } - private static void attachBindGeneral (DataBinding bind) { + private static void attachBindGeneral(DataBinding bind, FormInstance instance, EvaluationContext ec) { TreeReference ref = FormInstance.unpackReference(bind.getReference()); if (bind.relevancyCondition != null) { bind.relevancyCondition.addTarget(ref); + // Since relevancy can affect not only to individual fields, but also to + // groups, we need to register all descendant refs as targets for relevancy + // conditions to allow for chained reactions in triggerables registered in + // any of those descendants + for (TreeReference r : getDescendantRefs(instance, ec, ref)) + bind.relevancyCondition.addTarget(r); } if (bind.requiredCondition != null) { bind.requiredCondition.addTarget(ref); @@ -432,14 +447,54 @@ private static void attachBind(TreeElement node, DataBinding bind) { node.setBindAttributes(bind.getAdditionalAttributes()); } - /** Checks which repeat bindings have explicit template nodes; returns a list of the bindings that do not */ - private static void checkRepeatsForTemplate (FormInstance instance, FormInstance repeatTree, List missingTemplates) { + private static Set getDescendantRefs(FormInstance mainInstance, EvaluationContext evalContext, TreeReference target) { + Set allDescendants = new HashSet<>(); + for (AbstractTreeElement child : getChildrenElements(mainInstance, evalContext, target)) { + allDescendants.add(child.getRef().genericize()); + allDescendants.addAll(getDescendantRefs(child)); + } + return allDescendants; + } + + private static Set getDescendantRefs(AbstractTreeElement element) { + Set allDescendants = new HashSet<>(); + for (int i = 0; i < element.getNumChildren(); ++i) { + AbstractTreeElement child = element.getChildAt(i); + allDescendants.add(child.getRef().genericize()); + allDescendants.addAll(getDescendantRefs(child)); + } + return allDescendants; + } + + + private static Set> getChildrenElements(FormInstance mainInstance, EvaluationContext evalContext, TreeReference target) { + AbstractTreeElement repeatTemplate = mainInstance.getTemplatePath(target); + if (repeatTemplate != null) { + Set> elements = new HashSet<>(); + for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) + elements.add(repeatTemplate.getChildAt(i)); + return elements; + } + Set> elements = new HashSet<>(); + List refSet = evalContext.expandReference(target); + for (TreeReference ref : refSet) + elements.add(evalContext.resolveReference(ref)); + return elements; + } + + + /** + * Checks which repeat bindings have explicit template nodes; returns a list of the bindings that do not + */ + private static void checkRepeatsForTemplate(FormInstance instance, FormInstance repeatTree, List missingTemplates) { if (repeatTree != null) checkRepeatsForTemplate(repeatTree.getRoot(), TreeReference.rootRef(), instance, missingTemplates); } - /** Helper function for checkRepeatsForTemplate */ - private static void checkRepeatsForTemplate (TreeElement repeatTreeNode, TreeReference ref, FormInstance instance, List missing) { + /** + * Helper function for checkRepeatsForTemplate + */ + private static void checkRepeatsForTemplate(TreeElement repeatTreeNode, TreeReference ref, FormInstance instance, List missing) { String name = repeatTreeNode.getName(); int mult = (repeatTreeNode.isRepeatable() ? TreeReference.INDEX_TEMPLATE : 0); ref = ref.extendRef(name, mult); @@ -459,12 +514,12 @@ private static void checkRepeatsForTemplate (TreeElement repeatTreeNode, TreeRef //iterates through instance and removes template nodes that are not valid. a template is invalid if: // it is declared for a node that is not repeatable // it is for a repeat that is a child of another repeat and is not located within the parent's template node - private void removeInvalidTemplates (FormInstance instance, FormInstance repeatTree) { + private void removeInvalidTemplates(FormInstance instance, FormInstance repeatTree) { removeInvalidTemplates(instance.getRoot(), (repeatTree == null ? null : repeatTree.getRoot()), true); } //helper function for removeInvalidTemplates - private boolean removeInvalidTemplates (TreeElement instanceNode, TreeElement repeatTreeNode, boolean templateAllowed) { + private boolean removeInvalidTemplates(TreeElement instanceNode, TreeElement repeatTreeNode, boolean templateAllowed) { int mult = instanceNode.getMult(); boolean repeatable = repeatTreeNode != null && repeatTreeNode.isRepeatable(); @@ -496,7 +551,7 @@ private boolean removeInvalidTemplates (TreeElement instanceNode, TreeElement re } //if repeatables have no template node, duplicate first as template - private void createMissingTemplates (FormInstance instance, List missingTemplates) { + private void createMissingTemplates(FormInstance instance, List missingTemplates) { //it is VERY important that the missing template refs are listed in depth-first or breadth-first order... namely, that //every ref is listed after a ref that could be its parent. checkRepeatsForTemplate currently behaves this way for (TreeReference templRef : missingTemplates) { @@ -529,7 +584,7 @@ private void createMissingTemplates (FormInstance instance, List * Trims repeatable children of newly created template nodes; we trim because the templates are supposed to be devoid of 'data', * and # of repeats for a given repeat node is a kind of data. */ - private static void trimRepeatChildren (TreeElement node) { + private static void trimRepeatChildren(TreeElement node) { for (int i = 0; i < node.getNumChildren(); i++) { TreeElement child = node.getChildAt(i); if (child.isRepeatable()) { @@ -567,7 +622,7 @@ private void applyControlProperties(FormInstance instance) { } } - private List getRepeatableRefs () { + private List getRepeatableRefs() { List refs = new ArrayList<>(repeats); for (ItemsetBinding itemset : itemsets) { @@ -611,7 +666,7 @@ private List getRepeatableRefs () { * result is a FormInstance collapsed where all indexes are 0, and repeatable nodes are flagged as such. * Ignores (invalid) repeats that bind outside the top-level instance data node. Returns null if no repeats. */ - private static FormInstance buildRepeatTree (List repeatRefs, String topLevelName) { + private static FormInstance buildRepeatTree(List repeatRefs, String topLevelName) { TreeElement root = new TreeElement(null, 0); for (TreeReference repeatRef : repeatRefs) { @@ -639,6 +694,6 @@ private static FormInstance buildRepeatTree (List repeatRefs, Str } return (root.getNumChildren() == 0) ? null : - new FormInstance(root.getChild(topLevelName, TreeReference.DEFAULT_MULTIPLICITY)); + new FormInstance(root.getChild(topLevelName, TreeReference.DEFAULT_MULTIPLICITY)); } } From 1ddaed6ca91334ddc2fd1e8898a0bffa1d4fcfd7 Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 13:49:38 +0100 Subject: [PATCH 32/51] Inline methods only used once We want to reuse variables in a more clever way, which is easier if everything belongs to the same scoep --- .../xform/parse/FormInstanceParser.java | 85 +++++++++---------- 1 file changed, 39 insertions(+), 46 deletions(-) diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java index bca6379d4..49b921837 100644 --- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java +++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java @@ -393,58 +393,51 @@ private void applyInstanceProperties(FormInstance instance) { final List nodes = ec.expandReference(ref, true); if (nodes.size() > 0) { - attachBindGeneral(bind, instance, ec); + TreeReference ref1 = FormInstance.unpackReference(bind.getReference()); + + if (bind.relevancyCondition != null) { + bind.relevancyCondition.addTarget(ref1); + // Since relevancy can affect not only to individual fields, but also to + // groups, we need to register all descendant refs as targets for relevancy + // conditions to allow for chained reactions in triggerables registered in + // any of those descendants + for (TreeReference r : getDescendantRefs(instance, ec, ref1)) + bind.relevancyCondition.addTarget(r); + } + if (bind.requiredCondition != null) { + bind.requiredCondition.addTarget(ref1); + } + if (bind.readonlyCondition != null) { + bind.readonlyCondition.addTarget(ref1); + } + if (bind.calculate != null) { + bind.calculate.addTarget(ref1); + } } for (TreeReference nref : nodes) { - attachBind(instance.resolveReference(nref), bind); - } - } - - applyControlProperties(instance); - } - - private static void attachBindGeneral(DataBinding bind, FormInstance instance, EvaluationContext ec) { - TreeReference ref = FormInstance.unpackReference(bind.getReference()); - - if (bind.relevancyCondition != null) { - bind.relevancyCondition.addTarget(ref); - // Since relevancy can affect not only to individual fields, but also to - // groups, we need to register all descendant refs as targets for relevancy - // conditions to allow for chained reactions in triggerables registered in - // any of those descendants - for (TreeReference r : getDescendantRefs(instance, ec, ref)) - bind.relevancyCondition.addTarget(r); - } - if (bind.requiredCondition != null) { - bind.requiredCondition.addTarget(ref); - } - if (bind.readonlyCondition != null) { - bind.readonlyCondition.addTarget(ref); - } - if (bind.calculate != null) { - bind.calculate.addTarget(ref); - } - } + TreeElement node = instance.resolveReference(nref); + node.setDataType(bind.getDataType()); - private static void attachBind(TreeElement node, DataBinding bind) { - node.setDataType(bind.getDataType()); + if (bind.relevancyCondition == null) { + node.setRelevant(bind.relevantAbsolute); + } + if (bind.requiredCondition == null) { + node.setRequired(bind.requiredAbsolute); + } + if (bind.readonlyCondition == null) { + node.setEnabled(!bind.readonlyAbsolute); + } + if (bind.constraint != null) { + node.setConstraint(new Constraint(bind.constraint, bind.constraintMessage)); + } - if (bind.relevancyCondition == null) { - node.setRelevant(bind.relevantAbsolute); - } - if (bind.requiredCondition == null) { - node.setRequired(bind.requiredAbsolute); - } - if (bind.readonlyCondition == null) { - node.setEnabled(!bind.readonlyAbsolute); - } - if (bind.constraint != null) { - node.setConstraint(new Constraint(bind.constraint, bind.constraintMessage)); + node.setPreloadHandler(bind.getPreload()); + node.setPreloadParams(bind.getPreloadParams()); + node.setBindAttributes(bind.getAdditionalAttributes()); + } } - node.setPreloadHandler(bind.getPreload()); - node.setPreloadParams(bind.getPreloadParams()); - node.setBindAttributes(bind.getAdditionalAttributes()); + applyControlProperties(instance); } private static Set getDescendantRefs(FormInstance mainInstance, EvaluationContext evalContext, TreeReference target) { From d594ef49ba480feafd2ff4c121eaa99d0d728ef3 Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 13:49:52 +0100 Subject: [PATCH 33/51] Reuse the ref object --- .../org/javarosa/xform/parse/FormInstanceParser.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java index 49b921837..3fef5c5f4 100644 --- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java +++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java @@ -393,25 +393,23 @@ private void applyInstanceProperties(FormInstance instance) { final List nodes = ec.expandReference(ref, true); if (nodes.size() > 0) { - TreeReference ref1 = FormInstance.unpackReference(bind.getReference()); - if (bind.relevancyCondition != null) { - bind.relevancyCondition.addTarget(ref1); + bind.relevancyCondition.addTarget(ref); // Since relevancy can affect not only to individual fields, but also to // groups, we need to register all descendant refs as targets for relevancy // conditions to allow for chained reactions in triggerables registered in // any of those descendants - for (TreeReference r : getDescendantRefs(instance, ec, ref1)) + for (TreeReference r : getDescendantRefs(instance, ec, ref)) bind.relevancyCondition.addTarget(r); } if (bind.requiredCondition != null) { - bind.requiredCondition.addTarget(ref1); + bind.requiredCondition.addTarget(ref); } if (bind.readonlyCondition != null) { - bind.readonlyCondition.addTarget(ref1); + bind.readonlyCondition.addTarget(ref); } if (bind.calculate != null) { - bind.calculate.addTarget(ref1); + bind.calculate.addTarget(ref); } } for (TreeReference nref : nodes) { From d28f98aa64e635e96e0c5d6235cf8454515ba6ca Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 13:50:12 +0100 Subject: [PATCH 34/51] Remove redundant check If there was no node then there couldn't be a bind --- .../xform/parse/FormInstanceParser.java | 36 +++++++++---------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java index 3fef5c5f4..3d1edb987 100644 --- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java +++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java @@ -392,25 +392,23 @@ private void applyInstanceProperties(FormInstance instance) { EvaluationContext ec = new EvaluationContext(instance); final List nodes = ec.expandReference(ref, true); - if (nodes.size() > 0) { - if (bind.relevancyCondition != null) { - bind.relevancyCondition.addTarget(ref); - // Since relevancy can affect not only to individual fields, but also to - // groups, we need to register all descendant refs as targets for relevancy - // conditions to allow for chained reactions in triggerables registered in - // any of those descendants - for (TreeReference r : getDescendantRefs(instance, ec, ref)) - bind.relevancyCondition.addTarget(r); - } - if (bind.requiredCondition != null) { - bind.requiredCondition.addTarget(ref); - } - if (bind.readonlyCondition != null) { - bind.readonlyCondition.addTarget(ref); - } - if (bind.calculate != null) { - bind.calculate.addTarget(ref); - } + if (bind.relevancyCondition != null) { + bind.relevancyCondition.addTarget(ref); + // Since relevancy can affect not only to individual fields, but also to + // groups, we need to register all descendant refs as targets for relevancy + // conditions to allow for chained reactions in triggerables registered in + // any of those descendants + for (TreeReference r : getDescendantRefs(instance, ec, ref)) + bind.relevancyCondition.addTarget(r); + } + if (bind.requiredCondition != null) { + bind.requiredCondition.addTarget(ref); + } + if (bind.readonlyCondition != null) { + bind.readonlyCondition.addTarget(ref); + } + if (bind.calculate != null) { + bind.calculate.addTarget(ref); } for (TreeReference nref : nodes) { TreeElement node = instance.resolveReference(nref); From b4c83c2bea102b9f6f044dd772eb1b6e60cb2614 Mon Sep 17 00:00:00 2001 From: Guille Date: Wed, 8 Jan 2020 13:51:47 +0100 Subject: [PATCH 35/51] Clean up code: naming, simplify, comments --- .../xform/parse/FormInstanceParser.java | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java index 3d1edb987..1777e6c4e 100644 --- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java +++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java @@ -388,10 +388,11 @@ private void verifyItemsetSrcDstCompatibility(FormInstance instance) { private void applyInstanceProperties(FormInstance instance) { for (DataBinding bind : bindings) { - final TreeReference ref = FormInstance.unpackReference(bind.getReference()); + TreeReference ref = FormInstance.unpackReference(bind.getReference()); EvaluationContext ec = new EvaluationContext(instance); - final List nodes = ec.expandReference(ref, true); + List nodeRefs = ec.expandReference(ref, true); + // Add triggerable targets if needed if (bind.relevancyCondition != null) { bind.relevancyCondition.addTarget(ref); // Since relevancy can affect not only to individual fields, but also to @@ -401,31 +402,26 @@ private void applyInstanceProperties(FormInstance instance) { for (TreeReference r : getDescendantRefs(instance, ec, ref)) bind.relevancyCondition.addTarget(r); } - if (bind.requiredCondition != null) { + if (bind.requiredCondition != null) bind.requiredCondition.addTarget(ref); - } - if (bind.readonlyCondition != null) { + if (bind.readonlyCondition != null) bind.readonlyCondition.addTarget(ref); - } - if (bind.calculate != null) { + if (bind.calculate != null) bind.calculate.addTarget(ref); - } - for (TreeReference nref : nodes) { - TreeElement node = instance.resolveReference(nref); + + // Initialize present nodes under the provided ref + for (TreeReference nodeRef : nodeRefs) { + TreeElement node = instance.resolveReference(nodeRef); node.setDataType(bind.getDataType()); - if (bind.relevancyCondition == null) { + if (bind.relevancyCondition == null) node.setRelevant(bind.relevantAbsolute); - } - if (bind.requiredCondition == null) { + if (bind.requiredCondition == null) node.setRequired(bind.requiredAbsolute); - } - if (bind.readonlyCondition == null) { + if (bind.readonlyCondition == null) node.setEnabled(!bind.readonlyAbsolute); - } - if (bind.constraint != null) { + if (bind.constraint != null) node.setConstraint(new Constraint(bind.constraint, bind.constraintMessage)); - } node.setPreloadHandler(bind.getPreload()); node.setPreloadParams(bind.getPreloadParams()); From 47ee88e05685b19b83dfd96121316665f452a4a6 Mon Sep 17 00:00:00 2001 From: Guille Date: Thu, 9 Jan 2020 10:44:41 +0100 Subject: [PATCH 36/51] Replace construction that's not compatible with Android API level 16 --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 438d14a31..7719e27c2 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -395,7 +395,9 @@ private static void throwCyclesInDagException(Collection trigg private static Set getDependantTriggerables(QuickTriggerable triggerable, Map> triggerIndex) { Set result = new HashSet<>(); for (TreeReference targetRef : triggerable.getTargets()) { - Set children = triggerIndex.getOrDefault(targetRef, emptySet()); + Set children = triggerIndex.containsKey(targetRef) + ? triggerIndex.get(targetRef) + : emptySet(); result.addAll(children); for (QuickTriggerable child : children) if (child != triggerable && !result.contains(child)) From 1e3cf39c02b93fafdceea88cde11507830ec600e Mon Sep 17 00:00:00 2001 From: Guille Date: Thu, 9 Jan 2020 10:50:39 +0100 Subject: [PATCH 37/51] Remove unused methods --- .../javarosa/core/model/TriggerableDag.java | 45 ------------------- 1 file changed, 45 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 7719e27c2..e330a0226 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -440,51 +440,6 @@ private Set evaluateTriggerables(FormInstance mainInstance, Ev return doEvaluateTriggerables(mainInstance, evalContext, tv, anchorRef, alreadyEvaluated); } - /** - * This is a utility method to get all of the references of a node. It can - * be replaced when we support dependent XPath Steps (IE: /path/to//) - * - * @return - */ - private static Set getChildrenOfReference(FormInstance mainInstance, EvaluationContext ec, TreeReference ref) { - TreeElement repeatTemplate = mainInstance.getTemplatePath(ref); - if (repeatTemplate != null) - return getChildrenRefsByTemplate(mainInstance, repeatTemplate); - // TODO Write a test that goes through this path and see how and why childrenRef can be different than child.getRef() - return getChildrenByExpandedRef(mainInstance, ec, ref); - } - - private static Set getChildrenRefsOfElement(FormInstance mainInstance, AbstractTreeElement element) { - TreeElement repeatTemplate = mainInstance.getTemplatePath(element.getRef()); - if (repeatTemplate != null) - return getChildrenRefsByTemplate(mainInstance, repeatTemplate); - return getChildrenOfElement(mainInstance, element); - } - - private static Set getChildrenRefsByTemplate(FormInstance mainInstance, TreeElement repeatTemplate) { - return getChildrenOfElement(mainInstance, repeatTemplate); - } - - private static Set getChildrenByExpandedRef(FormInstance mainInstance, EvaluationContext ec, TreeReference initialRef) { - Set descendants = new HashSet<>(); - for (TreeReference childrenRef : ec.expandReference(initialRef)) { - AbstractTreeElement child = ec.resolveReference(childrenRef); - descendants.add(child.getRef().genericize()); - descendants.addAll(getChildrenRefsOfElement(mainInstance, child)); - } - return descendants; - } - - private static Set getChildrenOfElement(FormInstance mainInstance, AbstractTreeElement element) { - Set childrenRefs = new HashSet<>(); - for (int i = 0; i < element.getNumChildren(); ++i) { - AbstractTreeElement child = element.getChildAt(i); - childrenRefs.add(child.getRef().genericize()); - childrenRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); - } - return childrenRefs; - } - /** * Walks the current set of conditions, and evaluates each of them with the * current context. From 86db7bf0ffb8bed50a24cb541c1be4133305f7e1 Mon Sep 17 00:00:00 2001 From: Guille Date: Thu, 9 Jan 2020 11:00:37 +0100 Subject: [PATCH 38/51] Add comments explaining how the sets in the dag build get mutated --- .../javarosa/core/model/TriggerableDag.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index e330a0226..be040e67b 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -35,7 +35,6 @@ import org.javarosa.core.model.condition.EvaluationContext; import org.javarosa.core.model.condition.Recalculate; import org.javarosa.core.model.condition.Triggerable; -import org.javarosa.core.model.instance.AbstractTreeElement; import org.javarosa.core.model.instance.FormInstance; import org.javarosa.core.model.instance.TreeElement; import org.javarosa.core.model.instance.TreeReference; @@ -344,11 +343,18 @@ private static List getDagEdges(Set vertic } private static Set buildDag(Set vertices, List edges) { + // The dag and the set of remaining vertices will be mutated + // inside the while loop's block Set dag = new LinkedHashSet<>(); - Set remainingVertices = new HashSet<>(vertices); + + // The set of remaining edges will be replaced inside + // the while loop's block Set remainingEdges = new HashSet<>(edges); + while (remainingVertices.size() > 0) { + // Compute the set of roots (nodes that don't show up + // as edge targets) with the remaining vertices Set roots = new HashSet<>(remainingVertices); for (QuickTriggerable[] edge : remainingEdges) roots.remove(edge[1]); @@ -356,14 +362,12 @@ private static Set buildDag(Set vertices, Li if (roots.size() == 0) throwCyclesInDagException(vertices); + // "Move" the roots detected during this iteration + // from the remainingVertices to the DAG + remainingVertices.removeAll(roots); dag.addAll(roots); - Set newRemainingVertices = new HashSet<>(); - for (QuickTriggerable vertex : vertices) - if (!dag.contains(vertex)) - newRemainingVertices.add(vertex); - remainingVertices = newRemainingVertices; - + // Compute the new set of remaining edges to continue the iteration Set newRemainingEdges = new HashSet<>(); for (QuickTriggerable[] edge : remainingEdges) if (!roots.contains(edge[0])) From 150a469aae8de06a805ba97ac2a5c51b485e745e Mon Sep 17 00:00:00 2001 From: Guille Date: Fri, 10 Jan 2020 09:53:19 +0100 Subject: [PATCH 39/51] Add doc blocks for TriggerableDag members. Wrap at 80 chars --- .../javarosa/core/model/TriggerableDag.java | 64 ++++++++++++++++--- 1 file changed, 56 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index be040e67b..e40bbc7be 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -56,12 +56,54 @@ public interface EventNotifierAccessor { EventNotifier getEventNotifier(); } + /** + * Stores the event notifier required to create instances of this class. + *

    + * The event notifier is used to publish events that serve as a form for + * loose coupling between processes that could be running at the same time + * in a JavaRosa client. + */ protected final EventNotifierAccessor accessor; + /** + * Stores the unsorted set of all triggerables present in the form. + *

    + * This set is used during the DAG build process. + */ protected final Set allTriggerables = new HashSet<>(); + + /** + * Stores the sorted set of all triggerables using the dependency direction + * as ordering. + *

    + * Triggerables present in this set depend exclusively on preceding + * triggerables. + */ protected Set triggerablesDAG = emptySet(); + // TODO Make this member fit the expected behavior on calling sites by containing only relevance conditions + /** + * Stores an index for conditions (triggerables declared in + * readonly, required, or relevant + * attributes) belonging to repeat groups. + *

    + * This index is used to determine whether a repeat group instance is + * relevant or not. + *

    + * Warning: Calling site assumes that a repeat group would only have + * one object stored for its reference in this map. This is because, so + * far, relevant is the only attribute that makes sense adding + * to a repeat group, but a form could declare other conditions as well, + * leading to an unexpected scenario. + */ protected Map repeatConditionsPerTargets = new HashMap<>(); + /** + * Stores an index to resolve triggerables by their corresponding trigger's + * reference. + *

    + * Note that there's a m:n relationship between trigger references and + * triggerables. + */ protected final Map> triggerablesPerTrigger = new HashMap<>(); @@ -262,7 +304,9 @@ private QuickTriggerable findTriggerable(Triggerable t) { *

    * This method has side-effects: *

      - *
    • If a similar triggerable has been already added, its context gets intersected with the provided triggerable to cover both using only one entry
      • + *
    • If a similar triggerable has been already added, its context gets + * intersected with the provided triggerable to cover both using only + * one entry
      • *
    • This method builds the index of triggerables per trigger ref
      • *
    */ @@ -316,12 +360,16 @@ private static Map getRepeatConditionsPerTarget // TODO We can avoid having to resolve dependant refs using the mainInstance by adding descendant refs as targets of relevance triggerables /** - * Returns the list of edges in the DAG that can be built from the provided vertices. + * Returns the list of edges in the DAG that can be built from the provided + * vertices. *

    * This method has side-effects: *

      - *
    • Throws IllegalStateException when cycles are detected, either due to a self-reference or more complex cycle chains
      • - *
    • Builds a cache of immediate cascades of each vertex, meaning that we will remember the dependant vertices without having to traverse the DAG again
      • + *
    • Throws IllegalStateException when cycles are detected, either due + * to a self-reference or more complex cycle chains
      • + *
    • Builds a cache of immediate cascades of each vertex, meaning that + * we will remember the dependant vertices without having to traverse + * the DAG again
      • *
    */ private static List getDagEdges(Set vertices, Map> triggerIndex) { @@ -453,10 +501,10 @@ public Collection initializeTriggerables(FormInstance mainInst } /** - * Invoked to validate a filled-in form. Sweeps through from beginning - * to end, confirming that the entered values satisfy all constraints. - * The FormEntryController is based upon the FormDef, but has its own - * model and controller independent of anything at the UI layer. + * Invoked to validate a filled-in form. Sweeps through from beginning to + * end, confirming that the entered values satisfy all constraints. The + * FormEntryController is based upon the FormDef, but has its own model and + * controller independent of anything at the UI layer. */ public ValidateOutcome validate(FormEntryController formEntryControllerToBeValidated, boolean markCompleted) { From 8c4ef1e1e5db2612dcc4aa2b9763480d062f2a08 Mon Sep 17 00:00:00 2001 From: Guille Date: Fri, 10 Jan 2020 10:26:59 +0100 Subject: [PATCH 40/51] Make the DAG edge building algorithm easier to understand The inlined method was confusing and misguiding because the nested loop didn't make much sense, and because the naming suggested that the method would return all descendants recursively, which neither was needed or actually happening. Then, the naming wasn't helping to understand what was going on either. By using a language closer to DAG terminology, I hope it's easier now to understand what the buildDagEdges method does. --- .../javarosa/core/model/TriggerableDag.java | 46 +++++++------------ 1 file changed, 16 insertions(+), 30 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index e40bbc7be..4d19fd199 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -357,8 +357,6 @@ private static Map getRepeatConditionsPerTarget return repeatConditionsPerTargets; } - // TODO We can avoid having to resolve dependant refs using the mainInstance by adding descendant refs as targets of relevance triggerables - /** * Returns the list of edges in the DAG that can be built from the provided * vertices. @@ -374,18 +372,24 @@ private static Map getRepeatConditionsPerTarget */ private static List getDagEdges(Set vertices, Map> triggerIndex) { List edges = new ArrayList<>(); - for (QuickTriggerable vertex : vertices) { - Set dependants = getDependantTriggerables(vertex, triggerIndex); - - if (dependants.contains(vertex)) - throwCyclesInDagException(dependants); - - if (vertex.canCascade()) - for (QuickTriggerable dependantVertex : dependants) - edges.add(new QuickTriggerable[]{vertex, dependantVertex}); + for (QuickTriggerable source : vertices) { + // Compute the set of direct children triggerables of this source vertex + Set targets = new HashSet<>(); + for (TreeReference targetRef : source.getTargets()) + targets.addAll(triggerIndex.containsKey(targetRef) + ? triggerIndex.get(targetRef) + : emptySet()); + + // Account for cycles by self-reference + if (targets.contains(source)) + throwCyclesInDagException(targets); + + if (source.canCascade()) + for (QuickTriggerable target : targets) + edges.add(new QuickTriggerable[]{source, target}); // TODO Move this from Triggerable to TriggerableDag - vertex.setImmediateCascades(dependants); + source.setImmediateCascades(targets); } return edges; } @@ -440,24 +444,6 @@ private static void throwCyclesInDagException(Collection trigg throw new IllegalStateException(message); } - /** - * Get all of the elements which will need to be evaluated (in order) when - * the triggerable is fired. - */ - private static Set getDependantTriggerables(QuickTriggerable triggerable, Map> triggerIndex) { - Set result = new HashSet<>(); - for (TreeReference targetRef : triggerable.getTargets()) { - Set children = triggerIndex.containsKey(targetRef) - ? triggerIndex.get(targetRef) - : emptySet(); - result.addAll(children); - for (QuickTriggerable child : children) - if (child != triggerable && !result.contains(child)) - result.addAll(getDependantTriggerables(child, triggerIndex)); - } - return result; - } - /** * Step 2 in evaluating DAG computation updates from a value being changed * in the instance. This step is responsible for taking the root set of From 11537bf51e38264df6babb28e8efba43427b0ccb Mon Sep 17 00:00:00 2001 From: Guille Date: Fri, 10 Jan 2020 17:04:14 +0100 Subject: [PATCH 41/51] Remove redundant check If source can't cascade, then the targets set will be empty. --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 4d19fd199..c0d409e49 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -384,9 +384,8 @@ private static List getDagEdges(Set vertic if (targets.contains(source)) throwCyclesInDagException(targets); - if (source.canCascade()) - for (QuickTriggerable target : targets) - edges.add(new QuickTriggerable[]{source, target}); + for (QuickTriggerable target : targets) + edges.add(new QuickTriggerable[]{source, target}); // TODO Move this from Triggerable to TriggerableDag source.setImmediateCascades(targets); From 6e9aba96813a4806f95b77806aaefae24df4f458 Mon Sep 17 00:00:00 2001 From: Guille Date: Fri, 10 Jan 2020 16:56:25 +0100 Subject: [PATCH 42/51] Improve naming for less confusion --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index c0d409e49..1d3ba7fe2 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -370,14 +370,14 @@ private static Map getRepeatConditionsPerTarget * the DAG again * */ - private static List getDagEdges(Set vertices, Map> triggerIndex) { + private static List getDagEdges(Set triggerables, Map> triggerablesPerTrigger) { List edges = new ArrayList<>(); - for (QuickTriggerable source : vertices) { + for (QuickTriggerable source : triggerables) { // Compute the set of direct children triggerables of this source vertex Set targets = new HashSet<>(); for (TreeReference targetRef : source.getTargets()) - targets.addAll(triggerIndex.containsKey(targetRef) - ? triggerIndex.get(targetRef) + targets.addAll(triggerablesPerTrigger.containsKey(targetRef) + ? triggerablesPerTrigger.get(targetRef) : emptySet()); // Account for cycles by self-reference From 1f31f7d1e467ca59fa2ed9a39d37aac8a9dcc606 Mon Sep 17 00:00:00 2001 From: Guille Date: Mon, 13 Jan 2020 09:58:43 +0100 Subject: [PATCH 43/51] Make the getDagEdges method non-static to avoid confusion by arg names Also improve doc blocks --- .../javarosa/core/model/TriggerableDag.java | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 1d3ba7fe2..168f12e56 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -307,7 +307,8 @@ private QuickTriggerable findTriggerable(Triggerable t) { *
  • If a similar triggerable has been already added, its context gets * intersected with the provided triggerable to cover both using only * one entry
    • - *
  • This method builds the index of triggerables per trigger ref
    • + *
  • This method builds the index of triggerables per trigger ref + * at {@link #triggerablesPerTrigger}
    • * */ Triggerable addTriggerable(Triggerable triggerable) { @@ -342,8 +343,7 @@ Triggerable addTriggerable(Triggerable triggerable) { * conditions will be evaluated in the appropriate orders. */ public void finalizeTriggerables(FormInstance mainInstance) throws IllegalStateException { - List edges = getDagEdges(allTriggerables, triggerablesPerTrigger); - triggerablesDAG = buildDag(allTriggerables, edges); + triggerablesDAG = buildDag(allTriggerables, getDagEdges()); repeatConditionsPerTargets = getRepeatConditionsPerTargets(mainInstance, triggerablesDAG); } @@ -358,21 +358,21 @@ private static Map getRepeatConditionsPerTarget } /** - * Returns the list of edges in the DAG that can be built from the provided - * vertices. + * Returns the list of edges in the DAG that can be built from all the + * triggerables added to the DAG while parsing the form. *

    * This method has side-effects: *

      - *
    • Throws IllegalStateException when cycles are detected, either due - * to a self-reference or more complex cycle chains
      • + *
    • Throws IllegalStateException when cycles are detected involving + * self-references
      • *
    • Builds a cache of immediate cascades of each vertex, meaning that * we will remember the dependant vertices without having to traverse - * the DAG again
      • + * the DAG again {@link Triggerable#getImmediateCascades()} *
    */ - private static List getDagEdges(Set triggerables, Map> triggerablesPerTrigger) { + private List getDagEdges() { List edges = new ArrayList<>(); - for (QuickTriggerable source : triggerables) { + for (QuickTriggerable source : allTriggerables) { // Compute the set of direct children triggerables of this source vertex Set targets = new HashSet<>(); for (TreeReference targetRef : source.getTargets()) @@ -393,6 +393,16 @@ private static List getDagEdges(Set trigge return edges; } + /** + * Returns a set with the DAG that can be build using the provided vertices + * and edges. + *

    + * This method has side-effects: + *

      + *
    • Throws IllegalStateException when cycles are detected involving + * more than one node
      • + *
    + */ private static Set buildDag(Set vertices, List edges) { // The dag and the set of remaining vertices will be mutated // inside the while loop's block From a942f47d3d8f7f3dc6e8fe38423bb66739570aba Mon Sep 17 00:00:00 2001 From: Guille Date: Mon, 13 Jan 2020 10:07:07 +0100 Subject: [PATCH 44/51] Improve comment by using unambiguous language Avoid confusion by contextualizing the word "target" in the comment, which can be related to the DAG edge target and the triggerable target tree reference. --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 168f12e56..4a5d9520a 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -373,7 +373,10 @@ private static Map getRepeatConditionsPerTarget private List getDagEdges() { List edges = new ArrayList<>(); for (QuickTriggerable source : allTriggerables) { - // Compute the set of direct children triggerables of this source vertex + // Compute the set of edge targets from the source vertex in this + // loop using the triggerable's target tree reference set. + // We will create an edge for all the source's target references + // that, in turn, trigger another triggerable. Set targets = new HashSet<>(); for (TreeReference targetRef : source.getTargets()) targets.addAll(triggerablesPerTrigger.containsKey(targetRef) From 3cfecc1e4012937831ba87bf409905eae08a5cb3 Mon Sep 17 00:00:00 2001 From: Guille Date: Mon, 13 Jan 2020 10:08:16 +0100 Subject: [PATCH 45/51] Change the collection type to set for conformity The DAG edge is a set because there can't be duplicates. --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 4a5d9520a..bc32be98d 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -370,8 +370,8 @@ private static Map getRepeatConditionsPerTarget * the DAG again {@link Triggerable#getImmediateCascades()} * */ - private List getDagEdges() { - List edges = new ArrayList<>(); + private Set getDagEdges() { + Set edges = new HashSet<>(); for (QuickTriggerable source : allTriggerables) { // Compute the set of edge targets from the source vertex in this // loop using the triggerable's target tree reference set. @@ -406,7 +406,7 @@ private List getDagEdges() { * more than one node * */ - private static Set buildDag(Set vertices, List edges) { + private static Set buildDag(Set vertices, Set edges) { // The dag and the set of remaining vertices will be mutated // inside the while loop's block Set dag = new LinkedHashSet<>(); From f764f82e3f0b0cdb66d74bbf14764bac272b305c Mon Sep 17 00:00:00 2001 From: Guille Date: Mon, 13 Jan 2020 10:20:55 +0100 Subject: [PATCH 46/51] Add end-to-end test to verify that the DAG is sorted --- .../core/model/TriggerableDagTest.java | 69 +++++++++++++++---- 1 file changed, 56 insertions(+), 13 deletions(-) diff --git a/src/test/java/org/javarosa/core/model/TriggerableDagTest.java b/src/test/java/org/javarosa/core/model/TriggerableDagTest.java index 8bef8d9a2..4a1e5ea9d 100644 --- a/src/test/java/org/javarosa/core/model/TriggerableDagTest.java +++ b/src/test/java/org/javarosa/core/model/TriggerableDagTest.java @@ -277,10 +277,12 @@ public void deleteThirdRepeatGroup_evaluatesTriggerables_dependentOnTheRepeatGro } /** - * Indirectly means that the calculation - `concat(/data/house/name)` - does not take the + * Indirectly means that the calculation - `concat(/data/house/name)` - does + * not take the * `/data/house` nodeset (the repeat group) as an argument * but since it takes one of its children (`name` children), - * the calculation must re-evaluated once after a repeat group deletion because one of the children + * the calculation must re-evaluated once after a repeat group deletion + * because one of the children * has been deleted along with its parent (the repeat group instance). */ @Test @@ -325,8 +327,10 @@ public void deleteThirdRepeatGroup_evaluatesTriggerables_indirectlyDependentOnTh /** * This test was inspired by the issue reported at https://code.google.com/archive/p/opendatakit/issues/888 *

    - * We want to focus on the relationship between relevancy and other calculations because relevancy can be - * defined for fields **and groups**, which is a special case of expression evaluation in our DAG. + * We want to focus on the relationship between relevancy and other + * calculations because relevancy can be + * defined for fields **and groups**, which is a special case of expression + * evaluation in our DAG. */ @Test public void verify_relation_between_calculate_expressions_and_relevancy_conditions() throws IOException { @@ -375,12 +379,16 @@ public void verify_relation_between_calculate_expressions_and_relevancy_conditio } /** - * Ignored because the assertions about non-null next-numbers will fail because our DAG - * doesn't evaluate calculations in repeat instances that are previous siblings to the + * Ignored because the assertions about non-null next-numbers will fail + * because our DAG + * doesn't evaluate calculations in repeat instances that are previous + * siblings to the * one that has changed. *

    - * The expeculation is that, originally, only forward references might have been considered - * to speed up repeat group intance deletion because it was assumed that back references + * The expeculation is that, originally, only forward references might have + * been considered + * to speed up repeat group intance deletion because it was assumed that + * back references * were a marginal use case. *

    * This test explores how the issue affects to value changes too. @@ -453,8 +461,10 @@ public void calculate_expressions_should_be_evaluated_on_previous_repeat_sibling } /** - * Ignored because the count() function inside the predicate of the result_2 calculate - * expression isn't evaled correctly, as opposed to the predicate of the result_1 calculate + * Ignored because the count() function inside the predicate of the result_2 + * calculate + * expression isn't evaled correctly, as opposed to the predicate of the + * result_1 calculate * that uses an interim field as a proxy for the same computation */ @Test @@ -710,9 +720,12 @@ public void parsing_forms_with_self_reference_cycles_in_fields_of_repeat_groups_ } /** - * This test fails to parse the form because it thinks there's a self-reference cycle in /data/group/a, - * but this would be incorrect because each it depends on the same field belonging to the previous - * repeat instance, which wouldn't be a cycle, but an autoincremental feature. + * This test fails to parse the form because it thinks there's a + * self-reference cycle in /data/group/a, + * but this would be incorrect because each it depends on the same field + * belonging to the previous + * repeat instance, which wouldn't be a cycle, but an autoincremental + * feature. */ @Test @Ignore @@ -762,6 +775,36 @@ public void parsing_forms_with_cycles_between_fields_of_the_same_repeat_instance )); } + @Test + public void order_of_the_DAG_is_ensured() throws IOException { + Scenario scenario = Scenario.init("Some form", html( + head( + title("Some form"), + model( + mainInstance(t("data id=\"some-form\"", + t("a", "2"), + t("b"), + t("c") + )), + bind("/data/a").type("int"), + bind("/data/b").type("int").calculate("/data/a * 3"), + bind("/data/c").type("int").calculate("(/data/a + /data/b) * 5") + ) + ), + body(input("/data/a")) + )); + + assertThat(scenario.answerOf("/data/a"), is(intAnswer(2))); + assertThat(scenario.answerOf("/data/b"), is(intAnswer(6))); + assertThat(scenario.answerOf("/data/c"), is(intAnswer(40))); + + scenario.answer("/data/a", 3); + + assertThat(scenario.answerOf("/data/a"), is(intAnswer(3))); + assertThat(scenario.answerOf("/data/b"), is(intAnswer(9))); + // Verify that c gets computed using the updated value of b. + assertThat(scenario.answerOf("/data/c"), is(intAnswer(60))); + } private void assertDagEvents(List dagEvents, String... lines) { assertThat(dagEvents.stream().map(Event::getDisplayMessage).collect(joining("\n")), is(join("\n", lines))); From 0a2571244551bd52900880538859613022a172af Mon Sep 17 00:00:00 2001 From: Guille Date: Mon, 13 Jan 2020 10:25:03 +0100 Subject: [PATCH 47/51] Revert getChidlrenOfReference method to the original implementation Note that this method was originally declared in TriggerableDag --- .../xform/parse/FormInstanceParser.java | 61 ++++++++++--------- 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java index 1777e6c4e..45df79a37 100644 --- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java +++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java @@ -399,7 +399,7 @@ private void applyInstanceProperties(FormInstance instance) { // groups, we need to register all descendant refs as targets for relevancy // conditions to allow for chained reactions in triggerables registered in // any of those descendants - for (TreeReference r : getDescendantRefs(instance, ec, ref)) + for (TreeReference r : getChildrenOfReference(instance, ec, ref)) bind.relevancyCondition.addTarget(r); } if (bind.requiredCondition != null) @@ -432,39 +432,44 @@ private void applyInstanceProperties(FormInstance instance) { applyControlProperties(instance); } - private static Set getDescendantRefs(FormInstance mainInstance, EvaluationContext evalContext, TreeReference target) { - Set allDescendants = new HashSet<>(); - for (AbstractTreeElement child : getChildrenElements(mainInstance, evalContext, target)) { - allDescendants.add(child.getRef().genericize()); - allDescendants.addAll(getDescendantRefs(child)); - } - return allDescendants; - } + private Set getChildrenOfReference(FormInstance mainInstance, EvaluationContext evalContext, TreeReference original) { + // original has already been added to the 'toAdd' list. - private static Set getDescendantRefs(AbstractTreeElement element) { - Set allDescendants = new HashSet<>(); - for (int i = 0; i < element.getNumChildren(); ++i) { - AbstractTreeElement child = element.getChildAt(i); - allDescendants.add(child.getRef().genericize()); - allDescendants.addAll(getDescendantRefs(child)); + Set descendantRefs = new HashSet<>(); + TreeElement repeatTemplate = mainInstance.getTemplatePath(original); + if (repeatTemplate != null) { + for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { + TreeElement child = repeatTemplate.getChildAt(i); + descendantRefs.add(child.getRef().genericize()); + descendantRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); + } + } else { + List refSet = evalContext.expandReference(original); + for (TreeReference ref : refSet) { + descendantRefs.addAll(getChildrenRefsOfElement(mainInstance, evalContext.resolveReference(ref))); + } } - return allDescendants; + return descendantRefs; } - - private static Set> getChildrenElements(FormInstance mainInstance, EvaluationContext evalContext, TreeReference target) { - AbstractTreeElement repeatTemplate = mainInstance.getTemplatePath(target); + // Recursive step of utility method + private Set getChildrenRefsOfElement(FormInstance mainInstance, AbstractTreeElement el) { + Set childrenRefs = new HashSet<>(); + TreeElement repeatTemplate = mainInstance.getTemplatePath(el.getRef()); if (repeatTemplate != null) { - Set> elements = new HashSet<>(); - for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) - elements.add(repeatTemplate.getChildAt(i)); - return elements; + for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { + TreeElement child = repeatTemplate.getChildAt(i); + childrenRefs.add(child.getRef().genericize()); + childrenRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); + } + } else { + for (int i = 0; i < el.getNumChildren(); ++i) { + AbstractTreeElement child = el.getChildAt(i); + childrenRefs.add(child.getRef().genericize()); + childrenRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); + } } - Set> elements = new HashSet<>(); - List refSet = evalContext.expandReference(target); - for (TreeReference ref : refSet) - elements.add(evalContext.resolveReference(ref)); - return elements; + return childrenRefs; } From ac646eb02379fbacf9ed46f52bdbfa6e113a9092 Mon Sep 17 00:00:00 2001 From: Guille Date: Mon, 13 Jan 2020 10:25:54 +0100 Subject: [PATCH 48/51] Rename to avoid misguiding use of "children" The methods are recursive, and will return all descendants, not only children --- .../javarosa/xform/parse/FormInstanceParser.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java index 45df79a37..dcbcdeb64 100644 --- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java +++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java @@ -399,7 +399,7 @@ private void applyInstanceProperties(FormInstance instance) { // groups, we need to register all descendant refs as targets for relevancy // conditions to allow for chained reactions in triggerables registered in // any of those descendants - for (TreeReference r : getChildrenOfReference(instance, ec, ref)) + for (TreeReference r : getDescendantRefs(instance, ec, ref)) bind.relevancyCondition.addTarget(r); } if (bind.requiredCondition != null) @@ -432,7 +432,7 @@ private void applyInstanceProperties(FormInstance instance) { applyControlProperties(instance); } - private Set getChildrenOfReference(FormInstance mainInstance, EvaluationContext evalContext, TreeReference original) { + private Set getDescendantRefs(FormInstance mainInstance, EvaluationContext evalContext, TreeReference original) { // original has already been added to the 'toAdd' list. Set descendantRefs = new HashSet<>(); @@ -441,32 +441,32 @@ private Set getChildrenOfReference(FormInstance mainInstance, Eva for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { TreeElement child = repeatTemplate.getChildAt(i); descendantRefs.add(child.getRef().genericize()); - descendantRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); + descendantRefs.addAll(getDescendantRefs(mainInstance, child)); } } else { List refSet = evalContext.expandReference(original); for (TreeReference ref : refSet) { - descendantRefs.addAll(getChildrenRefsOfElement(mainInstance, evalContext.resolveReference(ref))); + descendantRefs.addAll(getDescendantRefs(mainInstance, evalContext.resolveReference(ref))); } } return descendantRefs; } // Recursive step of utility method - private Set getChildrenRefsOfElement(FormInstance mainInstance, AbstractTreeElement el) { + private Set getDescendantRefs(FormInstance mainInstance, AbstractTreeElement el) { Set childrenRefs = new HashSet<>(); TreeElement repeatTemplate = mainInstance.getTemplatePath(el.getRef()); if (repeatTemplate != null) { for (int i = 0; i < repeatTemplate.getNumChildren(); ++i) { TreeElement child = repeatTemplate.getChildAt(i); childrenRefs.add(child.getRef().genericize()); - childrenRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); + childrenRefs.addAll(getDescendantRefs(mainInstance, child)); } } else { for (int i = 0; i < el.getNumChildren(); ++i) { AbstractTreeElement child = el.getChildAt(i); childrenRefs.add(child.getRef().genericize()); - childrenRefs.addAll(getChildrenRefsOfElement(mainInstance, child)); + childrenRefs.addAll(getDescendantRefs(mainInstance, child)); } } return childrenRefs; From 757ea202ff0a18b690221a96ca5d4c4d8ba3f01c Mon Sep 17 00:00:00 2001 From: Guille Date: Thu, 16 Jan 2020 20:12:32 +0100 Subject: [PATCH 49/51] Remove unnecessary comment --- src/main/java/org/javarosa/xform/parse/FormInstanceParser.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java index dcbcdeb64..0cda28116 100644 --- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java +++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java @@ -433,8 +433,6 @@ private void applyInstanceProperties(FormInstance instance) { } private Set getDescendantRefs(FormInstance mainInstance, EvaluationContext evalContext, TreeReference original) { - // original has already been added to the 'toAdd' list. - Set descendantRefs = new HashSet<>(); TreeElement repeatTemplate = mainInstance.getTemplatePath(original); if (repeatTemplate != null) { From 0fa1549e2d2cbc5f60f77b85b682d5b1f4258119 Mon Sep 17 00:00:00 2001 From: Guille Date: Thu, 16 Jan 2020 20:22:04 +0100 Subject: [PATCH 50/51] Add TO DO notes to remember code exploration insights and questions --- src/main/java/org/javarosa/core/model/TriggerableDag.java | 3 +++ src/main/java/org/javarosa/form/api/FormEntryController.java | 1 + src/main/java/org/javarosa/xform/parse/FormInstanceParser.java | 1 + 3 files changed, 5 insertions(+) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index bc32be98d..19624f66b 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -212,6 +212,9 @@ private Set initializeTriggerables(FormInstance mainInstance, } } + // TODO Call doEvaluateTriggerables directly instead to avoid a redundant extra indirection level + // return doEvaluateTriggerables(mainInstance, evalContext, applicable, rootRef, alreadyEvaluated); + return evaluateTriggerables(mainInstance, evalContext, applicable, rootRef, alreadyEvaluated); } diff --git a/src/main/java/org/javarosa/form/api/FormEntryController.java b/src/main/java/org/javarosa/form/api/FormEntryController.java index c8bde4e05..e8b155939 100644 --- a/src/main/java/org/javarosa/form/api/FormEntryController.java +++ b/src/main/java/org/javarosa/form/api/FormEntryController.java @@ -112,6 +112,7 @@ public int answerQuestion(FormIndex index, IAnswerData data, boolean midSurvey) throw new RuntimeException("Itemsets do not currently evaluate constraints. Your constraint will not work, please remove it before proceeding."); } else { try { + // TODO Design a test that exercizes this branch. model.getForm().copyItemsetAnswer(q, element, data); } catch (InvalidReferenceException ire) { logger.error("Error", ire); diff --git a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java index 0cda28116..787d982aa 100644 --- a/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java +++ b/src/main/java/org/javarosa/xform/parse/FormInstanceParser.java @@ -442,6 +442,7 @@ private Set getDescendantRefs(FormInstance mainInstance, Evaluati descendantRefs.addAll(getDescendantRefs(mainInstance, child)); } } else { + // TODO Eventually remove this branch because a parsed form can't force the program flow throw it List refSet = evalContext.expandReference(original); for (TreeReference ref : refSet) { descendantRefs.addAll(getDescendantRefs(mainInstance, evalContext.resolveReference(ref))); From a3b7a18def913aa67a403acdb2801c45eaee7f64 Mon Sep 17 00:00:00 2001 From: Guille Date: Thu, 16 Jan 2020 20:38:03 +0100 Subject: [PATCH 51/51] Make private all the protected members It doesn't make sense to have protected members because now now one's extending this class. --- .../org/javarosa/core/model/TriggerableDag.java | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/javarosa/core/model/TriggerableDag.java b/src/main/java/org/javarosa/core/model/TriggerableDag.java index 19624f66b..b1c17322c 100644 --- a/src/main/java/org/javarosa/core/model/TriggerableDag.java +++ b/src/main/java/org/javarosa/core/model/TriggerableDag.java @@ -63,14 +63,14 @@ public interface EventNotifierAccessor { * loose coupling between processes that could be running at the same time * in a JavaRosa client. */ - protected final EventNotifierAccessor accessor; + private final EventNotifierAccessor accessor; /** * Stores the unsorted set of all triggerables present in the form. *

    * This set is used during the DAG build process. */ - protected final Set allTriggerables = new HashSet<>(); + private final Set allTriggerables = new HashSet<>(); /** * Stores the sorted set of all triggerables using the dependency direction @@ -79,7 +79,7 @@ public interface EventNotifierAccessor { * Triggerables present in this set depend exclusively on preceding * triggerables. */ - protected Set triggerablesDAG = emptySet(); + private Set triggerablesDAG = emptySet(); // TODO Make this member fit the expected behavior on calling sites by containing only relevance conditions /** @@ -96,7 +96,7 @@ public interface EventNotifierAccessor { * to a repeat group, but a form could declare other conditions as well, * leading to an unexpected scenario. */ - protected Map repeatConditionsPerTargets = new HashMap<>(); + private Map repeatConditionsPerTargets = new HashMap<>(); /** * Stores an index to resolve triggerables by their corresponding trigger's * reference. @@ -104,14 +104,13 @@ public interface EventNotifierAccessor { * Note that there's a m:n relationship between trigger references and * triggerables. */ - protected final Map> triggerablesPerTrigger = new HashMap<>(); + private final Map> triggerablesPerTrigger = new HashMap<>(); - - protected TriggerableDag(EventNotifierAccessor accessor) { + public TriggerableDag(EventNotifierAccessor accessor) { this.accessor = accessor; } - protected Set doEvaluateTriggerables(FormInstance mainInstance, EvaluationContext evalContext, Set tv, TreeReference anchorRef, Set alreadyEvaluated) { + private Set doEvaluateTriggerables(FormInstance mainInstance, EvaluationContext evalContext, Set tv, TreeReference anchorRef, Set alreadyEvaluated) { // tv should now contain all of the triggerable components which are going // to need to be addressed by this update. // 'triggerables' is topologically-ordered by dependencies, so evaluate @@ -554,7 +553,7 @@ public boolean shouldTrustPreviouslyCommittedAnswer() { return false; } - protected final void publishSummary(String lead, TreeReference ref, Collection quickTriggerables) { + final void publishSummary(String lead, TreeReference ref, Collection quickTriggerables) { accessor.getEventNotifier().publishEvent(new Event(lead + ": " + (ref != null ? ref.toShortString() + ": " : "") + quickTriggerables.size() + " triggerables were fired.")); }