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

[jfirebaugh]

As noted in #5200 (comment), MGLPolyline and MGLPolygon should not inherit from MGLMultiPoint. Neither a polyline nor a polygon is-a multipoint.

[1ec5]

Is it important that the class that represents the Simple Features/GeoJSON multipoint be called “MGLMultiPoint”, as opposed to “MGLSparseMultiPoint”, “MGLDiscreteMultiPoint”, or “MGLPointCollection”? If so, we’d need to bump the major version number to acknowledge this breaking change. It would also be a deviation from MapKit’s class structure, but I think it’s an acceptable and reasonable one, considering that developers rarely interact with MGLMultiPoint by name.

/cc @boundsj @friedbunny

[1ec5]

We already don't have a strict correspondence between Simple Features names and class names, for instance MGLShapeCollection, so named for consistency with the MGLShape protocol and to avoid confusion with the MGLFeature protocol.

[incanus]

From #6565 (review), when #6565 is landed, we should clean up the abstract class checks mentioned there as part of this cleanup.

[1ec5]

Now that #6565 has landed, we no longer treat MGLMultiPoint as an abstract class. The next step is to either:

1. Stop overloading MGLMultiPoint for GeoJSON multipoints, instead creating a new class for this purpose, as suggested in MGLMultiPoint is both an abstract superclass and a concrete class for multipoints #5201 (comment). This would be backwards-compatible and allow us to remove various isMemberOfClass checks.
2. Make MGLPolyline and MGLPolygon inherit directly from MGLShape, as suggested in MGLMultiPoint is both an abstract superclass and a concrete class for multipoints #5201 (comment). Existing client code might use MGLMultiPoint as an umbrella class for both MGLPolyline and MGLPolygon, so this change would be backwards-incompatible. We’d have to wait until the next major version.

My preference is for (1), which would allow us to stop the bleeding until we get an opportunity to completely rework the geometry classes as the Android SDK has done in Mapbox Java Services. In the meantime, I don’t think it’d be a problem to represent GeoJSON multipoints using something named “MGLPointCollection” instead of “MGLMultiPoint”. After all, we already have:

• MGLPointAnnotation instead of MGLPoint (for compatibility with MKPointAnnotation)
• MGLPolyline instead of MGLLineString (for compatibility with MKPolyline)
• MGLShape instead of MGLGeometry (for compatibility with MKShape and to avoid confusion with MGLGeometry.h)
• MGLShapeCollection instead of MGLFeatureCollection

[boundsj]

Noting that in #6524 we've formally added a convenience initializer so that an MGLMultiPoint can be created with coordinates and count (like its siblings) and changed the documentation in the header to remove the language about it being abstract. These changes in addition to adding a private method to produce the mbgl representation of the shape allow us to use MGLMultiPoint to init an MGLGeoJSONSource.

[1ec5]

We discovered that #6727 would be a breaking change for Swift applications. That fix was only necessary because MGLMultiPoint is being overloaded as an abstract superclass and a concrete class (this bug). So we’re going to implement the stopgap approach detailed in #5201 (comment) for the v3.4.0 release to avoid breaking existing applications and sample code.

[1ec5]

#6742 implemented a separate class, MGLPointCollection, to represent what GeoJSON calls a multipoint. If the existence of an abstract class named MGLMultiPoint continues to cause confusion, we can consider removing it as a backwards-incompatible change in the future. However, for now, the purely abstract MGLMultiPoint is useful as a way for MGLPolyline and MGLPolygon to share a fairly substantial amount of code.