-
Notifications
You must be signed in to change notification settings - Fork 157
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
Add internal map delegates, create platform Projection class #390
Conversation
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.
Seems like a trivial change. Could we add some testing around these new protocols though? Seems like the projection delegate could be mocked for a series of unit tests
Not sure what testing the protocol will give us, but will add a couple of small tests to test some assumptions (e.g. conformance) |
They are simple pass through functions, but unit tests for those will help us Catch any breaking changes upstream from Core. It will be a nice catch all |
static func unproject(_ mercatorCoordinate: MercatorCoordinate, zoomScale: CGFloat) -> CLLocationCoordinate2D | ||
} | ||
|
||
public class Projection: MapProjectionDelegate { |
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.
On Android the MapboxMap : MapProjectionDelegate
to be more discoverable, where the functions inside MapProjectionDelegate are not static.
gl-native exposed generated static APIs can be directly used as Projection.project(point, zoomScale)
.
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.
Let's leave this as a point of difference? At the moment there's no need on iOS because of the lack of plugins. /cc @tobrun
_ = map as CameraManagerProtocol | ||
_ = map as MapFeatureQueryable | ||
_ = map as ObservableProtocol | ||
_ = map as MapEventsObservable |
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.
Nice, this is an interesting concept as a test I like it
PRs must be submitted under the terms of our Contributor License Agreement CLA.
Pull request checklist:
mapbox-maps-ios
changelog:<changelog></changelog>
.Summary of changes
This PR:
MapTransformDelegate
andMapProjectionDelegate
protocols, both currentlyinternal
Projection
class that implementsMapProjectionDelegate
, shadowing the one from MapboxCoreMaps.tileRegionGeometry
The two protocols match Android, except for
getMetersPerPixelAtLatitude(latitude: Double)
. Also on AndroidMapboxMap
conforms toMapProjectionDelegate
, for iOS it's a new platformProjection
Developer impact
Developers will need to update any existing use of
Projection
in their code.