Support an intent group nested one level inside a field-list #6696
Conversation
|
I'm marking this "needs testing" early as it'd be good to see if we can find any problems, before getting to settled on the approach. I've had a skim, but should be able to take a proper look in the next few days. |
|
@lognaturel In familiarizing myself (again) with the field list implementation to be able to understand all this, I ended up with a few questions about the logic in
I took a couple of hours and have some pretty nice little changes that deal with 1 and 2, but I think 3 is probably another days work at least, so I'll pause. I basically ended up reducing private void updateFieldListQuestions(FormIndex lastChangedIndex) throws RepeatsInFieldListException {
ODKView odkView = getCurrentViewIfODKView();
if (odkView == null) {
return;
}
FormEntryPrompt[] questionsAfterSave = formEntryViewModel.saveFieldList(odkView.getAnswers());
this.odkView.onUpdated(lastChangedIndex, questionsAfterSave);
}Are you OK for me to merge those changes in? It felt hard not to do once I had my head around it, and I think it's a nice piece of positive progress to include with this new feature. |
|
Love it! Yes, please commit away. |
Closes #6695
On this branch, it should now be possible to have any other number of groups and questions along with a single intent group in a field-list. An intent group has an app launch button at the top and all of the fields contained directly in it are read-only. When answers are returned from an external app, any related questions in the field-list get updated. It should still be possible to have the whole field list be an intent group in which case all questions are read-only and the launch button is at the very top.
Why is this the best possible solution? Were any other approaches considered?
I believe this is the simplest approach. I kept everything about how field-lists are rendered and about how recomputation happens within a field-list the same and looked for the
intentattribute one level inside the field-list. We now save aFormEntryCaptionrepresenting the intent group on theODKView. Ideally a lot of the logic in the view would be moved out to aViewModelbut that felt like too much expanded scope to take on without clear user benefit right now.I have some uneasiness about how heavily everything about field list recomputation relies on mutation and knowing the right indexes of views in different lists. I considered deeper changes like hiding and mutating views instead of removing and adding them in
widgetList. I think that would be pretty invasive and risky even if it might make more sense overall.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?
I think the biggest risk is a hit on performance for rendering any question view. I tried to be really careful to always first check if there's an intent group before doing any logic related to that. That would be a good thing to double check in review.
I changed how
readOnlyOverrideworks for widgets. Now we can check whether a widget is in an intent group to decide whether to force the field to be read-only. I think that's easy to understand but there may be some risk there? Though there's so much test coverage it'd be hard to imagine what I could have missed.Do we need any specific form for testing your changes? If so, please attach one.
person_registration.xlsx is the form used in the test. I think I tested all interesting cases on it but maybe not. It might be useful to try some with other logic in the field list.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
getodk/docs#1942
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest