Temporarily get global formController from navigation methods #4328
Conversation
lognaturel
force-pushed
the
vm-formController
branch
from
97be109 to
8bc2bf5
Compare
lognaturel
force-pushed
the
vm-formController
branch
from
8bc2bf5 to
ec25643
Compare
collect_app/src/main/java/org/odk/collect/android/formentry/FormEntryViewModel.java
Show resolved
Hide resolved
| @@ -125,7 +127,12 @@ public boolean canAddRepeat() { | |||
| } | |||
| } | |||
|
|
|||
| public void moveForward() { | |||
| public void moveForward(FormController formController) { | |||
| if (this.formController == null) { | |||
There was a problem hiding this comment.
I'm thinking instead of passing FormController in here we could just grab the singleton and then wrap that in an requireFormController method. That way we're not having to change the signature so hopefully not leaving anything around we'll want to take out later.
In my head, if this all worked nicely the structure would actually be pretty similar anyway: the form load and the resulting FormController would be grabbed on demand from a cache anyway (but keyed/bucketed by the form entry session rather than an application singleton) so I could see us ending up with a structure where any method that needs it in the ViewModel just loads it in lazily.
I'll have a go at adding that in and push to this, and then we can discuss later.
| @@ -168,6 +176,13 @@ private String getFormIdentifierHash() { | |||
| } | |||
| } | |||
|
|
|||
| private void ensureFormController() { | |||
| if (this.formController == null) { | |||
| Timber.w("Null formController"); | |||
There was a problem hiding this comment.
I'm wondering if we want to add any more logging here. It'd be great to try and get as much detail as possible when this happens.
| @@ -168,6 +180,13 @@ private String getFormIdentifierHash() { | |||
| } | |||
| } | |||
|
|
|||
| private void ensureFormController() { | |||
There was a problem hiding this comment.
I'm not sure if I like this...
As I understand it's added to avoid NPE's in cases where we forget to set formController right?
so in collect if we call (in FormEntryActivity):
Collect.getInstance().setFormController(formController);
formControllerAvailable(formController);
it's not needed. It's needed if we forget the second line?
I'm not sure if it makes a lot of sense. We also might forget to call the first line then: (used in this method)
Collect.getInstance().getFormController();
will return null.
so it's still not a perfect way to avoid human mistakes. Additionally it makes the code more coupled.
There was a problem hiding this comment.
Yeah agreed that it's not full proof and increases coupling. I think really the ViewModels should be able to grab the FormController from some shared component that doesn't just live on Collect. I could imagine there being some FormSessionRepository or something that lets the ViewModel get the FormController lazily (it's loaded if it's not already in memory for instance). I'd be really keen to discuss and try and put the beginnings of something like that (or something better) in #4286.
I do think this is good enough to get out there and fix the crash as the change is small and isolated but yeah, lets not get used to it.
There was a problem hiding this comment.
As you point out as well it doesn't prevent us making any further mistakes. I think that's a great indicator that we need to find something better.
There was a problem hiding this comment.
Agreed with all this. @grzesiek2010 I wish we could just set the viewmodels’ formController exactly where we set the App’s but hopefully my explanation makes sense that we can’t without further restructuring because of an extra precondition on formcontrolleravailable. This is definitely meant to be temporary.
collect_app/src/main/java/org/odk/collect/android/activities/FormEntryActivity.java
Show resolved
Hide resolved
lognaturel
changed the title
Temporary fix that should prevent crashes from #4327 while we figure out when the form controller should be set.
What has been done to verify that this works as intended?
Ran tests. Instrumented run: https://console.firebase.google.com/u/0/project/api-project-322300403941/testlab/histories/bh.f6f8331e5ae6e371/matrices/6070831307506522034
when_storageMigrationNotPerformed_shouldBePerformedAutomaticallyfails but we've seen it be flaky before so I can't imagine it's related.Why is this the best possible solution? Were any other approaches considered?
I thought #4326 was a pretty good idea but it doesn't work with the way the identity logging works. This made me pretty nervous about any kind of significant change here. This solution should be truly very low risk so I think it's best for now.
The other area I've taken an interest in is
onResume. It seems like there might be a scenario where the form controller is still set at the App level but the viewmodels had to be recreated. I think in that caseformControllerAvailablewould never be called. I thought about adding aelse if (formController != null) { formControllerAvailable(formController) }toif (formController == null && formLoaderTask.getStatus() == AsyncTask.Status.FINISHED) {but I'm not confident enough that this would be a fix.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?
The only intentional change is preventing a crash when navigation happens. I can't think of any specific risk but everything in this code is tricky. Worst case would be some kind of bad behavior related to navigation.
Do we need any specific form for testing your changes? If so, please attach one.
No.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew checkAlland confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTestlocally.