Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios] fixes #5129 added annotation property to MGLAnnotationView #5307

Merged
merged 1 commit into from
Jun 14, 2016

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Jun 10, 2016

Also added a test case to make sure the annotation property is handled correctly but there's no reference to the platform/ios/test/MGLAnnotationViewTests.m file in the project file as I'm figuring out the details around #4641.

/cc @1ec5 @boundsj @friedbunny

@frederoni frederoni added iOS Mapbox Maps SDK for iOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold annotations Annotations on iOS and macOS or markers on Android labels Jun 10, 2016

_expectation = [self expectationWithDescription:@"test annotation property"];

MGLTestAnnotation *annotation = [MGLTestAnnotation new];
Copy link
Contributor

@1ec5 1ec5 Jun 10, 2016

Choose a reason for hiding this comment

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

Nit: use alloc/init rather than new. In general, using init directly makes it easier to add parameters to an initializer. (Admittedly, it used to matter more in the days before ARC.) In this codebase specifically, where many of the files are Objective-C++, we prefer to leave the new keyword to C++ usage.

@1ec5
Copy link
Contributor

1ec5 commented Jun 10, 2016

#4837 adds a UI test bundle to the iOS project. Until then, you may be able to add this test to the existing unit test bundle, which otherwise only tests non-UI code that's shared between the iOS and OS X SDKs.

@frederoni frederoni force-pushed the 5129-add-annotation-property branch 2 times, most recently from fa87191 to c550bdd Compare June 10, 2016 16:03
@frederoni frederoni removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jun 10, 2016
When the view is queued and waiting to be reused, the value will be set to nil.
This property should not be changed directly.
*/
@property (nonatomic, strong, nullable) id <MGLAnnotation> annotation;
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not look like there is a good reason for us to use the MK approach of warning about changing this property in the comment but not making it readonly. Let's go ahead and make it readonly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the reason MapKit leaves this property read-write is that a subclass might want to override -setAnnotation: to do something special. At least, that’s the reason MapKit exposes -setSelected:animated: publicly with a note saying never to call it (as seen in #5297).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The annotation corresponds to the visibility of the view so it could be useful to override in order to animate the adding of the annotations views. However, I'd also prefer to make this property readonly and maybe expose -mapView:didAddAnnotationViews:

Also added a test case for MGLAnnotationView
@frederoni frederoni force-pushed the 5129-add-annotation-property branch from c550bdd to 45cecef Compare June 13, 2016 14:03
@1ec5
Copy link
Contributor

1ec5 commented Jun 14, 2016

👍

@1ec5 1ec5 added this to the ios-v3.3.0 milestone Jun 14, 2016
@frederoni frederoni merged commit d564302 into release-ios-v3.3.0 Jun 14, 2016
@frederoni frederoni deleted the 5129-add-annotation-property branch June 14, 2016 11:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants