Fix camera method and address deprecations#3602
Fix camera method and address deprecations#3602naftalibeder wants to merge 21 commits intornmapbox:mainfrom
Conversation
|
@mfazekas What would be required to get this merged? |
ios/RNMBX/RNMBXCamera.swift
Outdated
|
|
||
| map.mapView.viewport.removeStatusObserver(self) | ||
| return super.removeFromMap(map, reason:reason) | ||
| withMapView { mapView in |
There was a problem hiding this comment.
I'm not sure about this on remove. Maybe just mapView?.viewport
There was a problem hiding this comment.
I don't understand the comment exactly - mapView doesn't need optional chaining if acquired in the withMapView callback. What's the concern?
There was a problem hiding this comment.
First it should be map.withMapView { } we're in remove so we have a RNMBXMapView we're removing from, passed in as argument .
Second if mapView of RNMBXMapView is null then what was the satusObserver was attached to? Why do we need to remove?
So I think withMapView is unnecessary in this case. Likely not harmful but you have the check carefully as it could easily keep the camera object alive, until we'll have a mapView. Which doesn't sound right, but probably no leak here.
There was a problem hiding this comment.
I don't have a strong sense of the implications of the callback architecture (_mapCallbacks.append(callback), etc.).
We can do
map._mapView.viewport.removeStatusObserver(self)
but since _mapView is force-unwrapped, I'm not sure if it could be nil when acquired that way. It'd be the only place in the camera module where map.mapView is referenced that way. Do you prefer the map._mapView syntax?
| camera: .init(cameraState: .init( | ||
| center: .init(), | ||
| padding: .zero, | ||
| zoom: zoom ?? 0, |
There was a problem hiding this comment.
I've found that I have more consistant results by using zoom: .zero. If zoom is set on this camera is seems to effect the positioning.
There was a problem hiding this comment.
@mfazekas @naftalibeder I'll aim to make a demo this afternoon of why i've highlighted this.. but when moving the camera with setCamera from a centerCoordinates position to setCamera with bounds it seems that zoom is applied to the final position. Adding this zoom: .zero looks to fix this problem thou I need to look at why.
| bearing: heading ?? map.mapboxMap.cameraState.bearing, | ||
| pitch: pitch ?? map.mapboxMap.cameraState.pitch | ||
| )), | ||
| coordinatesPadding: padding, |
There was a problem hiding this comment.
why do we use coordinatePadding and not camera padding?
Description
Fixes camera bounds-with-padding bug in v11 described in mapbox/mapbox-maps-ios#2134.
Checklist
CONTRIBUTING.mdyarn generatein the root folder/exampleapp.I added/updated a sample - if a new feature was implemented (/example)