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

grzesiek2010 · 2025-07-10T09:34:35Z

Why is this the best possible solution? Were any other approaches considered?

As I mentioned in the issue, this wasn’t a new bug, but due to recent changes, it became much more noticeable and started breaking some workflows, as we've seen on the forum. It’s something that needs to be addressed. I've included some comments below explaining the rationale behind my changes.
All the problems stem from the way external choices are handled by the search() function. Unlike internal choices or those from select_one_from_file, they are not immediately available in JavaRosa. Instead, they are loaded later, when the question is actually displayed. Because of this, if there is a default answer coming from the last-saved feature, we might not be able to immediately determine the user-visible label for that answer.
As far as I understand, this is not a priority for us, and select_one_from_file is the recommended way to load choices from external files. So we don’t plan to completely rework how search() handles external choices. Instead, I’ve made some fixes to improve its behavior in certain cases.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Please test the cases I described in the issue, as well as anything else you know that uses the search() function.

Do we need any specific form for testing your changes? If so, please attach one.

I've attached some forms in the issue.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

added or modified tests for any new or changed behavior
run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
added a comment above any new strings describing it for translators
added any new strings with date formatting to DateFormatsTest
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

grzesiek2010 · 2025-07-10T09:46:35Z

collect_app/src/main/java/org/odk/collect/android/formentry/ODKView.java


        setupAudioErrors();
        autoplayIfNeeded(advancingPage);
+        updateQuestions(questionPrompts);


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.

grzesiek2010 · 2025-07-10T09:57:04Z

collect_app/src/main/java/org/odk/collect/android/dynamicpreload/ExternalDataUtil.java

        }
    }

+    private static void updateQuestionAnswer(FormEntryPrompt formEntryPrompt, List<SelectChoice> returnedChoices) {


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.

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?

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

setting value via calculation like:

I don't know if it would be common.

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)?

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.

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?

@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

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?

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.

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?

grzesiek2010 · 2025-07-10T10:11:21Z

collect_app/src/main/java/org/odk/collect/android/formhierarchy/QuestionAnswerProcessor.kt

        }

-        if (!fep.answerText.isNullOrBlank() &&
+        val answerText = try {


Due to the nature of how external choices are handled by the search function, there are some cases we can't fully resolve. One of them is described here: #6801 (comment)

For example, if we open a blank form where a non-first select question uses the last-saved instance and immediately access the hierarchy view, the hierarchy attempts to determine the answer to display before the choices have been loaded (which only happens when the question is actually displayed). In older versions of the app, this led to an error.

With this change, we now display the underlying XML value until the choices are fully loaded.

seadowg

This needs to target v2025.2.x instead of master so we can use it for a hotfix. Additionally, I think we should remove any changes not needed to get the test passing to keep the risk as low as possible.

...ct_app/src/androidTest/java/org/odk/collect/android/feature/formentry/ExternalSelectsTest.kt

…e value

…eturn the value" This reverts commit 7710bc8.

seadowg

#6802 (comment)

Sorry I hadn't realized the PR was in a "needs review" state. I should have left this as a review rather than just a normal comment.

seadowg

#6802 (comment)

grzesiek2010 changed the title ~~Add support for using select questions with search and the last-saved feature~~ Correct support for using select questions with search and the last-saved feature Jul 10, 2025

grzesiek2010 commented Jul 10, 2025

View reviewed changes

grzesiek2010 marked this pull request as ready for review July 10, 2025 10:15

grzesiek2010 requested a review from seadowg July 10, 2025 10:15

seadowg requested changes Jul 10, 2025

View reviewed changes

...ct_app/src/androidTest/java/org/odk/collect/android/feature/formentry/ExternalSelectsTest.kt Show resolved Hide resolved

grzesiek2010 added 7 commits July 10, 2025 14:39

Added a failing test

59658c6

Update question answer after loading external choices

be112eb

Call #updateQuestions after building them in ODKView

5e0e80c

If translatable answer for select_one can't be determined - return th…

ab8ca73

…e value

Added missing form

662be56

Revert "If translatable answer for select_one can't be determined - r…

8ca4cea

…eturn the value" This reverts commit 7710bc8.

Updated test to ignore the bug with building answers for the hierarchy

401ceeb

grzesiek2010 force-pushed the COLLECT-6801 branch from 7710bc8 to 401ceeb Compare July 10, 2025 12:41

grzesiek2010 changed the base branch from master to v2025.2.x July 10, 2025 12:41

grzesiek2010 requested a review from seadowg July 10, 2025 13:14

seadowg requested changes Jul 23, 2025

View reviewed changes

grzesiek2010 requested a review from seadowg August 21, 2025 21:52

seadowg requested changes Aug 22, 2025

View reviewed changes

Resolve selected choice during main loop to avoid second pass

80c1a4e

grzesiek2010 requested review from lognaturel and seadowg November 19, 2025 21:37

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

Are you sure you want to change the base?

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

Conversation

grzesiek2010 commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grzesiek2010 Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seadowg Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seadowg Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seadowg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seadowg left a comment

Choose a reason for hiding this comment

Uh oh!

seadowg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

grzesiek2010 commented Jul 10, 2025 •

edited

Loading

grzesiek2010 Jul 10, 2025 •

edited

Loading

seadowg Jul 11, 2025 •

edited

Loading

seadowg Aug 22, 2025 •

edited

Loading