-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[ios, macos] Improve snap shotter documentation. #10020
Changes from 1 commit
5213133
64e0cc2
d16aea7
dff1bc0
b767c21
6ea1e41
6beeb87
9eaed01
21fe811
a072ecc
73f84e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,40 +12,46 @@ MGL_EXPORT | |
@interface MGLMapSnapshotOptions : NSObject | ||
|
||
/** | ||
Creates a set of options with the minimum required information | ||
@param styleURL the style url to use | ||
@param camera the camera settings | ||
@param size the image size | ||
Creates a set of options with the minimum required information. | ||
|
||
@param styleURL URL of the map style to snapshot. The URL may be a full HTTP or HTTPS URL, | ||
a Mapbox URL indicating the style’s map ID (mapbox://styles/{user}/{style}), or a path | ||
to a local file relative to the application’s resource path. Specify nil for the default style. | ||
@param size The image size. | ||
*/ | ||
- (instancetype)initWithStyleURL:(NSURL*)styleURL camera:(MGLMapCamera*)camera size:(CGSize)size; | ||
- (instancetype)initWithStyleURL:(nullable NSURL*)styleURL camera:(MGLMapCamera*)camera size:(CGSize)size; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Let’s take this opportunity to add spaces before |
||
|
||
#pragma mark - Configuring the map | ||
#pragma mark - Configuring the Map | ||
|
||
/** | ||
The style URL for these options. | ||
URL of the map style to snapshot. The URL may be a full HTTP or HTTPS URL, | ||
a Mapbox URL indicating the style’s map ID (mapbox://styles/{user}/{style}), or a path | ||
to a local file relative to the application’s resource path. Specify nil for the default style. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This property is read-only, so it isn’t possible for the developer to specify anything. In fact, if the developer passes |
||
*/ | ||
@property (nonatomic, readonly) NSURL* styleURL; | ||
@property (nonatomic, readonly) NSURL *styleURL; | ||
|
||
/** | ||
The zoom. Default is 0. | ||
The default zoom level is 0. Overrides the altitude in the mapCamera options if set. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
*/ | ||
@property (nonatomic) double zoom; | ||
@property (nonatomic) double zoomLevel; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It sure would be nice to implement #5583 instead of offering three different ways to set the viewport on a snapshot that interact in unintuitive ways. |
||
|
||
/** | ||
The `MGLMapcamera` options to use. | ||
A camera representing the viewport visible in the snapshot. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
*/ | ||
@property (nonatomic) MGLMapCamera* camera; | ||
@property (nonatomic) MGLMapCamera *camera; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is |
||
|
||
/** | ||
A region to capture. Overrides the center coordinate | ||
in the mapCamera options if set | ||
The cooordinate rectangle that encompasses the bounds to capture. Overrides the center coordinate | ||
in the mapCamera options if set. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What constitutes an unset MGLCoordinateBounds? A bounds with no area, or does it have to also be centered on Null Island? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A bounds with no area, if coordinateBounds are not set it will use mapCamera. Would you like to complement this sentence to make this more clear?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
*/ | ||
@property (nonatomic) MGLCoordinateBounds region; | ||
@property (nonatomic) MGLCoordinateBounds coordinateBounds; | ||
|
||
#pragma mark - Configuring the image | ||
#pragma mark - Configuring the Image | ||
|
||
/** | ||
The size of the output image. Minimum is 64x64 | ||
The size of the output image. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per #10088 (comment), this property is actually intended to be measured in points (even though the minimum is in pixels).
|
||
|
||
The image may be no less than 64 pixels wide and no less than 64 pixels tall. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, let’s clarify that we really mean pixels here, because most developers will read this thinking it’s a typo:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ultimately we should handle the conversion ourselves so that developers don’t have to multiply by the backing scale factor, which is inconvenient and inconsistent with MapKit: #10088. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Internally we handle the scale factor so size is actually in points instead of pixels in the end. Do you think this clarifies the issue?
|
||
*/ | ||
@property (nonatomic, readonly) CGSize size; | ||
|
||
|
@@ -57,18 +63,26 @@ MGL_EXPORT | |
|
||
@end | ||
|
||
#if TARGET_OS_IPHONE | ||
/** | ||
A block to processes the result or error of a snapshot request. | ||
|
||
The result will be either an `MGLImage` or a `NSError` | ||
@param snapshot The `UIImage` that was generated or `nil` if an error occurred. | ||
@param error The error that occured or `nil` when succesful. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo: successful. (×2) |
||
*/ | ||
typedef void (^MGLMapSnapshotCompletionHandler)(UIImage* _Nullable snapshot, NSError* _Nullable error); | ||
#else | ||
/** | ||
A block to processes the result or error of a snapshot request. | ||
|
||
@param snapshot The image that was generated or `nil` if an error occurred. | ||
@param snapshot The `NSImage` that was generated or `nil` if an error occurred. | ||
@param error The eror that occured or `nil` when succesful. | ||
*/ | ||
typedef void (^MGLMapSnapshotCompletionHandler)(MGLImage* _Nullable snapshot, NSError* _Nullable error); | ||
typedef void (^MGLMapSnapshotCompletionHandler)(NSImage* _Nullable snapshot, NSError* _Nullable error); | ||
#endif | ||
|
||
/** | ||
A utility object for capturing map-based images. | ||
An immutable utility object for capturing map-based images. | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As tail work, it would be helpful to add a small code example of working with MGLMapSnapshotter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tail work ticket in our ios-sdk repo https://github.com/mapbox/ios-sdk/issues/332 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was referring to a short code example here inside the header, similar to what we do for classes like MGLShapeSource and MGLMapView, so that the example shows up at the beginning of the MGLMapSnapshotter class reference. |
||
MGL_EXPORT | ||
@interface MGLMapSnapshotter : NSObject | ||
|
@@ -92,6 +106,9 @@ MGL_EXPORT | |
|
||
/** | ||
Cancels the snapshot creation request, if any. | ||
|
||
Once you call this method, you cannot resume the snapshot. In order to obtain the | ||
snapshot, create a new `MGLMapSnapshotter` object. | ||
*/ | ||
- (void)cancel; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,19 +13,26 @@ | |
#import "MGLOfflineStorage_Private.h" | ||
#import "MGLGeometry_Private.h" | ||
#import "NSBundle+MGLAdditions.h" | ||
#import "MGLStyle.h" | ||
|
||
#if TARGET_OS_IPHONE | ||
#import "UIImage+MGLAdditions.h" | ||
#else | ||
#import "NSImage+MGLAdditions.h" | ||
#endif | ||
|
||
const CGPoint MGLLogoImagePosition = CGPointMake(8, 8); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be interesting to make this position configurable as tail work. |
||
|
||
@implementation MGLMapSnapshotOptions | ||
|
||
- (instancetype _Nonnull)initWithStyleURL:(NSURL* _Nonnull)styleURL camera:(MGLMapCamera*)camera size:(CGSize) size; | ||
- (instancetype _Nonnull)initWithStyleURL:(nullable NSURL*)styleURL camera:(MGLMapCamera*)camera size:(CGSize) size; | ||
{ | ||
self = [super init]; | ||
if (self) { | ||
if ( !styleURL) | ||
{ | ||
styleURL = [MGLStyle streetsStyleURLWithVersion:MGLStyleDefaultVersion]; | ||
} | ||
_styleURL = styleURL; | ||
_size = size; | ||
_camera = camera; | ||
|
@@ -73,17 +80,17 @@ - (instancetype)initWithOptions:(MGLMapSnapshotOptions*)options; | |
cameraOptions.center = MGLLatLngFromLocationCoordinate2D(options.camera.centerCoordinate); | ||
} | ||
cameraOptions.angle = MAX(0, options.camera.heading) * mbgl::util::DEG2RAD; | ||
cameraOptions.zoom = MAX(0, options.zoom); | ||
cameraOptions.zoom = MAX(0, options.zoomLevel); | ||
cameraOptions.pitch = MAX(0, options.camera.pitch); | ||
|
||
// Region | ||
mbgl::optional<mbgl::LatLngBounds> region; | ||
if (!MGLCoordinateBoundsIsEmpty(options.region)) { | ||
region = MGLLatLngBoundsFromCoordinateBounds(options.region); | ||
mbgl::optional<mbgl::LatLngBounds> coordinateBounds; | ||
if (!MGLCoordinateBoundsIsEmpty(options.coordinateBounds)) { | ||
coordinateBounds = MGLLatLngBoundsFromCoordinateBounds(options.coordinateBounds); | ||
} | ||
|
||
// Create the snapshotter | ||
mbglMapSnapshotter = std::make_unique<mbgl::MapSnapshotter>(*mbglFileSource, *mbglThreadPool, styleURL, size, pixelRatio, cameraOptions, region); | ||
mbglMapSnapshotter = std::make_unique<mbgl::MapSnapshotter>(*mbglFileSource, *mbglThreadPool, styleURL, size, pixelRatio, cameraOptions, coordinateBounds); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
return self; | ||
} | ||
|
@@ -96,7 +103,8 @@ - (void)startWithCompletionHandler:(MGLMapSnapshotCompletionHandler)completion; | |
- (void)startWithQueue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshotCompletionHandler)completion; | ||
{ | ||
if ([self isLoading]) { | ||
NSDictionary *userInfo = @{NSLocalizedDescriptionKey: @"Already started this snapshotter"}; | ||
NSString *errorMessage = NSLocalizedStringWithDefaultValue(@"ALREADY_STARTED_SNAPSHOTTER", nil, nil, @"Already started this snapshotter", "User-friendly error description"); | ||
NSDictionary *userInfo = @{NSLocalizedDescriptionKey: errorMessage}; | ||
NSError *error = [NSError errorWithDomain:MGLErrorDomain code:1 userInfo:userInfo]; | ||
dispatch_async(queue, ^{ | ||
completion(nil, error); | ||
|
@@ -112,7 +120,7 @@ - (void)startWithQueue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshot | |
if (mbglError) { | ||
NSString *description = @(mbgl::util::toString(mbglError).c_str()); | ||
NSDictionary *userInfo = @{NSLocalizedDescriptionKey: description}; | ||
NSError *error = [NSError errorWithDomain:MGLErrorDomain code:1 userInfo:userInfo]; | ||
NSError *error = [NSError errorWithDomain:MGLErrorDomain code:MGLErrorCodeSnapshotFailed userInfo:userInfo]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per #10020 (comment), should we remove this error or replace it with an NSException? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it makes sense let the developer know that it has a snapshotter task running it's easy to forget this. A NSException is too drastic, I would like to continue with the NSError for the time being and evaluate this again once we made the snapshotter reusable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To elaborate, NSError is intended to problems outside the developer’s control, for issues that can’t be fixed in the developer’s own code. If the developer is reusing a snapshotter in their own code, I think that would be an error on their part and an appropriate use of NSException. But if MapKit simply sets the NSError in this case, then I guess it’s OK to follow their lead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with you NSError is intended to problems outside the developer's control. NSException is a better choice. Making the changes. |
||
|
||
// Dispatch result to origin queue | ||
dispatch_async(queue, ^{ | ||
|
@@ -130,7 +138,7 @@ - (void)startWithQueue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshot | |
UIGraphicsBeginImageContext(mglImage.size); | ||
|
||
[mglImage drawInRect:CGRectMake(0, 0, mglImage.size.width, mglImage.size.height)]; | ||
[logoImage drawInRect:CGRectMake(8, mglImage.size.height - (8 + logoImage.size.height), logoImage.size.width,logoImage.size.height)]; | ||
[logoImage drawInRect:CGRectMake(MGLLogoImagePosition.x, mglImage.size.height - (MGLLogoImagePosition.y + logoImage.size.height), logoImage.size.width,logoImage.size.height)]; | ||
UIImage *compositedImage = UIGraphicsGetImageFromCurrentImageContext(); | ||
|
||
UIGraphicsEndImageContext(); | ||
|
@@ -139,7 +147,7 @@ - (void)startWithQueue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshot | |
NSImage *compositedImage = mglImage; | ||
|
||
[compositedImage lockFocus]; | ||
[logoImage drawInRect:CGRectMake(8, 8, logoImage.size.width,logoImage.size.height)]; | ||
[logoImage drawInRect:CGRectMake(MGLLogoImagePosition.x, MGLLogoImagePosition.y, logoImage.size.width,logoImage.size.height)]; | ||
[compositedImage unlockFocus]; | ||
#endif | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,7 +163,7 @@ - (IBAction)takeSnapshot:(id)sender { | |
MGLMapCamera *camera = self.mapView.camera; | ||
|
||
MGLMapSnapshotOptions* options = [[MGLMapSnapshotOptions alloc] initWithStyleURL:self.mapView.styleURL camera:camera size:self.mapView.bounds.size]; | ||
options.zoom = self.mapView.zoomLevel; | ||
options.zoomLevel = self.mapView.zoomLevel; | ||
|
||
// Create and start the snapshotter | ||
snapshotter = [[MGLMapSnapshotter alloc] initWithOptions:options]; | ||
|
@@ -177,6 +177,7 @@ - (IBAction)takeSnapshot:(id)sender { | |
|
||
// Set the default name for the file and show the panel. | ||
NSSavePanel* panel = [NSSavePanel savePanel]; | ||
|
||
[panel setNameFieldStringValue:newName]; | ||
[panel beginSheetModalForWindow:window completionHandler:^(NSInteger result){ | ||
if (result == NSFileHandlingPanelOKButton) { | ||
|
@@ -196,16 +197,24 @@ - (IBAction)takeSnapshot:(id)sender { | |
|
||
} | ||
|
||
NSString *extension = [[fileURL pathExtension] lowercaseString]; | ||
NSBitmapImageFileType fileType; | ||
if ([extension isEqualToString:@"png"]) { | ||
NSString *uti; | ||
[fileURL getResourceValue:&uti forKey:NSURLTypeIdentifierKey error:nil]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turns out this doesn’t work because no file has been laid down by this point. This code ends up producing a TIFF no matter what extension is specified. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in #10090. |
||
NSBitmapImageFileType fileType = NSTIFFFileType; | ||
|
||
if (UTTypeConformsTo((__bridge CFStringRef)uti, kUTTypePNG)) { | ||
fileType = NSPNGFileType; | ||
} else if ([extension isEqualToString:@"gif"]) { | ||
fileType = NSGIFFileType; | ||
} else if ([extension isEqualToString:@"jpg"] || [extension isEqualToString:@"jpeg"]) { | ||
fileType = NSJPEGFileType; | ||
} else { | ||
fileType = NSTIFFFileType; | ||
} | ||
if (UTTypeConformsTo((__bridge CFStringRef)uti, kUTTypeGIF)) { | ||
fileType = NSGIFFileType; | ||
} | ||
if (UTTypeConformsTo((__bridge CFStringRef)uti, kUTTypeJPEG)) { | ||
fileType = NSJPEGFileType; | ||
} | ||
if (UTTypeConformsTo((__bridge CFStringRef)uti, kUTTypeJPEG2000)) { | ||
fileType = NSJPEGFileType; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in #10090. |
||
} | ||
if (UTTypeConformsTo((__bridge CFStringRef)uti, kUTTypeBMP)) { | ||
fileType = NSBitmapImageFileTypeBMP; | ||
} | ||
|
||
NSData *imageData = [bitmapRep representationUsingType:fileType properties:@{}]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enclose
mapbox://styles/{user}/{style}
andnil
in backticks.