Fix crash after rotating the screen with automatic location recording on in geoshape and geotrace#7019
Fix crash after rotating the screen with automatic location recording on in geoshape and geotrace#7019grzesiek2010 wants to merge 2 commits intogetodk:masterfrom
Conversation
ef3704f to
4c3d822
Compare
4c3d822 to
6fb6c87
Compare
seadowg
left a comment
There was a problem hiding this comment.
I'm a little worried that there could be regression in the stability of recording traces/shapes in the background with this change. The old solution was definitely not right, but it did appear to work for people pocketing their phone.
I think this should potentially use the Fragment's lifecycle rather than the Fragment's view lifecycle for simplicity, but as far as I can tell, this should work with the app in the background - the only scenario that the Fragment should be destroyed in is if the host Activity is killed as part of a full process kill and that isn't something we can prevent/work around.
That said, this kind of work should usually be handled by a foreground service and I think we can piggyback on the LocationTracker which is already being used here by exposing the current location as a StateFlow that gets updated on each location update (and then observing that as LiveData). I've actually just hit a point in #7017 where it would be useful to have the trace/shape state stored for GeoPolyFragment in a ViewModel rather than in the MapFragment so that I don't have to update every single MapFragment#getPolylinePoints implementation. Given that, I could take on that LocationTracker version as part of that, and we park this? What do you think? Alternatively, you could do the LocationTracker version here and I can just integrate that in.
Ahh right I thought that would be stooped but you are right. ok let's wait for #7017 |
Closes #6999
Why is this the best possible solution? Were any other approaches considered?
Replaced
ScheduledExecutorServicewith a lifecycle-aware coroutine inGeoPolyFragmentto handle automatic GPS recording. This ensures recording stops safely when the view is destroyed, preventing crashes on rotation or backgrounding. UsingScheduledExecutorService/ScheduledFutureis discouraged because it’s less lifecycle-aware and can easily lead to memory leaks or background work running after the view is destroyed.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?
I’ve reworked the automatic recording functionality for geotrace and geoshape questions, so please not only verify that the crash no longer occurs, but also test the feature for any regressions.
Do we need any specific form for testing your changes? If so, please attach one.
The form is attached to the issue
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 connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest