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

[ios] Guard against looking up annotation contexts MGLAnnotationTagNotFound #8686

Merged
merged 3 commits into from
Apr 7, 2017
Merged

[ios] Guard against looking up annotation contexts MGLAnnotationTagNotFound #8686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c2a7bc
[ios, macos] Guard against getting contexts with MGLAnnotationTagNotF…
boundsj Apr 7, 2017
99eee0a
Make changelogs more and less specific
boundsj Apr 7, 2017
de5cceb
Allow assertions for inconsistent state that should not happen
boundsj Apr 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions platform/ios/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

Mapbox welcomes participation and contributions from everyone. Please read [CONTRIBUTING.md](../../CONTRIBUTING.md) to get started.

## 3.5.2

* Fixed an issue that caused a crash when the user location annotation was presenting a callout view and the map was moved. ([#8686](https://github.com/mapbox/mapbox-gl-native/pull/8686))

## 3.5.1 - April 5, 2017

* Fixed an issue that caused the return type of a map view delegate method to bridge incorrectly to applications written in Swift. ([#8541](https://github.com/mapbox/mapbox-gl-native/pull/8541))
Expand Down
74 changes: 41 additions & 33 deletions platform/ios/src/MGLMapView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1452,7 +1452,9 @@ - (void)handleSingleTapGesture:(UITapGestureRecognizer *)singleTap
}
else
{
nextElement = _annotationContextsByAnnotationTag.at(_selectedAnnotationTag).accessibilityElement;
if (_selectedAnnotationTag != MGLAnnotationTagNotFound) {
nextElement = _annotationContextsByAnnotationTag.at(_selectedAnnotationTag).accessibilityElement;
}
}
[self deselectAnnotation:self.selectedAnnotation animated:YES];
UIAccessibilityPostNotification(UIAccessibilityScreenChangedNotification, nextElement);
Expand Down Expand Up @@ -2002,19 +2004,21 @@ - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(N
{
const mbgl::Point<double> point = MGLPointFromLocationCoordinate2D(annotation.coordinate);

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];
}
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;
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];
// 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];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]])
Expand Down Expand Up @@ -3055,17 +3059,18 @@ - (void)removeStyleClass:(NSString *)styleClass

for (auto const& annotationTag: annotationTags)
{
if (!_annotationContextsByAnnotationTag.count(annotationTag))
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];
Expand All @@ -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))
{
if ( ! _annotationContextsByAnnotationTag.count(tag) ||
tag == MGLAnnotationTagNotFound) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this return userLocation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return nil;
}

Expand Down Expand Up @@ -3680,10 +3685,12 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL)
{
return self.userLocation;
}
if ( ! _annotationContextsByAnnotationTag.count(_selectedAnnotationTag))
{

if ( ! _annotationContextsByAnnotationTag.count(_selectedAnnotationTag) ||
_selectedAnnotationTag == MGLAnnotationTagNotFound) {
return nil;
}

MGLAnnotationContext &annotationContext = _annotationContextsByAnnotationTag.at(_selectedAnnotationTag);
return annotationContext.annotation;
}
Expand Down Expand Up @@ -3749,19 +3756,16 @@ - (void)selectAnnotation:(id <MGLAnnotation>)annotation animated:(BOOL)animated
MGLAnnotationView *annotationView = nil;

if (annotation != self.userLocation)
{
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];
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];
}
}
}

Expand Down Expand Up @@ -5039,8 +5043,12 @@ - (void)updateCalloutView
if (isAnchoredToAnnotation)
{
MGLAnnotationTag tag = [self annotationTagForAnnotation:annotation];
MGLAnnotationContext &annotationContext = _annotationContextsByAnnotationTag.at(tag);
MGLAnnotationView *annotationView = annotationContext.annotationView;
MGLAnnotationView *annotationView = nil;

if (tag != MGLAnnotationTagNotFound) {
MGLAnnotationContext &annotationContext = _annotationContextsByAnnotationTag.at(tag);
annotationView = annotationContext.annotationView;
}

CGRect rect = [self positioningRectForCalloutForAnnotationWithTag:tag];

Expand Down
1 change: 1 addition & 0 deletions platform/macos/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## 0.4.1

* Fixed an issue causing code signing failures and bloating the framework. ([#8640](https://github.com/mapbox/mapbox-gl-native/pull/8640))
* Fixed an issue that could cause a crash if annotations unknown to the map view were interacted with. ([#8686](https://github.com/mapbox/mapbox-gl-native/pull/8686))
* Renamed the "Data-Driven Styling" guide to "Using Style Functions at Runtime" and clarified the meaning of data-driven styling in the guide's discussion of runtime style functions. ([#8627](https://github.com/mapbox/mapbox-gl-native/pull/8627))

## 0.4.0
Expand Down
11 changes: 8 additions & 3 deletions platform/macos/src/MGLMapView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1796,10 +1796,12 @@ - (IBAction)giveFeedback:(id)sender {

for (auto const& annotationTag: annotationTags)
{
if (!_annotationContextsByAnnotationTag.count(annotationTag))
if (!_annotationContextsByAnnotationTag.count(annotationTag) ||
annotationTag == MGLAnnotationTagNotFound)
{
continue;
}

MGLAnnotationContext annotationContext = _annotationContextsByAnnotationTag.at(annotationTag);
NSAssert(annotationContext.annotation, @"Missing annotation for tag %u.", annotationTag);
if (annotationContext.annotation)
Expand All @@ -1816,7 +1818,8 @@ - (IBAction)giveFeedback:(id)sender {

/// Returns the annotation assigned the given tag. Cheap.
- (id <MGLAnnotation>)annotationWithTag:(MGLAnnotationTag)tag {
if (!_annotationContextsByAnnotationTag.count(tag)) {
if ( ! _annotationContextsByAnnotationTag.count(tag) ||
tag == MGLAnnotationTagNotFound) {
return nil;
}

Expand Down Expand Up @@ -2147,9 +2150,11 @@ - (MGLAnnotationTag)annotationTagAtPoint:(NSPoint)point persistingResults:(BOOL)
}

- (id <MGLAnnotation>)selectedAnnotation {
if (!_annotationContextsByAnnotationTag.count(_selectedAnnotationTag)) {
if ( ! _annotationContextsByAnnotationTag.count(_selectedAnnotationTag) ||
_selectedAnnotationTag == MGLAnnotationTagNotFound) {
return nil;
}

MGLAnnotationContext &annotationContext = _annotationContextsByAnnotationTag.at(_selectedAnnotationTag);
return annotationContext.annotation;
}
Expand Down