[ios] Removing an annotation does not remove it from mapView.annotationContainerView.annotationViews

[JesseCrocker]

Platform: iOS
Mapbox SDK version: master@8eb23cbac8a6c5821dc5935e27689700216c3f1a

Steps to trigger behavior

1. Add an annotation to the map
2. Remove all annotations

Expected behavior

mapView.annotationContainerView.annotationViews will contain 0 items

Actual behavior

mapView.annotationContainerView.annotationViews still contains 1 item.

Why this is a problem

Im using a subclass of MGLAnnotationView, that sets self.annotation = nil in it's removeFromSuperviewMethod, the annotation property is nullable, so this seems like a legitimate thing to do, and i need to do it to make sure my annotation gets dealloced when it is removed from the map.
If the map is later tapped in the same spot where the annotation was, - (nullable id <MGLAnnotation>)annotationForGestureRecognizer:(UITapGestureRecognizer*)singleTap persistingResults:(BOOL)persist finds the annotationView, and raises an exception because it's annotation is nil. And since the method call that raised the exception was triggered by a UIGestureRecognizer, there is no way to catch it.

Test case

I have tested this by adding the following to MBXViewController.m

#import "MGLAnnotationContainerView.h"
#import "MGLAnnotationContainerView_Private.h"
@interface MGLMapView(foo)
@property (nonatomic) MGLAnnotationContainerView *annotationContainerView;
@end

and

- (void)viewDidAppear:(BOOL)animated {
  [super viewDidAppear:animated];
  [self.mapView removeAnnotations:self.mapView.annotations];
  MBXCustomCalloutAnnotation *firstAnnotation = [[MBXCustomCalloutAnnotation alloc] init];
  firstAnnotation.coordinate = CLLocationCoordinate2DMake(48.8533940, 2.3775439);
  [self.mapView addAnnotation:firstAnnotation];
  
  [self.mapView showAnnotations:@[firstAnnotation] animated:YES];
  dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(10 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
    [self.mapView removeAnnotations:self.mapView.annotations];
    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(5 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
      NSLog(@"map has %lu annotations", self.mapView.annotations.count);
      NSLog(@"self.mapView.annotationContainerView.annotationViews.count %lu", self.mapView.annotationContainerView.annotationViews.count);
    });
  });
}

[boundsj]

Thanks for the report @JesseCrocker!

I think the current behavior may have been intentional since annotation views can be reused and there is a significant performance hit in UIKit when adding a lot of subviews to a container view. I agree this is unexpected though and probably over optimizes for the edge case of many thousands of annotation views. I've added this ticket to the next release (v3.6.0) milestone.

cc @incanus @fabian-guerra

[incanus]

I am seeing fallout at least somewhat related to this in #2985 (comment), namely that without CADisplayLink, despite -updateAnnotationViews getting called per-frame, annotations stick in place even when "offscreen" until a fly animation ends. If they were no longer in the view hierarchy, we wouldn't see this (though there are other things at fault here, too).

🙏 @JesseCrocker for the find.

[boundsj]

@boundsj I think the current behavior may have been intentional since annotation views can be reused ...

Actually, I think there is no good reason that the annotation view should not be removed from the container's array.

The PR #8755 works around a bug caused by this issue but also removes an otherwise valid sanity check. That is, annotation views that are not visible because they are in the reuse queue or removed should have a nil annotation property. The other exceptions that were added as a result of #8139 (review) are all similarly valid. I think we should close #8755 and find another solution.

To that end, I created #9025 to close this issue. I also created #9029 to add an initializer that takes an annotation to facilitate customization of the view based on the annotation model properties.

Finally, from the OP:

@JesseCrocker Im using a subclass of MGLAnnotationView, that sets self.annotation = nil in it's removeFromSuperviewMethod, the annotation property is nullable, so this seems like a legitimate thing to do

The changes we've made (#8139, #9029) in the name of MapKit parity are really intended to allow for customization of the annotation view as function of the annotation. And, just like with MapKit, our documentation still states that setting the annotation property of the annotation view in application code is not advised:

MKAnnotationView

You should not change the value of this property directly. This property contains a non-nil value only while the annotation view is visible on the map. If the view is queued and waiting to be reused, the value is nil

MGLAnnotationView

You should not change the value of this property directly. This property contains a non-nil value while the annotation view is visible on the map. If the view is queued, waiting to be reused, the value is nil.

Hopefully #9025 will resolve the memory issues that that would happen because of #8754

[boundsj]

Closed by #9025 in the release-ios-v3.6.0-android-v5.1.0 branch.