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

Cannot introspect source or layer obtained from style #6584

Closed
1ec5 opened this issue Oct 5, 2016 · 6 comments
Closed

Cannot introspect source or layer obtained from style #6584

1ec5 opened this issue Oct 5, 2016 · 6 comments
Assignees
Labels
Android Mapbox Maps SDK for Android archived Archived because of inactivity bug Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling

Comments

@1ec5
Copy link
Contributor

1ec5 commented Oct 5, 2016

A source or style layer obtained from a map’s current style has its identifier set but has no other properties set. Therefore, there’s no way to obtain the source associated with a particular layer that comes with the Mapbox Streets style, nor is there a way to get the tile sets associated with that source.

I’m filing this issue in the course of fixing #6406, based on a close reading of the iOS/macOS and Android code base. To reproduce this issue on iOS or macOS, try the following code with the Mapbox Streets v9 style:

NSURL *url = [NSURL URLWithString:@"mapbox://mapbox.mapbox-terrain-v2,mapbox.mapbox-streets-v7"];
NSAssert([[mapView.style layerWithIdentifier:@"country-label-md"].source.URL isEqual:url]);

To fix this issue, we’ll have to expose more members of mbgl::style::Layer and mbgl::style::Source that up to now have been stashed away in private Impl classes. For instance, we’ll need an mbgl::style::VectorSource::getTileset() that returns the existing mbgl::style::TileSourceImpl::tileset. I’m sure these members were hidden for good reason. To mitigate any questions about the expected lifetime of the returned object, it may be necessary to fix #6254 in the course of fixing this bug. (#6254 probably applies to the Android SDK as well.)

/cc @jfirebaugh @ivovandongen @incanus @frederoni @boundsj

@1ec5 1ec5 added bug iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl runtime styling labels Oct 5, 2016
@jfirebaugh
Copy link
Contributor

I’m sure these members were hidden for good reason.

Not really, there's just never been a requirement to expose accessors for them before.

API design considerations:

  • I'd prefer not to expose a Tileset / TileJSON concept in public APIs. I think these concepts are on their way out, and sources will eventually just have inline properties for their URL templates, zoom range, etc. So those properties are best exposed directly on the source object, without an intermediate "Tileset" domain object.
  • In the current implementation, the tileset properties might not be available during the initial load, while the TileJSON is being requested. This wrinkle will go away if we inline TileJSON properties, but for now the API needs to take it into account.

@boundsj
Copy link
Contributor

boundsj commented Oct 5, 2016

I'd prefer not to expose a Tileset / TileJSON concept in public APIs.

Noting that this concept was recently added to the iOS SDK in #6316 to support the initialization of sources with tile templates.

@ivovandongen
Copy link
Contributor

Noting that this concept was recently added to the iOS SDK in #6316 to support the initialization of sources with tile templates.

Also has been a part of the Android API for a bit now. If we want to change this, we need to do this either before the upcoming 4.2 release or wait until 5.0.

@jfirebaugh
Copy link
Contributor

It's probably fine used as a package of parameters you can pass when creating a source. I'm saying, write VectorSource::getTileURLTemplates(), VectorSource::getZoomRange(), etc., not a single VectorSource::getTileset().

@1ec5
Copy link
Contributor Author

1ec5 commented Dec 11, 2016

The main thing developers would want to extract from a GeoJSON source would be the (untiled) GeoJSON it contains: #7376.

@1ec5 1ec5 modified the milestones: ios-v3.5.0, ios-v3.4.1 Jan 24, 2017
@jfirebaugh jfirebaugh removed this from the ios-v3.5.0 milestone Mar 9, 2017
@brunoabinader brunoabinader self-assigned this Mar 13, 2017
@stale stale bot added the archived Archived because of inactivity label Nov 11, 2018
@stale
Copy link

stale bot commented Nov 26, 2018

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this as completed Nov 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android archived Archived because of inactivity bug Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling
Projects
None yet
Development

No branches or pull requests

5 participants