Rewire map initialisation#9462
Conversation
tobrun
force-pushed
the
9136-post-mapboxmap
branch
2 times, most recently
from
4b2720c to
774ab89
Compare
|
|
||
| // JNI // | ||
|
|
||
| void destroy(jni::JNIEnv&); |
There was a problem hiding this comment.
this was unused, we are using the destructor of native_map_view for this
| ViewportMode::Default, jni::Make<std::string>(_env, _programCacheDir)); | ||
|
|
||
| _initializeDisplay(); | ||
| _initializeContext(); |
There was a problem hiding this comment.
current codebase always requires to have one map object for one display/context. This is now consolidated in the constructor of the map object. This as a result removes NativeMapView::initializeDisplay, NativeMapView::terminateDisplay, ....
| private final float pixelRatio; | ||
|
|
||
| // Listeners for Map change events | ||
| private CopyOnWriteArrayList<MapView.OnMapChangedListener> onMapChangedListeners; |
There was a problem hiding this comment.
postponing map creation requires to manage OnMapChangeListeners in MapView.java instead.
| public void onMapChanged(@MapChange int change) { | ||
| if (change == DID_FINISH_LOADING_STYLE && initialLoad) { | ||
| initialLoad = false; | ||
| new Handler().post(new Runnable() { |
There was a problem hiding this comment.
this resolves a memory leak when quickly rotating the device on map start.
There was a problem hiding this comment.
This does change the timing of the event a little though. The handler ensured that it was fired after the current loop cycle. Not sure if that is a big deal.
There was a problem hiding this comment.
afaik, we posted to the end of message queue to make sure the view was measured (to be able to initialize all components with correct width/height) . Now with this PR, we postpone map creation and thus style loading to after onResume (when the view is toggled visible). Can't really think of another reason why we needed that post there?
There was a problem hiding this comment.
Ok. Just noting. As long as the tests succeed it shouldn't be a big deal.
| } | ||
|
|
||
| if (nativeMapView == null) { | ||
| mapboxMapOptions.styleUrl(url); |
There was a problem hiding this comment.
keeping state in mapboxMapOptions is map isn't ready yet, this will be applied to the map when started later on.
ivovandongen
left a comment
ivovandongen
left a comment
There was a problem hiding this comment.
There are quite a few unrelated changes in this pr (removing #includes). Could you separate those if they are intentional?
tobrun
force-pushed
the
9136-post-mapboxmap
branch
from
774ab89 to
b4aebb5
Compare
tobrun
force-pushed
the
9136-post-mapboxmap
branch
4 times, most recently
from
58a9746 to
96c11f4
Compare
tobrun
force-pushed
the
9136-post-mapboxmap
branch
from
96c11f4 to
d52030c
Compare
I removed the cpp cleanup from this PR |
| ConstrainMode::HeightOnly, ViewportMode::Default); | ||
|
|
||
| // initialize egl components | ||
| _initializeDisplay(); |
There was a problem hiding this comment.
The only problem I see here is that when we move to rendering on a separate thread, these should probably not be called from the main thread. But we can adjust to that lateron.
3 participants
This PR rewrites the way we do map initialisation. Before this PR we created the map object when the hosting MapView was inflated. This PR now postpones creating the map object until the Surface of the MapView is ready. A benefit of this is that the view will be measured by the time that MapboxMap will be created and thus removes the necessity of posting the OnMapReadyCallback invocations.
Closes #9136