[ios, macos] Warn if MGLShapeSource is initialized with MGLShapeCollection #12625
Conversation
captainbarbosa
added
the
⚠️ DO NOT MERGE
captainbarbosa
changed the title
captainbarbosa
added
iOS
|
I've gotten this warning to show up when initializing a new |
| @"This will be logged only once."); | ||
| }); | ||
| } | ||
| break; |
There was a problem hiding this comment.
This code breaks out of the loop, unconditionally, after considering the first shape in the array. I think the intention is to consider each of the shapes, breaking if one of them is a feature.
| @@ -13,6 +14,18 @@ + (instancetype)shapeCollectionWithShapes:(NSArray<MGLShape *> *)shapes { | |||
| - (instancetype)initWithShapes:(NSArray<MGLShape *> *)shapes { | |||
| if (self = [super init]) { | |||
| _shapes = shapes.copy; | |||
|
|
|||
| for (MGLShape* shape in shapes ){ | |||
| if ([shape conformsToProtocol:@protocol(MGLFeature)]) { | |||
There was a problem hiding this comment.
Putting the feature check here does address a common mistake, but #11287 was specifically about warning when creating an MGLShapeSource with an MGLShapeCollection or setting MGLShapeSource.shape to an MGLShapeCollection. That would occur later than this code; the only difference is that there may theoretically be a legitimate reason to stuff MGLFeatures in an MGLShapeCollection, but there’s no reason to stick an MGLShapeCollection inside an MGLShapeSource or MGLComputedShapeSource. On the other hand, it’s always nicer to warn as soon as we see a potential problem unfolding. Your call as to the proper tradeoff.
MGLShapeCollectionFeature inherits from MGLShapeCollection. mapbox-gl-native/platform/darwin/src/MGLFeature.mm Lines 236 to 238 in c96d9df Per #12625 (comment), we could instead have MGLShapeSource and MGLComputedShapeSource check whether they’re getting MGLShapeCollections. That would be an O(1) operation rather than an O(n) operation, and we can be sure the check would only snag improper use of MGLShapeCollection where MGLShapeCollectionFeature was intended. |
captainbarbosa
changed the title
captainbarbosa
force-pushed
the
nb-warn-attribute-loss-11287
branch
from
270f9da to
a7f7b09
Compare
Move warning from ShapeCollection to ShapeSource Try checking MGLComputedShapeSources Include MGLShapeCollection header Add changelog entry
captainbarbosa
force-pushed
the
nb-warn-attribute-loss-11287
branch
from
3ca52d4 to
8ad51b0
Compare
captainbarbosa
removed
the
⚠️ DO NOT MERGE
|
I haven't tested and may be wrong, but since these all have a scoped dispatch_once_t, it seems to me one could trigger this three separate times if they vary their implementations. Not that it's a problem, I'd rather get it a few times than none, but thought I'd point it out. |
Closes #11287 by throwing a single warning if any
MGLShapeCollectioncontains a shape conforming toMGLFeatureprotocol, as attributes are not copied over intoMGLShapeCollections.