-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix DB ANRs during form entry #5867
Conversation
bdea42d
to
1ae3580
Compare
override fun onChanged(o: T) { | ||
data = o | ||
latch.countDown() | ||
this@getOrAwaitValue.removeObserver(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't work with cases where there was already a value set on the LiveData
.
fcd3f6d
to
6fda2a9
Compare
1382368
to
a6d1ec4
Compare
* Move select with CSV to feature level * Remove tests for features covered by other tests * Delete FormNavigationTest as it feels like our feature tests now cover navigation pretty well
collect_app/src/androidTest/java/org/odk/collect/android/support/TestScheduler.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/external/FormUriActivity.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/external/FormUriActivity.kt
Show resolved
Hide resolved
collect_app/src/test/java/org/odk/collect/android/external/FormUriActivityTest.kt
Outdated
Show resolved
Hide resolved
...ndroidTest/java/org/odk/collect/android/feature/formentry/ExternalSecondaryInstanceTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two new comments.
Looks like that last commit has broken an instrumentation test, so will mark this as a draft for the moment while I fix. |
Tested with Success! We didn't found any strange behaviors or issues related to the Form Entry in General |
Tested with Success! |
Blocked by #5836Work towards #5838
Fixes DB ANRs caused by DB access on the UI thread for the form entry flow (the bits covered by tests at least).
The main changes here:
I also ended up introducing Kotlin's Flows as an alternative to
LiveData
in one particular place (inBlankFormListViewModel
). This was because I needed to do a DB lookup in a ViewModel based on the value of aLiveData
. This is doable, but ends up being pretty clunky as observations are always run on the UI thread (which keepsLiveData
nice and safe), so we'd be jumping back and forwards between threads.Flow
lets us choose where transformations/observations are run (similar to RxJava). I've made sure that we can useFlow
withScheduler
so that we're not deviating from our existingasync
setup.Why is this the best possible solution? Were any other approaches considered?
I'd have preferred to move all the forms/instances DB operations to the background and be able to have the
StrictMode
calls constrained to justDatabaseConnection
, but it started to feel like too much work for one PR - fixing the cases within the form entry flow felt like a good chunk of progress in the end.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?
There's definitely a bunch of risk to form entry in general here, but there's nothing specific I'd have in mind to test. Playing around with form entry and seeing if there are ways to break it is all I can suggest really! It's not worth trying to reproduce the ANRs as none of them are going to be easy to recreate reliably.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest
(or./gradlew testLab
) and confirmed all checks still pass