From 684480a4567988803012aacced17028a96c85b9f Mon Sep 17 00:00:00 2001 From: Jesse Bounds Date: Fri, 29 Apr 2016 11:38:45 -0600 Subject: [PATCH] [ios] Guard against annotation reuse when there is no identifier This protects against a crash that would occur when view reuse was attempted in cases where the map view delegate did not ask for it. It also fixes a bug where annotation views were attempted when the map view delegate did not implement the correct delegate method. It also moves the call to `updateAnnotationViews` back to `glkView:drawInRect` since doing it in `notifyMapChange` introduced lag. Additional optimizations may occur in future commits to make the work required to hide and show views and handle view reuse less intensive. In addition, the example MBXAnnotationView is simplified to just a programatic view with a center panel that can change color. The color changing center panel is used to indicate if a vie has gone through the reuse queue. --- platform/ios/app/MBXAnnotationView.m | 18 +++----- platform/ios/app/MBXAnnotationView.xib | 28 ------------ platform/ios/app/MBXViewController.m | 7 ++- platform/ios/ios.xcodeproj/project.pbxproj | 3 -- platform/ios/src/MGLMapView.mm | 50 ++++++++++++++-------- 5 files changed, 44 insertions(+), 62 deletions(-) delete mode 100644 platform/ios/app/MBXAnnotationView.xib diff --git a/platform/ios/app/MBXAnnotationView.m b/platform/ios/app/MBXAnnotationView.m index f2a3af38d1b..eaf398a220c 100644 --- a/platform/ios/app/MBXAnnotationView.m +++ b/platform/ios/app/MBXAnnotationView.m @@ -2,24 +2,20 @@ @interface MBXAnnotationView () -@property (weak, nonatomic) IBOutlet UIView *centerView; +@property (nonatomic) UIView *centerView; @end @implementation MBXAnnotationView -- (instancetype)initWithCoder:(NSCoder *)coder -{ - self = [super initWithCoder:coder]; - if (self) { - self.layer.cornerRadius = CGRectGetWidth(self.bounds) / 2.0; - } - return self; -} - - (void)layoutSubviews { [super layoutSubviews]; - self.centerView.layer.cornerRadius = CGRectGetWidth(self.centerView.bounds) / 2.0; + if (!self.centerView) { + self.backgroundColor = [UIColor blueColor]; + self.centerView = [[UIView alloc] initWithFrame:CGRectMake(5,5, CGRectGetWidth(self.bounds) - 10, CGRectGetWidth(self.bounds) - 10)]; + self.centerView.backgroundColor = self.centerColor; + [self addSubview:self.centerView]; + } } - (void)setCenterColor:(UIColor *)centerColor { diff --git a/platform/ios/app/MBXAnnotationView.xib b/platform/ios/app/MBXAnnotationView.xib deleted file mode 100644 index 02aa73ab50b..00000000000 --- a/platform/ios/app/MBXAnnotationView.xib +++ /dev/null @@ -1,28 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/platform/ios/app/MBXViewController.m b/platform/ios/app/MBXViewController.m index fa415d5e066..24c0ee2bea1 100644 --- a/platform/ios/app/MBXViewController.m +++ b/platform/ios/app/MBXViewController.m @@ -612,10 +612,13 @@ - (void)dealloc - (MGLAnnotationView *)mapView:(MGLMapView *)mapView viewForAnnotation:(id)annotation { MBXAnnotationView *annotationView = (MBXAnnotationView *)[mapView dequeueReusableAnnotationViewWithIdentifier:@"MBXAnnotationView"]; - annotationView.centerColor = [UIColor redColor]; if (!annotationView) { - annotationView = [[[NSBundle mainBundle] loadNibNamed:@"MBXAnnotationView" owner:self options:nil] objectAtIndex:0]; + annotationView = [[MBXAnnotationView alloc] initWithFrame:CGRectMake(0, 0, 40, 40)]; + annotationView.centerColor = [UIColor whiteColor]; + } else { + // orange indicates that the annotation view was reused + annotationView.centerColor = [UIColor orangeColor]; } return annotationView; } diff --git a/platform/ios/ios.xcodeproj/project.pbxproj b/platform/ios/ios.xcodeproj/project.pbxproj index 81cb4d48d95..303a52b5c93 100644 --- a/platform/ios/ios.xcodeproj/project.pbxproj +++ b/platform/ios/ios.xcodeproj/project.pbxproj @@ -7,7 +7,6 @@ objects = { /* Begin PBXBuildFile section */ - 400165041CCFC618005583CC /* MBXAnnotationView.xib in Resources */ = {isa = PBXBuildFile; fileRef = 400165031CCFC618005583CC /* MBXAnnotationView.xib */; }; 40FC668F1CCE8D6000AFCF7D /* MGLAnnotationView_Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 40FC668B1CCE8CFC00AFCF7D /* MGLAnnotationView_Private.h */; }; 40FC66901CCE8D6A00AFCF7D /* MGLAnnotationView.h in Headers */ = {isa = PBXBuildFile; fileRef = 40FC668C1CCE8D4F00AFCF7D /* MGLAnnotationView.h */; settings = {ATTRIBUTES = (Public, ); }; }; 40FC66911CCE8D6A00AFCF7D /* MGLAnnotationView.m in Sources */ = {isa = PBXBuildFile; fileRef = 40FC668D1CCE8D4F00AFCF7D /* MGLAnnotationView.m */; }; @@ -311,7 +310,6 @@ /* End PBXCopyFilesBuildPhase section */ /* Begin PBXFileReference section */ - 400165031CCFC618005583CC /* MBXAnnotationView.xib */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = file.xib; path = MBXAnnotationView.xib; sourceTree = ""; }; 40FC668B1CCE8CFC00AFCF7D /* MGLAnnotationView_Private.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = MGLAnnotationView_Private.h; path = src/MGLAnnotationView_Private.h; sourceTree = ""; }; 40FC668C1CCE8D4F00AFCF7D /* MGLAnnotationView.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = MGLAnnotationView.h; path = src/MGLAnnotationView.h; sourceTree = ""; }; 40FC668D1CCE8D4F00AFCF7D /* MGLAnnotationView.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = MGLAnnotationView.m; path = src/MGLAnnotationView.m; sourceTree = ""; }; @@ -558,7 +556,6 @@ DA1DC9981CB6E054006E619F /* MBXAppDelegate.m */, 40FDA7691CCAAA6800442548 /* MBXAnnotationView.h */, 40FDA76A1CCAAA6800442548 /* MBXAnnotationView.m */, - 400165031CCFC618005583CC /* MBXAnnotationView.xib */, DA1DC9661CB6C6B7006E619F /* MBXCustomCalloutView.h */, DA1DC9671CB6C6B7006E619F /* MBXCustomCalloutView.m */, DA1DC9681CB6C6B7006E619F /* MBXOfflinePacksTableViewController.h */, diff --git a/platform/ios/src/MGLMapView.mm b/platform/ios/src/MGLMapView.mm index a4641b4308c..5967d779158 100644 --- a/platform/ios/src/MGLMapView.mm +++ b/platform/ios/src/MGLMapView.mm @@ -861,6 +861,7 @@ - (void)glkView:(__unused GLKView *)view drawInRect:(__unused CGRect)rect { _mbglMap->render(); + [self updateAnnotationViews]; [self updateUserLocationAnnotationView]; } } @@ -2792,8 +2793,9 @@ - (void)addAnnotations:(NS_ARRAY_OF(id ) *)annotations if (delegateImplementsViewForAnnotation) { - annotationView = [self addAnnotationViewWithAnnotation:annotation]; - if (annotationView) { + annotationView = [self annotationViewForAnnotation:annotation]; + if (annotationView) + { annotationViewsForAnnotation[annotationValue] = annotationView; annotationView.center = [self convertCoordinate:annotation.coordinate toPointToView:self]; [self.glView addSubview:annotationView]; @@ -2901,7 +2903,7 @@ - (MGLAnnotationImage *)defaultAnnotationImage return annotationImage; } -- (MGLAnnotationView *)addAnnotationViewWithAnnotation:(id)annotation +- (MGLAnnotationView *)annotationViewForAnnotation:(id)annotation { MGLAnnotationView *annotationView = (MGLAnnotationView *)[self.delegate mapView:self viewForAnnotation:annotation]; @@ -4236,9 +4238,6 @@ - (void)notifyMapChange:(mbgl::MapChange)change { [self.delegate mapViewRegionIsChanging:self]; } - - [self updateAnnotationViews]; - break; } case mbgl::MapChangeRegionDidChange: @@ -4326,21 +4325,29 @@ - (void)updateUserLocationAnnotationView - (void)updateAnnotationViews { + BOOL delegateImplementsViewForAnnotation = [self.delegate respondsToSelector:@selector(mapView:viewForAnnotation:)]; + + if (!delegateImplementsViewForAnnotation) + { + return; + } + // Update all visible annotation views std::set visibleTags; - std::vector annotationTags = [self annotationTagsInRect:CGRectInset(self.bounds, -50, -50)]; + std::vector annotationTags = [self annotationTagsInRect:self.bounds]; + for(auto const& annotationTag: annotationTags) { auto &annotationContext = _annotationContextsByAnnotationTag[annotationTag]; id annotation = annotationContext.annotation; + CGPoint annotationPoint = [self convertCoordinate:annotation.coordinate toPointToView:self]; // If there is no annotation view at this point, it means the context's view was reused by some - // other context so we need to reuse or make a new view. If we do get here, it does mean - // that th delegate supports annotation views so it is safe to ask it for another one + // other context so we need to reuse or make a new view. if (!annotationContext.annotationView) { - MGLAnnotationView *annotationView = [self addAnnotationViewWithAnnotation:annotation]; + MGLAnnotationView *annotationView = [self annotationViewForAnnotation:annotation]; if (annotationView) { @@ -4353,27 +4360,34 @@ - (void)updateAnnotationViews annotationContext.annotationView = annotationView; } } - + annotationContext.annotationView.center = annotationPoint; annotationContext.annotationView.hidden = NO; + visibleTags.insert(annotationTag); } - // Add offscreen annotation views to reuse queue + // Hide and add offscreen annotation views to reuse queue for (auto &pair : _annotationContextsByAnnotationTag) { MGLAnnotationTag annotationTag = pair.first; MGLAnnotationContext &annotationContext = pair.second; MGLAnnotationView *annotationView = annotationContext.annotationView; - NSMutableArray *annotationViewReuseQueue = [self annotationViewReuseQueueForIdentifier:annotationContext.viewReuseIdentifier]; - const bool tagIsNotVisible = visibleTags.find(annotationTag) == visibleTags.end(); - - if (tagIsNotVisible && annotationView && ![annotationViewReuseQueue containsObject:annotationView]) + + if (annotationView && tagIsNotVisible) { - [annotationViewReuseQueue addObject:annotationView]; annotationView.hidden = YES; - annotationContext.annotationView = NULL; + + if (annotationContext.viewReuseIdentifier) + { + NSMutableArray *annotationViewReuseQueue = [self annotationViewReuseQueueForIdentifier:annotationContext.viewReuseIdentifier]; + if (![annotationViewReuseQueue containsObject:annotationView]) + { + [annotationViewReuseQueue addObject:annotationView]; + annotationContext.annotationView = nil; + } + } } } }