[ios, macos] Improve snap shotter documentation. #10020

fabian-guerra · 2017-09-18T21:07:37Z

Update documentation.
Improve snapshoter code (localization, error handling, naming).

boundsj · 2017-09-20T20:00:20Z

platform/darwin/src/MGLMapSnapshotter.h


 /**
- The size of the output image. Minimum is 64x64
+ The size of the output image. Minimum is 64x64.


#9891 (comment) asks if this is points or pixels. My reading of the implementation is that it is pixels. We should add that context here.

boundsj

#9891 (comment) asks if the cancel method can be called more than once on the same instance of snapshotter. It appears to me that it can be and that N+1 calls would not cause any problem but I've not actually tested. I agree we should note that multiple calls are/are not a problem.

fabian-guerra · 2017-09-21T16:15:03Z

Tail work #10038 for making snapshotter reusable.

boundsj

This lgtm 👍

@1ec5 do you think there is anything else here we must do for v3.7.0?

1ec5 · 2017-09-23T01:23:53Z

platform/darwin/src/MGLMapSnapshotter.h

@@ -12,40 +12,41 @@ MGL_EXPORT
 @interface MGLMapSnapshotOptions : NSObject

 /**
- Creates a set of options with the minimum required information
+ Creates a set of options with the minimum required information.
+
 @param styleURL the style url to use


Parameter documentation should start with a capital letter and end with a period. URL should be capitalized. Note that this differs from the prevailing code style in the Android SDK.

This is based on the documentation for -[MGLMapView initWithFrame: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.

(Can local styles be snapshotted?)

/ref #9891 (comment)

1ec5 · 2017-09-23T01:27:01Z

platform/darwin/src/MGLMapSnapshotter.h

@@ -12,40 +12,41 @@ MGL_EXPORT
 @interface MGLMapSnapshotOptions : NSObject

 /**
- Creates a set of options with the minimum required information
+ 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
 */
 - (instancetype)initWithStyleURL:(NSURL*)styleURL camera:(MGLMapCamera*)camera size:(CGSize)size;


styleURL should be marked nullable, given the change to the implementation.

/ref #9891 (comment)

1ec5 · 2017-09-23T01:30:19Z

platform/darwin/src/MGLMapSnapshotter.h


 /**
- The zoom. Default is 0.
+ The zoom level. Default is 0.


Separate paragraph:

The default zoom level is 0.

1ec5 · 2017-09-23T01:32:07Z

platform/darwin/src/MGLMapSnapshotter.h

 */
-@property (nonatomic) double zoom;
+@property (nonatomic) double zoomLevel;

 /**
 The `MGLMapcamera` options to use.


Typo: MGLMapCamera. In any case, this description would make more sense:

A camera representing the viewport visible in the snapshot.

1ec5 · 2017-09-23T01:33:11Z

platform/darwin/src/MGLMapSnapshotter.h

 */
-@property (nonatomic) double zoom;
+@property (nonatomic) double zoomLevel;

 /**
 The `MGLMapcamera` options to use.


Like the coordinateBounds documentation, this comment should explain what happens if both it and coordinateBounds are set.

What happens if the camera specifies an altitude that differs from the zoomLevel property? What value ends up being preferred?

1ec5 · 2017-09-23T01:47:46Z

platform/darwin/src/MGLMapSnapshotter.mm

@@ -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_SNAPSHOTER", nil, nil, @"Already started this snapshotter", "User-friendly error description");


NSError is meant to be used for errors that are outside the developer’s control, and its localized error description may be displayed to the end user. But if a snapshotter can’t be reused and the developer tries to reuse one, then that’s avoidable programmer error, and we should assert instead of raising an error. If a snapshotter can be reused, then this error should be removed.

Either way, it does seem useful to retain an error parameter just in case, even if it goes unused for the time being.

1ec5 · 2017-09-23T01:48:14Z

platform/darwin/src/MGLTypes.h

@@ -47,6 +47,8 @@ typedef NS_ENUM(NSInteger, MGLErrorCode) {
 MGLErrorCodeParseStyleFailed = 4,
 /** An attempt to load the style failed. */
 MGLErrorCodeLoadStyleFailed = 5,
+ /** An error occurred while attempting to get the callback. */


An error occurred while snapshotting the map.

1ec5 · 2017-09-23T01:48:29Z

platform/darwin/src/MGLTypes.h

@@ -47,6 +47,8 @@ typedef NS_ENUM(NSInteger, MGLErrorCode) {
 MGLErrorCodeParseStyleFailed = 4,
 /** An attempt to load the style failed. */
 MGLErrorCodeLoadStyleFailed = 5,
+ /** An error occurred while attempting to get the callback. */
+ MGLErrorCodeSnapShotterCallbackFailed = 6,


MGLErrorCodeSnapshotFailed

1ec5 · 2017-09-23T01:52:57Z

platform/macos/app/MapDocument.m

+
+ for (NSString *imageType in [NSImage imageTypes]) {
+ if ([uti isEqualToString:imageType]) {
+ if ([uti containsString:@"png"]) {


public.png isn’t the only UTI that contains the substring png, and UTIs that don’t contain png may still be PNGs. Use UTTypeConformsTo() from the Core Services framework to perform this check:

UTTypeConformsTo((__bridge CFStringRef)uti, kUTTypePNG)

1ec5 · 2017-09-23T01:55:18Z

platform/macos/app/MapDocument.m

+ } else if ([uti containsString:@"gif"]) {
+ fileType = NSGIFFileType;
+ } else if ([uti containsString:@"jpeg"]) {
+ fileType = NSJPEGFileType;


NSBitmapImageFileTypeBMP and NSBitmapImageFileTypeJPEG2000 are also available.

1ec5

In addition to the below, #9891 (comment) points out that the menu item should be in title case, not sentence case.

1ec5 · 2017-09-29T05:27:33Z

platform/darwin/src/MGLMapSnapshotter.h

- The size of the output image. Minimum is 64x64
+ The size of the output image.
+
+ The image may be no less than 64 pixels wide and no less than 64 pixels tall.


OK, let’s clarify that we really mean pixels here, because most developers will read this thinking it’s a typo:

The image may be no less than 64 pixels wide (32 points wide on a 2× display) and no less than 64 pixels tall.

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.

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?

The size of the output image may be no less than the following dimensions:

Non-retina 64 points wide and tall.
2× 32 points wide and tall.
3× 22 points wide and tall.

1ec5 · 2017-09-29T05:29:35Z

platform/darwin/src/MGLMapSnapshotter.h

- 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.


The cooordinate rectangle that encompasses the bounds to capture.

If this property is non-empty and the camera property is non-nil, the camera’s center coordinate and altitude are ignored in favor of this property’s value.

1ec5 · 2017-09-29T05:31:29Z

platform/darwin/src/MGLMapSnapshotter.h


 /**
- The `MGLMapcamera` options to use.
+ A camera representing the viewport visible in the snapshot.


A camera representing the viewport visible in the snapshot.

If this property is non-nil and the coordinateBounds property is set to a non-empty coordinate bounds, the camera’s center coordinate and altitude are ignored in favor of the coordinateBounds property.

1ec5 · 2017-09-29T05:33:55Z

platform/darwin/src/MGLMapSnapshotter.h


 /**
- The zoom. Default is 0.
+ The default zoom level is 0. Overrides the altitude in the mapCamera options if set.


The zoom level.

The default zoom level is 0. If this property is non-zero and the camera property is non-nil, the camera’s altitude is ignored in favor of this property’s value.

1ec5 · 2017-09-29T05:34:09Z

platform/darwin/src/MGLMapSnapshotter.h

 */
-@property (nonatomic) double zoom;
+@property (nonatomic) double zoomLevel;


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.

1ec5 · 2017-09-29T05:39:08Z

platform/darwin/src/MGLMapSnapshotter.h

 /**
 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.


Typo: successful. (×2)

1ec5 · 2017-09-29T05:40:16Z

platform/darwin/src/MGLMapSnapshotter.h


 /**
- A utility object for capturing map-based images.
+ An immutable utility object for capturing map-based images.
 */


As tail work, it would be helpful to add a small code example of working with MGLMapSnapshotter.

Tail work ticket in our ios-sdk repo https://github.com/mapbox/ios-sdk/issues/332

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.

1ec5 · 2017-09-29T05:43:22Z

platform/darwin/src/MGLMapSnapshotter.mm

@@ -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];


Per #10020 (comment), should we remove this error or replace it with an NSException?

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.

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.

I agree with you NSError is intended to problems outside the developer's control. NSException is a better choice. Making the changes.

1ec5 · 2017-09-29T05:44:18Z

platform/macos/app/MapDocument.m

+ fileType = NSJPEGFileType;
+ }
+ if (UTTypeConformsTo((__bridge CFStringRef)uti, kUTTypeJPEG2000)) {
+ fileType = NSJPEGFileType;


kUTTypeJPEG2000 translates to NSJPEG2000FileType, not NSJPEGFileType.

Fixed in #10090.

1ec5 · 2017-09-29T05:53:03Z

platform/darwin/src/MGLMapSnapshotter.mm

 }

 // 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);


mbglMapSnapshotter and the other ivars on MGLMapSnapshotter should begin with underscores.

1ec5 · 2017-09-29T06:29:22Z

platform/macos/app/MapDocument.m

- NSBitmapImageFileType fileType;
- if ([extension isEqualToString:@"png"]) {
+ NSString *uti;
+ [fileURL getResourceValue:&uti forKey:NSURLTypeIdentifierKey error:nil];


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.

Fixed in #10090.

1ec5 · 2017-10-02T19:34:47Z

platform/darwin/src/MGLMapSnapshotter.mm

 mbgl::Size size = {
- static_cast<uint32_t>(MAX(options.size.width, 64)),
- static_cast<uint32_t>(MAX(options.size.height, 64))
+ static_cast<uint32_t>(MAX(options.size.width, MGLSnapshotterMinimumPixelSize/options.scale)),


If the developer sets a scale other than the device’s native scale, does the effective minimum pixel size change?

frederoni · 2017-10-02T23:48:09Z

platform/darwin/src/MGLMapSnapshotter.mm


 // Process image watermark in a work queue
 dispatch_queue_t workQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
 dispatch_async(workQueue, ^{
 #if TARGET_OS_IPHONE
 UIImage *logoImage = [UIImage imageNamed:@"mapbox" inBundle:[NSBundle mgl_frameworkBundle] compatibleWithTraitCollection:nil];

- UIGraphicsBeginImageContext(mglImage.size);
+ UIGraphicsBeginImageContextWithOptions(mglImage.size, NO, [UIScreen mainScreen].scale);


We shouldn't expose a scale property then override it with [UIScreen mainScreen].scale. Either remove the scale or grab the scale from the options.

I fixed it in you branch and going to cp into mine https://github.com/mapbox/mapbox-gl-native/pull/10105/files#diff-56bc5c1e41273957efd873aa83e6c522R139

1ec5 · 2017-10-03T05:16:19Z

platform/darwin/src/MGLMapSnapshotter.h


 /**
- The size of the output image. Minimum is 64x64
+ The size of the output image.


Per #10088 (comment), this property is actually intended to be measured in points (even though the minimum is in pixels).

The size of the output image, measured in points.

…ted.

1ec5

This is good to go, other than the exception name.

1ec5 · 2017-10-03T23:49:28Z

platform/darwin/src/MGLMapSnapshotter.mm

- completion(nil, error); 
- });
- return;
+ [NSException raise:@"MGLAlreadyStartedSnapshotterException"


Per #7258, use one of Foundation's predefined exception names or pull this string out into a public constant.

fabian-guerra force-pushed the fabian-snapshoter-9920 branch from 126c793 to 187ee23 Compare September 19, 2017 23:20

boundsj reviewed Sep 20, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-snapshoter-9920 branch from 187ee23 to d3cfe72 Compare September 20, 2017 20:02

boundsj reviewed Sep 20, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-snapshoter-9920 branch from d3cfe72 to 95fda11 Compare September 21, 2017 16:12

fabian-guerra requested review from frederoni and 1ec5 September 21, 2017 16:13

fabian-guerra self-assigned this Sep 21, 2017

boundsj approved these changes Sep 22, 2017

View reviewed changes

fabian-guerra added this to the ios-v3.7.0 milestone Sep 22, 2017

fabian-guerra force-pushed the fabian-snapshoter-9920 branch from 95fda11 to 4b2a93c Compare September 22, 2017 20:33

1ec5 reviewed Sep 23, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-snapshoter-9920 branch 3 times, most recently from 7cc01ab to 0895f64 Compare September 28, 2017 16:49

[ios, macos] Improve snap shotter documentation.

5213133

fabian-guerra force-pushed the fabian-snapshoter-9920 branch from 0895f64 to 5213133 Compare September 28, 2017 19:40

1ec5 reviewed Sep 29, 2017

View reviewed changes

1ec5 mentioned this pull request Sep 29, 2017

Snapshotter scales incorrectly #10089

Closed

1ec5 and others added 3 commits September 29, 2017 12:00

[macos] Save snapshots in correct format

64e0cc2

[macos] Renamed snapshot item to Export Image

d16aea7

[ios, macos] Clarify Snapshotter documentation.

dff1bc0

1ec5 reviewed Oct 2, 2017

View reviewed changes

frederoni mentioned this pull request Oct 2, 2017

[ios, macos] Fix snapshot scale #10105

Merged

frederoni and others added 2 commits October 2, 2017 18:39

[ios] Fix snapshot scale

b767c21

[macOS] Fix snapshotter 4x scaling.

6ea1e41

frederoni reviewed Oct 2, 2017

View reviewed changes

[ios] Fix snapshotter final image scale.

6beeb87

1ec5 reviewed Oct 3, 2017

View reviewed changes

fabian-guerra added 2 commits October 3, 2017 11:31

[ios, macos] Update snapshotter size documentation.

9eaed01

[ios, macos] Throw an exception when the snapshotter has already star…

21fe811

…ted.

friedbunny added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS labels Oct 3, 2017

1ec5 approved these changes Oct 3, 2017

View reviewed changes

fabian-guerra added 2 commits October 3, 2017 20:29

[ios, macos] Add snapshotter header example.

a072ecc

[ios, macos] Use one of the predefined Foundation's exception names.

73f84e7

fabian-guerra merged commit f01588c into master Oct 4, 2017

fabian-guerra deleted the fabian-snapshoter-9920 branch October 4, 2017 14:03

[ios, macos] Improve snap shotter documentation. #10020

[ios, macos] Improve snap shotter documentation. #10020

Conversation

fabian-guerra commented Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

boundsj left a comment

Choose a reason for hiding this comment

fabian-guerra commented Sep 21, 2017

boundsj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabian-guerra Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabian-guerra commented Sep 18, 2017 •

edited

Loading

fabian-guerra Sep 29, 2017 •

edited

Loading

frederoni Oct 2, 2017 •

edited

Loading