mapbox / mapbox-gl-native

Interactive, thoroughly customizable maps in native Android, iOS, macOS, Node.js, and Qt applications, powered by vector tiles and OpenGL
https://mapbox.com/mobile
Other
4.37k stars 1.33k forks source link

Create Mapbox GL Start Infrastructure #1225

Closed bleege closed 9 years ago

bleege commented 9 years ago

The current Mapbox GL startup system is all tied to MGLMapView. This means that in order for an app developer to use any Mapbox service a MapView has to be built. It also ties Metrics to MapView as well.

Future State Goals

  1. Start Mapbox GL in AppDelegate's didFinishLaunchingWithOptions
    • [MapboxGL startWithAccessToken:@"YOURACCESSTOKENGOESHERE"];
  2. MapboxGL startWithAccessToken's responsibilities
    • Serve as singleton holder of Access Token for all SDK uses
    • Start Metrics services

@mapbox/mobile

1ec5 commented 9 years ago

As you work on this, please keep in mind the XIB/storyboard scenario we enabled in #1070: it should still be possible to set an access token on a view-by-view basis, without overriding anything in the app delegate. Codeless setup is a compelling story (MapKit parity, makes for a good beginner’s guide, shorter demo GIFs), even if we expect most developers to go the programmatic route eventually.

jfirebaugh commented 9 years ago

I'm not sure this is a good idea. A required singleton init method typically makes libraries harder to use.

in order for an app developer to use any Mapbox service a MapView has to be built

What Mapbox services does Mapbox GL provide besides MapViews?

bleege commented 9 years ago

A required singleton init method typically makes libraries harder to use.

Generally I agree, but in this particular case it's a very common iOS design pattern is to to initialize 3rd party code at app startup inside the app's AppDelegate. Specifically in the following callback method:

- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions

What Mapbox services does Mapbox GL provide besides MapViews?

Currently there are no other services besides MapViews, but if others were to come along in the future (ex: Directions API wrapper) the existing infrastructure would require developers to first create a MapView in order to get access tokens to use. This way would provide a service agnostic one line of code common setup.

1ec5 commented 9 years ago

A required singleton init method typically makes libraries harder to use.

This is why any singleton setup step needs to be optional: individual top-level view classes like MGLMapView should continue to accept access tokens, so that you can instantiate a working map from within a XIB or stroryboard, or from within a custom React Native component, as @bsudekum is trying to do.

incanus commented 9 years ago

I think that we should hold off on this if the reason is possible future uses, but my understanding was that one might want to trigger metrics without necessary showing a map view at launch — or at all. For that we would need a singleton, and @bleege is right that this is common pattern. HockeyApp and other crash log services do this, as do other map libraries like Google for iOS:

Add the following to your application:didFinishLaunchingWithOptions: method, replacing _APIKEY with your API key.

[GMSServices provideAPIKey:@"API_KEY"];

incanus commented 9 years ago

individual top-level view classes like MGLMapView should continue to accept access tokens

Agree here as well, and services like directions and geocoding are probably better served using their own setters too, as MapboxDirections.swift and MapboxGeocoder.swift do.

bleege commented 9 years ago

individual top-level view classes like MGLMapView should continue to accept access tokens

Agreed. The goal isn't / wasn't to replace these (nor the ones in Directions and Geocoder), it was to provide a common area for the token to be set so that developers wouldn't have to set it in N number of places in their app as well as to allow Metrics to start without needing to display a MapView at launch.

That said after now seeing the recent updates to MapboxDirections.swift and MapboxGeocoder.swift and how they're being used as separate Legos this isn't likely to be that onerous in the short term. Also Metrics is now started in MGLMapView.commonInit so they'll actually be configured once the MGLMapView is initially created and not require the map to be displayed.

This is a good discussion. Let's pull back on this idea for the time being and come back to it later if the need arrises. :+1:

incanus commented 9 years ago

Also Metrics is now started in MGLMapView.commonInit so they'll actually be configured once the MGLMapView is initially created and not require the map to be displayed.

Isn't this still a problem though? If a map exists on a second tab, it won't be created until that tab is first shown, but metrics might be needed before then?

I think we do need a singleton, but we just weren't talking about the right reasons above. Does that make sense?

bleege commented 9 years ago

If a map exists on a second tab, it won't be created until that tab is first shown, but metrics might be needed before then?

Yeah, the more I think about it this does sound like a distinct possibility. Metrics should be on an app wide basis where it's either on or it's off.

I'm going to put this back on the iOS Beta 1 Milestone and go back to building this out.

bleege commented 9 years ago

Continued work on this issue this afternoon. I was able to get the MapboxEvents stated inside the MapboxGL shared instance as well as halfway through @1ec5's code review suggestions. Big thanks to @1ec5 for that BTW.... super helpful for learning some of the finer points of Objective-C!

Next up is to finish the other half of the recommendations.

incanus commented 9 years ago

Happening in #1329 PR.