Style API cleanup, designables, inspectables #1184

1ec5 · 2015-04-02T17:45:26Z

This PR addresses the following:

Fixes a race condition on launch (fixes Crash on launch without access token — mbgl::Style::loadJSON() #1147)
Cleans up various initializers and style APIs, removing raw JSON access and fixes KVO compliance (fixes Support XIBs and storyboards on iOS #1070)
Frictionless, codeless setup story for developers via designables and inspectables (fixes Make MGLMapView IB_DESIGNABLE (for iOS XIBs) #929)

So, what’s the easiest way to get started with Mapbox GL?

Add MapboxGL to your Podfile and run pod install.
Drag a UIView out from the Object library into a UIViewController in a storyboard.
In the Identity inspector, set the custom class to “MGLMapView”.
In the Attributes inspector, set your access token. Optionally set an access token, initial coordinates, and initial zoom level.
Build and run.

Will squash before merging.

1ec5 · 2015-04-02T18:07:24Z

Some details from individual commits:

This change removes most of the ways you used to be able to apply a style to the map, so existing clients may need to be updated. styleURL (HTTP(S), mapbox:, asset:) is now the canonical way to apply a style, and mapID is a convenient shorthand for Mapbox-hosted styles. Internally, we now construct an asset: URL for the default style in lieu of a “bundled style name”.
When an access token is set in the Attributes inspector (or as a user-defined runtime attribute), we draw some lovely Mapbox branding so you don’t have to manipulate an invisible rectangle. I’m open to suggestions on the design (cc @samanpwbb). In the absence of an access token, the view displays a helpful message with directions for obtaining and setting the access token.
We have to turn off some functionality while rendering the designable:
- We completely cut MGLMapboxEvents out of the picture. No one cares what beach the developer happens to be at while they’re building with Mapbox GL. 😉 In fact, if you touch Core Location anywhere inside a designable, it throws this exception (typo and all):
```
Application Specific Information:
*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Non-UI clients cannont be autopaused'
terminating with uncaught exception of type NSException
abort() called

Application Specific Backtrace 1:
0   CoreFoundation                      0x0000000109ccda75 __exceptionPreprocess + 165
1   libobjc.A.dylib                     0x00000001089d8bb7 objc_exception_throw + 45
2   CoreFoundation                      0x0000000109ccd8da +[NSException raise:format:arguments:] + 106
3   Foundation                          0x00000001085f1b6f -[NSAssertionHandler handleFailureInMethod:object:file:lineNumber:description:] + 195
4   CoreLocation                        0x00000001150df265 CLClientGetCapabilities + 7718
```
- We also avoid starting or updating mbgl::Map to avoid a potential minefield of problems, such as this exception:
  terminating with uncaught exception of type mbgl::util::ShaderException: Program plain failed to link: Validation Failed: Current draw framebuffer is invalid.
  It would be a mistake to rely on synchronous network requests inside a designable anyways.
- Xcode supports a TARGET_INTERFACE_BUILDER target conditional for just this situation, except that we’re distributing Mapbox GL as a prebuilt static library, so we can’t rely on conditional compilation, even if the library eventually makes its way into the CocoaPods-generated framework. Instead, we check whether we’re running inside the IBDesignablesAgentCocoaTouch process.
-[MGLMapView initWithFrame:] is overridden to call -commonInit. We’ve marked this initializer unavailable in the header, but IB still calls it regardless.
IB-specific inspectables are located a separate category that developers are not expected to import. (So it doesn’t appear in the umbrella header.) Currently these inspectables are order-dependent due to Cannot set zoom immediately after setting center coordinate #1181. In the screencap above, I purposely set Zoom before Longitude or Latitude. The bearing also appears to be somewhat order-dependent, so I left it out – it probably isn’t common to need a specific bearing at launch.

1ec5 · 2015-04-02T18:10:20Z

/cc @incanus for the MGLMapView changes; @bleege for the MGLMapboxEvents change

1ec5 · 2015-04-02T18:46:38Z

platform/ios/MGLMapView_IBAdditions.m

+    NSNumber *pendingLongitude;
+    if ((pendingLongitude = objc_getAssociatedObject(self, @selector(longitude))))
+    {
+        objc_setAssociatedObject(self, @selector(latitude), nil, OBJC_ASSOCIATION_RETAIN_NONATOMIC);


I know this associated object stuff is crazy, but I wanted two separate numeric inspectables instead of one comma-separated string inspectable (since it wouldn’t be clear whether latitude or longitude comes first). Setting only longitude or latitude has no effect: the coordinate remains (NaN, NaN).

This is why these properties are strictly IB-only. Maybe I should assert that the coordinate is still invalid before proceeding?

Distinct latitude and longitude fields seems like the right way to go.

This is why these properties are strictly IB-only. Maybe I should assert that the coordinate is still invalid before proceeding?

I think this might be a good idea, so that you never get someone trying to set these via KVC or something.

incanus · 2015-04-02T19:17:49Z

Will take a look shortly.

bleege · 2015-04-02T19:21:17Z

Just seeing this now and will look into it. First Impression though is that customizing Xcode for Access Tokens is awesome! 👍

bleege · 2015-04-02T19:38:32Z

platform/ios/MGLMapboxEvents.m

-        };
-        if ( ! [[NSThread currentThread] isMainThread]) {
-            dispatch_sync(dispatch_get_main_queue(), ^{
+        if ( ! NSProcessInfo.processInfo.mgl_isInterfaceBuilderDesignablesAgent) {


I'm not sure what wrapping this in a "not IBDesignableAgent" means. I assume this is some extra process that is only run when Xcode is working with the code via IB?

That’s correct. Whenever you modify an instance of MGLMapView in a storyboard (for instance by resizing it), Interface Builder spawns an IBDesignablesAgentCocoaTouch process in the background that instantiates an MGLMapView and calls methods like -drawRect: on it. There are two hooks for IB-only code that should never run in front of a user: TARGET_INTERFACE_BUILDER (which we can’t use) and -prepareForInterfaceBuilder. Here’s more information about designables, in two flavors: corporate, hipster.

In NSProcessInfo+MGLAdditions, I added a category method that cheaply determines whether the MGLMapView is being run inside of IBDesignablesAgentCocoaTouch. I’m using it here to prevent the shared MGLMapboxEvents instance from ever being created. The assumption is that you’d never release an app named IBDesignablesAgentCocoaTouch. 😄

That's really cool! I didn't realize IB could do that. Thanks for info @1ec5!

bleege · 2015-04-02T19:42:03Z

@1ec5 Just the one question ^^. Other than that just please make sure MGLEventTypeLocation doesn't slip back to "Location" for a value when the merge happens. Overall 👍

incanus · 2015-04-02T22:11:13Z

gyp/platform-ios.gypi

+        '../platform/ios/NSString+MGLAdditions.h',
+        '../platform/ios/NSString+MGLAdditions.m',
+        '../include/mbgl/ios/MGLMapView_IBAdditions.h',
+        '../platform/ios/MGLMapView_IBAdditions.m',


Why not name these with the same convention? (MGLMapView+IBAdditions.h etc.)

Answered in https://github.com/mapbox/mapbox-gl-native/pull/1184/files#r27702538.

incanus · 2015-04-02T22:45:32Z

Just one question @1ec5 while I'm still testing things out. Can you, like the styleURL, set the mapID to nil in order to use the default style?

incanus · 2015-04-02T23:04:48Z

Cool, this feels great. Testing some of the in-code methods out a bit now.

incanus · 2015-04-02T23:08:18Z

Does AppleDoc not support doc comments on categories?

It does, but do we need them when they are intended for IB and not code? They are purely visual.

incanus · 2015-04-02T23:09:52Z

Can you, like the styleURL, set the mapID to nil in order to use the default style?

I see that the case is no, but it's an easy fix that I'm putting in now.

incanus · 2015-04-02T23:23:06Z

I think functionality-wise we are good here now. I think we should shoot for the following API cleanups though:

Get rid of the associated objects stuff by making latitude and longitude private properties on MGLMapView. As an example of how this could work, setting latitude could:
```
- (void)setLatitude:(CLLocationDegrees)latitude
{
    self.centerCoordinate = CLLocationCoordinate2DMake(latitude, self.centerCoordinate.longitude);
}
```
That would take care of the "staging" necessary. Then, these methods can be exposed only in the private MGLMapView_IBAdditions.h so that the general public doesn't use them.
We should still address -bundledStyleNames because its output is no longer of much use. We could instead make a method that returns values that could be passed to -styleURL such as asset://styles/emerald-v7.json.

1ec5 · 2015-04-02T23:42:27Z

setLatitude: can't just call CLLocationCoordinate2DMake() and set it as the center coordinate because the resulting center coordinate will be (NaN, NaN) – a coordinate is either valid or invalid, never half valid. That said, I'll replace the pending associated objects with pending ivars and declare them in the IB-specific header.

1ec5 · 2015-04-02T23:45:19Z

In other words, your proposed code above passes a NaN longitude into CLLocationCoordinate2DMake().

incanus · 2015-04-02T23:46:29Z

In other words, your proposed code above passes a NaN longitude into CLLocationCoordinate2DMake().

Ah, right. Sorry, I know you've been through the trenches on the order of operations here. Just ideas.

I'll replace the pending associated objects with pending ivars and declare them in the IB-specific header.

This does sound more standard.

incanus · 2015-04-03T00:16:22Z

We could instead make a method that returns values that could be passed to -styleURL such as asset://styles/emerald-v7.json.

BTW on this front, I'm hesitant to expose asset:// publicly, but I think it's kind of required here if we are going to bundle styles and we are going to require a URL set to use them.

1ec5 · 2015-04-03T08:10:02Z

We could instead make a method that returns values that could be passed to -styleURL such as asset://styles/emerald-v7.json.

Rather than enumerating the styles at runtime, the most Cocoa-like way of doing this would be to define a series of NSURL constants, one for each bundled style. Or we’d have a class method +URLForBundledStyleNamed: à la -[UIImage imageNamed:]. Either approach would make sense for the real world case of applying a specific desired bundled style, but it would frustrate demo apps that cycle through all the possible bundled styles.

For now I’ve done as you suggest above but we should definitely revisit this method for beta 2.

incanus · 2015-04-03T15:58:21Z

Cool, I think b376fa6 is a good first step solution here.

it would frustrate demo apps that cycle through all the possible bundled styles.

Not sure I follow. Just because of the two-part setup?

I do think something like +URLForBundledStyleNamed: would be good down the road, because we'll likely package and advertise the bundled styles as something a little dressier than filenames, e.g. Emerald or Bright. Then, that utility method could do the legwork of handling the asset:// URL without the dev having to care much.

incanus · 2015-04-03T15:59:00Z

I think this is good if you're happy with testing @1ec5. Want to squash and push a new branch or force-push here?

1ec5 · 2015-04-03T16:28:41Z

it would frustrate demo apps that cycle through all the possible bundled styles.

Not sure I follow. Just because of the two-part setup?

Not exactly. The demo apps all list the styles by name, so you have to do some parsing to get a human-readable name, meaning even tighter coupling to the nascent asset: URL standard.

I do think something like +URLForBundledStyleNamed: would be good down the road

We have that already, as a private MGLURLForBundledStyleNamed() function in MGLMapView.mm. I didn’t expose it publicly because I’m not sure what to do with the versions: treat them as part of the name or as a separate piece of information?

1ec5 · 2015-04-03T16:30:05Z

platform/ios/MGLMapView.mm

-    else
-    {
-        mbglMap->setStyleURL(styleURLString);
+        styleURLString = std::string("asset://") + styleURLString;


I think we should get rid of this case. We can easily be consistent internally about using the asset: scheme. If the developer then passes in a relative URL, we can treat it as being relative to the main bundle rather than MapboxGL.bundle.

`Map` should be able to deal with having no access token or JSON even while it’s running. Most of `Map` accounts for this situation, but `reloadStyle()` incorrectly assumes that one or the other is set. This change corrects the assumption in `reloadStyle()`. Also expose the access token and style name to clients as ordinary KVO-compliant properties. Actually, they’re not so ordinary because they’re inspectable! Ref #1070, #1147

This change removes most of the ways you used to be able to apply a style to the map. Building on #1163, `styleURL` (HTTP(S), mapbox:, asset:) is the canonical way to apply a style, and `mapID` is a convenient shorthand for Mapbox-hosted styles. A relative style URL is interpreted as a path relative to the app’s main bundle. We now construct asset: URLs in lieu of “bundled style names”.

When an access token is set in the Attributes inspector (or as a user-defined runtime attribute), we draw some lovely Mapbox branding so the view shows up. (Manipulating invisible rectangles is a frustrating exercise, I’m told.) In the absence of an access token, the view displays a helpful message with directions for obtaining and setting the access token. Along the way, completely opt out of `MGLMapboxEvents` when targeting Interface Builder, because touching Core Location throws an exception in that environment and it doesn’t make sense to record any metrics when designing on the Interface Builder canvas. Also, don’t start `mbgl::Map` at all (and don’t update it) because none of the runtime drawing code should ever be run in the designable. Normally these chunks of code would be excluded in IB using the TARGET_INTERFACE_BUILDER preprocessor macro. However, Mapbox GL is being packaged as a static library, so the macro is only evaluated when the library is prebuilt, even if the library eventually makes its way into the CocoaPods-generated framework. Instead, we detect that we’re being run by the IBDesignablesAgentCocoaTouch process. Overrode `-[MGLMapView initWithFrame:]` to call `-commonInit`. We’ve marked this initializer unavailable in the header, but IB still calls it regardless. Fixes #929.

Declared these IB-specific inspectables in a separate category that developers are not expected to import. Currently these inspectables are order-dependent due to #1181. Ref #929

Ran this through `./ios/docs/install_docs.sh` to double-check things. * Less-specific initializers above more-specific per Apple convention. * Shores up `accessToken` docs as property, not a method. * Edited comments for consistency and/or to keep them out of the docs.

Added inspectables for toggling zooming, panning, rotating, and showing the user location for parity with `MKMapView`.

Style API cleanup, designables, inspectables

DavidChouinard · 2015-04-03T20:48:59Z

1ec5 · 2015-04-03T21:20:27Z

@DavidChouinard How are you integrating Mapbox GL into your project? We’re planning to distribute a prebuilt static library via CocoaPods but haven’t uploaded the binary yet; in the meantime, you can follow these steps to use Mapbox GL as it’ll be used once it’s officially released:

Create a prebuilt mapbox-gl-ios-0.1.0.zip – everything but the last two lines.
Set up a private spec repo that contains a single Specs/0.1.0/MapboxGL.podspec. The podspec should be identical to the version in master save for the following line:

m.source = { :http => "file:///absolute/path/to/mapbox-gl-native/build/ios/pkg/mapbox-gl-ios-#{m.version.to_s}.zip" }

Point the Podfile to your private repo (from whence you pushed):

source 'file:///absolute/path/to/pods/'
pod 'MapboxGL', '~> 0.1.0'
use_frameworks!

pod install && open MyApp.xcworkspace

If all that’s too much, there’s also the option of building from source (via make iproj) and turning the demo app into your app.

incanus · 2015-04-03T21:26:15Z

To clarify, GL has to be included as a framework (even if built statically) right now in order for the IB stuff to work.

1ec5 · 2015-04-03T21:26:18Z

@DavidChouinard Before you go through the rigmarole above, though, I wonder if your project has -ObjC set in ldflags in your target’s build settings?

#1184 added `-[NSProcessInfo(MGLAdditions) mgl_isInterfaceBuilderDesignablesAgent]`, which only gets pulled in if you add `-ObjC` to `OTHER_LDFLAGS`.

DavidChouinard · 2015-04-20T16:19:34Z

@1ec5 Sorry for taking so long to try this.

After a bit of fiddling with Cocoapods, it works. I get the blue Mapbox view in Interface Builder. However, when running the application, I get the following console messages and the view is completely white:

2015-04-20 12:17:37.944 Demo1[1671:572449] Unknown class MGLMapView in Interface Builder file.
2015-04-20 12:17:37.960 Demo1[1671:572449] Failed to set (accessToken) user defined inspected property on (UIView): [<UIView 0x15457dc00> setValue:forUndefinedKey:]: this class is not key value coding-compliant for the key accessToken.
2015-04-20 12:17:37.960 Demo1[1671:572449] Failed to set (latitude) user defined inspected property on (UIView): [<UIView 0x15457dc00> setValue:forUndefinedKey:]: this class is not key value coding-compliant for the key latitude.
2015-04-20 12:17:37.961 Demo1[1671:572449] Failed to set (longitude) user defined inspected property on (UIView): [<UIView 0x15457dc00> setValue:forUndefinedKey:]: this class is not key value coding-compliant for the key longitude.
2015-04-20 12:17:37.961 Demo1[1671:572449] Failed to set (zoomLevel) user defined inspected property on (UIView): [<UIView 0x15457dc00> setValue:forUndefinedKey:]: this class is not key value coding-compliant for the key zoomLevel.
2015-04-20 12:17:37.961 Demo1[1671:572449] Failed to set (allowsRotating) user defined inspected property on (UIView): [<UIView 0x15457dc00> setValue:forUndefinedKey:]: this class is not key value coding-compliant for the key allowsRotating.

1ec5 · 2015-04-20T16:47:26Z

@DavidChouinard It looks like MapboxGL.framework or MapboxGL.a inside it still aren’t getting linked into your app for some reason. If you instantiate the MGLMapView programmatically, does the app build successfully?

1ec5 added the iOS Mapbox Maps SDK for iOS label Apr 2, 2015

1ec5 added this to the iOS Beta 1 milestone Apr 2, 2015

1ec5 reviewed Apr 2, 2015
View reviewed changes

bleege reviewed Apr 2, 2015
View reviewed changes

incanus reviewed Apr 2, 2015
View reviewed changes

1ec5 reviewed Apr 3, 2015
View reviewed changes

1ec5 self-assigned this Apr 3, 2015

1ec5 added 2 commits April 3, 2015 12:07

1ec5 and others added 5 commits April 3, 2015 12:16

Made lat/lon/zoom inspectable

836de8b

Declared these IB-specific inspectables in a separate category that developers are not expected to import. Currently these inspectables are order-dependent due to #1181. Ref #929

allow setting nil mapID for default style

6eb7933

Inspectable toggles

d1fb43e

Added inspectables for toggling zooming, panning, rotating, and showing the user location for parity with `MKMapView`.

1ec5 force-pushed the 1070-kvo branch from 1720036 to d1fb43e Compare April 3, 2015 19:44

1ec5 added a commit that referenced this pull request Apr 3, 2015

Merge pull request #1184 from mapbox/1070-kvo

e6e340a

Style API cleanup, designables, inspectables

1ec5 merged commit e6e340a into master Apr 3, 2015

1ec5 deleted the 1070-kvo branch April 3, 2015 20:22

1ec5 added a commit that referenced this pull request Apr 3, 2015

iOS projects require -ObjC in OTHER_LDFLAGS

db4b6bd

#1184 added `-[NSProcessInfo(MGLAdditions) mgl_isInterfaceBuilderDesignablesAgent]`, which only gets pulled in if you add `-ObjC` to `OTHER_LDFLAGS`.

This was referenced Apr 3, 2015

iOS projects require -ObjC in OTHER_LDFLAGS #1201

Merged

Accept nil access token #1211

Merged

Create Mapbox GL Start Infrastructure #1225

Closed

Document iOS GL Installation Instructions #1226

Closed

1ec5 mentioned this pull request May 26, 2015

Provide "mapType"-like abstraction for built-in styles #1640

Closed

1ec5 mentioned this pull request Jul 14, 2015

legacy raster mapID API #1844

Closed

1ec5 mentioned this pull request Jan 11, 2016

Absolute file URL is treated as mapbox: style URL #3505

Closed

rmnblm mentioned this pull request Sep 26, 2016

Crash when adding and removing the same StyleLayer at runtime #6460

Closed

friedbunny mentioned this pull request Oct 20, 2017

Interface Builder designable fails to compile using CocoaPods #10072

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style API cleanup, designables, inspectables #1184

Style API cleanup, designables, inspectables #1184

1ec5 commented Apr 2, 2015

1ec5 commented Apr 2, 2015

1ec5 commented Apr 2, 2015

1ec5 Apr 2, 2015

bleege Apr 2, 2015

incanus Apr 2, 2015

incanus commented Apr 2, 2015

bleege commented Apr 2, 2015

bleege Apr 2, 2015

1ec5 Apr 2, 2015

bleege Apr 2, 2015

bleege commented Apr 2, 2015

incanus Apr 2, 2015

incanus Apr 2, 2015

incanus commented Apr 2, 2015

incanus commented Apr 2, 2015

incanus commented Apr 2, 2015

incanus commented Apr 2, 2015

incanus commented Apr 2, 2015

1ec5 commented Apr 2, 2015

1ec5 commented Apr 2, 2015

incanus commented Apr 2, 2015

incanus commented Apr 3, 2015

1ec5 commented Apr 3, 2015

incanus commented Apr 3, 2015

incanus commented Apr 3, 2015

1ec5 commented Apr 3, 2015

1ec5 Apr 3, 2015

jfirebaugh Apr 3, 2015

DavidChouinard commented Apr 3, 2015

1ec5 commented Apr 3, 2015

incanus commented Apr 3, 2015

1ec5 commented Apr 3, 2015

DavidChouinard commented Apr 20, 2015

1ec5 commented Apr 20, 2015

Style API cleanup, designables, inspectables #1184

Style API cleanup, designables, inspectables #1184

Conversation

1ec5 commented Apr 2, 2015

1ec5 commented Apr 2, 2015

1ec5 commented Apr 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incanus commented Apr 2, 2015

bleege commented Apr 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleege commented Apr 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incanus commented Apr 2, 2015

incanus commented Apr 2, 2015

incanus commented Apr 2, 2015

incanus commented Apr 2, 2015

incanus commented Apr 2, 2015

1ec5 commented Apr 2, 2015

1ec5 commented Apr 2, 2015

incanus commented Apr 2, 2015

incanus commented Apr 3, 2015

1ec5 commented Apr 3, 2015

incanus commented Apr 3, 2015

incanus commented Apr 3, 2015

1ec5 commented Apr 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidChouinard commented Apr 3, 2015

1ec5 commented Apr 3, 2015

incanus commented Apr 3, 2015

1ec5 commented Apr 3, 2015

DavidChouinard commented Apr 20, 2015

1ec5 commented Apr 20, 2015