Fields dependent upon an earlier field are not updated

[yanokwa]

Problem
"After answering a question in ODK collect, I expect to see an additional question displayed on the same prompt, but the screen is not being updated. The updated screen will only be displayed after going to the next prompt and then back, or after locking and unlocking the screen on the tablet. "

Explanation
ODK Collect does not recompute the relevance (visibility) of all the questions on a screen as data is entered into those questions. As a result, if you have Q2 relevance (visibility) depending upon an answer to Q1, and both are on the same screen, Q2 will be visible or hidden based upon the initial state of Q1 when the combined screen is shown, and it will never display until you leave and re-enter the screen, at which point the value of Q1 will be re-examined and Q2 will be made visible based upon Q1's (now potentially different) value.

Example
Attached is a fieldlist.xlsx that shows this issue. Enter a name and select a color or two, then swipe to the end screen. When you swipe back, you'll see new prompts which are now relevant. Ideally, the screen should re-draw as you enter data.

[lognaturel]

@yanokwa Do you only want to recompute relevance or also calculates, functions like now(), etc?

[yanokwa]

I haven't thought about this hard, but everything that can be recomputed should be recomputed

[lognaturel]

See also #140, #169

[grzesiek2010]

If we use widgets that use other activities like ImageWidget, BarcodeWidget, AudioWidget etc it works well (attached form) as ODKView is rebuilt. The problem is related to the others. Currently I don't have any good idea how to do that. For example if we use SelectOneWidget or SelectMultiWidget we could trigger the action after onCheckChanged() but we have lots of others widgets. What about if we want to do that with StringWidget when we should rebuild the view?

[dcbriccetti]

I’ve spent a few hours on this, learning the code and stepping through in the debugger. I understand what @grzesiek2010 said above 20 days ago. There are 40 subclasses of QuestionWidget. Do we know offhand how many of them would trigger regenerating or modifying the OdkView?

[mitchellsundt]

LOL. This functionality was implemented in ODK Collect 1.2, but Yaw and Carl ostensibly hated the implementation, so they ripped it out over my objections, giving us the broken state of 1.4. The Javarosa library has full support for this functionality via on-field-change callbacks. Refer back to the 1.2 tree to see how the widgets were changed to make all this work. A straightforward change.

[lognaturel]

Thank you, @mitchellsundt! @dcbriccetti how about taking a bit to look at that implementation and letting us know what you find?

[dcbriccetti]

Yes.

[mitchellsundt]

Look for my checkins. It will require some sluething, as I am not sure when the widget implementations were replaced.

…

On Mon, Mar 6, 2017 at 10:52 AM, Dave Briccetti ***@***.***> wrote: Yes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#378 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACLO0ylapI9hyMtsqARHq-ZWnd5O9io6ks5rjFYHgaJpZM4L1-mZ> .

-- Mitch Sundt Software Engineer University of Washington mitchellsundt@gmail.com

[dcbriccetti]

@mitchellsundt more clues would be helpful. I just examined your entire commit history log. And I looked at the whole recorded history of ODKView, QuestionWidget and WidgetFactory. Perhaps you can remember more?

[dcbriccetti]

I put a video in the collect channel showing how we can regenerate the page as checkboxes are clicked. This is just the result of my playing around trying to learn the code. I hope Mitch does indeed have a magical solution.

[mitchellsundt]

Too long ago to remember the code check-in. The javarosa function for monitoring change is registerStateObservers e.g., https://bitbucket.org/m.sundt/javarosa/src/054cd1ec5507b5639ef3839a6303cca935b3cf9f/core/src/org/javarosa/core/model/instance/TreeElement.java?at=default&fileviewer=file-view-default#TreeElement.java-640 The callback class is FormElementStateListener https://bitbucket.org/m.sundt/javarosa/src/054cd1ec5507b5639ef3839a6303cca935b3cf9f/core/src/org/javarosa/core/model/FormElementStateListener.java?at=default&fileviewer=file-view-default Mitch

…

On Mon, Mar 6, 2017 at 8:26 PM, Dave Briccetti ***@***.***> wrote: I put a video in the collect channel showing how we can regenerate the page as checkboxes are clicked. This is just the result of my playing around trying to learn the code. I hope Mitch does indeed have a magical solution. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#378 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACLO0-HB_Q_17gMXyyUxi0HlEH5xsDr7ks5rjNx3gaJpZM4L1-mZ> .

-- Mitch Sundt Software Engineer University of Washington mitchellsundt@gmail.com

[dcbriccetti]

@mitchellsundt, did you find a better way than re-instantiating ODKView after every change? I discussed this issue with @jnordling, and he and I were both unable to imagine a “straightforward change” that would solve this. If you think of something more that would help, I’d love to hear it. Thanks.

[dcbriccetti]

I would be happy to continue to look at this, but I think it will be very complex and time consuming. I am leaving it until I hear something further.

[lognaturel]

Thanks all who have given some input here. I agree with @dcbriccetti that we should leave it aside for now. Let's be on the lookout for clear usecases from users so we can get a better sense of exactly what events should trigger refresh (leaving a field, scrolling, changing a field...).

[heyjamesknight]

Hey everyone, jumping in on this one now. I'll be sure to reach out if I have any questions.

[dcbriccetti]

@jknightco More power to you. This is not an easy change. Where I left off (see Mar 23 above) the best idea I had was re-instantiating ODKView after every change (which I don’t think is practicable).

[heyjamesknight]

Just adding some notes from what I've found so far. I'm sure most of you already know some or all of these things—just want to get them out in text for reference:

• JavaRosa handles relevancy like so:

  • Relevancy is parsed within StandardBindAttributesProcessor createBinding method and stored within a Condition object.
  • The Condition object is added to the FormDef as a Triggerable.
  • FormDef calls triggerTriggerables in its init method, which then calls initializeTriggerables in Safe2014DagImpl, then evaluateTriggerables, and finally doEvaluateTriggerables in LatestDagBase. It is here the Triggerables are actually evaluated and the relevancy is initially computed and set true/false based on the current state of the form.

• Collect only looks at the computed relevancy when FormController getQuestionPrompts is called. This is only ever called from within FormEntryActivity createView.

• getQuestionPrompts specifically ignores any non-relevant forms, meaning our current implementation will require a full recreate of the ODK View in order to show previously non-relevant forms.

[heyjamesknight]

The core finding here is that registerStateObservers isn't of much use to us currently, as internal changes within the Form data from JavaRosa have no way of being easily propagated out to Collect. I think the first goal may be to figure out how to best update visible forms (like the "your name is ${name}" field) and then worry about changing the ODKView implementation to load (but hide) irrelevant forms.

[lognaturel]

Thanks for that background @jknightco. I like that breakdown -- first looking at evaluating calculates and constraints across questions in a field list and then handling changes in relevance. Did I frame that correctly?

[heyjamesknight]

Yep, much better than my post-debugging word salad 😛
We'll need to figure out how to get changes to occur at all before worrying about the fact that some widgets aren't even present to update.

[lognaturel]

Not at all, I was just checking my understanding! 😊

[heyjamesknight]

Still trying to get a good grasp of our options here. Is there any reason why we wouldn't want to call saveAnswer when each field is updated? That method triggers reevaluation of the Triggerables (heh), which in turn spits out updates via the registerStateObservers method mentioned above.

It would still require some pretty drastic changes, but I'm thinking our best bet may be modifying the Widgets to update their underlying Form Answers when they're updated rather than just whenever the form is closed, which would then allow us to watch for CHANGE_RELEVANT events to get spit out.

We'd then have to handle these updates either by recreating the ODKView, or possibly by having the Widgets watch for their own relevancy changes. First we need to figure out the best way to get these events broadcast, however.

[lognaturel]

For the interested, we had a good discussion in Slack here.

[anz000]

Any updates on this feature?

[mdudzinski]

I think a simple approach could work with rebuilding the whole screen. The rebuilding should be triggered only if the relevancy has changed for a question on the current screen. So the input must be inside of a group with the field-list appearance. The problem is that the observer implementation requires a FormEntryPrompt instance which - as it's been already pointed earlier - is problematic because non-relevant questions are ignored by the FormController class. This can be solved by low-level scan of the form and set FormElementStateListener instances on the TreeElements which relevancy is based on the calculations from other nodes from the same group. It doesn't sound as an elegant solution but should be - relatively - simple to implement as it leverages the existing code architecture.
I can try to prepare a simple proof of concept to see if my assumptions are right.

A better, long term solution most likely will require a lot of refactoring and a lot of changes in the ODKView management. Perhaps changes in JavaRosa API as well. Such re-working would probably address more issues.

[drik]

@mdudzinski Did you go further with your first proposed solution ? If yes, can you share with us? Thanks

If any other person has something about this issue, please share with us :)

[tiritea]

I dont know the inner workings of how Collect populates a 'screen' (page of questions) but I do think this issue needs addressing somehow soon. Folks are going to start wanting to exploit increasing mobile device screen sizes and resolutions to display more (related) questions at once, particularly select lists with an associated other (!). So I only see the use of field-lists (in ODK) increasing. I tend to agree with @mdudzinski that the simplest and - in the general case - probably only viable approach is being prepared to rebuild the whole screen; ie re-layout all visible controls. [aside: this was a design point in my iXForms from the beginning, and prob w/ Enketo too]. This will also accommodate not just relevant show/hide logic, but also any visual changes needing to be refreshed as a consequence of read-only calculations, embedded values in text labels, etc

Which is to say, doing it 'right' and dynamically refreshing all visible controls could well solve other potential issues down the road... Just my $0.02

[tiritea]

FYI ran across this one again, translating a form that ask for #male and #female, computes the total, and then does a constraint check against total before proceeding. But because the total computation is in the same field-list it doesn't actually get computed, so you cant proceed. Only way around it it rip the entire field-list apart and display every question separately. This is a serious limitation in trying to use field-list to visually present intimately related questions together for a better user experience in Collect :-(

[tiritea]

BTW Survey123 supports this [I'm translating a Survey123 form] - perhaps ODK Collect can 'plagiarize' how they accomplished it? :-)

[yanokwa]

Survey123 is an entirely different codebase, so I don't think that helps. I think we all agree that it's a problem. It's the who, how, and when that remains the challenge.

[tiritea]

K. I was hopeful perhaps Survey123 was just a (very) old fork of Collect, and there might be something we could reuse... guess not.

FWIW, how I handle this in iXForms is that I basically determine all the (potentially) visible controls in the currently displayed 'page' (ie field-list group or root level page); these are then rendered and that's what you see. Then, whenever any the controls is changed, in addition to re-running all the (dependent) calculations in the entire form, all these controls have their relevant, readonly, and constraints re-calculated, and all visible control widgets are then refreshed - basically I redraw the page [controls never change groups, so its only a matter of refreshing the controls previously determined to be potentially visible on the current page]. This handles show/hide logic for controls on the same page, updating values displayed in other controls, as well as potentially re-flagging controls if they become required and unanswered (red) or not, or read-only (grey,italic) since this can be an XPath expression too.

[lognaturel]

I've finally had a chance to put together a proof of concept for this using some of the ideas described above. In particular, I think it makes sense to call saveAnswer each time an answer is modified as described here so that the form engine can recompute all that it needs to. I don't see an alternative to that and I think it's as efficient as can be given the DAG optimization.

With that, when a widget's value changes, we can get relevant questions before saving answers, save answers, and then get relevant questions after. We can essentially do a diff between the two to know which widgets to add and remove. I was getting stuck on how to do the diff and saw that CommCare does a similar thing by comparing user-visible strings to determine what questions have and haven't changed. I can't come up with better.

Here is the code I'm working with. I've verified the form from the original issue as well as number-string-number.xml.zip. There are various bugs documented in TODOs, one with focus jumping to the first question, and certainly a lot of cleanup that can be done but I think it's fairly close.

Any violent objections to this approach? If not, I'll clean up and send in a PR.

CC @grzesiek2010 @zestyping @cooperka

[tiritea]

Just a heads up that something you may have to consider (as I did) is when potentially all the questions in the current field-list become irrelevant as a consequence of something you did. Presumably, under Collect, you'd want to automatically flick to the next control after the field-list, right (rather than displaying an empty screen...)?

[tiritea]

(btw, strictly speaking, "earlier" doesn't matter... any control within a field-list/page may appear or disappear as a consequence of other controls in the same field-list/page, irrespective of their partial ordering)

[lognaturel]

"earlier" doesn't matter... any control within a field-list/page may appear or disappear

Absolutely. Going further, the whole form needs to be re-evaluated because changes within the page could affect fields somewhere else in the form which could in turn have impact on parts of the currently-displayed page. That is, even if it were practical to only re-evaluate the parts of the form that are currently displayed, that wouldn't be sufficient. This re-evaluation is relatively efficient because JavaRosa ensures only questions that depend on fields that have changed are re-computed.

all the questions in the current field-list become irrelevant as a consequence of something you did

Good edge case, I hadn't considered that one! 👍

[tiritea]

the whole form needs to be re-evaluated because changes within the page could affect fields somewhere else in the form which could in turn have impact on parts of the currently-displayed page.

Correct. ALL calculations in the form must be re-evaluated whenever any control is updated (DAG helps here...), but only the relevant, read-only, constraint expression of visible controls (which in Collect means those thing in the current field-list) need to be re-evaluated.

[lognaturel]

One more thing I wanted to explicitly address. I did look into using FormElementStateListener as described in this comment. However, events are only dispatched for changes to data, locale, and whether a question is read-only, relevant or required. Notably, there are no events for when the label, hint or select choices text change. Those are dynamically computed when requested and I can't think of any reasonable way to dispatch events when they change. I don't think any solution to that would be cleaner or more efficient than what I'm proposing but that would be a good thing to get a quick sanity check on.

[tiritea]

oh yes, since you'll want to re-display every visible control anyway, that should take care of updating any labels containing references to instance elements (irrespective of if the associated control has a calculation/relevant/read-only or not...)

[tiritea]

another one i just thought of: relevant expression of enclosing field-list group may trigger dependent on enclosed control. [Of course, it takes a bit of a perverted mind to come up with a form like that, but such is the playground of an ex-tester... :-) ]

[grzesiek2010]

I took a look, played with some forms and the general approach looks good to me. Nice!