[ios] Warn about using MGLFeature-conforming annotations #9540
Conversation
friedbunny
added
annotations
There was a problem hiding this comment.
There’s some mention of features-as-annotations in the “Working with GeoJSON data” guide as well.
We should definitely warn in the feature querying documentation that those methods may return annotations as features – the other way around – but those annotations-as-features aren’t object-equal to the ones that were added to the map. So we should recommend against using feature querying to get annotations.
platform/ios/src/MGLMapView.mm
Outdated
| @@ -3187,6 +3187,11 @@ - (void)addAnnotations:(NS_ARRAY_OF(id <MGLAnnotation>) *)annotations | |||
| { | |||
| NSAssert([annotation conformsToProtocol:@protocol(MGLAnnotation)], @"annotation should conform to MGLAnnotation"); | |||
|
|
|||
| if ([annotation conformsToProtocol:@protocol(MGLFeature)]) | |||
| { | |||
| NSLog(@"Warning: Annotations that conform to the MGLFeature protocol do not keep their `identifier` or `attributes` properties when added to the map as annotations. Add this feature using runtime styling if you intend to later query the map for it. %@", annotation.description); | |||
There was a problem hiding this comment.
Let’s make this a one-time warning. It could get very spammy if you try to pass in a full array of features.
platform/darwin/src/MGLFeature.h
Outdated
| annotations using `-[MGLMapView addAnnotations:]` and related methods, doing so | ||
| will convert feature objects into plain annotations. Features added as | ||
| annotations will not have `identifier` or `attributes` properties set and | ||
| should not be used be with feature querying. |
There was a problem hiding this comment.
I think a more charitable way to put it is that when you add a feature to the map as an annotation, you give up some feature querying fidelity in exchange for built-in interactivity. It becomes possible to react to taps using -[MGLMapViewDelegate mapView:didSelectAnnotation:], so that in many cases it isn’t necessary to use feature querying at all. (The “annotation” passed into that delegate method does retain the attributes and ID.)
There was a problem hiding this comment.
Gave this a go in 5fabed1:
friedbunny
force-pushed
the
fb-features-as-annotations-trap
branch
from
64258db
to
032157a
Compare
platform/darwin/src/MGLFeature.h
Outdated
|
|
||
| - Features added as annotations are converted into plain annotations. These | ||
| annotations will not have `identifier` or `attributes` properties and should | ||
| not be used be with feature querying. |
There was a problem hiding this comment.
The features do continue to have identifier and attributes properties, for example when they’re passed into -[MGLMapViewDelegate mapView:didSelectAnnotation:]. It’s only feature querying that drops this information, as the feature querying API returns a lossy reconstruction of the original data.
platform/ios/src/MGLMapView.mm
Outdated
| { | ||
| static dispatch_once_t onceToken; | ||
| dispatch_once(&onceToken, ^{ | ||
| NSLog(@"Annotations that conform to the MGLFeature protocol do not keep their `identifier` or `attributes` properties when added to the map as annotations. Add this feature using runtime styling if you intend to later query the map for it. This warning is only shown once. %@", annotation.description); |
There was a problem hiding this comment.
I wonder if we could move this warning to when the developer attempts to query the map. Inside -visibleFeaturesAt:inStyleLayersWithIdentifiers:, warn if any of the values in _annotationContextsByAnnotationTag or keys in _annotationTagsByAnnotation conforms to MGLFeature. Or to make it more performant, set a flag here that enables the check later in -visibleFeaturesAt:inStyleLayersWithIdentifiers:.
There was a problem hiding this comment.
Coming back to this, I think this doesn’t need a warning — let’s leave it up to the developer to read the docs.
friedbunny
force-pushed
the
fb-features-as-annotations-trap
branch
from
646e553
to
19e9315
Compare
friedbunny
force-pushed
the
fb-features-as-annotations-trap
branch
from
19e9315
to
b5b84ef
Compare
|
@1ec5 I’ve slimmed down the scope of this PR to remove the console warning and to focus specifically on the limitations of using features-as-annotations with feature querying. |
Until #6178 is addressed, this adds two warnings about using
MGLFeature-conforming objects as annotations:Prints a warning to console when a developer does so.MGLFeature’s documentation to make developers aware of the limitations around using features-as-annotations with feature querying.This also adds friendlier debug descriptions for various feature/shape types.
/cc @1ec5 @captainbarbosa @boundsj @fabian-guerra