-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[ios, macos] Add overlays
property to MGLMapView.mm
#8617
Conversation
overlays
property to MGLMapView.mm
overlays
property to MGLMapView.mm
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.
Somehow I didn’t notice this PR fly by – sorry for the delay! This looks almost good to go, but there are a couple issues to resolve first. Also, would you do the honors and add a note to the changelogs describing the MGLMapView addition.
platform/ios/src/MGLMapView.h
Outdated
overlays are associated with the map view, the value of this property is | ||
`nil`. | ||
*/ | ||
@property (nonatomic, readonly, nullable) NS_ARRAY_OF(id <MGLOverlay>) *overlays; |
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.
As of iOS 9, MKMapView.annotations
and MKMapView.overlays
have changed to being non-nullable, returning an empty array when the map contains no annotation or no overlay. This would be a good opportunity to follow suit. (Since making the property non-nullable would also change it from optional to non-optional in Swift, we whould need to add a blurb to both changelogs about this change.)
platform/ios/src/MGLMapView.mm
Outdated
{ | ||
if (self.annotations == nil) { return nil; } | ||
|
||
NS_MUTABLE_ARRAY_OF(id <MGLOverlay>) *mutableOverlays = [NSMutableArray new]; |
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.
Nit: use +[NSMutableArray array]
instead of +[NSMutableArray new]
. In general, we avoid +new
because it’s inconsistent with the normal init pattern.
Should I match [non]nullability to macOS SDK too? |
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.
Thanks, this looks good! (We can always take care of the annotations
property's nullability in a separate PR, since that would have the potential to be more disruptive than this change.)
Implemented by filtering the `annotations` property for annotations that conform to `MGLOverlay` protocol.
Copied verbatim from `annotations` property. Changed types.
Implementation is identical to the one in macOS SDK.
Implemented by filtering the
annotations
property forannotations that conform to
MGLOverlay
protocol.Fixes #4281.