-
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
Avoid crashes that might occur while initializing map providers #3953
Conversation
2629931
to
2f1a3df
Compare
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.
I think we could do without some of the more esoteric lifecycle events like onLowMemory
but I'm also fine to not risk it. All of these call out to native code so who knows what's going on. Good call on the first commit.
I agree we can merge this without testing. I'll give others a couple days to comment if desired (@seadowg, @getodk/testers). |
protected void onStart() { | ||
super.onStart(); | ||
if (mapView != null) { | ||
mapView.onStart(); |
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.
I'm just wondering if it's possible to put the MapView
in a Fragment
so we can have all the lifecycle "forwarding" in there. Thinking we have like a MapBoxInitializationFragment
that the MainMenuActivity
can host. Then we can keep all this noise out of MainMenuActivity
.
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.
I think it's a good idea and I did it please take a look.
29e5436
to
223406b
Compare
223406b
to
ab1d362
Compare
ab1d362
to
f5e815b
Compare
I'm delighted with how the code has turned out. Thanks, @grzesiek2010! |
@getodk/testers This pulls the mapbox initialization for offline layers out of the |
Closes #3846
What has been done to verify that this works as intended?
I reviewed implemented changes.
Why is this the best possible solution? Were any other approaches considered?
The first commit is not strictly a fix for the issue but should catch possible exceptions like:
https://console.firebase.google.com/u/0/project/api-project-322300403941/crashlytics/app/android:org.odk.collect.android/issues/45c613c546aa3cea42be7d9fa55c286d?time=last-seven-days&sessionId=5EF99A4C0180000114A81ED19C1D1285_DNE_0_v2
The second commit I prepared based on:
mapbox/mapbox-gl-native-android#126 (comment)
and
https://docs.mapbox.com/android/maps/overview/#5-lifecycle-methods
@lognaturel you said that we should at least handle on destroy but in bot cases (links) they say that we should handle all lifecycle methods so just in case I think it's a good idea to do that.
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?
It shouldn't change anything in behavior. It should just prevent from crashes so i'm not even sure if this pr requires testing.
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 checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.