Updates in field lists

[lognaturel]

Closes #378
Closes #1475 (in ee784a1)
Closes #2537 (double instantiation of widgets)

The high-level approach is to introduce updateFieldListQuestions(lastIndexChanged) in FormEntryActivity which saves the answers in a field list, compares questions before and after the save, and adds and removes any questions that have changed in a user-visible way. Each widget triggers this process by calling widgetValueChanged.

When I first did the proof of concept, I was concerned about performance since all of the views are always built and stored in memory. It's not as bad as I had imagined. On my Samsung Galaxy Tab A, it's reasonable to have about a hundred questions without logic. At around 50 questions ALL with logic, answering each question gets slow but still usable. I think we should document these limitations and if we learn that users want to have very long vertically-scrolling forms, we can look at alternate options like using a RecyclerView.

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

I added Espresso tests verifying changes in relevance, changes in question text, focus in question types with text fields and cascading selects.

I also tried all-widgets-fieldlist.xlsx which is the standard All Widgets form but in a field list and with each question revealing the next when the former's value is set. I manually set a value and then cleared it for each question type to verify that updates work as expected. Do you think it would be worth adding an Espresso test for this?

Currently, any widget that involves the FormEntryActivity going to the background (geo widgets, binary widgets, etc), only work incidentally because the whole view is rebuilt. Addressed by ee784a1

The last big remaining piece of work is to make sure scroll and focus are properly maintained.

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

This is the only narrow change I could think of. It isolates the new behavior in the updateFieldListQuestions(lastIndexChanged) method and only requires added method calls in widgets to let the activity know of value changes.

For most of the question types, there was only one option for when to register a value change and trigger an update. For questions that have a text box, it could be either as characters are being typed or on focus change. See age-name-school-sex.xlsx (age-name-school-sex.xml.txt) for an example. I chose to update on character typed because I think it very clearly links the changing of the question's value to the change in questions displayed. If tapping on the next question is what triggers a refresh, that link becomes less clear, especially if the question you tap on to answer is suddenly replaced. The downsides to updates happening as characters are typed are that the questions displayed may update more than once as an answer is being supplied (type 23 as the age in the form above and the school question will appear then disappear) and that it does have an impact on typing speed if there is a lot of logic in the field list. @cooperka, particularly interested in feedback from your users about this one.

I also had to make some decisions around focus. See ec22b65 and 04eb3f3. I tried to describe the decisions in detail in the commit descriptions. @grzesiek2010, this changes some previous decisions we'd made together so I'd love for you to check my reasoning.

See ImmutableDisplayableQuestion for how we decide whether a widget needs to be updated. I included answer there because I think it should be possible to have an answer that is updated due to a change elsewhere in the form. This could be useful to display a changing value with a readonly prompt. It doesn't work as I expected, though, so I commented out the corresponding test. Anyone have any insights as to what's going on? I don't think it's all that important so we can file an issue and come back to it.

There's also unexpected behavior around cascading selects. Again, I don't think it's critical so I can explore as a follow-up.

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?

This enables users to include logic in field lists. The highest risk changes are introduced in cd304f8. This removes several calls on saving data and rebuilding the questions view. It's possible that this introduces lifecycle-related issues in certain conditions. There's risk around anything that might launch or be launched by FormEntryActivity including the hierarchy view and external applications (both directions).

ebe12a5 changes the behavior of the navigation buttons in a way @grzesiek2010 should review. Previously, the back button was disabled but visible when navigating back was disabled in admin settings. Now the back button is no longer visible. This feels more consistent and makes the code a little bit cleaner.

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

all-widgets-fieldlist.xlsx

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

getodk/docs#1016

Before submitting this PR, please make sure you have:

• run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
• 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

CC @cooperka, @grzesiek2010

April 13 update

As will surprise absolutely no one, I'm running into interesting issues as I work on logic updates in field lists. I'm opening this draft PR to describe some of the issues I'm facing in case anyone has ideas and to communicate about status.

I really needed to have some kind of testing support to feel confident in the changes and I decided to do Espresso form-based tests that simulates what a user would do. If someone does want to review this, I'd recommend starting from the tests. If you have any comments or suggestions on how the tests are structured or the level it is written at, please share.

The first big surprise I've run into is that multi-level cascading selects are not updating as I would expect them to. See this test for an example. If I have three levels in my cascade, updates to the first level don't immediately propagate to the third level. Changes do occur as expected if I change the value of the first level a second time, though. I have not yet figured out at where the problem is occurring. I can see that the third level dynamic choices aren't being updated to what I expect them to in populateDynamicChoices.

There are also a lot of issues with focus. StringWidget and its descendants lose focus when their answers are saved. This was introduced with e66f4a8, presumably because a user did run into an issue of typing after a save had been initiated and losing some characters. I'm guessing this is not an issue with modern devices but I don't think I can prove that. I haven't quite decided how I'm going to deal with it but I'm currently thinking of blocking edits from ODKView.getAnswers() instead of from with the widget's getAnswer().

I'll keep updating as I make progress.

[lognaturel]

This makes the back button work as intended by going to onActivityResult where we can rebuild the view.

[lognaturel]

The view update is now done by the widgets when setBinaryData is called.

[lognaturel]

See image map widgets for example

[lognaturel]

This is ready for a review.

I highlighted some known issues that I'm pretty sure are JavaRosa-related in the PR description. I'll be exploring those soon. Here are a few more that I think are not super important and that we can file issues for or that perhaps a reviewer can help with if they spot the issue right away:

• date time widget doesn't update as intended
• the first time a date or time widget value is set, the form scrolls to the top (view is rebuilt) but not subsequent times
• scroll position is lost on orientation change
  • this means draw, annotate, signature lose position unless the user forces vertical orientation when coming back from activity

[cooperka]

Too bad React wasn't around when Collect was created, we're starting to implement their lib from scratch 😂

It sounds like you're still tracking several to-do items remaining; as it stands, I think this looks great and functions well. It must have been a bear to test it all 🐻 I agree with your reasoning for all the commits you mentioned could be controversial. You've clearly considered all the edge cases I can currently think of.

A few potential concerns:

• The performance is fine but I wonder if it can be improved further. Things occasionally slow down on my emulator when typing a string into a text box; the typing events queue up and then I need to wait for it to finish inputting letters before I can do anything else.
  • Particularly noticeable for me with "Integer widget with thousands separators"
  • Could we throttle the updates?
  • Also see my comment below, there's a nice React blog post you might be interested in
• Does anything prevent the ImmutableDisplayableQuestion class from becoming out of sync with the "actual" code for displayed questions in the future? I can imagine adding a new question attribute (especially a widget-specific one) and forgetting to update ImmutableDisplayableQuestion, leading to accidentally out-of-sync UI
• It seems like the "next" button (when toggled on in the UI settings) behaves oddly after these changes. It's disabled on the very first screen when it should be enabled, and visible on the very last screen when it should be invisible. It might not be getting refreshed properly.

[cooperka]

Can we call this by the conventional name equals or is this intentionally different?

[lognaturel]

I thought equals would be confusing because the two types we're comparing are different and we ignore almost all of the FormEntryPrompt fields. Really I want representsTheSameQuestionAsFarAsTheUserIsConcerned but I think that's a little excessive even for me.

[cooperka]

Refactor is much clearer 😉 thanks for taking the time to do it

[cooperka]

Could we implement hashCode then? Maybe an issue for later.

[lognaturel]

Ahh, yes, thank you. I'll address getodk/javarosa#425 and then update this when JR gets released.

This had me thinking I was crazy for more than a minute.

[cooperka]

The logic here comparing beforeSave vs. afterSave makes sense, and there's no urgent need to change it, but my two cents:

The way React handles updates like this -- described fabulously in this detailed blog post -- is basically by having a key for each element and comparing keys in order to determine what actually changed (vs. what simply changed order but doesn't need to be re-rendered). If we give each question a UUID, then map them below using this UUID instead of their formIndex, that could help to more easily determine additions/subtractions from the UI.

It might also just throw a wrench in an already-working system, so no hard feelings.

[lognaturel]

That's a really good blog post, thank you for sharing.

I'm not immediately seeing a good way to build those keys and have them be meaningful across form saves. That is, different objects get mutated and that results in computed values like label text being different. That's why I do that gymnastics with ImmutableDisplayableQuestion and sameAs. Anything I can come up with would require doing something like getting all of what I save in ImmutableDisplayableQuestion to compute the key. Did you see a good option off the top of your head?

[cooperka]

The benefit I'm trying to get from this is in the case where a new question is added above several other questions -- in that case it seems like the FormIndex of the old questions would change (can you confirm if this is true?)

questionsBeforeSave = [a, b, c]
questionsAfterSave = [a, x, b, c]

b and c don't need to re-render, but their FormIndex may have changed. They are technically sameAs each other but questionAtSameFormIndex is different so they aren't ever compared. If they were assigned some ephemeral UUID (doesn't need to be persisted anywhere), they could be matched using that UUID instead of the FormIndex.

Is this even a problem worth solving? Now that I write it down I realize this is an exceptional case -- the "typical" case to optimize for is when you're typing in a text field and no re-renders are triggered. So this change wouldn't help in the typical case.

[lognaturel]

FormIndex is based on the position in the form definition and so isn't affected by relevance and the like so I think we're doing basically what you suggest with the FormIndex being the UUID.

[grzesiek2010]

For me, the general approach seems fine. We can move into QA if you want.

[lognaturel]

Thanks, @grzesiek2010! I've added the "needs testing" tag. I'm sure there are edge cases I haven't thought of so I might need to do some restructuring after a first QA pass.

@cooperka:

Too bad React wasn't around when Collect was created, we're starting to implement their lib from scratch

I should have mentioned that I spent some time seeing what kind of refactor I could do that would leverage more modern Android patterns and especially dependencies like RxJava and LiveData which provide different levels of access to reactive patterns. As is the theme (you saw this with the form overview work you did), it feels too deep and risky. In this case, it really would mean changing everything about how forms are rendered, I think. I'm hoping that putting more high-level tests like the ones I put in here will help make that kind of broad change easier to consider.

Does anything prevent the ImmutableDisplayableQuestion class from becoming out of sync with the "actual" code for displayed questions in the future? I can imagine adding a new question attribute (especially a widget-specific one) and forgetting to update ImmutableDisplayableQuestion, leading to accidentally out-of-sync UI

No. In fact, you remind me that I should have included guidance hints. Yikes. Since most of the properties I use are computed, I don't really have a good sense of how I could establish that link. Any ideas?

Am looking into throttling and the next/prev buttons, thanks.

[cooperka]

Since most of the properties I use are computed, I don't really have a good sense of how I could establish that link. Any ideas?

No good ideas... as a hacky guard we could assert that the number of member vars in each class is a constant difference -- e.g. 5 vars in FormEntryPrompt and 4 vars in ImmutableDisplayableQuestion means a difference of 1.

If you add a new var to FormEntryPrompt, the test will only pass if you also add a var to ImmutableDisplayableQuestion or explicitly alter the expected difference. Feels gross just describing it, but it would be safer... I'm ok merging without this and relying on manual testing.

[lognaturel]

We're going to do an early beta for #2662 and some new translations. Since there's some risk involved in this, it would be great to get it in as well. I believe there are no deal breakers at this point, just some annoying issues. Here is current status:

• 796dab8 is not directly related to this but I'm hoping that since it's just testing related and so presents no user risk, I will be forgiven
• the behavior introduced in 18d8176 could be controversial. It's what makes the most sense to me but I can understand not wanting to scroll in that case. I think it's enough of an edge case that we can leave it as-is and change the behavior if a user complains.
• 2ab3954 fixes the navigation button issues @cooperka identified and adds tests for it
• 5ba3bd2 adds guidance hint support as pointed out by @cooperka
• 518fd6f adds support to the datetime widgets as @kkrawczyk123 pointed out didn't work. This is a little bit odd because once you select a date or time, the other one defaults to the current value but I think it's the best we can do.

Remaining todos:

• Try search() and fast itemset widgets to see if they need to be rebuilt
• Try intent widgets and think about readOnlyOverride -> issues identified and fixed in 9303aec
• Consider some kind of throttling
• Try to reproduce the issue with the range rating widget -> test added at
  9e4bf5b
• Look into the OSM widget issue
• Look into possible issue with choosing image that I've run into. It looks like the choose button may not work the first time an image widget is visited? @kkrawczyk123 have you noticed something like that?
• Look into why a widget with a calculation isn't showing the calculation result
• Look into why the third level in a cascading select isn't cleared

@kkrawczyk123, I can't identify any update-related problems with the range rating widget. Are you sure it was that one and not one of the other range widgets? My memory is fuzzy on that. While I was writing the test, I did notice a different problem with that widget. Although the default value is blank, when the widget was cleared, the widget value went to 0. That is not a good inconsistency and also meant my relevance changes weren't working as expected. Typically it's not possible to give a rating of 0 with these kinds of UI controls so I made the clearing behavior set the value to null.

I've started looking into search() and am confused about current behavior. Even on master, search-appearance-function-filter.zip seems to only filter down the list based on the first value entered into the textbox. This behavior seems fundamentally incompatible with updates in field lists. I'm not sure we want to fix this now since it goes deeper.

@grzesiek2010, perhaps you could take another look and decide whether you'd be comfortable merging this for now or whether you'd prefer another QA pass now.

[grzesiek2010]

perhaps you could take another look and decide whether you'd be comfortable merging this for now or whether you'd prefer another QA pass now.

I'm on it.

[grzesiek2010]

Both onDateChanged() and onRankingChanged() have the same body so probably we can improve - move the body to a separate method or even use just one listener for both.

[lognaturel]

Agreed. Do you want this done here? I feel like there's already a lot of random stuff going on in this PR and it may be better to separate that out so there can be a discussion on the approach.

[grzesiek2010]

It can be in a follow-up pr, just added a comment to keep it in mind.

[grzesiek2010]

@lognaturel
only a few small things, you can address them in a follow-up pr and merge this one.

[lognaturel]

Thanks so much, @grzesiek2010! I have opened #3052 to address your feedback and track the remaining todos.

[mmarciniak90]

Final tests were carried out under #3052

@opendatakit-bot unlabel "needs testing"