[ios] Guard against looking up annotation contexts MGLAnnotationTagNotFound #8686
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1452,7 +1452,9 @@ - (void)handleSingleTapGesture:(UITapGestureRecognizer *)singleTap | |
| } | ||
| else | ||
| { | ||
| if (_selectedAnnotationTag != MGLAnnotationTagNotFound) { | ||
| nextElement = _annotationContextsByAnnotationTag.at(_selectedAnnotationTag).accessibilityElement; | ||
| } | ||
| } | ||
| [self deselectAnnotation:self.selectedAnnotation animated:YES]; | ||
| UIAccessibilityPostNotification(UIAccessibilityScreenChangedNotification, nextElement); | ||
|
|
@@ -2002,19 +2004,21 @@ - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(N | |
| { | ||
| const mbgl::Point<double> point = MGLPointFromLocationCoordinate2D(annotation.coordinate); | ||
|
|
||
| if (annotationTag != MGLAnnotationTagNotFound) { | ||
| MGLAnnotationContext &annotationContext = _annotationContextsByAnnotationTag.at(annotationTag); | ||
| if (annotationContext.annotationView) | ||
| { | ||
| // Redundantly move the associated annotation view outside the scope of the animation-less transaction block in -updateAnnotationViews. | ||
| annotationContext.annotationView.center = [self convertCoordinate:annotationContext.annotation.coordinate toPointToView:self]; | ||
| } | ||
|
|
||
| MGLAnnotationImage *annotationImage = [self imageOfAnnotationWithTag:annotationTag]; | ||
| NSString *symbolName = annotationImage.styleIconIdentifier; | ||
|
|
||
| // Update the annotation’s backing geometry to match the annotation model object. Any associated annotation view is also moved by side effect. However, -updateAnnotationViews disables the view’s animation actions, because it can’t distinguish between moves due to the viewport changing and moves due to the annotation’s coordinate changing. | ||
| _mbglMap->updateAnnotation(annotationTag, mbgl::SymbolAnnotation { point, symbolName.UTF8String }); | ||
| [self updateCalloutView]; | ||
|
There was a problem hiding this comment. This change is correct with respect to the user location annotation, because we rely on Core Location, not KVO, to tell us when the model object has changed. |
||
| } | ||
| } | ||
| } | ||
| else if ([keyPath isEqualToString:@"coordinates"] && [object isKindOfClass:[MGLMultiPoint class]]) | ||
|
|
@@ -3055,17 +3059,18 @@ - (void)removeStyleClass:(NSString *)styleClass | |
|
|
||
| for (auto const& annotationTag: annotationTags) | ||
| { | ||
| if (!_annotationContextsByAnnotationTag.count(annotationTag) || | ||
| annotationTag == MGLAnnotationTagNotFound) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| MGLAnnotationContext annotationContext = _annotationContextsByAnnotationTag.at(annotationTag); | ||
| NSAssert(annotationContext.annotation, @"Missing annotation for tag %u.", annotationTag); | ||
| if (annotationContext.annotation) | ||
| { | ||
| [annotations addObject:annotationContext.annotation]; | ||
| } | ||
| } | ||
|
|
||
| return [annotations copy]; | ||
|
|
@@ -3077,8 +3082,8 @@ - (void)removeStyleClass:(NSString *)styleClass | |
| /// Returns the annotation assigned the given tag. Cheap. | ||
| - (id <MGLAnnotation>)annotationWithTag:(MGLAnnotationTag)tag | ||
| { | ||
| if ( ! _annotationContextsByAnnotationTag.count(tag) || | ||
| tag == MGLAnnotationTagNotFound) { | ||
|
There was a problem hiding this comment. Should this return There was a problem hiding this comment. Never mind. The code used to overload MGLAnnotationTagNotFound to mean the user location annotation tag. But in retrospect that's a pretty risky practice. This change moves us further away from that practice, forcing us to explicitly account for the user location annotation explicitly, which is a good thing. |
||
| return nil; | ||
| } | ||
|
|
||
|
|
@@ -3550,6 +3555,10 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL) | |
| auto end = std::remove_if(nearbyAnnotations.begin(), nearbyAnnotations.end(), | ||
| [&](const MGLAnnotationTag annotationTag) | ||
| { | ||
| if (annotationTag == MGLAnnotationTagNotFound) { | ||
|
There was a problem hiding this comment. This is redundant to the check in -annotationWithTag:. Should we put an assertion in right above for this case? Is it legitimate for the user location annotation to be a nearby annotation? There was a problem hiding this comment. Going to answer my own question here: There was a problem hiding this comment. That makes sense. I'll remove this check to rely on the assertion below. There was a problem hiding this comment. Also, this is the assertion we hit in debug builds if annotations are removed at almost the same time and just barely before the user tries to select them (part of the repro from #8163 (comment)). The check below recently added in #8637 to make sure the annotation is not nil protects release builds from crashing at the call to |
||
| return true; | ||
| } | ||
|
|
||
| id <MGLAnnotation> annotation = [self annotationWithTag:annotationTag]; | ||
| NSAssert(annotation, @"Unknown annotation found nearby tap"); | ||
| if ( ! annotation) | ||
|
|
@@ -3680,10 +3689,12 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL) | |
| { | ||
| return self.userLocation; | ||
| } | ||
|
|
||
| if ( ! _annotationContextsByAnnotationTag.count(_selectedAnnotationTag) || | ||
| _selectedAnnotationTag == MGLAnnotationTagNotFound) { | ||
| return nil; | ||
| } | ||
|
|
||
| MGLAnnotationContext &annotationContext = _annotationContextsByAnnotationTag.at(_selectedAnnotationTag); | ||
| return annotationContext.annotation; | ||
| } | ||
|
|
@@ -3749,19 +3760,16 @@ - (void)selectAnnotation:(id <MGLAnnotation>)annotation animated:(BOOL)animated | |
| MGLAnnotationView *annotationView = nil; | ||
|
|
||
| if (annotation != self.userLocation) | ||
| if (annotationTag != MGLAnnotationTagNotFound) { | ||
| MGLAnnotationContext &annotationContext = _annotationContextsByAnnotationTag.at(annotationTag); | ||
| annotationView = annotationContext.annotationView; | ||
| if (annotationView && annotationView.enabled) { | ||
| { | ||
| // Annotations represented by views use the view frame as the positioning rect. | ||
| positioningRect = annotationView.frame; | ||
| [annotationView.superview bringSubviewToFront:annotationView]; | ||
| [annotationView setSelected:YES animated:animated]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -5039,8 +5047,12 @@ - (void)updateCalloutView | |
| if (isAnchoredToAnnotation) | ||
| { | ||
| MGLAnnotationTag tag = [self annotationTagForAnnotation:annotation]; | ||
| MGLAnnotationView *annotationView = nil; | ||
|
|
||
| if (tag != MGLAnnotationTagNotFound) { | ||
| MGLAnnotationContext &annotationContext = _annotationContextsByAnnotationTag.at(tag); | ||
| annotationView = annotationContext.annotationView; | ||
| } | ||
|
|
||
| CGRect rect = [self positioningRectForCalloutForAnnotationWithTag:tag]; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we tend to call it the “user location annotation”, to make clear that it’s not some annotation otherwise belonging to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made that change for the iOS changelog in 99eee0a. In that same commit, I also made the macOS changelog less specific since the possibility of a crash there is more theoretical since there is currently no user location annotation.