Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

MGLFeature should contain MGLShape, not inherit from it #7454

Open
1ec5 opened this issue Dec 15, 2016 · 3 comments
Open

MGLFeature should contain MGLShape, not inherit from it #7454

1ec5 opened this issue Dec 15, 2016 · 3 comments
Labels
annotations Annotations on iOS and macOS or markers on Android gl-ios iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor runtime styling SEMVER-MAJOR Requires a major release according to Semantic Versioning rules
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Dec 15, 2016

MGLFeature should be a class, not a protocol, and it should contain an MGLShape in its shape property. The various MGL*Feature classes that conform to the MGLFeature protocol should go away. MGLShapeCollectionFeature should be renamed to MGLFeatureCollection. MGLShape, MGLFeature, and MGLFeatureCollection should conform to a new MGLGeoJSONRepresentable protocol.

In #5110, I needed to add classes for all the possible feature types that could be returned from the feature querying API. I made a series of MGL*Feature classes: each class conforms to the MGLFeature protocol and inherits from an MGLShape class (which corresponds to a GeoJSON geometry type). For example, MGLPolygonFeature inherits from MGLPolygon. By contrast, in the GeoJSON specification, a feature contains a geometry; it isn’t a kind of geometry.

I don’t remember if the taxonomy I implemented was a misreading of the GeoJSON specification on my part or a deliberate shortcut, but it proved to be very convenient for working with feature querying results. You can take the feature querying results and turn right around and add those results to the map as annotations (since some MGLShape types are annotation types). Unfortunately, this taxonomy works less well with the runtime styling API: for #7453, what happens when an array of MGLShapes is really a mixture of shapes and features?

The proposed refactoring mirrors the GeoJSON specification exactly and removes the rough edges around the runtime styling API.

/ref #5201 #7160 (comment)
/cc @boundsj @frederoni @jfirebaugh

@1ec5 1ec5 added annotations Annotations on iOS and macOS or markers on Android iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor runtime styling SEMVER-MAJOR Requires a major release according to Semantic Versioning rules labels Dec 15, 2016
@1ec5 1ec5 added this to the ios-future milestone Dec 15, 2016
@1ec5
Copy link
Contributor Author

1ec5 commented Dec 18, 2016

Since +[MGLShape shapeWithData:encoding:error:] could return any instance of a class conforming to MGLGeoJSONRepresentable, not necessarily an MGLShape, we’ll need to pull out this method and -[MGLShape geoJSONDataUsingEncoding:] into a new class, MGLGeoJSONSerialization.

@fabian-guerra

This comment has been minimized.

@1ec5
Copy link
Contributor Author

1ec5 commented Jul 18, 2018

The diagram in #7454 (comment) isn’t quite accurate: only MGLPolyline and MGLPolygon inherit from MGLMultiPoint; the rest of the geometry classes inherit directly from MGLShape. #12419 proposes eliminating MGLMultiPoint to align MGLPolyline and MGLPolygon with the rest of the geometry classes.

/cc @captainbarbosa

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android gl-ios iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor runtime styling SEMVER-MAJOR Requires a major release according to Semantic Versioning rules
Projects
None yet
Development

No branches or pull requests

3 participants