MGLMultiPolygon doesn't support coordinate property

[aayers-agrible]

Platform:
iOS
Mapbox SDK version:
3.3.6

Steps to trigger behavior

1. Create a MGLMultiPolygon object (I parse them from a geojson source)
2. Request the MGLMultiPolygon object's 'coordinate' property
3. Get "MGLShape is an abstract class error"

Expected behavior

The centroid of the MultiPoly would be returned from the coordinate property

Actual behavior

I get an 'MGLShape is an abstract class' error.

[1ec5]

There are two issues here:

• -[MGLMultiPolygon coordinate] should be implemented in order to avoid the exception in MGLShape. For now, we can have it return the first coordinate, as MGLPolyline and MGLPolygon do.
• -[MGLPolyline coordinate], -[MGLPolygon coordinate], and -[MGLMultiPolygon coordinate] should return the centroid. This is a larger task that would require Polylabel.

/cc @mourner

[mourner]

@1ec5 wasn't Polylabel already integrated recently by @jfirebaugh?

[1ec5]

Ah, I missed that it got integrated (for label placement) in #7465. We'll have to figure out a way to make it accessible to SDK code as well.

[jfirebaugh]

I would argue that the problem here is that -coordinate is even in the interface of MGLShape and subclasses. It should be in the interface of MGLPointAnnotation only. The artificial definition of a "coordinate" for polygons, multipolygons, line strings -- and the lengths we will need to go through to implement it -- are signs that this is a design error.

[1ec5]

coordinate is defined on the MGLAnnotation protocol, so any class that conforms to it must implement the property. This is consistent with MapKit.

[1ec5]

To expand on #7070 (comment), having coordinate return something meaningful for the more exotic shape types is just icing on the cake. The real bug here is that -[MGLMultiPolygon coordinate] is unimplemented, leading to a crash. Even having -[MGLMultiPolygon coordinate] return the first coordinate of the first polygon would’ve been fine, but #8713 goes the extra mile to return something more meaningful.

/cc @fabian-guerra

[jfirebaugh]

Right -- I'm suggesting that either:

• MGLShape should not conform to MGLAnnotation, or
• MGLAnnotation should not define coordinate.

If you find yourself implementing methods where the implementation is arbitrary, then something is off about your class design.

[1ec5]

If you find yourself implementing methods where the implementation is arbitrary, then something is off about your class design.

That’s a good argument for making the coordinate property optional in the MGLAnnotation protocol, so that it would only be provided by the classes where it makes sense.

I would contend that it should be possible to implement a non-arbitrary coordinate for every shape type – after all, we do that for labeling features in vector sources – but that it isn’t a priority for the more exotic shape types. The highest priority is fixing the crash, and I believe that’s why this issue is on the v3.6.0 milestone. A secondary priority would be to implement a sensible coordinate for MGLPolyline and MGLPolygon specifically, to simplify the task in #2082/#5502.

Changing either MGLShape’s protocol conformance or making MGLAnnotation.coordinate optional would be a major, backwards-incompatible change. We’d want to track that separately and maybe consider it at the same time as #7454.

[1ec5]

Fixed in #8713 on the release-ios-v3.6.0-android-v5.1.0 branch.