Avoid leaking compassView context through animator #13743
Conversation
| @@ -411,6 +411,11 @@ public void onDestroy() { | |||
| mapCallback.onDestroy(); | |||
| initialRenderCallback.onDestroy(); | |||
|
|
|||
| if (compassView != null) { | |||
| // avoid leaking context through animator #13742 | |||
| compassView.resetAnimation(); | |||
There was a problem hiding this comment.
Thinking in terms of https://github.com/mapbox/mapbox-gl-native/issues/13152, maybe we can use compassView.setEnabled(false) here to limit the amount of publicly exposed methods?
There was a problem hiding this comment.
the enabled flag is also used for saving instance state:
outState.putBoolean(MapboxConstants.STATE_COMPASS_ENABLED, isCompassEnabled());
Since we do not control the execution order for OnDestroy vs OnSaveInstance state. We could end up in a situation where the compass remains disabled when recreating the underlying context while it initially was enabled.
There was a problem hiding this comment.
I believe that onSaveInstanceState is always going to be called before onDestroy, this is the takeaway I get from the docs. That said, I don't have strong feelings about this, so 
.
There was a problem hiding this comment.
For normal cases as an Activity yes, but since our MapView#OnDestroy is called from Fragment#onDestroyView it can occur that MapView#OnSaveInstanceState is called after MapView#OnDestroy. For example destroying an Activity while you have a MapView fragment in the backstack.
Closes #13742
cc @Guardiola31337