Prevent most common restore crashes #5366
Conversation
| @@ -60,6 +57,12 @@ public class QuitFormDialogFragment extends DialogFragment { | |||
| private FormEntryViewModel formEntryViewModel; | |||
| private Listener listener; | |||
|
|
|||
| private final FormSaveViewModel.FactoryFactory formSaveViewModelFactoryFactory; | |||
|
|
|||
| public QuitFormDialogFragment(FormSaveViewModel.FactoryFactory formSaveViewModelFactoryFactory) { | |||
There was a problem hiding this comment.
I've been going back and forth on if we should be thinking about replacing Dagger in Fragments with Fragment constructors (and FragmentFactory) and the direction here is a big push there. I'm thinking we should look at restructuring how we generally do ViewModel factories and create one per Activity that can then be passed to Fragment constructors. That would mean that here the constuctor could just be:
class QuitFormDialogFragment(private val viewModelFactory: ViewModelProvider.Factory) {...and then we'd have a single factory for FormEntryActivity:
class Factory(...dependencues...) : ViewModelProvider.Factory {
override fun <T : ViewModel> create(modelClass: Class<T>): T {
when (modelClass) {
FormEntryViewModel::class.java -> { ... }
FormSaveViewModel::class.java -> { ... }
...
}
}
}As an aside, this approach would also make moving to Hilt (#4940) easier, as we'd have less of a problem with the incompatibility with Fragment.
I'd like to have finished the though in this PR, but reorganizing the view model construction for FormEntryActivity is probably not something we'd want to release as part of a hotfix, so I think it makes sense to discuss here briefly, and then create an issue (or I just follow up immediately with a different PR).
There was a problem hiding this comment.
I agree if we can use factories and constructors to pass dependencies to fragments we should do that instead of suing any DI tool.
| @@ -145,6 +145,11 @@ public void onCreate(Bundle savedInstanceState) { | |||
| formEntryViewModel = new ViewModelProvider(this, formEntryViewModelFactory).get(FormEntryViewModel.class); | |||
|
|
|||
| FormController formController = formEntryViewModel.getFormController(); | |||
| if (formController == null) { | |||
| finish(); | |||
There was a problem hiding this comment.
This is how the previous version of Collect behaved, but there was no test, and we ended up missing the behaviour change in code review.
|
Wouldn't it be easier in case of |
So you mean just replace it with a standard How about we leave this as-is for a hot fix, and then I convert |
Yes simple
Whatever is more convenient for you, if you prefer to do that in this way I'm ok with it. |
Yeah I think so? But also wondering if I've missed something here.
Ace. I think it'll be easier to follow-up. |
You will be implementing that change so if there is anything we might be missing now you should catch it later. We can also ask @lognaturel if she agrees that this dialog is not important enough to be a |
|
Tested with Success! Verified on Android 13 Verified cases:
|
|
Tested with Success! Verified on Androids: 10 |
Work towards #5240
This fixes crashes that occur when restoring to the quit form dialog or the form hierarchy which are currently the most "popular" crashes for v2022.4. Both cases result in slightly strange behaviour:
Both these quirks are definitely preferable to a crash and shouldn't be a huge problem for users (and I believe both would have been existing problems in earlier versions). I think we should merge this with them, and then update the issue so that we can fix them when we deal with the other related crashes and problems.
I ended up changing the structure of
QuitFormDialogFragmentto accommodate the restore scenario (where the dialog is recreated by the Activity before the form has loaded) and it led me down a path I've considered before (passing dependencies in Fragment constructors instead of using Dagger), but never really had the push to get there. That's discussed in https://github.com/getodk/collect/pull/5366/files#r1032477176, and I'd be especially interested in thoughts from @grzesiek2010.What has been done to verify that this works as intended?
New tests where possible and verified manually.
Why is this the best possible solution? Were any other approaches considered?
Discussed inline.
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?
Should just fix restoring (the process of reproducing that is in the issue) for the quit form dialog and form hierarchy cases. It'd be good to test these flows as well as the dialog and hierarchy in more normal contexts.
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.