Upgrade to Mapbox GL–based Mapbox Static API #54

1ec5 · 2017-03-06T10:12:09Z

This PR adds support for the Mapbox GL–powered Mapbox Static API alongside the existing support for the classic Static API. In the process, the existing Swift files have been split up to be more manageable.

The Snapshot class and Overlay subclasses are largely untouched, a consequence of the fact that the new API tries to mimic the classic API’s overlay options. SnapshotOptions has been renamed ClassicSnapshotOptions. This PR takes advantage of the SnapshotOptionsProtocol protocol – meant to provide a common interface for ClassicSnapshotOptions and MarkerOptions – to add another conforming protocol, named SnapshotOptions, that exposes options for working with the new API.

SnapshotOptions is a bit more complex than ClassicSnapshotOptions. Instead of accepting separate centerCoordinate and zoomLevel options, it takes a single camera option of type SnapshotCamera. SnapshotCamera defines both a conventional zoomLevel property and an altitude property (for compatibility with the Mapbox iOS and macOS SDKs’ MGLMapCamera class); the developer can use either.

SnapshotCamera closely parallels the Mapbox iOS and macOS SDKs’ MGLMapCamera class, in much the same way that MapKit’s MKMapSnapshotOptions and MKMapView classes both have camera properties of type MKMapCamera. This library can’t simply reuse MGLMapCamera because it lives in the Mapbox iOS and macOS SDKs, which are separate frameworks. (Perhaps we should move this entire library’s functionality into those SDKs to fix mapbox/mapbox-gl-native#1701?)

GeoJSON(object:) is once again throwable. #23 made it throwable, but #28 had to make it failable instead due to a type mismatch when bridging to Objective-C. As of Swift 3, SE-0112 resolved the type mismatch, so the initializer can once again be throwable without breaking Objective-C support.

The classic snapshot tests have been rewritten to replace clever but untested URL parsing code in tests with much more straightforward string literals that can be verified easily. Now, if you want to know the URL that a particular ClassicSnapshotOptions produces, you can copy the stub’s isPath() call’s argument into a browser instead of having to run the test code. Test fixture images have been added, and the tests verify that an NSImage or UIImage can be created from each test fixture image produced by Snapshot.

To recap:

Fixes #9.

/cc @frederoni @bsudekum @incanus @friedbunny

Renamed SnapshotOptions to ClassicSnapshotOptions. Added a similar SnapshotOptions class tailored to the GL-based Static API. Split out ClassicSnapshotTests.

We previously made this initializer failable instead of throwable because of a compiler bug, but the bug appears to have been resolved in Swift 3.

Split out classic overlay tests into a separate test class. Replaced clever but untested URL parsing code in tests with much more straightforward string literals that can be verified easily. Added test fixture images. Verify that NSImage or UIImage can be created from each test fixture image. Switched to mapbox.streets in overlay tests to make fixtures easier to identify. Added configuration tests.

Upgraded example applications to use the new SnapshotOptions initializer. Refined MBSnapshotCamera initializer for Objective-C in the form of a factory method.

1ec5 · 2017-03-07T23:23:32Z

Test coverage is at 76.97%:

Most of the untested code is in convenience methods added for Objective-C compatibility.

1ec5 · 2017-03-16T17:42:37Z

The server-side rendering issues have been fixed, so the remaining work is no longer blocked.

Added altitude as an alternative to zoom level in SnapshotCamera, for consistency with MapKit and the Mapbox iOS SDK.

friedbunny

Not entirely sold on the map camera approach, but that’s a matter of taste.

My first time using the playground here — it’s nice.

friedbunny · 2017-03-21T01:43:11Z

MapboxStatic/ClassicSnapshotOptions.swift

+#endif
+
+/**
+ A structure that determines what a snapshot depicts and how it is formatted. A classic snapshot is made by compositing one or more [tile sets](https://www.mapbox.com/help/define-tileset/) with optional overlays using the [classic Mapbox Static API](https://www.mapbox.com/api-documentation/?language=Swift#static-classic).


Is it a historical oversight that these lines aren’t manually wrapped?

Since Xcode automatically reformats the comment in the generated interface (whether you're coming from Swift or Objective-C code), I've been less concerned about documentation comment formatting here than in the gl-native codebase. But if you're concerned about readability for contributors to this codebase, we can address the line wrapping issue in a separate PR.

We’d have to tread carefully, because the component that translates Swift into Objective-C headers (and thus reStructuredText into Doxygen) is a bit fragile. Objective-C bridging has broken in the past due to something as innocuous as a blank line between - parameter and - note.

Mostly the line length just made it slightly more painful to review.

friedbunny · 2017-03-21T02:01:00Z

MapboxStatic/Snapshot.swift


- If you use `Snapshot` to display a [vector tile set](https://www.mapbox.com/help/define-tileset/#vector-tilesets), the snapshot image will depict a wireframe representation of the tile set. To generate a static, styled image of a vector tile set, use the [vector Mapbox Static API](https://www.mapbox.com/api-documentation/?language=Swift#static).
+ The snapshot image can be used in an image view (`UIImage` on iOS and tvOS, `NSImage` on macOS, `WKImage` on watchOS). To add interactivity, use the `MGLMapView` class provided by the [Mapbox iOS SDK](https://www.mapbox.com/ios-sdk/) or [Mapbox macOS SDK](https://github.com/mapbox/mapbox-gl-native/tree/master/platform/macos/). (See the “[Custom raster style](https://www.mapbox.com/ios-sdk/examples/raster-styles/)” example to display a raster tile set in an `MGLMapView`.)


To add interactivity, use the MGLMapView...

This makes the two libraries sound as if one is necessary for the other or that they are somehow integrated. I think we should frame this more as, “For interactive maps, see our fully-featured SDKs...” and avoid mentioning classnames that impatient readers might expect to exist in this library.

I’m also unsure of the mention of the iOS SDK’s raster tiles example — it strikes me as a non-sequitor, given that that example deals in tilesets and not individual static images.

Clarified in 5ccc930.

friedbunny · 2017-03-21T02:08:17Z

MapboxStatic/SnapshotOptions.swift

+    /**
+     Size of a tile.
+     */
+    static let tileSize = CGSize(width: 512, height: 512)


Strictly reading this code, it could be made more clear that tileSize represents an internal implementation detail and not size.

Besides a little disambiguation, is there a source of truth for this that we can link to?

This is an private variable factored out for the purpose of keeping the altitude conversion code readable.

friedbunny · 2017-03-21T02:16:54Z

MapboxStatic/SnapshotOptions.swift

+    }
+
+    func string(size: CGSize) -> String {
+        assert(-90...90 ~= centerCoordinate.latitude, "Center latitude must be between −90° and 90°.")


Nice range comparison.

friedbunny · 2017-03-21T02:18:32Z

MapboxStatic/SnapshotOptions.swift

+        assert(0...20 ~= zoomLevel, "Zoom level must be between 0 and 20.")
+        components.append(Double(zoomLevel))
+
+        if heading > 0 {


Does heading need a range check, or will >360 just keep wrapping?

The API wraps it automatically.

friedbunny · 2017-03-21T02:24:06Z

MapboxStatic/SnapshotOptions.swift

+    /**
+     A Boolean determining whether the resulting image includes the Mapbox logo.
+
+     When shown, the Mapbox logo is located in the lower-left corner of the image. By default, this property is set to `true`.


Chances are quite high that the developer is required to include the logo, so linking https://www.mapbox.com/help/attribution/ here also makes sense.

Noted in 5ccc930.

friedbunny · 2017-03-21T02:30:44Z

README.md

@@ -16,13 +16,13 @@ MapboxStatic.swift pairs well with [MapboxDirections.swift](https://github.com/m
 Specify the following dependency in your [Carthage](https://github.com/Carthage/Carthage/) Cartfile:

 ```sh
-github "Mapbox/MapboxStatic.swift" "~> 0.7"
+github "Mapbox/MapboxStatic.swift" "master"


Risky... is it necessary not to specify a version?

For now. Whenever we rewrite the readme, we have to instruct the developer to get the master version so that the instructions are consistent. Otherwise we'd have to stage changes to the documentation, which would render them inconsistent with the rest of the repository in the meantime.

This shouldn't be a problem, because both CocoaPods and Carthage instruct developers to check in their lock files.

friedbunny · 2017-03-21T02:35:12Z

iOS.playground/Contents.swift

 */

-let mapIdentifiers = ["justin.tm2-basemap"]
+let styleURL = URL(string: "mapbox://styles/mapbox/streets-v9")!
+let mapIdentifiers = ["mapbox.satellite"]
 let accessToken = "pk.eyJ1IjoianVzdGluIiwiYSI6IlpDbUJLSUEifQ.4mG8vhelFMju6HpIY-Hi5A"


This token belongs to @incanus — it should be replaced with one belonging to the mapbox account, if we’re going to include one at all.

We do need to include the token if we want the playground to function as it does. We can replace it with a Mapbox corporate token, though.

Replaced in 49ec1d0.

friedbunny · 2017-03-21T02:36:52Z

macOS.playground/Contents.swift

 */

-let mapIdentifiers = ["justin.tm2-basemap"]
+let styleURL = URL(string: "mapbox://styles/mapbox/streets-v9")!
+let mapIdentifiers = ["mapbox.satellite"]
 let accessToken = "pk.eyJ1IjoianVzdGluIiwiYSI6IlpDbUJLSUEifQ.4mG8vhelFMju6HpIY-Hi5A"


Ditto to this access token.

Dittoed in 49ec1d0.

Clarified the relationship between Snapshot and the iOS and macOS SDKs. Added a notice about the general requirement to leave the logo enabled.

quicklywilliam · 2017-03-27T17:19:28Z

Huzzah!

1ec5 added 9 commits March 4, 2017 18:06

Split Snapshot.swift by class

ccb1d89

Upgraded to GL-based Static API

3c87b52

Renamed SnapshotOptions to ClassicSnapshotOptions. Added a similar SnapshotOptions class tailored to the GL-based Static API. Split out ClassicSnapshotTests.

Make GeoJSON initializer throwable

0cc8c09

We previously made this initializer failable instead of throwable because of a compiler bug, but the bug appears to have been resolved in Swift 3.

Re-typed apiEndpoint as URL

2fd31c3

JSON object values are Any, not AnyObject

dc8d6d7

Made SnapshotCamera initializer public

892850e

Fixed example applications

a628377

Upgraded example applications to use the new SnapshotOptions initializer. Refined MBSnapshotCamera initializer for Objective-C in the form of a factory method.

Updated readme and playgrounds

5a42efc

1ec5 added the enhancement label Mar 6, 2017

1ec5 self-assigned this Mar 6, 2017

1ec5 requested a review from incanus March 6, 2017 10:12

1ec5 mentioned this pull request Mar 6, 2017

Upgrade to Mapbox GL-based Mapbox Static API #9

Closed

1ec5 added the ⚠️ DO NOT MERGE label Mar 6, 2017

1ec5 added 6 commits March 6, 2017 23:22

styles:tiles scope required for GL snapshots

cc91770

Simplified zoom test

e8e4457

Auto-fitting for GL API

bc81dc3

Added SnapshotOptions tests

88cee66

Fixed pitch without heading

346993d

Test auto, logo, attribution options

a95cc20

1ec5 mentioned this pull request Mar 7, 2017

more unit test coverage #7

Closed

Fixed auto description in macOS playground

ec4956d

1ec5 added the refactor label Mar 14, 2017

1ec5 removed the ⚠️ DO NOT MERGE label Mar 16, 2017

1ec5 added 3 commits March 16, 2017 17:51

Updated readme screenshots

09a812a

Corrected image width

41ac689

Added altitude to SnapshotCamera

53a1660

Added altitude as an alternative to zoom level in SnapshotCamera, for consistency with MapKit and the Mapbox iOS SDK.

1ec5 requested a review from bsudekum March 18, 2017 07:11

1ec5 requested a review from frederoni March 18, 2017 07:11

friedbunny reviewed Mar 21, 2017

View reviewed changes

1ec5 added 3 commits March 25, 2017 23:39

Clarified documentation

5ccc930

Clarified the relationship between Snapshot and the iOS and macOS SDKs. Added a notice about the general requirement to leave the logo enabled.

Updated example access token

49ec1d0

Merge branch 'master' into 1ec5-gl-9

189229b

friedbunny approved these changes Mar 27, 2017

View reviewed changes

1ec5 merged commit 8ade96f into master Mar 27, 2017

1ec5 deleted the 1ec5-gl-9 branch March 27, 2017 16:51

friedbunny mentioned this pull request Mar 27, 2017

Add map snapshot API mapbox/mapbox-gl-native#1701

Closed

1ec5 mentioned this pull request Apr 10, 2017

support new Static API and rename current as Classic. mapbox/mapbox-sdk-js#139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Mapbox GL–based Mapbox Static API #54

Upgrade to Mapbox GL–based Mapbox Static API #54

1ec5 commented Mar 6, 2017 •

edited

Loading

1ec5 commented Mar 7, 2017

1ec5 commented Mar 16, 2017

friedbunny left a comment

friedbunny Mar 21, 2017

1ec5 Mar 21, 2017

1ec5 Mar 21, 2017

friedbunny Mar 21, 2017

friedbunny Mar 21, 2017

1ec5 Mar 26, 2017

friedbunny Mar 21, 2017 •

edited

Loading

1ec5 Mar 21, 2017

friedbunny Mar 21, 2017

friedbunny Mar 21, 2017

1ec5 Mar 21, 2017

friedbunny Mar 21, 2017

1ec5 Mar 26, 2017

friedbunny Mar 21, 2017

1ec5 Mar 21, 2017

friedbunny Mar 21, 2017

1ec5 Mar 21, 2017

1ec5 Mar 26, 2017

friedbunny Mar 21, 2017

1ec5 Mar 26, 2017

quicklywilliam commented Mar 27, 2017


		If you use `Snapshot` to display a [vector tile set](https://www.mapbox.com/help/define-tileset/#vector-tilesets), the snapshot image will depict a wireframe representation of the tile set. To generate a static, styled image of a vector tile set, use the [vector Mapbox Static API](https://www.mapbox.com/api-documentation/?language=Swift#static).
		The snapshot image can be used in an image view (`UIImage` on iOS and tvOS, `NSImage` on macOS, `WKImage` on watchOS). To add interactivity, use the `MGLMapView` class provided by the [Mapbox iOS SDK](https://www.mapbox.com/ios-sdk/) or [Mapbox macOS SDK](https://github.com/mapbox/mapbox-gl-native/tree/master/platform/macos/). (See the “[Custom raster style](https://www.mapbox.com/ios-sdk/examples/raster-styles/)” example to display a raster tile set in an `MGLMapView`.)

Upgrade to Mapbox GL–based Mapbox Static API #54

Upgrade to Mapbox GL–based Mapbox Static API #54

Conversation

1ec5 commented Mar 6, 2017 • edited Loading

1ec5 commented Mar 7, 2017

1ec5 commented Mar 16, 2017

friedbunny 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

friedbunny Mar 21, 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

quicklywilliam commented Mar 27, 2017

1ec5 commented Mar 6, 2017 •

edited

Loading

friedbunny Mar 21, 2017 •

edited

Loading