-
Notifications
You must be signed in to change notification settings - Fork 115
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
[Draw area task] Fix crashes on config change #2761
Conversation
...in/java/com/google/android/ground/ui/datacollection/tasks/polygon/DrawAreaTaskMapFragment.kt
Outdated
Show resolved
Hide resolved
…o gino-m/2725/fix-task-constructors
Config changes are now supported on DrawAreaTask: Screen_recording_20240923_172230.webmUnsaved draft Changes are still restored when the app is killed: Screen_recording_20240923_172342.webm@anandwana001 would you like to apply a similar fix to remaining Task(Map)Fragments? |
@@ -95,6 +96,8 @@ abstract class BaseTaskFragmentTest<F : AbstractTaskFragment<VM>, VM : AbstractT | |||
preTransactionAction = { | |||
fragment = this as F | |||
fragment.taskId = task.id | |||
// TODO: Attach `fragment` to mock DataCollectionFragment |
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.
@shobhitagarwal1612 @anandwana001 DrawAreaTaskFragmentTest
is failing with:
Fragment DrawAreaTaskMapFragment{44fdc5b3} (07ec492d-1efe-474f-94ab-b361fdc1063e id=0x2c93 tag=DrawAreaTaskMapFragment) is not a child Fragment, it is directly attached to dagger.hilt.android.internal.managers.ViewComponentManager$FragmentContextWrapper@33500213
java.lang.IllegalStateException: Fragment DrawAreaTaskMapFragment{44fdc5b3} (07ec492d-1efe-474f-94ab-b361fdc1063e id=0x2c93 tag=DrawAreaTaskMapFragment) is not a child Fragment, it is directly attached to dagger.hilt.android.internal.managers.ViewComponentManager$FragmentContextWrapper@33500213
at androidx.fragment.app.Fragment.requireParentFragment(Fragment.java:1183)
...
I believe we have to update the way we mock out the task view model here, but not sure how. Any ideas?
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.
Resharing @anandwana001 's suggestion via chat:
adding childfragment:
inline fun <reified T : Fragment> launchFragmentWithNavController(
fragmentArgs: Bundle? = null,
@StyleRes themeResId: Int = R.style.FragmentScenarioEmptyFragmentActivityTheme,
destId: Int,
fragment: Fragment?=null,
crossinline preTransactionAction: Fragment.() -> Unit = {},
crossinline postTransactionAction: Fragment.() -> Unit = {},
): ActivityScenario<HiltTestActivity> =
hiltActivityScenario(themeResId).launchFragment<T>(
fragmentArgs,
{
// Attach `fragment` as a child of the mock `DataCollectionFragment`
parentFragment?.childFragmentManager?.beginTransaction()
?.add(R.id.data_collection_fragment, fragment!!, "childFragmentTag")
?.commitNow()
this.preTransactionAction()
viewLifecycleOwnerLiveData.observeForever { viewLifecycleOwner ->
if (viewLifecycleOwner != null) {
val navController = TestNavHostController(getApplicationContext())
navController.setViewModelStore(ViewModelStore())
// Required for graph scoped view models.
navController.setGraph(R.navigation.nav_graph)
navController.setCurrentDestination(destId, fragmentArgs ?: bundleOf())
// Bind the controller after the view is created but before onViewCreated is called.
Navigation.setViewNavController(requireView(), navController)
}
}
},
) {
this.postTransactionAction()
}
and pass the parent fragment
launchFragmentWithNavController<T>(
destId = R.id.data_collection_fragment,
fragment = DataCollectionFragment(),
preTransactionAction = {
// Set up fragment-specific properties
fragment = this as F
fragment.taskId = [task.id](http://task.id/)
}
)
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.
@shobhitagarwal1612 Any ideas on the best way to create and attach DataCollectionFragment
as the parent to the task under test here? We need to get the view model used by the tasks.
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.
@shobhitagarwal1612 Gentle ping
mapViewModel = getViewModel(BaseMapViewModel::class.java) | ||
val taskId = arguments?.getString(TASK_ID_ARG_KEY) ?: error("null taskId arg") | ||
val dcf = requireParentFragment() as DataCollectionFragment |
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.
requireParentFragment()
can throw an exception, can we handle it?
This is the edge case, if this fails, no ViewModel will be there
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.
Can that ever happen in practice, and is it something we can recover from?
@@ -57,7 +64,6 @@ class DrawAreaTaskMapFragment(private val viewModel: DrawAreaTaskViewModel) : | |||
} | |||
|
|||
companion object { | |||
fun newInstance(viewModel: DrawAreaTaskViewModel, map: MapFragment) = | |||
DrawAreaTaskMapFragment(viewModel).apply { this.map = map } | |||
const val TASK_ID_ARG_KEY = "taskId" |
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.
AbstractTaskFragment already has this, how about using the same rather than creating this new?
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.
AbstractTaskFragment has the key used in Bundle
, not Fragment args. They happen to have the same value, but that can be considered coincidental.
…s' into gino-m/2725/fix-task-constructors
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2761 +/- ##
============================================
- Coverage 60.93% 60.86% -0.07%
- Complexity 1158 1159 +1
============================================
Files 265 265
Lines 6141 6146 +5
Branches 854 856 +2
============================================
- Hits 3742 3741 -1
- Misses 1916 1921 +5
- Partials 483 484 +1
|
collect
call is moved to after the map has loaded.Provider
to create new instances so that@Inject
works.DataCollectionViewModel
on config changes so that view is restored.Fixes #2725
Towards #2624, #2682
Screen_recording_20240918_224809.webm
The data collection flow still reverts to the previous step in the flow on config change, and exits if in the first position, but this is likely an unrelated issue.