Add convenience initializers to shape source #7665

boundsj · 2017-01-10T21:29:09Z

This adds two new convenience initializers to MGLShapeSource:

-initWithIdentifier:features:options: takes an array of shape objects that conform to MGLFeature, inserts them in a shape collection feature, and creates a source with that shape.
-initWithIdentifier:shapes:options: does the same but with concrete MGLShape objects that get added to a shape collection.

Although both are technically convenience initializers, developers will most often want to use -initWithIdentifier:features:options: since features objects unlock key runtime styling functionality. So, the inline example has been updated to demonstrate MGLShapeSource usage with -initWithIdentifier:features:options:

The type system will help ensure that Swift developers use the correct convenience initializer for objects that are features (i.e. it is not possible to send plain shapes to the features initializer without an unsafe cast). There is currently no warning system for this though so Objective-C developers will not be protected from bugs where layer properties or predicate filters don't work as expected because plain shapes were previously used with the features initializer.

cc @frederoni @bsudekum @ericrwolfe

boundsj · 2017-01-10T21:33:33Z

Do we actually need a -initWithIdentifier:shapes:options:? It's nice that we support geometries when GeoJSON is loaded via a URL (or by the existing shape API) but I think, to avoid confusion, that we could get away with just -initWithIdentifier:features:options:.

boundsj · 2017-01-10T21:34:31Z

platform/darwin/src/MGLShapeSource.mm

@@ -56,6 +56,16 @@ - (instancetype)initWithIdentifier:(NSString *)identifier shape:(nullable MGLSha
    return self;
 }

+- (instancetype)initWithIdentifier:(NSString *)identifier features:(NS_ARRAY_OF(MGLFeature) *)features options:(nullable NS_DICTIONARY_OF(MGLShapeSourceOption, id) *)options {
+    MGLShapeCollectionFeature *shapeCollectionFeature = [MGLShapeCollectionFeature shapeCollectionWithShapes:features];


Should we check the type of each object in features and log a one time warning if a non-feature makes it through?

Since lightweight generic qualifiers are completely optional in Objective-C, I think it’s fairly likely that a developer would incorrectly attempt to pass in an array of MGLShapes, especially if we get rid of -initWithIdentifier:shapes:options:. I think we should raise an exception in that case, since the value would contradict the type expected by this method.

1ec5

In addition to the comments below, refer to -initWithIdentifier:features:options: in the -initWithIdentifier:shape:options: initializer as a way to put multiple features in the same source.

1ec5 · 2017-01-10T22:28:18Z

platform/darwin/src/MGLShapeSource.h

@@ -143,6 +142,41 @@ extern const MGLShapeSourceOption MGLShapeSourceOptionSimplificationTolerance;
 */
 - (instancetype)initWithIdentifier:(NSString *)identifier shape:(nullable MGLShape *)shape options:(nullable NS_DICTIONARY_OF(MGLShapeSourceOption, id) *)options NS_DESIGNATED_INITIALIZER;

+/**
+ Returns a shape source with an identifier, a shape that is a 
+ `MGLShapeCollectionFeature` holding the features passed in, and dictionary of 


This method takes an array of features, not a single shape collection feature object.

It does take an array but that is not what it returns in the "shape" property of the instantiated object. Isn't that what this sentence is trying to explain?

This sentence explains what you pass into the method.

Ah of course! I think not seeing "initialized with" threw me off for some reason today :D

1ec5 · 2017-01-10T22:29:00Z

platform/darwin/src/MGLShapeSource.h

+ `MGLShapeCollectionFeature` holding the features passed in, and dictionary of 
+ options for the source.
+
+ Passing an array of `MGLShape` subclass instances that conform to the 


This is a given.

Maybe this could be a @note instead? My thinking is that we should take every chance we have to point out the subtle difference between shapes and features. If we do get rid of -initWithIdentifier:shapes:options: then this note could probably go away completely.

Unlike -initWithIdentifier:shapes:options:, this method accepts MGLFeature instances, such as MGLPointFeature objects, whose attributes you can use when applying a predicate to MGLVectorStyleLayer or configuring a style layer’s appearance.

1ec5 · 2017-01-10T22:29:22Z

platform/darwin/src/MGLShapeSource.h

+
+/**
+ Returns a shape source with an identifier, a shape that is a
+ `MGLShapeCollection` holding the shapes passed in, and dictionary of


This method takes an array of shapes, not a single shape collection object.

Same as #7665 (comment)

1ec5 · 2017-01-10T22:32:11Z

platform/darwin/src/MGLShapeSource.h

+ `+[MGLShape shapeWithData:encoding:error:]` method.
+
+ @param identifier A string that uniquely identifies the source.
+ @param features An array of `MGLShape` subclass instances that conform to the `MGLFeature` protocol


An array of objects that conform to the MGLFeature protocol.

1ec5 · 2017-01-10T22:33:39Z

platform/darwin/src/MGLShapeSource.h

+ `+[MGLShape shapeWithData:encoding:error:]` method.
+
+ @param identifier A string that uniquely identifies the source.
+ @param shapes An array of `MGLShape` subclass instances


Missing a period, and “subclass instances” is a bit confusing:

An array of shapes; each shape is a member of a concrete subclass of MGLShape.

1ec5 · 2017-01-10T22:34:50Z

platform/darwin/src/MGLShapeSource.mm

@@ -56,6 +56,16 @@ - (instancetype)initWithIdentifier:(NSString *)identifier shape:(nullable MGLSha
    return self;
 }

+- (instancetype)initWithIdentifier:(NSString *)identifier features:(NS_ARRAY_OF(MGLFeature) *)features options:(nullable NS_DICTIONARY_OF(MGLShapeSourceOption, id) *)options {


NS_ARRAY_OF(MGLFeature) * resolves to NSArray<MGLFeature> *, meaning the passed-in value must be an NSArray object that conforms to the MGLFeature protocol. I think what you meant was NS_ARRAY_OF(id <MGLFeature>) *.

Correct. The header is written that way but it did not make it here somehow.

1ec5 · 2017-01-10T22:37:52Z

platform/darwin/src/MGLShapeSource.mm

@@ -56,6 +56,16 @@ - (instancetype)initWithIdentifier:(NSString *)identifier shape:(nullable MGLSha
    return self;
 }

+- (instancetype)initWithIdentifier:(NSString *)identifier features:(NS_ARRAY_OF(MGLFeature) *)features options:(nullable NS_DICTIONARY_OF(MGLShapeSourceOption, id) *)options {
+    MGLShapeCollectionFeature *shapeCollectionFeature = [MGLShapeCollectionFeature shapeCollectionWithShapes:features];


Since lightweight generic qualifiers are completely optional in Objective-C, I think it’s fairly likely that a developer would incorrectly attempt to pass in an array of MGLShapes, especially if we get rid of -initWithIdentifier:shapes:options:. I think we should raise an exception in that case, since the value would contradict the type expected by this method.

1ec5 · 2017-01-10T22:38:42Z

platform/darwin/test/MGLShapeSourceTests.mm

+    MGLShapeCollectionFeature *shape = (MGLShapeCollectionFeature *)source.shape;
+
+    XCTAssertTrue([shape isKindOfClass:[MGLShapeCollectionFeature class]]);
+    XCTAssert(shape.shapes.count == 1, @"Shape collection should contain 1 shape");


Use XCTAssertEqual() (×2).

1ec5 · 2017-01-10T22:53:00Z

Also, can you see if the “Working with GeoJSON Data” guide needs any updates?

1ec5 · 2017-01-10T23:39:34Z

platform/darwin/src/MGLShapeSource.h

+ Returns a shape source with an identifier, a shape that is a
+ `MGLShapeCollection` holding the shapes passed in, and dictionary of
+ options for the source.
+


Any MGLFeature instance passed into this initializer is treated as an ordinary shape, causing any attributes to be inaccessible to an MGLVectorStyleLayer when evaluating a predicate or configuring certain layout or paint attributes. To preserve the attributes associated with each feature, use the -initWithIdentifier:features:options: or -initWithIdentifier:shape:options: method instead.

1ec5 · 2017-01-10T23:42:14Z

platform/darwin/src/MGLShapeSource.h

@@ -143,6 +142,41 @@ extern const MGLShapeSourceOption MGLShapeSourceOptionSimplificationTolerance;
 */


The documentation for this method currently says:

To include multiple shapes in the source, use an MGLShapeCollection or MGLShapeCollectionFeature object.

Change it to:

To include multiple shapes in the source, use an MGLShapeCollection or MGLShapeCollectionFeature object, or use the -initWithIdentifier:features:options: or -initWithIdentifierr:shapes:options: method.

This was done in 6ba0034

boundsj · 2017-01-11T02:25:09Z

Updated the guide in 6ba0034

1ec5 · 2017-01-11T04:09:23Z

platform/darwin/docs/guides/Working with GeoJSON Data.md

@@ -36,6 +36,17 @@ resulting shape or feature object into the
 the map, or you can use the object and its properties to power non-map-related
 functionality in your application.

+To include multiple shapes in the source, create and pass an `MGLShapeCollection` or
+ `MGLShapeCollectionFeature` object to 
+ `-[MGLShapeSource initWithIdentifier:shape:options:]`. Or, use the


s/Or/Alternatively/

1ec5 · 2017-01-11T04:09:49Z

platform/darwin/docs/guides/Working with GeoJSON Data.md

+ `MGLShapeCollectionFeature` object to 
+ `-[MGLShapeSource initWithIdentifier:shape:options:]`. Or, use the
+ `-[MGLShapeSource initWithIdentifier:features:options:]` or 
+ `-[MGLShapeSource initWithIdentifier:shapes:options:]` methods to create shape


method to create a shape source with an array

1ec5 · 2017-01-11T04:14:27Z

platform/darwin/src/MGLShapeSource.mm

+            [NSException raise:NSInvalidArgumentException format:@"The object %@ included in the features argument does not conform to the MGLFeature protocol.", feature];
+        }
+    }
+    MGLShapeCollectionFeature *shapeCollectionFeature = [MGLShapeCollectionFeature shapeCollectionWithShapes:(NSArray *)features];


You likely had to cast here because +[MGLShapeCollectionFeature shapeCollectionWithShapes:] accepts an array of MGLShape <MGLFeature> * instead of an array of id <MGLFeature>. Let’s make this method and +[MGLShapeCollectionFeature shapeCollectionWithShapes:] consistent with each other. (I have no strong preference one way or the other, but id <MGLFeature> might leave more options open for fixing #7454.)

Since MGLShapeCollectionFeature inherits from MGLShapeCollection and MGLShapeCollection uses an NS_ARRAY_OF(MGLShape *) I don't think I can change MGLShapeCollectionFeature since it would require the change to bubble up to MGLShapeCollection 😬

The implementation of MGLShapeCollectionFeature does not use lightweight generics (just a plain old NSArray). I made the same change to MGLShapeSource in 6287c6b. Are you ok with this solution for this case, for now?

The implementation of +[MGLShapeCollectionFeature shapeCollectionWithShapes:] uses an unqualified NSArray in its signature, but the declaration has a qualified NSArray<MGLShape<MGLFeature> *> *. Omitting the qualifier in the implementation only takes away opportunities for the compiler to catch mistakes. We should qualify as much as possible.

Since +[MGLShapeCollectionFeature shapeCollectionWithShapes:] is declared to take an NSArray<MGLShape<MGLFeature> *> * rather than the broader type NSArray<id<MGLFeature>> *, -[MGLShapeSource initWithIdentifier:features:options:] should take an NSArray<MGLShape<MGLFeature> *> * as well.

Got it. This is done in e85da7f. I'll merge after the build passes.

This adds two new convenience initializers to MGLShapeSource: -initWithIdentifier:features:options: takes an array of shape objects that conform to MGLFeature, inserts them in a shape collection feature and creates a source with that shape. -initWithIdentifier:shapes:options does the same but with concrete MGLShape objects that get added to a shape collection. Throw an exception if an shape source is created with the features initializer but is sent an array of features that contains something that is not actually an object that conforms to the feature protocol. Updates to geojson data guide Qualify APIs that take arrays of shapes that are features

boundsj added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling labels Jan 10, 2017

boundsj added this to the ios-v3.4.0 milestone Jan 10, 2017

boundsj self-assigned this Jan 10, 2017

boundsj requested a review from 1ec5 January 10, 2017 21:29

boundsj force-pushed the boundsj-add-shapesource-conveniences branch from bb0446f to 6b99f55 Compare January 10, 2017 21:29

boundsj changed the title ~~[ios, macos] Add convenience initializers to shape source~~ Add convenience initializers to shape source Jan 10, 2017

boundsj commented Jan 10, 2017

View reviewed changes

1ec5 suggested changes Jan 10, 2017

View reviewed changes

1ec5 added the refactor label Jan 10, 2017

1ec5 reviewed Jan 10, 2017

View reviewed changes

boundsj force-pushed the boundsj-add-shapesource-conveniences branch from 3a6f2f4 to 6ba0034 Compare January 11, 2017 02:24

1ec5 approved these changes Jan 11, 2017

View reviewed changes

boundsj force-pushed the boundsj-add-shapesource-conveniences branch from e85da7f to 4166635 Compare January 11, 2017 19:51

boundsj merged commit 6ad47cf into release-ios-v3.4.0 Jan 12, 2017

boundsj deleted the boundsj-add-shapesource-conveniences branch January 12, 2017 00:05

boundsj mentioned this pull request Jan 12, 2017

MGLShapeSource should have a shape array convenience initializer #7453

Closed

1ec5 mentioned this pull request Jan 31, 2017

Cannot initialize a MGLShapeSource with array of MGLFeatures #7913

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add convenience initializers to shape source #7665

Add convenience initializers to shape source #7665

boundsj commented Jan 10, 2017

boundsj commented Jan 10, 2017

boundsj Jan 10, 2017

1ec5 Jan 10, 2017

1ec5 left a comment

1ec5 Jan 10, 2017

boundsj Jan 10, 2017

1ec5 Jan 10, 2017

boundsj Jan 11, 2017

1ec5 Jan 10, 2017

boundsj Jan 10, 2017

1ec5 Jan 10, 2017 •

edited

Loading

1ec5 Jan 10, 2017

boundsj Jan 10, 2017

1ec5 Jan 10, 2017

1ec5 Jan 10, 2017

1ec5 Jan 10, 2017

boundsj Jan 10, 2017

1ec5 Jan 10, 2017

1ec5 Jan 10, 2017

1ec5 commented Jan 10, 2017

1ec5 Jan 10, 2017 •

edited

Loading

1ec5 Jan 10, 2017

boundsj Jan 11, 2017

boundsj commented Jan 11, 2017

1ec5 Jan 11, 2017

1ec5 Jan 11, 2017

1ec5 Jan 11, 2017

boundsj Jan 11, 2017

1ec5 Jan 11, 2017

boundsj Jan 11, 2017

		@@ -143,6 +142,41 @@ extern const MGLShapeSourceOption MGLShapeSourceOptionSimplificationTolerance;
		*/

Add convenience initializers to shape source #7665

Add convenience initializers to shape source #7665

Conversation

boundsj commented Jan 10, 2017

boundsj commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Jan 10, 2017

1ec5 Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boundsj commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Jan 10, 2017 •

edited

Loading

1ec5 Jan 10, 2017 •

edited

Loading