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

Adopt lightweight generics #1711

Closed
wants to merge 0 commits into from
Closed

Adopt lightweight generics #1711

wants to merge 0 commits into from

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Jun 11, 2015

Added lightweight generics annotations to collection-typed method and protocol signatures to streamline usage of these members in Swift. I forgot to take care of this as part of #1578. Again, there are no stability implications, but two big benefits:

  • Compiler warnings in Objective-C when we accidentally add non-annotations as annotations or send invalid types inside events.
  • Much less as! casting in Swift code that uses Mapbox GL.

/cc @bleege @incanus @friedbunny

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS refactor ready labels Jun 11, 2015
@1ec5 1ec5 self-assigned this Jun 11, 2015
@1ec5 1ec5 added this to the iOS Beta 2 milestone Jun 11, 2015
@1ec5 1ec5 added in progress and removed ready labels Jun 11, 2015
@@ -186,7 +186,7 @@ - (void)parseFeaturesAddingCount:(NSUInteger)featuresCount

if ([features isKindOfClass:[NSDictionary class]])
{
NSMutableArray *annotations = [NSMutableArray array];
NSMutableArray <MBXAnnotation *> *annotations = [NSMutableArray array];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be <id <MBXAnnotation *>>?

Copy link
Contributor

Choose a reason for hiding this comment

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

When #1655 lands, it will actually be <MGLPointAnnotation *>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, MBXAnnotation is a concrete implementation of MGLAnnotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but that PR implements MGLPointAnnotation as the SDK-provided concrete implementation of MGLAnnotation; that's my point (heh). It mirrors MapKit. Devs will no longer have to create their own point annotation concrete implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, sorry @incanus, that was in response to @friedbunny. (I was using GitHub’s mobile version, which doesn’t poll for updates.)

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 11, 2015

So Travis informs me that lightweight genetics are still off-limits. Xcode 7+ only. 😞

@1ec5 1ec5 removed this from the iOS Beta 2 milestone Jun 11, 2015
@1ec5 1ec5 added ready ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold and removed in progress labels Jun 11, 2015
@1ec5 1ec5 changed the title Adopt lightweight generics [blocked] Adopt lightweight generics Jun 11, 2015
@1ec5 1ec5 changed the title [blocked] Adopt lightweight generics Adopt lightweight generics Jun 12, 2015
@1ec5 1ec5 added in progress and removed ready labels Jun 12, 2015
@1ec5
Copy link
Contributor Author

1ec5 commented Jun 12, 2015

Actually, this isn’t blocked: since we only use collections to hold a small number of types, we can use conditionally-defined macros to indicate the same information. Such macros would fool Interface Builder’s parser, but none of the inspectables are collection-typed anyways.

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 13, 2015

0d9f251 wraps lightweight generic usage in a conditionally-compiled macro. It all works beautifully, except that the signatures in appledoc-generated documentation now go from:

@property (nonatomic) NSArray<NSString*> *styleClasses

to:

@property (nonatomic) MGL_COLLECTION ( NSArray , NSString **styleClasses

Ouch. This issue is tracked upstream as tomaz/appledoc#373.

@1ec5 1ec5 added ready and removed in progress ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jun 13, 2015
@1ec5
Copy link
Contributor Author

1ec5 commented Jun 13, 2015

OK, we’re all set now. I worked around tomaz/appledoc#373 by manually replacing all instances of MGL_COLLECTION() in package.sh.

@incanus
Copy link
Contributor

incanus commented Jun 16, 2015

Doesn't this make for a harder-to-read header file?

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 16, 2015

Compared to the native Objective-C syntax, not really, in my opinion: MGL_COLLECTION(NSArray, NSString *) * is a little more verbose than NSArray <NSString *> *, but also less likely to be mistaken for good ol’ protocol syntax. It looks like the IBOutletCollection() syntax. For Swift developers, it’s the difference between:

mapView.styleURL = mapView.bundledStyleURLs[0] as! NSURL
mapView.removeAnnotation(mapView.annotations[0] as! MGLAnnotation)
//                                              ^^^ obviously

and:

mapView.styleURL = mapView.bundledStyleURLs[0]
mapView.removeAnnotation(mapView.annotations[0])

And in both Objective-C and Swift, you get an error when you try to pass the wrong kind of objects into -[MGLMapView addAnnotations:] or forget to make your annotation class conform to MGLAnnotation. All in all, I think it’s a useful tradeoff, and we can go back to the native Objective-C syntax when Xcode 7 becomes our minimum supported IDE.

But given that lightweight generics are only bridged for NS[Mutable]Array, NS[Mutable]Set, and NS[Mutable]Dictionary, we could also do something more compact, like NS_ARRAY_OF(NSString *) *. Would that be more readable?

@1ec5 1ec5 closed this Jun 17, 2015
@1ec5 1ec5 removed the ready label Jun 17, 2015
@1ec5
Copy link
Contributor Author

1ec5 commented Jun 17, 2015

Well that confused GitHub. This was 91641ff by the way.

@1ec5 1ec5 deleted the 1ec5-generics branch June 17, 2015 22:35
1ec5 referenced this pull request Jun 17, 2015
Added lightweight generics annotations to collection-typed method and protocol signatures to streamline usage of these members in Swift. Lightweight generic type specifiers are wrapped in conditionally-compiled macros for compatibility with Xcode 6.x.

Manually preprocess the NS_*_OF() macros in a temporary copy of each header before appledoc sees the headers. Also removed the --ignore flag because we no longer have a private headers folder under include/mbgl/ios/.
@1ec5 1ec5 added this to the iOS Beta 2 milestone Jun 17, 2015
@1ec5 1ec5 mentioned this pull request Jun 18, 2015
1ec5 added a commit that referenced this pull request Jun 19, 2015
Followup to 91641ff for #1711: also preprocess the generics macros when generating a docset.
1ec5 added a commit that referenced this pull request Jun 22, 2015
Followup to 91641ff for #1711: also preprocess the generics macros when generating a docset.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants