Removed redundant `loading` property. Added (currently private) `cancelled` property.upstream/jrex/12336-snapshotter-lifetime

7 files changed, 25 insertions, 22 deletions

diff --git a/platform/darwin/src/MGLMapSnapshotter.h b/platform/darwin/src/MGLMapSnapshotter.h
index 82d1d65efd..a5ddcac0ac 100644
--- a/platform/darwin/src/MGLMapSnapshotter.h
+++ b/platform/darwin/src/MGLMapSnapshotter.h
@@ -197,9 +197,6 @@ MGL_EXPORT
  
  Once you call this method, you cannot resume the snapshot. In order to obtain
  the snapshot, create a new `MGLMapSnapshotter` object.
-
- The completion handler will be called with an `NSError` with domain
- `MGLErrorDomain` and error code `MGLErrorCodeSnapshotUserCancelled`.
  */
 - (void)cancel;
 
diff --git a/platform/darwin/src/MGLMapSnapshotter.mm b/platform/darwin/src/MGLMapSnapshotter.mm
index 4a7d95c607..aa1a988b1b 100644
--- a/platform/darwin/src/MGLMapSnapshotter.mm
+++ b/platform/darwin/src/MGLMapSnapshotter.mm
@@ -109,7 +109,7 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64;
 @end
 
 @interface MGLMapSnapshotter()
-@property (nonatomic) BOOL loading;
+@property (nonatomic) BOOL cancelled;
 @property (nonatomic) dispatch_queue_t resultQueue;
 @property (nonatomic, copy) MGLMapSnapshotCompletionHandler completion;
 + (void)completeWithErrorCode:(MGLErrorCode)errorCode description:(nonnull NSString*)description onQueue:(dispatch_queue_t)queue completion:(MGLMapSnapshotCompletionHandler)completion;
@@ -122,8 +122,8 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64;
 }
 
 - (void)dealloc {
-    if (_snapshotCallback) {
-        NSAssert(_loading, @"Snapshot in progress - `loading` should = YES");
+    if (_completion) {
+        NSAssert(_snapshotCallback, @"Snapshot in progress - there should be a valid callback");
 
         [MGLMapSnapshotter completeWithErrorCode:MGLErrorCodeSnapshotFailed
                                      description:@"MGLMapSnapshotter deallocated prior to snapshot completion."
@@ -137,8 +137,6 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64;
     self = [super init];
     if (self) {
         [self setOptions:options];
-        _loading = false;
-
     }
     return self;
 }
@@ -155,15 +153,15 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64;
                     format:@"startWithQueue:completionHandler: must be called from a thread with an active run loop."];
     }
 
-    if ([self isLoading]) {
+    if (self.completion) {
+        // Consider replacing this exception with an error passed to the completion block.
         [NSException raise:NSInternalInconsistencyException
                     format:@"Already started this snapshotter."];
     }
     
-    self.loading = true;
-
     self.completion = completion;
     self.resultQueue = queue;
+    self.cancelled = NO;
 
     __weak __typeof__(self) weakSelf = self;
     // mbgl::Scheduler::GetCurrent() scheduler means "run callback on current (ie UI/main) thread"
@@ -177,8 +175,6 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64;
         // If self had died, _snapshotCallback would have been destroyed and this block would not be executed
         NSCAssert(strongSelf, @"Snapshot callback executed after being destroyed.");
 
-        strongSelf.loading = false;
-
         if (!strongSelf.completion)
             return;
 
@@ -504,8 +500,10 @@ const CGFloat MGLSnapshotterMinimumPixelSize = 64;
 
 - (void)cancel
 {
+    self.cancelled = YES;
+    
     if (_snapshotCallback) {
-        [MGLMapSnapshotter completeWithErrorCode:MGLErrorCodeSnapshotUserCancelled
+        [MGLMapSnapshotter completeWithErrorCode:MGLErrorCodeSnapshotFailed
                                      description:[NSString stringWithFormat:@"MGLMapSnapshotter cancelled from %s", __PRETTY_FUNCTION__]
                                          onQueue:self.resultQueue
                                       completion:self.completion];
diff --git a/platform/darwin/src/MGLTypes.h b/platform/darwin/src/MGLTypes.h
index f2ea6e3a6c..035c6f6593 100644
--- a/platform/darwin/src/MGLTypes.h
+++ b/platform/darwin/src/MGLTypes.h
@@ -49,9 +49,7 @@ typedef NS_ENUM(NSInteger, MGLErrorCode) {
     /** An attempt to load the style failed. */
     MGLErrorCodeLoadStyleFailed = 5,
     /** An error occurred while snapshotting the map. */
-    MGLErrorCodeSnapshotFailed = 6,
-    /** The user cancelled a map snapshot. */
-    MGLErrorCodeSnapshotUserCancelled = 7,
+    MGLErrorCodeSnapshotFailed = 6
 };
 
 /** Options for enabling debugging features in an `MGLMapView` instance. */
diff --git a/platform/ios/CHANGELOG.md b/platform/ios/CHANGELOG.md
index 1e68fd3ab3..7842af318e 100644
--- a/platform/ios/CHANGELOG.md
+++ b/platform/ios/CHANGELOG.md
@@ -11,6 +11,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
 * Added `MGLShapeOfflineRegion` for defining arbitrarily shaped offline regions [#11447](https://github.com/mapbox/mapbox-gl-native/pull/11447)
 * Added a one-time warning about possible attribute loss when initializing an `MGLShapeSource` with an `MGLShapeCollection` [#12625](https://github.com/mapbox/mapbox-gl-native/pull/12625)
 * Added an `-[MGLMapViewDelegate mapView:shapeAnnotationIsEnabled:]` method to specify whether an annotation is selectable. ([#12352](https://github.com/mapbox/mapbox-gl-native/pull/12352))
+* Fixed bug where completion block passed to `-[MGLMapSnapshotter startWithQueue:completionHandler:]` was not being called in all code paths. ([#12355](https://github.com/mapbox/mapbox-gl-native/pull/12355))
 
 ### Offline
 * Improved SQLite error handling for offline databases that return specific errors. [#12224](https://github.com/mapbox/mapbox-gl-native/pull/12224)
diff --git a/platform/ios/Integration Tests/Snapshotter Tests/MGLMapSnapshotterTest.m b/platform/ios/Integration Tests/Snapshotter Tests/MGLMapSnapshotterTest.m
index de8bc5c23a..fd5b3db978 100644
--- a/platform/ios/Integration Tests/Snapshotter Tests/MGLMapSnapshotterTest.m
+++ b/platform/ios/Integration Tests/Snapshotter Tests/MGLMapSnapshotterTest.m
@@ -1,5 +1,10 @@
 #import "MGLMapViewIntegrationTest.h"
 
+@interface MGLMapSnapshotter ()
+@property (nonatomic) BOOL cancelled;
+@end
+
+
 @interface MGLMapSnapshotterTest : MGLMapViewIntegrationTest
 @end
 
@@ -79,7 +84,8 @@ MGLMapSnapshotter* snapshotterWithCoordinates(CLLocationCoordinate2D coordinates
 
         dispatch_async(dispatch_get_main_queue(), ^{
             MGLMapSnapshotter *snapshotter = snapshotterWithCoordinates(coord, size);
-
+            __weak MGLMapSnapshotter *weakSnapshotter = snapshotter;
+            
             [snapshotter startWithCompletionHandler:^(MGLMapSnapshot * _Nullable snapshot, NSError * _Nullable error) {
                 // We expect this completion block to be called with an error
                 __typeof__(self) strongself = weakself;
@@ -88,6 +94,8 @@ MGLMapSnapshotter* snapshotterWithCoordinates(CLLocationCoordinate2D coordinates
                 MGLTestAssert(strongself,
                               ([error.domain isEqualToString:MGLErrorDomain] && error.code == MGLErrorCodeSnapshotFailed),
                               @"Should have errored");
+                MGLTestAssertNil(strongself, weakSnapshotter, @"Snapshotter should have been deallocated");
+
                 dispatch_group_leave(dg);
             }];
         });
@@ -170,8 +178,9 @@ MGLMapSnapshotter* snapshotterWithCoordinates(CLLocationCoordinate2D coordinates
 
         MGLTestAssertNil(strongself, snapshot);
         MGLTestAssert(strongself,
-                      ([error.domain isEqualToString:MGLErrorDomain] && error.code == MGLErrorCodeSnapshotUserCancelled),
+                      ([error.domain isEqualToString:MGLErrorDomain] && error.code == MGLErrorCodeSnapshotFailed),
                       @"Should have been cancelled");
+        MGLTestAssert(strongself, snapshotter.cancelled, @"Should have been cancelled");
         [expectation fulfill];
     }];
 
diff --git a/platform/ios/ios.xcodeproj/project.pbxproj b/platform/ios/ios.xcodeproj/project.pbxproj
index 8d0db63705..5733475fbf 100644
--- a/platform/ios/ios.xcodeproj/project.pbxproj
+++ b/platform/ios/ios.xcodeproj/project.pbxproj
@@ -986,7 +986,7 @@
 		927FBCFA1F4DAA8300F8BF1F /* MBXSnapshotsViewController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MBXSnapshotsViewController.h; sourceTree = "<group>"; };
 		927FBCFB1F4DAA8300F8BF1F /* MBXSnapshotsViewController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MBXSnapshotsViewController.m; sourceTree = "<group>"; };
 		927FBCFD1F4DB05500F8BF1F /* MGLMapSnapshotter.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLMapSnapshotter.h; sourceTree = "<group>"; };
-		927FBCFE1F4DB05500F8BF1F /* MGLMapSnapshotter.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MGLMapSnapshotter.mm; sourceTree = "<group>"; };
+		927FBCFE1F4DB05500F8BF1F /* MGLMapSnapshotter.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MGLMapSnapshotter.mm; sourceTree = "<group>"; wrapsLines = 0; };
 		92F2C3EC1F0E3C3A00268EC0 /* MGLRendererFrontend.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLRendererFrontend.h; sourceTree = "<group>"; };
 		92FC0AE7207CEE16007B6B54 /* MGLShapeOfflineRegion.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLShapeOfflineRegion.h; sourceTree = "<group>"; };
 		92FC0AE8207CEE16007B6B54 /* MGLShapeOfflineRegion_Private.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLShapeOfflineRegion_Private.h; sourceTree = "<group>"; };
diff --git a/platform/macos/CHANGELOG.md b/platform/macos/CHANGELOG.md
index 503685ac72..8937e828a4 100644
--- a/platform/macos/CHANGELOG.md
+++ b/platform/macos/CHANGELOG.md
@@ -7,6 +7,8 @@
 * Fixed inconsistencies in exception naming. ([#12583](https://github.com/mapbox/mapbox-gl-native/issues/12583))
 * Added `MGLShapeOfflineRegion` for defining arbitrarily shaped offline regions [#11447](https://github.com/mapbox/mapbox-gl-native/pull/11447)
 * Added an `-[MGLMapViewDelegate mapView:shapeAnnotationIsEnabled:]` method to specify whether an annotation is selectable. ([#12352](https://github.com/mapbox/mapbox-gl-native/pull/12352))
+* Fixed bug where completion block passed to `-[MGLMapSnapshotter startWithQueue:completionHandler:]` was not being called in all code paths. ([#12355](https://github.com/mapbox/mapbox-gl-native/pull/12355))
+
 
 # 0.10.0 - August 15, 2018
 
@@ -42,8 +44,6 @@
 * Fixed an issue where `-[MGLMapShapshot pointForCoordinate:]` returned incorrect points. ([#12221](https://github.com/mapbox/mapbox-gl-native/pull/12221))
 * Improved caching performance. ([#12072](https://github.com/mapbox/mapbox-gl-native/pull/12072))
 * Remove unnecessary memory use when collision debug mode is disabled. ([#12294](https://github.com/mapbox/mapbox-gl-native/issues/12294))
-* Changed `-[MGLMapSnapshotter cancel]` to call the completion block (originally passed to `-[MGLMapSnapshotter startWithQueue:completionHandler:]`) with an `NSError` (error code `MGLErrorCodeSnapshotUserCancelled`) if the snapshot has not finished. ([#12355](https://github.com/mapbox/mapbox-gl-native/pull/12355))
-
 
 ## 0.8.0 - June 20, 2018