Add features initializer to MGLGeoJSONSource by ituaijagbone · Pull Request #6524 · mapbox/mapbox-gl-native

ituaijagbone · 2016-09-29T23:46:49Z

mention-bot · 2016-09-29T23:46:51Z

@ituaijagbone, thanks for your PR! By analyzing this pull request, we identified @incanus, @1ec5 and @boundsj to be potential reviewers.

1ec5

You're on the right track. Here are a few preliminary comments.

1ec5 · 2016-09-30T00:17:39Z

+
+    NSMutableArray *coordinates = [NSMutableArray array];
+
+    for (int index = 0; index < self.pointCount; index++) {


int might not be large enough for the number of coordinates that may be in this polyline. Use the same type as pointCount.

1ec5 · 2016-09-30T00:18:30Z

 - (instancetype)initWithSourceIdentifier:(NSString *)sourceIdentifier URL:(NSURL *)url options:(NS_DICTIONARY_OF(NSString *, id) *)options NS_DESIGNATED_INITIALIZER;

+
+- (instancetype)initWithSourceIdentifier:(NSString *)sourceIdentifier Features:(NS_ARRAY_OF(id<MGLFeature>) *)features options:(nullable NS_DICTIONARY_OF(NSString *, id) *)options NS_DESIGNATED_INITIALIZER;


Nit: lowercase the Features parameter.

1ec5 · 2016-09-30T00:19:58Z

        _features = MGLFeaturesFromMBGLFeatures(geojson);
+    } else {
+
+        NSMutableArray<NS_DICTIONARY_OF(NSString *, id) *> *featuresArray = [NSMutableArray array];


Use the macros consistently: either change NSMutableArray to NS_MUTABLE_ARRAY_OF, or change NS_DICTIONARY_OF to NSDictionary.

1ec5 · 2016-09-30T00:21:30Z

+                                            @"features":featuresArray};
+
+        NSError *error;
+        NSData *featuresJSONData = [NSJSONSerialization dataWithJSONObject:featureCollection options:NSJSONWritingPrettyPrinted error:&error];


Pretty printing is good for debugging purposes, but it'll need to be removed before merging because it increases memory consumption.

1ec5 · 2016-09-30T05:07:54Z

+- (NS_DICTIONARY_OF(NSString *, id) *)featureDictionary {
+    // TODO: MGLPolygonFeature
+
+    NS_MUTABLE_ARRAY_OF(NS_MUTABLE_ARRAY_OF(NS_ARRAY_OF(NSNumber *) *) *) *coordinates = [NSMutableArray array];


These macros (or the lightweight generics syntax) are useful in headers and method signatures, but they’re totally optional inside method implementations. As you can see, they’re a bit unwieldy, so there’s no shame in leaving them out inside the method.

1ec5 · 2016-09-30T15:07:00Z

+    return geometries;
+}
+
+- (void)addTypeAndCoordinatesFrom:(id<MGLFeature>)feature to:(NS_MUTABLE_ARRAY_OF(NS_DICTIONARY_OF(NSString *, id) *)**)geometries {


This is a questionable refactoring. First of all, since the array is mutable, you can simply pass a pointer to it rather than a double-pointer. Secondly, if all the classes that implement MGLFeature have a -featureDictionary method, it would be cleaner to declare that method on MGLFeature. That way, -geometryCollection: can simply call feature.featureDictionary without caring which specific class feature happens to be.

Yes You are right. Calling -featureDictionary should work directly. Thank you.

1ec5 · 2016-09-30T15:08:32Z

+             };
 }

+- (NS_MUTABLE_ARRAY_OF(NS_DICTIONARY_OF(NSString *, id) *) *)geometryCollection:(NS_ARRAY_OF(MGLShape *) *)shapes {


MGLShapeCollectionFeature already knows about its shapes, so there’s no need to pass it in here. Just use self.shapes inside the method.

Done in 087f013

boundsj

Looking good! Seems like we will need to regenerate the layer code to get the tests passing again now that the GeoJSON source method signatures have been updated.

boundsj · 2016-09-30T18:43:25Z


 @synthesize identifier;
 @synthesize attributes;
+// synthesize the feature dictionary


It'd fine to remove this comment

boundsj · 2016-09-30T18:46:02Z


+- (NS_DICTIONARY_OF(NSString *, id) *)featureDictionary {
+    return @{@"type":@"Feature",
+             @"properties":(self.attributes) ? self.attributes : [NSDictionary dictionary],


nit this could be an empty NSDictionary literal

boundsj · 2016-09-30T18:48:59Z


+- (NS_DICTIONARY_OF(NSString *, id) *)featureDictionary {
+    // TODO: MGLMultiPointFeature
+    NS_MUTABLE_ARRAY_OF(NS_ARRAY_OF(NSNumber *) *) *coordinates = [NSMutableArray array];


nit: I definitely prefer, inside method implementations, to just write NSMutableArray *coordinates (+1 to #6524 (comment))

boundsj · 2016-09-30T18:59:16Z

 @param sourceIdentifier A string that uniquely identifies the source.
- @param URL An HTTP(S) URL, absolute file URL, or local file URL relative to the
-    current application’s resource bundle.
+ @param features An array of features that conform to the MGLFeature protocol.


nit: It'd be nice to surround MGLFeature in back ticks so that the generated documentation highlights the word like MGLFeature

boundsj · 2016-09-30T19:05:19Z

+
+
+- (void)testMGLGeoJSONSourceWithPolygonFeatures {
+    // TODO: Create feature


nit remove the scaffolding comments

boundsj · 2016-09-30T19:14:19Z

+            @"features":featuresArray};
+
+        NSError *error;
+        NSData *featuresJSONData = [NSJSONSerialization dataWithJSONObject:featureCollection options:0 error:&error];


#6524 (comment) suggested removing the pretty printing. Noting also that this will be refactored away in a future commit where we eventually just set mbgl::GeoJSON{ featureCollection } and avoid the trip to create JSON data.

boundsj · 2016-10-06T01:44:53Z

We should probably wait for #6588 to land before continuing with this because of the the duplicate work and conflicts in MGLGeoJSONSource and related tests

cc @1ec5 @ituaijagbone

1ec5 · 2016-10-07T15:50:31Z

#6588 has landed. As expected, there are a number of conflicts, but they should be straightforward to resolve.

boundsj · 2016-10-11T23:35:57Z

@incanus @1ec5 @frederoni please feel free to have another look at this when you have a chance. I'm not sure why the CI builds started failing today but it does not seem to be related to any changes in this PR.

Note the comment in 70962eb. I'm up for suggestions about where / how to share the mbgl::Value conversion logic.

boundsj · 2016-10-11T23:38:26Z

Example of a query for features in rect that are then used to initialize a GeoJSON source and styled with a fill layer.

1ec5 · 2016-10-12T17:28:27Z

How about stuffing the value in an NSExpression and calling the existing category method?

Fixed in 3770a036d68a4bcca7ffa91a4ff11036262d491a

1ec5 · 2016-10-12T17:28:50Z

Nit: funky spacing.

Fixed in 3770a036d68a4bcca7ffa91a4ff11036262d491a

1ec5 · 2016-10-12T17:29:34Z

Pass C++ objects around by reference rather than as pointers.

Fixed in 3770a036d68a4bcca7ffa91a4ff11036262d491a

1ec5 · 2016-10-12T17:30:20Z

Use .UTF8String.

Fixed in 3770a036d68a4bcca7ffa91a4ff11036262d491a

1ec5 · 2016-10-12T17:35:30Z

Would it be more convenient for NSDictionary to know how to turn itself into a PropertyMap?

Fixed in 3770a036d68a4bcca7ffa91a4ff11036262d491a

1ec5 · 2016-10-12T17:38:43Z

MGLShapeCollectionFeature is a concrete class.

Fixed in 3a6be23e3a922e739b6e7b1687d5630621bb7ba6

1ec5 · 2016-10-12T17:40:45Z

Mention that this class also doubles as a superclass of MGLPolyline and MGLPolygon, despite being a distinct geometry.

Fixed in 3a6be23e3a922e739b6e7b1687d5630621bb7ba6

1ec5 · 2016-10-12T17:41:18Z

Nit: lowercase.

Fixed in 3a6be23e3a922e739b6e7b1687d5630621bb7ba6

1ec5 · 2016-10-12T17:42:29Z

This is really unwieldy. Just use an unqualified mutable array.

Also initialize it with a capacity.

Fixed in 3770a036d68a4bcca7ffa91a4ff11036262d491a

1ec5 · 2016-10-12T17:45:15Z

A bunch of these files are missing from the macOS project.

Fixed in 4268576d1e5dc05514ebf412a6b9723ab4623b93

boundsj · 2016-10-12T23:59:48Z

Since yesterday afternoon, I'm still getting npm ERR! Permission denied (publickey). for all CI builds and I have no idea why.

1ec5 · 2016-10-13T00:21:43Z

This class doesn't seem to be adding much. The lone function can be a standalone C++ function.

Fixed in 206af31

1ec5 · 2016-10-13T00:24:00Z

The non-feature direct subclasses of MGLShape should have geoJSONDictionary methods that encapsulate these geometry structures.

Once that happens, the bodies of most of these featureDictionary methods can be factored out.

I took this I idea and ran with it a bit in ad4e54d

1ec5 · 2016-10-13T00:28:18Z

+[NSArray arrayWithArray:] creates a non-mutable array.

This is intentional. The method says that it returns a non-mutable array even though it uses a mutable array internally to create the collection

(NSArray *)geometryCollection

My bad, I figured -[NSMutableArray copy] returns an NSMutableArray, but it returns an NSArray. Carry on.

1ec5 · 2016-10-13T00:33:06Z

Passing in the property map by non-const reference is similar to an inout C parameter: you're actually mutating the map that's being passed in. So there's no need to return anything. In fact, this method could be renamed to "populatePropertyMap" or somesuch. But if an empty property map is always passed in, why not create it locally instead of accepting it as a parameter?

If the intent is to convert a dictionary into a property map, this could be a category method on NSDictionary that takes no parameters and returns a property map.

This was fixed in bc1dda3. Note thatpropertyMapWithDictionary: and vectorForArray: (name change in 95d548b) still take NSDictionarys and NSArrays so that the entire dictionary can be traversed (recursively) including if any node is a dictionary or array. The (mbgl::PropertyMap)mbgl_propertyMap method already exists and is the only public method of the category. It is intended to take no parameters and return a property map.

1ec5 · 2016-10-13T00:35:37Z

Similarly, this would be a category method on NSArray that returns a vector of values, and the method name would be something like mgl_valueVector.

1ec5 · 2016-10-13T22:45:55Z

The word “feature” in this method name is redundant.

Fixed in 0c317fa

1ec5 · 2016-10-13T22:46:55Z

Pedantry: it’s a representation of an instance of the class, not of the class itself.

Fixed in 0c317fa

1ec5 · 2016-10-13T22:48:51Z

The dictionary includes a geometry key corresponding to the receiver’s underlying geometry data, a properties key corresponding to the receiver’s attributes property, and an id key corresponding to the receiver’s identifier property.

Fixed in 0c317fa

1ec5 · 2016-10-13T22:53:43Z

Rename -geoJSONGeometryDictionary to -geoJSONDictionary and -geoJSONFeatureDictionary to geoJSONDictionary. Then this method can call super’s implementation and wrap it in NSDictionaryFeatureForGeometry().

Fixed in 0c317fa

1ec5 · 2016-10-13T22:54:35Z

Nit: the if statement is unnecessary; it’s perfectly fine to set to nil using subscript notation.

If I remove that conditional, I get a runtime exception when [NSExpression expressionForConstantValue:identifier]; is called. I'm using NSExpression for the conversion of the id to the mbgl variant .

Huh, I wouldn't've expected foo[@"bar"] = nil to do anything other than remove the "bar" key from foo.

Sorry, I was in mbglFeature() not NSDictionaryFeatureForGeometry(). Fixed in acf792b

1ec5 · 2016-10-13T22:56:07Z

Remove the dictionary parameter and use self instead. Below, call the method recursively on dictionary[key] rather than on self. Then rename this method to -mgl_propertyMap (not -mbgl_propertyMap – MGL is the class prefix for this target).

Fixed in 0c317fa

And 3909852

1ec5 · 2016-10-13T22:57:29Z

Should we throw an exception if we encounter an unknown type?

I thought about this and decided to not throw an exception when I added it yesterday.

The reason is that the first thing mgl_featureIdentifier does is call mgl_convertedValueWithValue which will throw an exception if there is an unknown type. mgl_featureIdentifier exists only to be able to get the correct raw value from mbgl::Value returned by mgl_convertedValueWithValue and the only reason we are doing any of this here is to share the conversion logic that happens to currently live in the NSExpression category.

I think you could make the case that we should catch an exception thrown by mgl_convertedValueWithValue and re-raise it with a different message (instead of the Can’t convert %s:%@ to mbgl::Value) that a developer will see if they send an invalid value for the id / identifier. However, I was thinking that it might make sense to wait and refactor this if we introduce a category method on NSObject as you've suggested in conversation. It is a bit weird at the moment though.

1ec5 · 2016-10-14T00:57:00Z

Remove this parameter too.

Fixed in 61f4d69

1ec5 · 2016-10-14T00:57:23Z

Once we refactor this so that any object knows how to "valueize" itself, we'll be able to remove these conditionals too.

1ec5 · 2016-10-14T00:58:10Z

Nit: feature feature! 🍕

🍕 - 1 in 184c475

1ec5

Almost there!

1ec5 · 2016-10-14T05:10:41Z

These files are missing from macos.xcodeproj. Also, NSArray+MGLAdditions.mm appears to be in Sources twice.

Will fix macos. I used the standard mechanism in xcode for adding files. I guess I'll manually edit the file but seems strange that I need to do this.

Looks like this is common. I think every impl file is in there twice and often headers are too. Look just below:
404C26E21D89B877000AA13D /* MGLTileSet.h in Headers */ = {isa = PBXBuildFile; fileRef = 404C26E01D89B877000AA13D /* MGLTileSet.h */; settings = {ATTRIBUTES = (Public, ); }; }; 404C26E31D89B877000AA13D /* MGLTileSet.h in Headers */ = {isa = PBXBuildFile; fileRef = 404C26E01D89B877000AA13D /* MGLTileSet.h */; settings = {ATTRIBUTES = (Public, ); }; }; 404C26E41D89B877000AA13D /* MGLTileSet.mm in Sources */ = {isa = PBXBuildFile; fileRef = 404C26E11D89B877000AA13D /* MGLTileSet.mm */; }; 404C26E51D89B877000AA13D /* MGLTileSet.mm in Sources */ = {isa = PBXBuildFile; fileRef = 404C26E11D89B877000AA13D /* MGLTileSet.mm */; };

Maybe this is a bug related to adding to both static and dynamic targets?

I'm not going to touch this in this PR. Let's open a new issue to clean this up if we need to.

Added to macos in c4ea541

Oh, you’re right, I thought I was looking at the project structure but I was looking at target membership instead.

Regarding the macOS side, adding a file to ios.xcodeproj won’t automatically add it to macos.xcodeproj. I realize it’s a bit inconvenient. #5942 was an attempt at merging the two projects so that you always get presented with all three targets when adding a file. Unfortunately it slowed down the index too much for comfort.

Yeah that's fine. I just forgot

This adds a features initializer to MGLGeoJSONSource. The initializer takes shapes and converts them to JSON representation before passing to core. This also adds methods to the MGLShape concrete subclasses so that they can represent themselves in NSDictionary form suitable for transforming to JSON (GeoJSON).

boundsj · 2016-10-14T15:11:28Z

Noting that although the Qt and Node builds show that they were unsuccessful they actually passed if you click through to bitrise.

1ec5

Whew!

MGLShape subclasses can now return NSDictionaries that represent the shapes' GeoJSON geometries. MGLFeature classes can now return NSDictionaries that represent the features as GeoJSON features. This makes it trivial to serialize iOS and macOS shapes and features into GeoJSON. MGLFeature instances can also return a representation of themselves as an mbgl::Feature. This capability is used in a refactoring of the implementations of MGLGeoJSONSource to more efficiently transform MGLFeatures into mbgl::Features so they can be fed directly into mbgl::GeoJSONSource without having to round trip through NSJSONSerialization. The MGLFeature identifier is converted into the mbgl::identifier type based on the type of the identifier. The MGLFeature attributes are recursively converted into an mbgl::PropertyMap and each value is converted into one of the types that the PropertyMap's Value variant supports.

boundsj · 2016-10-14T21:59:39Z

ituaijagbone added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold runtime styling labels Sep 29, 2016

1ec5 reviewed Sep 30, 2016

View reviewed changes

1ec5 suggested changes Sep 30, 2016

View reviewed changes

1ec5 mentioned this pull request Sep 30, 2016

Annotation classes don’t conform to NSCoding #6200

Closed

boundsj suggested changes Sep 30, 2016

View reviewed changes

1ec5 mentioned this pull request Oct 5, 2016

Revamp MGLStyleLayer, MGLSource inheritance #6588

Merged

boundsj changed the title ~~[WIP] added GeoJSON Serialization~~ [WIP] Add features initializer to MGLGeoJSONSource Oct 6, 2016

boundsj force-pushed the ituaijagbone-geojson-with-features branch 2 times, most recently from e073b10 to 113c1dd Compare October 7, 2016 19:38

boundsj mentioned this pull request Oct 11, 2016

MGLMultiPoint is both an abstract superclass and a concrete class for multipoints #5201

Closed

boundsj approved these changes Oct 11, 2016

View reviewed changes

boundsj force-pushed the ituaijagbone-geojson-with-features branch from 48ff580 to 44f88ab Compare October 11, 2016 23:25

boundsj added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Oct 11, 2016

1ec5 suggested changes Oct 12, 2016

View reviewed changes

boundsj changed the title ~~[WIP] Add features initializer to MGLGeoJSONSource~~ Add features initializer to MGLGeoJSONSource Oct 12, 2016

1ec5 suggested changes Oct 13, 2016

View reviewed changes

boundsj force-pushed the ituaijagbone-geojson-with-features branch from 3a6be23 to 206af31 Compare October 13, 2016 16:17

boundsj mentioned this pull request Oct 13, 2016

Make features and annotations conform to NSSecureCoding #6559

Merged

20 tasks

1ec5 suggested changes Oct 13, 2016

View reviewed changes

1ec5 suggested changes Oct 14, 2016

View reviewed changes

boundsj force-pushed the ituaijagbone-geojson-with-features branch from c4ea541 to ac22db1 Compare October 14, 2016 14:29

1ec5 approved these changes Oct 14, 2016

View reviewed changes

boundsj removed the ✓ ready for review label Oct 14, 2016

boundsj force-pushed the ituaijagbone-geojson-with-features branch from ac22db1 to 6ffe51b Compare October 14, 2016 21:10

boundsj merged commit 0a8858f into master Oct 14, 2016

boundsj deleted the ituaijagbone-geojson-with-features branch October 14, 2016 21:59

boundsj mentioned this pull request Oct 18, 2016

[ios, macos] Use swift_name for MGLMultiPoint and subclasses #6727

Merged

1ec5 mentioned this pull request Oct 18, 2016

Add and modify individual features in a GeoJSON source on the fly #6177

Open


		NSMutableArray *coordinates = [NSMutableArray array];

		for (int index = 0; index < self.pointCount; index++) {

		- (instancetype)initWithSourceIdentifier:(NSString )sourceIdentifier URL:(NSURL )url options:(NS_DICTIONARY_OF(NSString , id) )options NS_DESIGNATED_INITIALIZER;


		- (instancetype)initWithSourceIdentifier:(NSString )sourceIdentifier Features:(NS_ARRAY_OF(id<MGLFeature>) )features options:(nullable NS_DICTIONARY_OF(NSString , id) )options NS_DESIGNATED_INITIALIZER;



		- (void)testMGLGeoJSONSourceWithPolygonFeatures {
		// TODO: Create feature

Conversation

ituaijagbone commented Sep 29, 2016

Uh oh!

mention-bot commented Sep 29, 2016

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boundsj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boundsj Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boundsj Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boundsj Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boundsj commented Oct 6, 2016

Uh oh!

1ec5 commented Oct 7, 2016

Uh oh!

boundsj commented Oct 11, 2016

Uh oh!

boundsj commented Oct 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

boundsj Sep 30, 2016 •

edited

Loading

boundsj Sep 30, 2016 •

edited

Loading

boundsj Sep 30, 2016 •

edited

Loading