Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

MapboxGL Startup Infrastructure #1329

Merged
merged 10 commits into from
May 5, 2015
1 change: 1 addition & 0 deletions gyp/platform-ios.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
'../platform/darwin/image.mm',
'../platform/darwin/reachability.m',
'../include/mbgl/ios/MapboxGL.h',
'../platform/ios/MapboxGL.m',
'../include/mbgl/ios/MGLMapboxEvents.h',
'../platform/ios/MGLMapboxEvents.m',
'../include/mbgl/ios/MGLMapView.h',
Expand Down
5 changes: 5 additions & 0 deletions include/mbgl/ios/MGLMapView.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ IB_DESIGNABLE

/** @name Initializing a Map View */

/** Initialize a map view with the default style, given frame, and access token set in MapboxGL singleton.
* @param frame The frame with which to initialize the map view.
* @return An initialized map view, or `nil` if the map view was unable to be initialized. */
- (instancetype)initWithFrame:(CGRect)frame;

/** Initialize a map view with the default style and a given frame and access token.
* @param frame The frame with which to initialize the map view.
* @param accessToken A Mapbox API access token.
Expand Down
7 changes: 7 additions & 0 deletions include/mbgl/ios/MapboxGL.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,10 @@
#import "MGLMapView.h"
#import "MGLTypes.h"
#import "MGLUserLocation.h"

@interface MapboxGL : NSObject
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header was meant to be an umbrella header, which does nothing but import all the other public-facing headers. (A developer should only ever have to import the one umbrella header unless they’re doing something out of the ordinary.)

I’d move this class to another header and rename it MGLAccountManager or somesuch, sticking to the framework’s established prefix.


+ (id) sharedInstanceWithAccessToken:(NSString *)token;
+ (NSString *) getAccessToken;

@end
16 changes: 16 additions & 0 deletions ios/app/MBXAppDelegate.m
Original file line number Diff line number Diff line change
@@ -1,11 +1,27 @@
#import "MBXAppDelegate.h"
#import "MBXViewController.h"
#import <mbgl/ios/MapboxGL.h>
#import <mbgl/ios/MGLMapboxEvents.h>

@implementation MBXAppDelegate

- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions
{
// Set Access Token
NSString *accessToken = [[NSProcessInfo processInfo] environment][@"MAPBOX_ACCESS_TOKEN"];
if (accessToken) {
// Store to preferences so that we can launch the app later on without having to specify
// token.
[[NSUserDefaults standardUserDefaults] setObject:accessToken forKey:@"access_token"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: A common strategy for user defaults keys is to make them match the names of the constants that define them. So you’d define a constant called MGLMapboxAccessTokenDefaultsKey with the value MGLMapboxAccessToken in MGLTypes.h and use that constant here. Obviously it doesn’t matter as long as you’re consistent, but it’s one less thing to worry about naming.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd be better off putting things in a constant to avoid future potential problems with the @"access_token" magic string, too.

} else {
// Try to retrieve from preferences, maybe we've stored them there previously and can reuse
// the token.
accessToken = [[NSUserDefaults standardUserDefaults] objectForKey:@"access_token"];
}
if ( ! accessToken) NSLog(@"No access token set. Mapbox vector tiles won't work.");

[MapboxGL sharedInstanceWithAccessToken:accessToken];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you’re creating a MapboxGL and throwing it away. Someone might come along later and delete the line, thinking it does nothing, when in fact you’re setting the access token globally by side effect. Can we call this class method +setAccessToken: or +registerAccessToken: instead?


self.window = [[UIWindow alloc] initWithFrame:[[UIScreen mainScreen] bounds]];
self.window.rootViewController = [[UINavigationController alloc] initWithRootViewController:[MBXViewController new]];
[self.window makeKeyAndVisible];
Expand Down
15 changes: 1 addition & 14 deletions ios/app/MBXViewController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -52,20 +52,7 @@ - (void)viewDidLoad
{
[super viewDidLoad];

NSString *accessToken = [[NSProcessInfo processInfo] environment][@"MAPBOX_ACCESS_TOKEN"];
if (accessToken) {
// Store to preferences so that we can launch the app later on without having to specify
// token.
[[NSUserDefaults standardUserDefaults] setObject:accessToken forKey:@"access_token"];
} else {
// Try to retrieve from preferences, maybe we've stored them there previously and can reuse
// the token.
accessToken = [[NSUserDefaults standardUserDefaults] objectForKey:@"access_token"];
}

if ( ! accessToken) NSLog(@"No access token set. Mapbox vector tiles won't work.");

self.mapView = [[MGLMapView alloc] initWithFrame:self.view.bounds accessToken:accessToken];
self.mapView = [[MGLMapView alloc] initWithFrame:self.view.bounds];
self.mapView.autoresizingMask = UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight;
self.mapView.viewControllerForLayoutGuides = self;
self.mapView.showsUserLocation = YES;
Expand Down
2 changes: 2 additions & 0 deletions platform/ios/MGLMapView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#import "SMCalloutView.h"

#import "MGLMapboxEvents.h"
#import "MapboxGL.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import statement likle

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import statement likely makes some of the other ones redundant.


#import <algorithm>

Expand Down Expand Up @@ -122,6 +123,7 @@ - (instancetype)initWithFrame:(CGRect)frame
if (self && [self commonInit])
{
self.styleURL = nil;
self.accessToken = [MapboxGL getAccessToken];
return self;
}

Expand Down
48 changes: 48 additions & 0 deletions platform/ios/MapboxGL.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#import <Foundation/Foundation.h>

#import "MapboxGL.h"
#import "NSProcessInfo+MGLAdditions.h"

@interface MapboxGL()

@property (atomic) NSString *accessToken;

@end


@implementation MapboxGL

static MapboxGL *_sharedManager;

// Can be called from any thread. Called implicitly from any
// public class convenience methods.
//
+ (id) sharedInstanceWithAccessToken:(NSString *)token {
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
if ( ! NSProcessInfo.processInfo.mgl_isInterfaceBuilderDesignablesAgent) {
void (^setupBlock)() = ^{
_sharedManager = [[self alloc] init];
_sharedManager.accessToken = token;
};
if ( ! [[NSThread currentThread] isMainThread]) {
dispatch_sync(dispatch_get_main_queue(), ^{
setupBlock();
});
}
else {
setupBlock();
}
}
});
return _sharedManager;
}

+ (NSString *) getAccessToken {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just +accessToken. “Get” would imply some kind of non-trivial activity like a network request or disk read.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, if the developer can just get the access token from a class method (+accessToken), why make them create a shared instance in the first place?

Let’s strive for parallelism in one of two ways:

  • A class method +sharedManager that returns an MGLAccountManager with nothing set, and two instance methods: -setAccessToken: and -accessToken. This would be similar to how NSProcessInfo works.
  • Two standalone C functions: MGLRegisterAccessToken() and MGLGetRegisteredAccessToken(). (On Apple platforms, Objective-C getters don’t say “get”, but C getters do, to distinguish from “copy” functions.) Instead of keeping a singleton object around, keep just a static access token string. This approach is simpler but less extensible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up going with just renaming to +sharedAccessToken to avoid any get convention. The instance methods route wouldn't work due to MGLMapView needing to retrieve it. By keeping it in a +sharedManager this class stays more extensible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MGLMapView could’ve always getting the access token through [[MGLAccountManager sharedInstance] accessToken].

if (_sharedManager) {
return _sharedManager.accessToken;
}
return nil;
}

@end