Add MGLPointCollection for GeoJSON multipoints #6742

boundsj · 2016-10-18T23:56:24Z

Stop overloading MGLMultiPoint for GeoJSON multipoints, instead creating a new class for this purpose, as suggested in #5201 (comment). This would be backwards-compatible and allow us to remove various isMemberOfClass checks.

mention-bot · 2016-10-18T23:56:28Z

@boundsj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ituaijagbone, @1ec5 and @rmnblm to be potential reviewers.

1ec5

Also update the iOS and macOS changelogs to note that we fixed a bug in which feature querying returned abstract MGLMultiPointFeature objects for multipoint features; now it returns concrete MGLPointCollectionFeature objects instead.

1ec5 · 2016-10-19T01:58:24Z

platform/darwin/src/MGLFeature.h

@@ -129,7 +130,7 @@ NS_ASSUME_NONNULL_BEGIN
 The `MGLMultiPointFeature` class represents a multipoint in a


Update this comment.

1ec5 · 2016-10-19T01:59:21Z

platform/darwin/src/MGLMultiPoint.h

@@ -14,17 +14,6 @@ NS_ASSUME_NONNULL_BEGIN
 @interface MGLMultiPoint : MGLShape


Revert this class’s comment to what it used to say.

Done in 95d8c15

1ec5 · 2016-10-19T02:00:35Z

platform/darwin/src/MGLPointCollection.h

+
+#import "MGLShape.h"
+
+@interface MGLPointCollection : MGLShape


Document this class.

1ec5 · 2016-10-19T02:01:45Z

platform/darwin/src/MGLPointCollection.h

+
+#import "MGLShape.h"
+
+@interface MGLPointCollection : MGLShape


Should this class conform to MGLOverlay? (Intriguingly, we could have this class be a subclass of MGLMultiPoint, in order to share mutable coordinate code. But best to leave this as is for now.)

1ec5 · 2016-10-19T02:01:56Z

platform/darwin/src/MGLPointCollection.h

+
+/** The number of coordinates associated with the shape. */
+@property (nonatomic, readonly) NSUInteger pointCount;
+


Add a getter for the coordinates.

Done in 2ad5889

1ec5 · 2016-10-19T02:02:13Z

platform/darwin/src/MGLPointCollection.h

+ @return A new multipoint object.
+ */
+ (instancetype)pointCollectionWithCoordinates:(CLLocationCoordinate2D *)coords count:(NSUInteger)count;
+


Add a public initializer to go with the factory method.

We don't have public initializers for any of the other shapes. Why add one in this case? Especially since MGLPointCollection currently can't be added to the map as an since there is no representation for that sort of annotation in mbgl core.

Interesting, OK, whatever's consistent.

1ec5 · 2016-10-19T02:03:05Z

platform/ios/ios.xcworkspace/xcshareddata/xcdebugger/Breakpoints_v2.xcbkptlist

@@ -6,7 +6,7 @@
      <BreakpointProxy
         BreakpointExtensionID = "Xcode.Breakpoint.ExceptionBreakpoint">
         <BreakpointContent
-            shouldBeEnabled = "No"
+            shouldBeEnabled = "Yes"


Back out this change.

1ec5 · 2016-10-19T02:03:12Z

platform/ios/jazzy.yml

@@ -30,10 +30,11 @@ custom_categories:
      - MGLAnnotationView
      - MGLCalloutView
      - MGLCalloutViewDelegate
-      - MGLMultiPoint
+      - MGLMultiPoint      


Nit: trailing whitespace.

1ec5 · 2016-10-19T02:06:27Z

platform/ios/src/MGLMapView.mm

@@ -2855,13 +2855,6 @@ - (void)addAnnotations:(NS_ARRAY_OF(id <MGLAnnotation>) *)annotations

        if ([annotation isKindOfClass:[MGLMultiPoint class]])
        {
-            // Actual multipoints aren’t supported as annotations.


Make the same change on the macOS side.

Done in 5813f4f

boundsj · 2016-10-19T19:44:21Z

Fixes #6764

boundsj · 2016-10-19T19:45:11Z

@1ec5 @friedbunny I still need to add the changelog entries but this is ready for review (thanks @1ec5 for the early review!)

boundsj · 2016-10-19T19:46:40Z

platform/darwin/src/MGLPointCollection.h

+
+/**
+ The `MGLPointCollection` class is used to define shapes composed of multiple 
+ points.


@1ec5 I'm thinking about mentioning that this class is really only useful as an MGLPointCollectionFeature in an MGLGeoJSONSource. What do you think?

This moves the method that creates a "multi point" object in the GeoJSON sense to MGLPointCollection. The methods for creating mbgl::Feature and GeoJSON dictionary representations are also moved.

- Add WIP description for MGLPointCollection class - Change the name of MGLPointCollectionFeature description - Update MGLMultiPoint class description - Add NSObject description method to MGLPointCollection - Update jazzy yml rename of multipoint feature to point collection feature

We don't want MGLMultiPoints to be used directly. Again.

This matches the multipoint implementation

This updates the iOS and macOS annotation adding logic so that unwanted shapes really are avoided. Previously the combined OR conditions meant that an annotation had to logically be NOT a kind of all three types so the check always let the annotation slip through. This also expands the guard to deflect the recently introduced MGLPointCollection. This removes the check in the macOS SDK for multipoints as annotations for the polygon and polyline case so this logic now matches iOS. Finally, this fixes up the macOS project so that MGLPointCollection is linked correctly.

1ec5

A few minor things, but this is looking great.

1ec5 · 2016-10-20T00:08:53Z

platform/darwin/src/MGLPointCollection.h

+#import "MGLShape.h"
+
+/**
+ The `MGLPointCollection` class is used to define an array of coordinates.


Emphasize that these coordinates are disconnected (as opposed to MGLMultiPoint).

1ec5 · 2016-10-20T00:09:24Z

platform/darwin/src/MGLPointCollection.h

+ @param coords The array of coordinates defining the shape. The data in this
+ array is copied to the new object.
+ @param count The number of items in the `coords` array.
+ @return A new multipoint object.


s/multipoint/point collection/

1ec5 · 2016-10-20T00:11:19Z

platform/ios/src/MGLMapView.h

+ @note `MGLMultiPolyline`, `MGLMultiPolygon`, `MGLShapeCollection`, and 
+    `MGLPointCollection` objects cannot be added to the map view at this time. 
+    Any multipoint, multipolyline, multipolygon, shape collection or point 
+    collection, object that is specified is silently ignored.


Nit: remove comma after "collection".

boundsj · 2016-10-20T23:36:19Z

Thanks @1ec5! I think this is ready to go.

boundsj 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 runtime styling labels Oct 18, 2016

boundsj added this to the ios-v3.4.0 milestone Oct 18, 2016

boundsj self-assigned this Oct 18, 2016

1ec5 suggested changes Oct 19, 2016

View reviewed changes

1ec5 reviewed Oct 19, 2016

View reviewed changes

boundsj mentioned this pull request Oct 19, 2016

Map view does not actually guard against adding multipolyline(gon) and shape collection annotations #6764

Closed

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

boundsj commented Oct 19, 2016

View reviewed changes

boundsj added 15 commits October 19, 2016 15:46

[ios, macos] Introduce MGLPointCollection

6b6b793

[ios, macos] Move class factory method to point collection class

9830d67

This moves the method that creates a "multi point" object in the GeoJSON sense to MGLPointCollection. The methods for creating mbgl::Feature and GeoJSON dictionary representations are also moved.

[ios, macos] Replace multi-point feature with point collection feature

bc856b9

[ios, macos] Remove swift_name for polyline and polygon

20756f5

[ios] Add style example using MGLPointCollectionFeature

1566a57

[ios, macos] Revert MGLMultiPoint class comments

23e4bbc

We don't want MGLMultiPoints to be used directly. Again.

[ios, macos] Use internal count in point collection

ba3ce84

This matches the multipoint implementation

[ios, macos] Make point collection conform to MGLOverlay

4896a54

[ios, macos] Add a getter for coordinates

fb0d22f

[ios] Revert change to exception breakpoint

a97ca60

[ios] Remove whitespace

6db8563

[ios, macos] Make point collection conform to MGLAnnotation

53a4c8c

[ios, macos] Clarify API docs for point collection

c6288a7

boundsj force-pushed the boundsj-stop-overloading-multipoint branch from a777c16 to c6288a7 Compare October 19, 2016 22:56

[ios, macos] Update changelog

25ae34b

1ec5 suggested changes Oct 20, 2016

View reviewed changes

[ios, macos] Documentation fixes

a0a78c0

1ec5 approved these changes Oct 21, 2016

View reviewed changes

boundsj merged commit 1216050 into release-ios-v3.4.0 Oct 21, 2016

boundsj deleted the boundsj-stop-overloading-multipoint branch October 21, 2016 00:14

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

1ec5 mentioned this pull request Oct 21, 2016

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

Closed

boundsj mentioned this pull request Nov 22, 2016

Add a public, designated initializers to go with MGLPolygon(line) factory methods #7151

Closed

1ec5 mentioned this pull request Dec 18, 2016

MGLMultiPointFeature should be aliased to MGLPointCollectionFeature #7473

Closed

1ec5 mentioned this pull request Jan 27, 2017

Flatten point collections, not multipoints #7881

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MGLPointCollection for GeoJSON multipoints #6742

Add MGLPointCollection for GeoJSON multipoints #6742

boundsj commented Oct 18, 2016

mention-bot commented Oct 18, 2016

1ec5 left a comment

1ec5 Oct 19, 2016

1ec5 Oct 19, 2016

boundsj Oct 19, 2016

1ec5 Oct 19, 2016

1ec5 Oct 19, 2016

1ec5 Oct 19, 2016

boundsj Oct 19, 2016

1ec5 Oct 19, 2016

boundsj Oct 19, 2016

1ec5 Oct 20, 2016

1ec5 Oct 20, 2016

1ec5 Oct 19, 2016

1ec5 Oct 19, 2016

1ec5 Oct 19, 2016

boundsj Oct 19, 2016

boundsj commented Oct 19, 2016

boundsj commented Oct 19, 2016

boundsj Oct 19, 2016

1ec5 left a comment

1ec5 Oct 20, 2016

1ec5 Oct 20, 2016

1ec5 Oct 20, 2016

boundsj commented Oct 20, 2016

		@@ -129,7 +130,7 @@ NS_ASSUME_NONNULL_BEGIN
		The `MGLMultiPointFeature` class represents a multipoint in a

		@@ -14,17 +14,6 @@ NS_ASSUME_NONNULL_BEGIN
		@interface MGLMultiPoint : MGLShape


		#import "MGLShape.h"

		@interface MGLPointCollection : MGLShape


		/** The number of coordinates associated with the shape. */
		@property (nonatomic, readonly) NSUInteger pointCount;

Add MGLPointCollection for GeoJSON multipoints #6742

Add MGLPointCollection for GeoJSON multipoints #6742

Conversation

boundsj commented Oct 18, 2016

mention-bot commented Oct 18, 2016

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

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

boundsj commented Oct 19, 2016

boundsj commented Oct 19, 2016

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

boundsj commented Oct 20, 2016