Grab app-wide data from Info.plist

[1ec5]

That way there’s no ambiguity about when you should call things like +[MGLAccountManager setMapboxMetricsEnabledSettingShownInApp:]. In fact, that method is now deprecated because it’s so easy to call in the wrong place.

Fixes #1535.

/cc @incanus @bleege

[incanus]

Do we need to deprecate, vs. just pulling? We're still in beta.

[1ec5]

I want the compiler warning to say what the alternative is rather than "I don't know what you're talking about", but come to think of it, an UNAVAILABLE macro makes more sense, as it's unignorable.

[1ec5]

Does it make sense to keep -[MGLMapView setAccessToken:]? It’s a mostly redundant code path, since setting MGLMapboxAccessToken in Info.plist – either in the property list editor or in the project editor – is by far the preferred way to set the access token. The only use case I can think of for keeping this extra property is different access tokens for different MGLMapViews in the same app. But Metrics would be quite confused in that case, sending events tied to different maps based on which one happens to have been initialized last.

If we decide to remove -[MGLMapView setAccessToken:], I’d probably mark it unavailable with a helpful message, as I’ve done for +[MGLAccountManager setMapboxMetricsEnabledSettingShownInApp:], remove the IBInspectable redeclaration, and make the implementation assert with a helpful message for apps built with storyboards. (The assertion message would appear in the Issue navigator as an error while designing in Interface Builder.) It would not be possible to simply remove this property, because the value would still linger in the storyboard as a user-defined runtime attribute and cause a crash with an unhelpful message at runtime.

[1ec5]

With these changes, if you wanted to upload your project to GitHub, leave out your access token (#1404), and avoid accidentally checking in your access token after you develop locally, you could employ the following Run Script build phase in Xcode (add it anywhere after the Copy Bundle Resources build phase):

token="$(cat ~/.mapbox)"
if [ "$token" ]; then
  plutil -replace MGLMapboxAccessToken -string $token $TARGET_BUILD_DIR/$INFOPLIST_PATH
fi

Then all you have to do is create a file .mapbox in your user folder that contains an access token and build and run. There’s no risk of ever checking in that hidden file, and no need to interact with MGLAccountManager at all: the plist entry is read the very moment the framework is loaded.

[friedbunny]

I still see using -[MGLMapView setAccessToken:] +[MGLMapView setAccessToken:] in the app delegate as the primary way developers will want to set access tokens.

If token security is a priority, the plist will be the go-to option, but if it's a lone developer or a private codebase, setting the token in code feels like the easiest path to me. Using the app delegate's application:didFinishLaunchingWithOptions: method is also a super-common and expected pattern in other SDKs.

[1ec5]

@friedbunny, that would be an argument for keeping +[MGLAccountManager setAccessToken:] around, and I haven’t considered removing it. -[MGLMapView setAccessToken:] is kinda weird because it affects the app globally via MGLAccountManager but looks like a view-local property. It can only be used inside -[UIViewController viewDidLoad], not -[UIApplicationDelegate application:didFinishLaunchingWithOptions:].

In any case, I think it’s fair to expect developers to modify their Info.plist, since you can do so within the project editor. It’s a rare app that can be developed without ever opening the project editor.

@bsudekum, does React Native let you modify the Info.plist that customarily comes with a project? I’m thinking about obsoleting -[MGLMapView setAccessToken:] and removing the accessToken parameter from MGLMapView’s init methods, now that you can add the token to the Info.plist. But if editing the Info.plist isn’t an option in your environment, that might be a good reason to keep the method around. Then again, I think +[MGLAccountManager setAccessToken:] might be an option for you too.

[friedbunny]

Ah, bleh on my late night reading comprehension.

[bleege]

-[MGLMapView setAccessToken:] was originally put in as a way for +[MGLAccountManager setAccessToken:] to update it. It's not likely to be a volatile data object, but I like the idea of +[MGLAccountManager setAccessToken:] being the "ultimate source of access token truth at runtime".

[1ec5]

-[MGLMapView setAccessToken:] has been around forever, but that was before we had any singletons like MGLAccountManager. In further defense of the Info.plist approach, note that the installation guide we have planned for beta 1 already has developers fiddle with settings in the project editor, so the UI won’t be foreign to them – it’s just the next tab over.

Unfortunately, mbgl::Map still stores the access token independently, so we’ll still need -[MGLMapView setAccessToken:] at least internally to sync between it and MGLAccountManager.

[bsudekum]

From a react-native point of view it would not be a deal breaker, but it would be 👎 to have to set the access token in the plist. One of the goals/advantages of react-native is to keep out of xcode.

[1ec5]

OK, I’m definitely convinced that +[MGLAccountManager setAccessToken:] should stay (and be used by react-native-mapbox-gl, but I’m souring on -[MGLMapView setAccessToken:] with each passing day.

To clarify, the intent of this PR is not about keeping the access token private – it just happens to make that easier. Rather, the intent is to replace a tricky setup dance with something that looks more like plain-vanilla, all-UIKit development.

Currently, if you want to implement the Metrics switch inside your app, rather than in Settings, you need to call +[MGLAccountManager setMapboxMetricsEnabledSettingShownInApp:] before calling any other MGLAccountManager method, initializing any MGLMapView with an access token (whether programmatically or in a storyboard), or calling -[MGLMapView setAccessToken:]. The number of settings to set this way may even go up over time. It’s fine to expect developers to modify -[UIApplicationDelegate application:didFinishLaunchingWithOptions:], but not to require an order that’s only enforced at runtime, with assertions that can’t really tell you what you did wrong.

[1ec5]

Before merging this PR, I’m also going to create an -[MGLMapView initWithFrame:styleURL:] and deprecate or remove the initializers that take access tokens. That’ll simply things greatly in MGLMapView.