Make iOS delegate notifications consistent with MapKit

[friedbunny]

MapKit loading delegate callbacks are exclusively about fetching map data, while rendering ones are exclusively the drawing of the map.

The flow for Mapkit looks like this on initialization:

2015-10-23 19:39:28.470 start RENDERING
2015-10-23 19:39:28.503 start LOADING
2015-10-23 19:39:29.435 finish LOADING
2015-10-23 19:39:30.470 finish RENDERING fullyRendered: YES

Panning around continues to issue the same notification flow.

We don't follow MapKit conventions currently. Instead, at the core-level:

• ❌ MapChangeWillStartLoading notifications are fired when the style is set
• ❌ MapChangeDidFinishLoading is fired after the map is fully rendered, in Map::renderSync(), not at the point where data is finished loading
• ❌ MapChangeWillStartRendering is sent in Map::renderSync() but only initially (it should always come before DidFinish, but it does not)
• ✅ MapChangeDidFinishRenderingMapFullyRendered is sent when rendering is done
  • ❌ but does not handle the non-fully-rendered case (support mapViewDidFinishRenderingMap:fullyRendered:NO #1804)
• ❌ MapChangeDidFailLoadingMap is not implemented (Implement MapChangeDidFailLoadingMap event notification #2762)

Our initialization flow looks like:

2015-10-23 19:42:21.695 start RENDERING
2015-10-23 19:42:24.260 finish RENDERING fullyRendered: YES
2015-10-23 19:42:24.262 finish LOADING

Panning around our map subsequently only sends more DidFinishRenderingMap, nothing else.

Making our iOS map delegate notifications consistent with MapKit will require core changes, rather than iOS changes, which will in turn affect Android. Should Android follow these conventions, too?

/cc @mapbox/mobile

[ljbade]

For Android, the main concern is the loading/failed loading events are the only way to return an error about loading the style to the app since everything is async.

@friedbunny How does iOS return async style load failures?

[friedbunny]

MapChangeDidFinishRenderingMapFullyRendered with a false value would indicate some tile(s) failed to render, while MapChangeDidFailLoadingMap would fire when map data failed to load from the network. Neither of these are implemented.

We could also potentially use MapChangeDidFailLoadingMap to indicate that styles have failed to load. Currently, we throw somewhat cryptic errors from MapContext::setStyleURL — usually just the HTTP error code. I don't believe iOS has any direct way of knowing if a style has failed to load.

[ljbade]

As for async style load failures, both iOS and Android are going to implement proper C++ exception handling for this in #2825

[1ec5]

@ljbade, #2762 is the most likely way for this to be implemented in iOS, although we could take an approach similar to #2825 to turn exceptions into NSExceptions, which would result in a slightly improved experience. But #2825 is about Android, which needs this much more than iOS right now.

[1ec5]

#4527 proposes an additional mbgl notification for when the style has finished loading that could be useful for MGLMapViewDelegate.

[lightandshadow68]

It also appears the order in which mapView:didDeselectAnnotationView and mapView:didSelectAnnotationView is different. in MapKit, didDeselect is called first and then didSeselect. However, the opposite order is used with MGLMapView.

For example, I show a UICollectionView when an annotation is tapped and hide it when a user taps on an empty area of the map. To accomplish this, I start a timer in didDeselect to hide the view, which is invalidated in didSelect. However, this strategy no longer works since didSelect is called first.

[lightandshadow68]

Actually, it looks like update code called from my implementation of didSelectAnnotationView has started causing didDeselectAnnotationView to be called with Mapbox GL. So, it only appeared that the order had changed. I'll try to track down where the actual behavior differs. But, as of now, the order in which didSelect and didDeselect is called no longer appears to be where the difference lies.

[lightandshadow68]

It looks like one of these two cases are causing mapView:didDeselectAnnotationView to be called when changing the region of the map via [MGLMapView showAnnotations:edgePadding:].

case mbgl::MapChangeRegionWillChange:
        case mbgl::MapChangeRegionWillChangeAnimated:
        {
            if ( ! _userLocationAnnotationIsSelected
                || self.userTrackingMode == MGLUserTrackingModeNone
                || self.userTrackingState != MGLUserTrackingStateChanged)
            {
                UIView<MGLCalloutView> *calloutView = self.calloutViewForSelectedAnnotation;
                BOOL dismissesAutomatically = (calloutView
                                               && [calloutView respondsToSelector:@selector(dismissesAutomatically)]
                                               && calloutView.dismissesAutomatically);
                // dismissesAutomatically is an optional property and we want to dismiss
                // the callout view if it's unimplemented.
                if (dismissesAutomatically || ![calloutView respondsToSelector:@selector(dismissesAutomatically)])
                {
                    [self deselectAnnotation:self.selectedAnnotation animated:NO];
                }
                else
                {
                    // Deselect annotation if it lies outside the viewport
                    if (self.selectedAnnotation) {
                        MGLAnnotationTag tag = [self annotationTagForAnnotation:self.selectedAnnotation];
                        MGLAnnotationContext &annotationContext = _annotationContextsByAnnotationTag.at(tag);
                        MGLAnnotationView *annotationView = annotationContext.annotationView;
                        
                        CGRect rect = [self positioningRectForCalloutForAnnotationWithTag:tag];
                        
                        if (annotationView)
                        {
                            rect = annotationView.frame;
                        }
                        
                        if ( ! CGRectIntersectsRect(rect, self.frame)) {
                            [self deselectAnnotation:self.selectedAnnotation animated:NO];
                        }
                    }
                }
            }

            if ( ! [self isSuppressingChangeDelimiters] && [self.delegate respondsToSelector:@selector(mapView:regionWillChangeAnimated:)])
            {
                BOOL animated = change == mbgl::MapChangeRegionWillChangeAnimated;
                [self.delegate mapView:self regionWillChangeAnimated:animated];
            }
            break;
        }

When tapping on an annotation that is in the center of the screen, and remains there throughout the region change, the annotation is still deselected. I'm not sure if the viewport check or callout deselection is being incorrectly triggered.

I attempted to set a flag in my delegate to ignore the deselection callback when the region is changing, but deselection occurs before regionWillChangeAnimated is called.

[stale]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

[stale]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

[stale]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.