Do not use cached choices if the cached list comes from a different repeat

[grzesiek2010]

Closes #642

What has been done to verify that this works as intended?

I tested this change with ODK Collect.

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

My understanding of the issue is:

• in Cache and reuse dynamic choice lists Cache and reuse dynamic choice lists #619 Bind select values to SelectChoice(s) and implement equals for IAnswerData subclasses #626 we implemented caching choices to make the process of loading them faster
• the responsibility of the method that does it ItemsetBinding.getChoices() is also updating answers in model ItemsetBinding.updateQuestionAnswerInModel()
• if we already have a list of cached choices it is returned and updating that answer in model does not take place
• everything works fine if we have normal questions (not in repeatable groups) because then the answer is always the same
• if we have a question in repeatable group then despite the fact it's the same question with exactly the same choices we can have different answers so we can't just return the cached list of choices and skip ItemsetBinding.updateQuestionAnswerInModel()

so to solve the issue I also added caching cachedRef to avoid using the same cached choices for all repeatable groups.

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?

It should just handle the case where dynamic choices are used in repeatable groups. Nothing else should be affected.

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

The form attached to the issue.

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

No.

[grzesiek2010]

@dcbriccetti you were reviewing the prs that introduced this bug so you are much more familiar with this code. Could you take a look and check if my thinking is right? I don't want to bother @lognaturel
Also @seadowg if you have some time you can take a look.

[yanokwa]

Dave is not available. Callum, please help when you have a chance.

[lognaturel]

🙏🙏

I’m having trouble getting to a computer but I do have some thinking time for things I can do on my phone. I am sleep deprived so do check me!! 👶🏽

This approach will work but I believe it’s not necessary to recompute choices. I think the issue is that the object that represents the selection is not bound to the choice when an existing instance is opened. I think the same issue would happen with a dynamic default so that might be a way to write a test. For example, for a select in a repeat, using integers as the choice names and position() as the dynamic default.

I vaguely remember fixing a related issue before release. I’m not sure whether the fix was in JavaRosa or Collect but you might be able to find it by searching for getTextID. I think I had found a reasonable way to make the bind happen.

Will try to get to a computer to get more specifics but I figured I’d get you this information in case it’s actionable. I’m also ok with going with this for now if looking for a more targeted fix seems like too much work.

[lognaturel]

Aha. 7d483f1 I believe fixed the same issue for non-repeats and might have useful ideas.

[grzesiek2010]

This approach will work but I believe it’s not necessary to recompute choices. I think the issue is that the object that represents the selection is not bound to the choice when an existing instance is opened.

Yes it's not necessary to recompute choices as you said but it's necessary to call updateQuestionAnswerInModel() passing selected choices (to bound selections as you said).
So another option would be something like this: 0cba44e then I do not recompute all the choices just read the list of selected choices again to call updateQuestionAnswerInModel() (I could improve it even more caching that list and calling updateQuestionAnswerInModel()` only if something has changed) but is that a much better solution?

I think it's kind of strange that a method called getChoices() not only returns a list of choices but also performs such an additional operations.
Maybe it would be better to do that in a separate place but that would require much more investigation at least from me since I'm not familiar with that code, but that's something for the future I think. I'm off next week so I can get back to it after my break.

[seadowg]

@grzesiek2010 sounds like you're going to be looking at this again next week, so just tag me back in then!

[grzesiek2010]

Ok after a one-week break I got back to this issue and investigated how @lognaturel fixed her similar problem for non-repeats 7d483f1. My solution just follows that fix so I think it's a good solution (either of the two I presented).

@lognaturel improved updateQuestionAnswerInModel() so that it binds selection to the choice when a choice list for a given question is computed for the first time and I used the same approach.

The current caching mechanism computes a list of choices for given question (binding selections too) when the list is read for the first time but then it just returns a cached list (avoiding all the other operations like binding selections too). It's a great improvement of course but if we have a repeatable group that cached list is returned for every single group after being computed once. Of course the same question might have a different answer for every group so we need to updateQuestionAnswerInModel() for every repeat.

The question is which of the two solutions I presented we should use?

@seadowg I think you can take a look at it when you are free.

[lognaturel]

I believe your second option would be much faster in cases where the list is long and multiple repeat instances have the same filter. It's computing the filtered list that's expensive in the case of a big list. Is it something you can quickly quantify? I'd use the nigeria wards choice lists. Have questions outside the repeat that ask for state and lga and then a question in a repeat that asks for ward. You should see a big difference between having to recompute for every repeat instance and not.

[grzesiek2010]

The form you attached is not good for reproduction because:

• it does not contain repeats
• it uses dynamic filters (based on previous answers) and then the problem does not occur

I created my own form with 15k choices: dyn.xlsx
Testing that form using the profiler in Android studio I didn't see any difference in behavior in favor of the second solution. I think it's because of the fact that to call updateQuestionAnswerInModel() we need to pass (and build before) Map<String, SelectChoice> selectChoicesForAnswer so we have to loop through all items what takes a lot of time.

[lognaturel]

My suggestion was to take the choices from the Nigeria Wards form and build a form with repeats as I described. Two filtering questions outside the repeat and one filtered question inside. I would expect the second solution to show the question in the repeat instantly and the first solution to take a bit of time.

What device are you checking on? How long does your 15k item question take to load? It may not be a good test case because I imagine rendering all 15k choices takes way longer than any filtering. The reason I suggested the Nigeria wards data is that it actually filters down lists.

[lognaturel]

To be a little more explicit, your test form isn’t realistic because the point of using a choice filter is to reduce the number of options that get shown to the data collector. It would be unusual to have more than a couple dozen choices shown at once. Additionally, it’s common to have the same filter across many repeat instances. Using the Nigeria wards data, imagine you’re in a particular region and your task is to capture data across the wards in that area. The questions outside the repeat would let you to narrow down what wards you’re responsible for. Then each repeat iteration would show the same heavily filtered list. Iterating through just those filtered wards to bind the repeat iteration’s answer would be quite different from having to iterate over all 40k wards.

How about I try to take a deeper look within the next couple of weeks? If I can’t get to it we can go with this PR and come back to it some other time.

[grzesiek2010]

Ok so edited that for as you recommended:
nig.xlsx

There is a small difference in performance like 1.5s vs 0.5s reading that choices (during navigating in hierarchy). I used an emulator Android 11.

How about I try to take a deeper look within the next couple of weeks? If I can’t get to it we can go with this PR and come back to it some other time.

I thought that we needed to add this to next release because a couple of users complained about this bug. Generally it would be great to have your investigation too since you know the code much better but I feel like we can go with this fix (either of the solutions) and then get back to it after releasing a version without the bug.

[lognaturel]

Great, thanks!

small difference in performance like 1.5s vs 0.5s reading that choices (during navigating in hierarchy). I used an emulator Android 11.

I believe this means that a device that takes 3s with the faster fix would take 9s with the slower one. Keep in mind that's for every single repeat. It makes the common case of filling out data the first time much slower in order to make edits possible. I agree edits are important but we should consider that impact. So I think going with your faster fix should be the default.

we needed to add this to next release

I agree. I see a milestone due date of October 15, 2021, is that right? I should be able to get to it by next week. Again, if it gets urgent, let's go with your faster solution.

[seadowg]

I see a milestone due date of October 15, 2021, is that right?

@lognaturel yup that's when we're looking to do code freeze.

[grzesiek2010]

I should be able to get to it by next week. Again, if it gets urgent, let's go with your faster solution

I'm off next two weeks so one of you will have to cherry-pick that commit if you accept it.

[lognaturel]

That should be no problem. Enjoy your time off!

[seadowg]

Closing in favour #646