Skip to content

Conversation

@danesfeder
Copy link
Contributor

Closes #1624

Please see checklist in ticket for details.

cc @akitchen

@codecov-io
Copy link

codecov-io commented Jan 15, 2019

Codecov Report

Merging #1679 into master will increase coverage by 0.2%.
The diff coverage is 63.33%.

@@             Coverage Diff             @@
##             master    #1679     +/-   ##
===========================================
+ Coverage     26.48%   26.69%   +0.2%     
- Complexity      815      823      +8     
===========================================
  Files           202      202             
  Lines          8513     8533     +20     
  Branches        623      627      +4     
===========================================
+ Hits           2255     2278     +23     
+ Misses         6046     6042      -4     
- Partials        212      213      +1

}

@Nullable
private Source findSourceById(List<Source> sources, String streetsUrl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method name threw me for a momentary loop: it’s actually finding the source by URL (specifically, by a tileset ID wrapped in a URL).

if (source instanceof VectorSource) {
VectorSource vectorSource = (VectorSource) source;
String url = vectorSource.getUrl();
if (url != null && url.contains(streetsUrl)) {
Copy link
Contributor

@1ec5 1ec5 Jan 15, 2019

Choose a reason for hiding this comment

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

A source URL might look like this and still be Streets source v8 for the purposes of this feature:

  • mapbox://mapbox.mapbox-streets-v8
  • mapbox://mapbox.mapbox-streets-v8,mapbox.mapbox-traffic-v1
  • mapbox://mapbox.mapbox-streets-v8?foo=bar
  • mapbox://mapbox.mapbox-streets-v8/?foo=bar

On iOS, we create a URL struct, check that the scheme is mapbox, and split the host on commas. Is it reasonably efficient to create a java.net.URL for each source URL? If not, then it’d be reasonable to search for just the tileset ID within the full URL string. That’ll work until we release Mapbox Streets source v70. 😉

private static final String MAPBOX_STREETS_V7 = "mapbox://mapbox.mapbox-streets-v7";
private static final String MAPBOX_STREETS_V7_URL = "mapbox://mapbox.mapbox-streets-v7";
private static final String MAPBOX_STREETS_V8_URL = "mapbox://mapbox.mapbox-streets-v8";
private static final String STREETS_SOURCE_ID = "streetsSource";
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to take this opportunity to make the source ID more unique, like com.mapbox.navigation.streets.

@danesfeder danesfeder force-pushed the dan-streets-v8 branch 2 times, most recently from 3fb3952 to 2857f5a Compare January 16, 2019 18:39
@danesfeder
Copy link
Contributor Author

@1ec5 this is up-to-date and ready for another round when you have a cycle.

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Looks good to me ✅

@danesfeder danesfeder merged commit cd56a39 into master Jan 16, 2019
@danesfeder danesfeder deleted the dan-streets-v8 branch January 16, 2019 19:50
@danesfeder danesfeder mentioned this pull request Jan 16, 2019
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants