Skip to content

Correct support for using select questions with search and the last-saved feature #6802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: v2025.2.x
Choose a base branch
from
+102 −1
Open

Correct support for using select questions with search and the last-saved feature #6802

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
59658c6
Added a failing test
grzesiek2010 Jul 10, 2025
be112eb
Update question answer after loading external choices
grzesiek2010 Jul 10, 2025
5e0e80c
Call #updateQuestions after building them in ODKView
grzesiek2010 Jul 10, 2025
ab8ca73
If translatable answer for select_one can't be determined - return th…
grzesiek2010 Jul 10, 2025
662be56
Added missing form
grzesiek2010 Jul 10, 2025
8ca4cea
Revert "If translatable answer for select_one can't be determined - r…
grzesiek2010 Jul 10, 2025
401ceeb
Updated test to ignore the bug with building answers for the hierarchy
grzesiek2010 Jul 10, 2025
80c1a4e
Resolve selected choice during main loop to avoid second pass
grzesiek2010 Nov 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions ...app/src/androidTest/java/org/odk/collect/android/feature/formentry/ExternalSelectsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,31 @@ class ExternalSelectsTest {
.assertText("File: $formsDirPath/search_and_select-media/nombre.csv is missing.")
.assertText("File: $formsDirPath/search_and_select-media/nombre2.csv is missing.")
}

@Test // https://github.com/getodk/collect/issues/6801
fun searchFunctionWorksWellWithLastSaved() {
rule.startAtMainMenu()
// Fill out and finalize the first form
.copyForm("search-with-last-saved.xml", listOf("fruits.csv"))
.startBlankForm("Search with last-saved")
.clickOnText("Mango")
.swipeToNextQuestion("Select fruit 2")
.clickOnText("Oranges")
.swipeToEndScreen()
.clickFinalize()

// Start a new form to verify that answers from the previous form are retained
.startBlankForm("Search with last-saved")
.swipeToNextQuestion("Select fruit 2")
.clickGoToArrow()
.assertHierarchyItem(0, "Select fruit 1", "Mango")
.assertHierarchyItem(1, "Select fruit 2", "Oranges")

// Change an answer in a field-list and verify no errors occur
.clickOnQuestion("Select fruit 2")
.clickOnText("Strawberries")
.clickGoToArrow()
.assertHierarchyItem(0, "Select fruit 1", "Mango")
.assertHierarchyItem(1, "Select fruit 2", "Strawberries")
}
}
20 changes: 20 additions & 0 deletions collect_app/src/main/java/org/odk/collect/android/dynamicpreload/ExternalDataUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

import org.javarosa.core.model.SelectChoice;
import org.javarosa.core.model.condition.EvaluationContext;
import org.javarosa.core.model.data.IAnswerData;
import org.javarosa.core.model.data.helper.Selection;
import org.javarosa.core.model.instance.FormInstance;
import org.javarosa.form.api.FormEntryCaption;
import org.javarosa.form.api.FormEntryPrompt;
Expand Down Expand Up @@ -210,6 +212,7 @@ public static ArrayList<SelectChoice> populateExternalChoices(FormEntryPrompt fo
}
}
}
updateQuestionAnswer(formEntryPrompt, returnedChoices);
return returnedChoices;
} catch (Exception e) {
String fileName = String.valueOf(xpathfuncexpr.args[0].eval(null, null));
Expand All @@ -228,6 +231,23 @@ public static ArrayList<SelectChoice> populateExternalChoices(FormEntryPrompt fo
}
}

private static void updateQuestionAnswer(FormEntryPrompt formEntryPrompt, List<SelectChoice> returnedChoices) {
Copy link
Member Author

@grzesiek2010 grzesiek2010 Jul 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

When the last-saved feature is enabled and we open a blank form after saving at least one instance, we correctly populate default answers for questions that rely on that feature. For most question types, this works smoothly. However, things get trickier with select questions that use the search function.

In these cases, we initially only have the raw XML value, since that's what gets stored in the submission. At that stage, external choices are not loaded yet, so we can’t resolve the user-visible label. But once the choices are loaded (exactly when we call this method), we can update the formEntryPrompt. Similar to haow JR deals with choices that it is aware of: https://github.com/getodk/javarosa/blob/master/src/main/java/org/javarosa/core/model/ItemsetBinding.java#L144

Why did this seem to work in earlier versions of the app? Mostly because of how operations were ordered. As I noted in the issue, I was able to reproduce bugs caused by the same underlying problem even in older versions. It was just less noticeable because calling certain functions in a particular order could update the answer internally in a similar way to what this fix now handles explicitly.

Copy link
Member

@seadowg seadowg Jul 10, 2025

Choose a reason for hiding this comment

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

I'm worried about the performance implications of the change here as we end up doing a filter through the choices. SearchBenchmarkTest currently passes as this code is not hit (I think the selection is already attached). What cases other than the "default with last-saved pointing at a search question" will the filter actually happen?

Copy link
Member Author

@grzesiek2010 grzesiek2010 Jul 10, 2025

Choose a reason for hiding this comment

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

What cases other than the "default with last-saved pointing at a search question" will the filter actually happen?

setting value via calculation like:
image
I don't know if it would be common.

Copy link
Member

@seadowg seadowg Jul 11, 2025

Choose a reason for hiding this comment

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

Yeah, that doesn't seem like it'd be common. The case in the issue (using the value in a default with last-saved) will hit it though and if search returns n items we end up introducing a (worst case) n item filter here right? (O(n) complexity)?

Copy link
Member Author

@grzesiek2010 grzesiek2010 Jul 11, 2025

Choose a reason for hiding this comment

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

Yes, depending on the answer's position in the list, the lookup might be very fast (if it's near the beginning) or require scanning the entire list (if it's one of the last entries).
But as you mentioned, this only happens when last-saved is used; otherwise, the search function behaves as it did before.

Copy link
Member

@seadowg seadowg Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, depending on the answer's position in the list, the lookup might be very fast (if it's near the beginning) or require scanning the entire list (if it's one of the last entries).

Yeah, the average is still just "linear" (O(n)) for linear search though.

But as you mentioned, this only happens when last-saved is used; otherwise, the search function behaves as it did before.

Right. I'm worried that this implementation introduces a performance drop that could make using search in this case pointless (when compared to select_one_from_file) as we now do that scan whereas we potentially didn't before. Can we set up a form with a large data set that uses search in this way to experiment?

Copy link
Member Author

@grzesiek2010 grzesiek2010 Aug 21, 2025

Choose a reason for hiding this comment

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

@seadowg, how many choices would you expect in such a form? I've built one with 2k (not that big, maybe) and was looking for one of the last elements. The operation was very fast and it took only 0.001s. Here is the form:
SelectsWithLastSaved.zip

Copy link
Member

@lognaturel lognaturel Aug 22, 2025

Choose a reason for hiding this comment

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

It’s hard to predict. Generally the list should be filtered but it could be big. I generally agree we should try not changing the perf characteristics much.

There’s a loop over all choices right before. Could we identify the selected choice and save its label in that pass rather than doing an additional one?

Copy link
Member

@seadowg seadowg Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There’s a loop over all choices right before. Could we identify the selected choice and save its label in that pass rather than doing an additional one?

I think first we should have a benchmark that exercises the code paths here so we can make sure we have a measure of if there is a significant change or not (and protect ourselves moving forward). Potentially using the 100k item data set with a new form (that uses last-saved) would work here? There might be a simpler way to trigger the new code though.

Copy link
Member Author

@grzesiek2010 grzesiek2010 Nov 19, 2025

Choose a reason for hiding this comment

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

There’s a loop over all choices right before. Could we identify the selected choice and save its label in that pass rather than doing an additional one?

Yes, this is possible, and I've just added this change. In this case, if there is no additional loop, I'm not sure if we need any benchmark test as a part of this pr?

IAnswerData value = formEntryPrompt.getAnswerValue();
if (value == null) {
return;
}

Selection selection = (Selection) value.getValue();
if (selection.index != -1) {
return;
}

returnedChoices.stream()
.filter(choice -> selection.getValue().equals(choice.getValue()))
.findFirst()
.ifPresent(selection::attachChoice);
}

/**
* We could simple return new String(displayColumns + "," + valueColumn) but we want to handle
* the cases
Expand Down
2 changes: 1 addition & 1 deletion collect_app/src/main/java/org/odk/collect/android/formentry/ODKView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ public ODKView(
LifecycleOwner viewLifecycle
) {
super(context);
updateQuestions(questionPrompts);

this.viewLifecycle = viewLifecycle;
this.audioPlayer = audioPlayer;
Expand Down Expand Up @@ -209,6 +208,7 @@ public ODKView(

setupAudioErrors();
autoplayIfNeeded(advancingPage);
updateQuestions(questionPrompts);
Copy link
Member Author

@grzesiek2010 grzesiek2010 Jul 10, 2025

Choose a reason for hiding this comment

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

One of the steps performed by #updateQuestions is reading the user-visible answers in order to compare the entire content and determine whether a question has entered a new state that should be displayed: https://github.com/getodk/collect/blob/master/collect_app/src/main/java/org/odk/collect/android/logic/ImmutableDisplayableQuestion.java#L84.

For select-type questions, the underlying data is represented by Selection, which contains the user-visible answer, the index, and an XML value. If we attempt to perform the update at the beginning of this block, answers from external choices (those using the search function) are not yet available, although we already have access to the XML value. In such cases, trying to retrieve the user-visible answer results in an error. Therefore, we must defer this operation until all widgets are built and all choices have been fully loaded.

}

private void setupAudioErrors() {
Expand Down
55 changes: 55 additions & 0 deletions test-forms/src/main/resources/forms/search-with-last-saved.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?xml version="1.0"?>
<h:html
xmlns="http://www.w3.org/2002/xforms"
xmlns:h="http://www.w3.org/1999/xhtml"
xmlns:ev="http://www.w3.org/2001/xml-events"
xmlns:xsd="http://www.w3.org/2001/XMLSchema"
xmlns:jr="http://openrosa.org/javarosa"
xmlns:orx="http://openrosa.org/xforms"
xmlns:odk="http://www.opendatakit.org/xforms">
<h:head>
<h:title>Search with last-saved</h:title>
<model odk:xforms-version="1.0.0">
<instance>
<data id="search-with-last-saved">
<fruit/>
<group>
<fruit2/>
<details/>
</group>
<meta>
<instanceID/>
</meta>
</data>
</instance>
<instance id="__last-saved" src="jr://instance/last-saved"/>
<bind nodeset="/data/fruit" type="string"/>
<setvalue ref="/data/fruit" value=" instance('__last-saved')/data/fruit " event="odk-instance-first-load"/>
<bind nodeset="/data/group/fruit2" type="string"/>
<setvalue ref="/data/group/fruit2" value=" instance('__last-saved')/data/group/fruit2 " event="odk-instance-first-load"/>
<bind nodeset="/data/group/details" type="string"/>
<bind nodeset="/data/meta/instanceID" type="string" readonly="true()" jr:preload="uid"/>
</model>
</h:head>
<h:body>
<select1 ref="/data/fruit" appearance="search('fruits')">
<label>Select fruit 1</label>
<item>
<label>name</label>
<value>name_key</value>
</item>
</select1>
<group appearance="field-list" ref="/data/group">
<select1 ref="/data/group/fruit2" appearance="search('fruits')">
<label>Select fruit 2</label>
<item>
<label>name</label>
<value>name_key</value>
</item>
</select1>
<input ref="/data/group/details">
<label>Fruit details</label>
</input>
</group>
</h:body>
</h:html>