Platform bindings for runtime styling APIs on MapSnapshotter

[chloekraw]

mapbox/mapbox-gl-native#16268

[1ec5]

mapbox/mapbox-gl-native#16268 has yet to be released. In the meantime, I’ll update the submodule to master locally to work on the integration.

[1ec5]

Changes to mbgl’s public API in mapbox/mapbox-gl-native#16268 are forcing me to rewrite MGLMapSnapshotter. It’s a long-overdue rewrite. I think we’ll be able to clear away many of the pessimizations and unsafe thread safety patches we’ve applied to this class over the years by removing indirection and parallel state.

The refactoring centers around passing the completion handler around in local scope without storing it as an ivar. It has been going well, except for one detail: if you pass a completion handler into -startWithCompletionHandler:, then call -cancel, the completion handler gets called from a class method. I would like to remove this behavior so that -cancel simply stops all activity. Apple’s asynchronous, NSOperation-friendly APIs similarly avoid calling the completion handler when canceling; examples include MKMapSnapshotter and MKDirections. If we don’t call the completion handler when canceling, then there’s no need to store the completion handler anywhere in SDK code, avoiding a whole class of memory and concurrency issues.

As far as I can tell, we don’t document anywhere that the completion handler does get called in this situation, so it would be a backwards-compatible change. The developer has full control over what happens when they call -cancel; they’re free to call that completion block themselves.

[1ec5]

For the actual runtime styling feature, I plan to add an “actions” or “style handler” block to either -[MGLMapSnapshotter startWithQueue:overlayHandler:completionHandler:] or MGLMapSnapshotOptions that accepts an MGLStyle. This would afford more flexibility than mapbox/mapbox-gl-native#16286, which only allows the developer to add a single layer, and would be consistent with familiar APIs like +[UIGraphicsImageRenderer imageWithActions:] and +[NSImage imageWithSize:flipped:drawingHandler:]. We’ll need to internally decouple MGLStyle from MGLMapView to some extent, but I’ll try to keep that from creeping into a full solution to mapbox/mapbox-gl-native#6386.

In this case, a block is more appropriate than a full-on delegate protocol, because we only expect the style to load a single time per snapshot. If we need to support the onDidFailLoadingStyle and onStyleImageMissing events, then we
could introduce a MGLMapSnapshotterDelegate protocol along the lines of NSURLSessionDelegate. This would keep the starting method from getting unwieldy. But we might still want to add an action block to the starting method as a convenience.

[1ec5]

if you pass a completion handler into -startWithCompletionHandler:, then call -cancel, the completion handler gets called from a class method. I would like to remove this behavior so that -cancel simply stops all activity.

For consistency, -cancel calling the completion handler meant that the completion handler also had to be called when the snapshotter got deallocated or the application terminated. This behavior was introduced in mapbox/mapbox-gl-native#12355, but I believe we should reconsider.

From my perspective, it’s counterintuitive that a completion handler passed into an instance method would get called after the instance’s lifetime. The normal developer expectation is that the instance owns the completion handler, not the other way around. mapbox/mapbox-gl-native#12336 (comment) suggested keeping the snapshotter alive by capturing it in the completion handler. But the operation pattern actually calls for the snapshotter to be held by the surrounding class or placed in an operation queue. If the completion handler has any need for the snapshotter, it should be weakly referenced within the block.

[1ec5]

Apple’s asynchronous, NSOperation-friendly APIs similarly avoid calling the completion handler when canceling; examples include MKMapSnapshotter and MKDirections.

MKMapSnapshotter doesn’t call the completion handler when canceling, but it does call the completion handler when MKMapSnapshotter is only a local variable, so it must be retained by something while loading. This behavior is convenient for things like Swift playgrounds where storing an ivar feels like overkill.

As far as I can tell, MapKit keeps the snapshotter alive by having a block capture the snapshotter. We could do that, but it might lead to the same muddled situation we’ve found ourselves in with the current MGLMapSnapshotter implementation. It’s far simpler to require the developer to hang onto the snapshotter, so I think we should go with that until we find a clean way to keep the snapshotter alive, which would be a seamless change.

[1ec5]

#211 keeps the snapshotter alive until after the completion handler executes, like MapKit’s MKMapSnapshotter, now that #210 makes the control flow more manageable.