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

DefaultFileSource has responsibility for handling mapbox:// URLs #1607

Merged
merged 1 commit into from
May 26, 2015

Conversation

jfirebaugh
Copy link
Contributor

This moves Map::setAccessToken to DefaultFileSource::setAccessToken and passes mapbox:// URLs through the FileSource interface. No change to the iOS or Android SDK public API. The node bindings will need the following changes (cc @mikemorris):

  • New values for kind enum.
  • FileSource implementations need to handle mapbox:// URLs.
  • Remove setAccessToken from Map interface. node-mapbox-gl-native itself no longer needs to concern itself with access tokens. This is a task for FileSource implementations now.

Before merging this I'm planning to make another change to the interface, removing the kind enum in favor of specific FileSource methods like requestStyle, requestTile, etc.

@tmpsantos
Copy link
Contributor

Nice!

@1ec5
Copy link
Contributor

1ec5 commented May 20, 2015

At a glance, it looks like this will fix #1334 as well. 👍

@mikemorris
Copy link
Contributor

Liking the direction here 👍

Thinking that breaking out separate requestTile, requestStyle methods could be unnecessary overhead though.

@1ec5
Copy link
Contributor

1ec5 commented May 20, 2015

I also like that the access token is no longer part of the style URL, so if the access token changes at some point during the lifetime of the file source, we don’t have to go updating other fields. This turns out to jive nicely with the last bit of refactoring I’m doing for #1553, where I’m also removing accessToken from MGLMapView.

1ec5 added a commit that referenced this pull request May 20, 2015
The singleton `MGLAccountManager` wants to be the sole arbiter of the access token, but each instance of `mbgl::Map` (`mbgl::DefaultFileSource` in #1607) has its own copy of the access token. Now `MGLMapView` observes for changes to the `MGLAccountManager`’s access token and synchronizes `mbgl::Map` with it.
@@ -148,12 +148,12 @@ - (instancetype)initWithCoder:(NSCoder *)decoder

- (NSString *)accessToken
{
return @(_mbglMap->getAccessToken().c_str()).mgl_stringOrNilIfEmpty;
return @(_mbglFileSource->getAccessToken().c_str()).mgl_stringOrNilIfEmpty;
Copy link
Contributor

Choose a reason for hiding this comment

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

With #1553, MGLMapView no longer calls getAccessToken() anywhere.

@jfirebaugh
Copy link
Contributor Author

@1ec5 Thanks -- I'll rebase this on master (post #1553) and redo the iOS portions.

@jfirebaugh jfirebaugh force-pushed the file-source-access-token branch 2 times, most recently from c7afc7b to e5d659a Compare May 22, 2015 00:45
@ljbade
Copy link
Contributor

ljbade commented May 24, 2015

Looks good on Android side

@kkaefer kkaefer merged commit 87b1da7 into master May 26, 2015
@kkaefer kkaefer deleted the file-source-access-token branch May 26, 2015 10:40
@kkaefer
Copy link
Contributor

kkaefer commented May 26, 2015

Before merging this I'm planning to make another change to the interface, removing the kind enum in favor of specific FileSource methods like requestStyle, requestTile, etc.

We can revisit this in another pull request. Any particular reason for doing so? We don't actually use these enums anywhere (except as a hint for compressing responses), so we could do away with them as well?

@jfirebaugh
Copy link
Contributor Author

We can revisit this in another pull request. Any particular reason for doing so?

I would like to avoid interface churn for node-mbgl or whatever the downstream consumer that implements the FileSource interface in node is nowadays. Maybe we can avoid updating it until the FileSource interface is finalized?

@mikemorris
Copy link
Contributor

Working on implementing this, and I think we'll need to add some capacity to pass an access token through a Map::RenderStill call through to the mbgl::Request objects created by the FileSource implementation. Explicitly setting a global access token is going to cause race conditions when multiple maps use the same FileSource.

^ This concern will eventually be irrelevant, as the pluggable FileSource backend won't need to handle access tokens and will load resources directly.

mikemorris added a commit to cutting-room-floor/node-mapbox-gl-native that referenced this pull request May 28, 2015
- Document Node.js v0.12.x and io.js support
- Document map.release functionality.
- Add notes on garbage collection and request handling.
- Update binary platforms and build instructions.
- Document API token FileSource implementation from
  mapbox/mapbox-gl-native#1607
mikemorris added a commit to cutting-room-floor/node-mapbox-gl-native that referenced this pull request May 28, 2015
- Document Node.js v0.12.x and io.js support
- Document map.release functionality.
- Add notes on garbage collection and request handling.
- Update binary platforms and build instructions.
- Document API token FileSource implementation from
  mapbox/mapbox-gl-native#1607
- Update Kind enum values.
mikemorris added a commit to cutting-room-floor/node-mapbox-gl-native that referenced this pull request Jun 15, 2015
- Document Node.js v0.12.x and io.js support
- Document map.release functionality.
- Add notes on garbage collection and request handling.
- Update binary platforms and build instructions.
- Document API token FileSource implementation from
  mapbox/mapbox-gl-native#1607
- Update Kind enum values.
@mikemorris
Copy link
Contributor

removing the kind enum in favor of specific FileSource methods

-> #3374

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants