rename MGLGeoJSONSource to MGLFeatureSource

[incanus]

This is a proposal after experimentation and discussion with @1ec5.

A developer doesn't necessarily need to touch or view GeoJSON when using this type of source. They can create MGLFeature instances, instantiate a source with them, and mutate them later.

let feature = MGLPointFeature()
feature.coordinate = CLLocationCoordinate2D(latitude: 45, longitude: -122)
let maybeSource = MGLGeoJSONSource(identifier: "point", features: [feature], options: nil)
if let source = maybeSource, let parks = style.layer(withIdentifier: "national_park") {
    style.add(source)
    let layer = MGLCircleStyleLayer(identifier: "layer", source: source)
    layer.circleColor = MGLStyleValue(rawValue: UIColor.red)
    layer.circleRadius = MGLStyleValue(rawValue: 10)
    layer.circleOpacity = MGLStyleValue(rawValue: 0.25)
    style.insert(layer, below: parks)
    map.setCenter(feature.coordinate, zoomLevel: 10, animated: false)
    let maybeTimer = Timer(timeInterval: 1, repeats: true) { (timer) in
        feature.coordinate = CLLocationCoordinate2D(latitude: feature.coordinate.latitude + 0.01, longitude: feature.coordinate.longitude + 0.01)
        source.features = [feature]
    }
    if let timer = maybeTimer {
        RunLoop.main.add(timer, forMode: .commonModes)
    }
}

This would make it clearer that you can do such a thing, it would make #6940 easier or irrelevant, and it will tie in to #6177.

/cc @1ec5 @boundsj @jfirebaugh @ericrwolfe

[1ec5]

This issue is currently on the ios-future milestone, but we really should rename the class as part of the v3.4.0 milestone if we do it at all, in order to avoid having to perform a deprecation song and dance in the future.

Putting on the v3.4.0 milestone to make a call about whether to do this or not.

[1ec5]

This would have some implications for #6940. In that PR, MGLGeoJSONSource and MGLCustomVectorSource both inherit from a new MGLGeoJSONSourceBase class, which is a poor name for a class. I’ve made a couple proposals in #6940 (comment), one of which has MGLGeoJSONSource remain the same and the other of which has MGLGeoJSONSource become MGLCollectionSource, per #7145.

[1ec5]

#7145 adds a wrinkle to the “MGLFeatureSource” idea, since the source can be created from any MGLShape (in other words, any GeoJSON geometry). So, MGLShapeSource?

[lucaswoj]

This change sounds good to me. We may end up with something conceptually similar in GL JS as we work on the custom source API https://github.com/mapbox/mapbox-gl-js/projects/2

[incanus]

MGLShapeSource

Yeah, but it can just be points also.

[1ec5]

MGLShapeSource

Yeah, but it can just be points also.

Points are represented in the iOS and macOS SDKs as instances of MGLPointAnnotation, which is a subclass of MGLShape.

[1ec5]

Aside: You may be wondering why on Earth we’d use a class named “annotation” for runtime styling. All the original geometry classes were used for annotations/overlays just like in MapKit. They were named after MapKit’s classes, too, so MGLPointAnnotation was named after MKPointAnnotation. (MKPointAnnotation got its awkward name to distinguish it from MKMapPoint, which is akin to mbgl’s TileCoordinatePoint type, and CGPoint/NSPoint, which represents a screen distance measured in points.)

When we needed geometry classes to represent feature querying results, we reused the annotation geometry classes and added a few for the more exotic Simple Features types. In hindsight, we should’ve named the class MGLPointShape, but parity and consistency with MapKit was an early goal for our annotation API. See #5201 (comment) for more examples of the mess we’re in.

[incanus]

Points are represented in the iOS and macOS SDKs as instances of MGLPointAnnotation, which is a subclass of MGLShape.

Sure, but this is an implementation detail. When I see "shape" (two dimensions) I don't think "point" (one dimension) and I think this will be confusing to developers.

[1ec5]

Unfortunately, we inherited MapKit’s habit of using “shape” to mean “geometry”. Otherwise I’d be all for calling it MGLGeometrySource.

[frederoni]

I'd prefer to follow the GeoJSON standard as strictly as possible to avoid confusion. It makes it easier to reason about both from SDK and application level perspective. Something along these lines:

3.4	3.5
MGLGeoJSONSource	MGLGeoJSONSource
---	MGLGeoJSONObject
MGLShape	MGLGeoJSONGeometry (subclass of GeoJSONObject)

the rest is basically the same except that all GeoJSON features and geometry classes would be a subclass of MGLGeoJSONObject.

[1ec5]

I think it’s important to distinguish between GeoJSON (a data format) and the geometry models defined in the Simple Features standard. From a developer standpoint, what does a point object representing a view-backed annotation – or a polygon feature resulting from a vector tile feature query – have to do with GeoJSON, or JSON for that matter? Developers work with NSDictionary, not NSJSONDictionary or NSPlistDictionary.

In any case, renaming MGLShape (and MGLShapeCollection) will require a major version bump to v4.0.0. It’s certainly within the realm of possibility that we’ll rationalize these geometry classes at some point, because we aren’t as concerned about consistency with MapKit as we once were. But that’s outside of the scope of this ticket, which is about deciding on a name for MGLGeoJSONSource in v3.4.0 that’s flexible enough to accommodate custom sources in v3.5.0. If we decide MGLGeoJSONSource is fine, then we can close this ticket and revisit the name when we consider the MGLGeoJSONObject proposal for v4.0.0.

[boundsj]

Now that #7145 has landed, it'd be a good time to do this rename. I agree that MGLShapeSource makes the most sense (especially after #7145 since you can now create one of these with a shape and the shape property is how you view the content (unless you've created it with a URL, but that is another story)).

I'm going to assign to me and make the change by tomorrow unless there are any last final concerns @1ec5 @frederoni @friedbunny @jmkiley @incanus @ericrwolfe

[1ec5]

My only concern is what happens in #6940, which would introduce a programmatically generated source that shares an abstract superclass with the source in question. Let’s go with MGLShapeSource for now and figure out a clever solution for that problem when we get to it.

[boundsj]

This was done in #7334