Add support for the odk:recordaudio action and odk-instance-load event to support Collect background audio recording

[lognaturel]

https://forum.getodk.org/t/form-spec-proposal-add-background-audio-recording/31889

Defines the odk:recordaudio action. Clients must register a listener through the static Actions.registerActionListener method to get notified when the action is triggered. When it is, they'll receive the absolute TreeReference they're supposed to populate with audio.

getodk/collect@master...lognaturel:recordaudio-action shows super basic usage.

The concept I have in mind is that Collect will register some class as a listener and each time the action is triggered, it will add the provided TreeReference to a collection. The first time it is triggered, it will start recording. Once the recording is complete, each TreeReference in the collection will get the filename of the recording.

To trigger any work that needs to happen on form load when a form contains an odk:recordaudio action, formDef.hasAction(RecordAudioActionHandler.ELEMENT_NAME) can be used.

I like that this approach stays somewhat true to the event/action concept. I also like that it doesn't further clutter up FormDef. It's easy for clients like Validate or Briefcase that won't record audio to ignore all this entirely.

I'm not in love with the static Actions.registerActionListener but I can't come up with a better alternative. The main challenge is that Actions are serialized into a cache file and so they can't have any reference to a client class. The only real other option I can think of is doing this same kind of registration through the FormDef object. Every action controller (for each element that has an action) gets passed the FormDef so could access a listener that way.

There's really nothing in this implementation that makes it specific to audio recording other than the action name. I think we could evolve this into a generic ClientHandledAction or something. I believe SetGeopoint could use that concept as well and that would be cleaner than the current implementation.

[lognaturel]

This call is intent on haunting me: https://github.com/getodk/collect/pull/4367/files#diff-3cd25b78cc06d80783f3578f5ab3d7f33d62b3e726a86dcfddc4122fd9c63357R27

I think it's telling that we keep forgetting it in test code. It feels like some higher-level abstraction should be responsible for keeping track of/determining whether an instance is new and initializing the form def. Something for another time.

[codecov-io]

Codecov Report

Merging #620 (fc3250f) into master (3746adc) will increase coverage by 0.10%.
The diff coverage is 80.88%.

@@             Coverage Diff              @@
##             master     #620      +/-   ##
============================================
+ Coverage     55.03%   55.14%   +0.10%     
- Complexity     3242     3259      +17     
============================================
  Files           242      245       +3     
  Lines         13345    13393      +48     
  Branches       2568     2573       +5     
============================================
+ Hits           7345     7386      +41     
- Misses         5197     5202       +5     
- Partials        803      805       +2     

Impacted Files	Coverage Δ	Complexity Δ
.../java/org/javarosa/core/model/CoreModelModule.java	16.66% <ø> (ø)	1.00 <0.00> (ø)
...n/java/org/javarosa/core/model/actions/Action.java	100.00% <ø> (ø)	5.00 <0.00> (-4.00)
.../actions/recordaudio/RecordAudioActionHandler.java	64.70% <64.70%> (ø)	3.00 <3.00> (?)
.../java/org/javarosa/core/model/actions/Actions.java	66.66% <66.66%> (ø)	5.00 <5.00> (?)
src/main/java/org/javarosa/core/model/FormDef.java	71.67% <77.77%> (+0.34%)	163.00 <1.00> (+2.00)
...ain/java/org/javarosa/xform/parse/XFormParser.java	67.75% <80.00%> (+0.02%)	257.00 <0.00> (ø)
.../javarosa/core/model/actions/ActionController.java	92.30% <100.00%> (+2.30%)	16.00 <0.00> (+3.00)
...e/model/actions/recordaudio/RecordAudioAction.java	100.00% <100.00%> (ø)	7.00 <7.00> (?)
.../org/javarosa/core/reference/ReferenceManager.java	60.63% <0.00%> (-1.07%)	20.00% <0.00%> (-1.00%)
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3746adc...fc3250f. Read the comment docs.

[seadowg]

This is pretty different from what we'd chatted about, but it seems there were some unexpected inside JavaRosa (which I guess is to be expected). I'm not seeing having to use TreeReference and FormIndex as sessionIds a problem. As long as we can work out a way to save using a TreeReference we can just wrap them in another Serializable.

Initially I'm seeing two problems to solve here:

1. Storing references in Collect - these will probably have to be static or maybe included in the sessionId somehow
2. Action lifecycle - the action can fire before Collect has had a chance to inspect the form for the action itself and deal with recording permission.

I'll have a play around this morning and see how this all ends up looking in Collect. The first easy step is to start using FormDef#hasAction.

[seadowg]

We probably don't need it in this case, but it's a little strange that this unregisters all listeners rather than taking one in to unregister.

[seadowg]

Another thing that's bothering me is that Action listeners end up being called on a background thread as FormDef.initialize is currently being called from the doInBackground of FormLoaderTask in Collect. We'll have to work around this as I'm not sure that's something we want to change right now given time constraints.

Maybe it's an initial bad attitude, but it feels like when "form load" happens should be up to Collect, not JavaRosa.

[seadowg]

Ok I've got a spike that is working but with some lifecycle related problems.

One thing is that with problem 1) Collect is having to build a list of references that we need to work out a way to store if the Activity is blown away and recreated to save memory. This is doable with some static repo or view model saved state, but it does feel like we're doing a lot of work here to maintain state that JavaRosa knows but is keeping to itself.

With problem 2) we end up with a bit of jank (we need to compensate for the aciton being triggered on a background thread) but it feels like it might be ok. I'm unclear on really what the advantage of using JavaRosa's idea of when the form loads vs Collect's.

I'm still leaning towards having FormDef expose the actions and then Collect choosing when to fire/trigger them.

@lognaturel lets discuss when you come online.

[lognaturel]

@seadowg and I discussed. This action continues to feel strange and not very action-y because there can only ever be one recording session going on at a time and because all of the work must be done client side and with an Android client like Collect, there are lots of lifecycle aspects to consider.

I should have explicitly addressed what we had previously discussed which was providing a list of actions from the FormDef. The biggest challenges that I ran into are that we may not have contextualized/qualified references until the action is triggered and that we'd have to separately provide actions triggered by odk-instance-first-load vs odk-instance-load.

For now it looks like @seadowg has a reasonable approach that works with this!

[seadowg]

Ok this is working in #4385. I'll now do a proper review of this PR, so we can get a snapshot out.

[lognaturel]

Thanks, @seadowg. Will get you a snapshot before the end of my day.