iOS/macOS snapshotter by ivovandongen · Pull Request #9891 · mapbox/mapbox-gl-native

ivovandongen · 2017-08-30T15:46:08Z

iOS/macOS companion for #9748

TODO:

Work out options class
Extend test view
Documentation

frederoni · 2017-09-02T09:27:51Z

+ @param mapCamera the camera settings
+ @param size the image size
+ */
+- (instancetype )initWithStyleURL:(NSURL* )styleURL mapCamera:(MGLMapCamera*) mapCamera size:(CGSize) size;


Nit: Inconsistent formatting. (only spaces before *)
Drop map from mapCamera. (×3)

frederoni · 2017-09-02T09:30:02Z

+@property (nonatomic) MGLMapCamera* mapCamera;
+
+/**
+ (Optionally) a region to capture. Overrides the center coordinate


Let's drop optional since region is ~~invalid~~ empty by default and Obj-C doesn't support optional structs.

frederoni · 2017-09-02T09:31:27Z

+/**
+ A block to processes the result or error of a snapshot request.
+
+ The result will be either an `MGLImage*` or a `NSError*`


Nit: s/MGLImage*/MGLImage/NSError*/NSError (I don't think jazzy will link otherwise)

frederoni · 2017-09-02T09:34:43Z

+- (void)cancel;
+
+/**
+ Indicates wether as snapshot is currently being generated.


typo: s/wether/whether

frederoni · 2017-09-02T09:42:52Z

+    [self startWithQueue:dispatch_get_main_queue() completionHandler:completion];
+}
+
+- (void)startWithQueue:(dispatch_queue_t)queue completionHandler: (MGLMapSnapshotCompletionHandler)completion;


This is slightly confusing. startWithQueue implies that we generate the snapshot on the given queue but here we're calling the completion on the given queue. This is usually called handlerQueue or delegateQueue.
I think we should dispatch to the given queue before handing off to core so we can decide if we want to snapshot in parallel or serial.

frederoni · 2017-09-02T09:47:06Z

+    std::unique_ptr<mbgl::MapSnapshotter> mbglMapSnapshotter;
+    std::unique_ptr<mbgl::Actor<mbgl::MapSnapshotter::Callback>> snapshotCallback;
+
+    BOOL loading;


Nit: loading is already synthesized. This can be dropped. Use _loading to bypass readonly when you set the variable.

frederoni · 2017-09-02T09:47:25Z

+    mbglMapSnapshotter.reset();
+}
+
+-(BOOL)isLoading;


Nit: already synthesized. Drop this method.

ivovandongen · 2017-09-05T07:56:15Z

@frederoni Thanks for the review! I've fixed up the comments and made sure the work is now all done on the provided queue (or the main queue).

I also added some code to add the logo on the image in a work queue (https://github.com/mapbox/mapbox-gl-native/pull/9891/files#diff-56bc5c1e41273957efd873aa83e6c522R116). Could you check if that is the right queue to do this on?

1ec5

Thanks for implementing this feature so swiftly! It’s certainly going to be well received. I left some comments below that the team will need to address as tail work, ideally before the first v3.7.0 beta.

In addition to the comments below, we’ll probably want to add MGLMapSnapshotter and MGLMapSnapshotOptions to the jazzy table of contents in jazzy.yml and update the changelog to mention the new feature, both for iOS and macOS.

1ec5 · 2017-09-05T22:05:24Z

+ */
+- (instancetype)initWithStyleURL:(NSURL*)styleURL camera:(MGLMapCamera*)camera size:(CGSize)size;
+
+#pragma mark - Configuring the map


For our public headers, we use Title Case in marks, since jazzy uses the marks to produce section headings in the generated documentation. We could move to sentence case for that, but it would be nice to do it consistently across all the headers when we make that decision.

1ec5 · 2017-09-05T22:06:05Z

+/**
+  The style URL for these options.
+ */
+@property (nonatomic, readonly) NSURL* styleURL;


FYI, the predominant style in our Objective-C code is to place the space before the asterisk. I realize this is different than the predominant style for our C++ code.

1ec5 · 2017-09-05T22:07:23Z

+/**
+ The zoom. Default is 0.
+ */
+@property (nonatomic) double zoom;


The iOS/macOS APIs consistently use the term “zoom level” rather than “zoom” for this concept. The main reason is that “zoom” is also a verb; in Objective-C, [mapView zoom] looks like a call to an action method rather than a reference to a property getter.

1ec5 · 2017-09-05T22:09:42Z

+ A region to capture. Overrides the center coordinate
+ in the mapCamera options if set
+ */
+@property (nonatomic) MGLCoordinateBounds region;


Most of the API uses the term “coordinate bounds” instead of “region”, although there is some holdover from MapKit in the delegate methods. It’s a confusing situation overall, but I think it would be slightly less confusing to use coordinateBounds here, to align with MGLMapView.visibleCoordinateBounds.

1ec5 · 2017-09-05T22:09:59Z

+#pragma mark - Configuring the image
+
+/**
+ The size of the output image. Minimum is 64x64


Is this size measured in points or pixels?

1ec5 · 2017-09-05T22:16:50Z

+    MGLMapCamera* mapCamera = [[MGLMapCamera alloc] init];
+    mapCamera.pitch = 20;
+    mapCamera.centerCoordinate = coordinates;
+    MGLMapSnapshotOptions* options = [[MGLMapSnapshotOptions alloc] initWithStyleURL:[NSURL URLWithString:@"mapbox://styles/mapbox/traffic-day-v2"] camera:mapCamera size:CGSizeMake(imageView.frame.size.width, imageView.frame.size.height)];


MGLStyle has factory methods for style URLs. We use them whenever possible.

In this case the traffic style is deprecated, even tho it has not yet merged into master. Our recommendation is create an URL with this string mapbox://styles/mapbox/traffic-day-v2.

In this case, the demo doesn’t depend on the traffic congestion information displayed by the Traffic Day style, so I’d recommend choosing a non-deprecated method like +[MGLStyle satelliteStreets].

1ec5 · 2017-09-05T22:43:13Z

+
+                    }
+
+                    NSString *extension = [[fileURL pathExtension] lowercaseString];


File name extensions aren’t especially reliable on Apple platforms, since a file’s canonical type is represented by a UTI. The save panel even lets you save without an extension unless you restrict the allowed file types to those that can be represented as images:

panel.allowedFileTypes = [NSImage imageTypes];

Here’s a more robust way to go from a file extension to a UTI.

An alternative to looking at file extensions:

The save panel tells us the file path. use NSFileManager to determine the UTI at that path. (Not sure if this only works for existing files or if it would work for unsaved files as well.)

Use UTTypeConformsTo() to determine whether the UTI at that path conforms to public.jpeg etc.

Avoid hard-coding UTIs like public.jpeg by iterating over +[NSImage imageTypes].

1ec5 · 2017-09-05T22:45:25Z

+ @param camera the camera settings
+ @param size the image size
+ */
+- (instancetype)initWithStyleURL:(NSURL*)styleURL camera:(MGLMapCamera*)camera size:(CGSize)size;


Do we want to allow the style URL to be nil and default to Mapbox Streets, as MGLMapView and MGLOfflineStorage do?

@1ec5 could you please help me understand why defaulting to Mapbox streets is a good option.

When you create an MGLMapView or MGLOfflineRegion with a nil style URL, we default to the current version of Mapbox Streets:

mapbox-gl-native/platform/ios/src/MGLMapView.mm

Line 397 in 53d072c

styleURL = [MGLStyle streetsStyleURLWithVersion:MGLStyleDefaultVersion];

mapbox-gl-native/platform/darwin/src/MGLTilePyramidOfflineRegion.mm

Line 36 in 53d072c

styleURL = [MGLStyle streetsStyleURLWithVersion:MGLStyleDefaultVersion];

For consistency with these APIs, I think we should treat a nil style URL here the same way.

1ec5 · 2017-09-05T22:46:20Z

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


Nit: Start with a capital letter and end with a period. This makes things more consistent in the event that we have to cram more than one sentence in a parameter description, which isn’t uncommon.

1ec5 · 2017-09-05T22:47:53Z

+- (void)startWithQueue:(dispatch_queue_t)queue completionHandler:(MGLMapSnapshotCompletionHandler)completion;
+{
+    if ([self isLoading]) {
+        NSDictionary *userInfo = @{NSLocalizedDescriptionKey: @"Already started this snapshotter"};


This string should be localizable, since it can be presented to the user.

boundsj · 2017-09-05T23:10:42Z

update the changelog to mention the new feature

For iOS: #9919

1ec5 · 2017-09-05T23:24:36Z

Filed #9920 so we can better track the fit-and-finish detailed in #9891 (review).

1ec5 · 2017-09-20T20:08:19Z

                                    <action selector="revertDocumentToSaved:" target="-1" id="iJ3-Pv-kwq"/>
                                </connections>
                            </menuItem>
+                            <menuItem title="Save snapshot" id="vjX-0E-kLO">


Use title case on menu items.

ivovandongen added the iOS Mapbox Maps SDK for iOS label Aug 30, 2017

ivovandongen self-assigned this Aug 30, 2017

ivovandongen force-pushed the ivd-ios-snapshotter branch from 625715b to 3a9c31b Compare August 31, 2017 14:45

ivovandongen requested a review from frederoni August 31, 2017 15:55

ivovandongen force-pushed the ivd-ios-snapshotter branch 3 times, most recently from 4cfb29f to 28a84a7 Compare September 1, 2017 14:32

frederoni reviewed Sep 2, 2017

View reviewed changes

ivovandongen force-pushed the ivd-ios-snapshotter branch 2 times, most recently from 6a6957e to f4ee3fb Compare September 5, 2017 07:54

ivovandongen force-pushed the ivd-ios-snapshotter branch 2 times, most recently from 61159dd to 4ad3a56 Compare September 5, 2017 09:31

ivovandongen added 4 commits September 5, 2017 15:48

[ios] extend ui image additions with init from premultiplied image

b74470f

[darwin] snapshotter

9343978

[ios] snapshotter

a25d68b

[macos] snapshotter

3337f0c

ivovandongen force-pushed the ivd-ios-snapshotter branch from b6a46f4 to 3337f0c Compare September 5, 2017 12:54

ivovandongen added the macOS Mapbox Maps SDK for macOS label Sep 5, 2017

ivovandongen changed the title ~~iOS snapshotter~~ iOS/macOS snapshotter Sep 5, 2017

frederoni approved these changes Sep 5, 2017

View reviewed changes

ivovandongen merged commit d736429 into master Sep 5, 2017

ivovandongen deleted the ivd-ios-snapshotter branch September 5, 2017 13:29

frederoni mentioned this pull request Sep 5, 2017

Add support for runtime styling when snapshotting #9914

Closed

jmkiley mentioned this pull request Sep 5, 2017

Drag and drop example mapbox/ios-sdk-examples#110

Closed

friedbunny added this to the ios-v3.7.0 milestone Sep 5, 2017

1ec5 reviewed Sep 5, 2017

View reviewed changes

1ec5 mentioned this pull request Sep 5, 2017

Polish map snapshotter #9920

Closed

tobrun mentioned this pull request Sep 12, 2017

Add map snapshot API #1701

Closed

boundsj mentioned this pull request Sep 20, 2017

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

Merged

2 tasks

1ec5 reviewed Sep 20, 2017

View reviewed changes


		}

		NSString *extension = [[fileURL pathExtension] lowercaseString];

Conversation

ivovandongen commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederoni Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederoni Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederoni Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederoni Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivovandongen commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1ec5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boundsj commented Sep 5, 2017

Uh oh!

1ec5 commented Sep 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ivovandongen commented Aug 30, 2017 •

edited

Loading

frederoni Sep 2, 2017 •

edited

Loading

frederoni Sep 2, 2017 •

edited

Loading

frederoni Sep 2, 2017 •

edited

Loading

frederoni Sep 2, 2017 •

edited

Loading

ivovandongen commented Sep 5, 2017 •

edited

Loading

1ec5 left a comment •

edited

Loading