From 323d4cb17a83266f50df4472884af095b6b88fcb Mon Sep 17 00:00:00 2001 From: Julian Rex Date: Wed, 29 Aug 2018 12:11:21 -0400 Subject: Removed redundant `loading` property. Added (currently private) `cancelled` property. --- platform/darwin/src/MGLMapSnapshotter.h | 3 --- platform/darwin/src/MGLMapSnapshotter.mm | 20 +++++++++----------- platform/darwin/src/MGLTypes.h | 4 +--- platform/ios/CHANGELOG.md | 1 + .../Snapshotter Tests/MGLMapSnapshotterTest.m | 13 +++++++++++-- platform/ios/ios.xcodeproj/project.pbxproj | 2 +- platform/macos/CHANGELOG.md | 4 ++-- 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 = ""; }; 927FBCFB1F4DAA8300F8BF1F /* MBXSnapshotsViewController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MBXSnapshotsViewController.m; sourceTree = ""; }; 927FBCFD1F4DB05500F8BF1F /* MGLMapSnapshotter.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLMapSnapshotter.h; sourceTree = ""; }; - 927FBCFE1F4DB05500F8BF1F /* MGLMapSnapshotter.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MGLMapSnapshotter.mm; sourceTree = ""; }; + 927FBCFE1F4DB05500F8BF1F /* MGLMapSnapshotter.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MGLMapSnapshotter.mm; sourceTree = ""; wrapsLines = 0; }; 92F2C3EC1F0E3C3A00268EC0 /* MGLRendererFrontend.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLRendererFrontend.h; sourceTree = ""; }; 92FC0AE7207CEE16007B6B54 /* MGLShapeOfflineRegion.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLShapeOfflineRegion.h; sourceTree = ""; }; 92FC0AE8207CEE16007B6B54 /* MGLShapeOfflineRegion_Private.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLShapeOfflineRegion_Private.h; sourceTree = ""; }; 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 -- cgit v1.2.1