Skip to content
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

Introduce MapboxMap #280

Merged
merged 4 commits into from
Apr 21, 2021
Merged

Introduce MapboxMap #280

merged 4 commits into from
Apr 21, 2021

Conversation

macdrevx
Copy link
Contributor

@macdrevx macdrevx commented Apr 20, 2021

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Changelog updated
  • Update the migration guide, API Docs, Markdown files - Readme, Developing, etc

Summary of changes

Breaking

  • BaseMapView's __map property has been moved to mapboxMap.__map

Non-breaking

  • BaseMapView has a new mapboxMap property
  • Conversion initializers between Size and CGSize have been added
  • MapboxMap is introduced. It creates and owns the Map
  • MapboxMap exposes internal conveniences for size, cameraOptions,
    and updateCamera(with:)

- BaseMapView's `__map` property has been removed
- BaseMapView has a new `mapboxMap` property
- Conversion initializers between Size and CGSize have been added
- MapboxMap is introduced. It creates and owns the Map
- MapboxMap exposes internal conveniences for `size`, `cameraOptions`,
  and `updateCamera(with:)`
@macdrevx macdrevx added skip changelog Add this label if this item does not need to be included in the changelog breaking change ⚠️ If your pull request introduces a breaking change and updates are required when version is published labels Apr 20, 2021
@@ -7,10 +7,18 @@ import Turf

internal typealias PendingAnimationCompletion = (completion: AnimationCompletion, animatingPosition: UIViewAnimatingPosition)

// swiftlint:disable:next type_body_length
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be removed in a follow-on PR that migrates more functionality from BaseMapView into MapboxMap

return __map.getCameraOptions(forPadding: nil)
}

internal func updateCamera(with cameraOptions: CameraOptions) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not unit tested because __map.setCameraFor(_:) may clamp the output.

crossSourceCollisions: NSNumber(value: mapInitOptions.mapOptions.crossSourceCollisions),
size: mapInitOptions.mapOptions.size.map(Size.init),
pixelRatio: mapInitOptions.mapOptions.pixelRatio,
glyphsRasterizationOptions: nil) // __map.getOptions() always returns nil for glyphsRasterizationOptions
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__map.getOptions() always returns nil for glyphsRasterizationOptions

Is this expected behavior @alexshalamov?

I was hoping that the input map options when creating the Map would be equal to the output when getting the options from the Map. It would certainly clean up this test :)

Comment on lines -89 to +118
public static func == (lhs: CameraOptions, rhs: CameraOptions) -> Bool {
return lhs.center == rhs.center &&
lhs.padding == rhs.padding &&
lhs.anchor == rhs.anchor &&
lhs.zoom == rhs.zoom &&
lhs.bearing == rhs.bearing &&
lhs.pitch == rhs.pitch

/// :nodoc:
public override func isEqual(_ object: Any?) -> Bool {
guard let other = object as? CameraOptions else {
return false
}
return other.isMember(of: CameraOptions.self)
&& center == other.center
&& padding == other.padding
&& anchor == other.anchor
&& zoom == other.zoom
&& bearing == other.bearing
&& pitch == other.pitch
}

/// :nodoc:
public override var hash: Int {
var hasher = Hasher()
hasher.combine(center?.latitude)
hasher.combine(center?.longitude)
hasher.combine(padding?.top)
hasher.combine(padding?.left)
hasher.combine(padding?.bottom)
hasher.combine(padding?.right)
hasher.combine(anchor?.x)
hasher.combine(anchor?.y)
hasher.combine(zoom)
hasher.combine(bearing)
hasher.combine(pitch)
return hasher.finalize()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test these directly yet. I think we'll be able to delete all of this if we are successful with our plan to use structs for types like CameraOptions

Copy link
Contributor

@julianrex julianrex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One question is do we want to go with MapboxMap and mapboxMap as the names? Can we call these just Map? "Mapbox" is used widely across the Android SDK where iOS doesn't. Seem superfluous for iOS.

@macdrevx macdrevx merged commit f77ae7d into main Apr 21, 2021
@macdrevx macdrevx deleted the ah/mapbox-map-2 branch April 21, 2021 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ If your pull request introduces a breaking change and updates are required when version is published skip changelog Add this label if this item does not need to be included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants