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

[ios, macos] Put MGLStyleLayer inits in respective classes #6269

Merged
merged 1 commit into from
Sep 8, 2016

Conversation

boundsj
Copy link
Contributor

@boundsj boundsj commented Sep 7, 2016

Previously, we declared MGLStyleLayer initializers in the MGLStyleLayer protocol as optional methods. In the generated implementation, this made it easier to opt in and out of initializers that did or or did not make sense for the subclass. However, this approach was dangerous since it was easy for an application developer to initialize an MGLStyleLayer subclass with an init method that was actually not implemented in that class causing an exception at runtime.

This commit moves the init methods that each subclass supports to each subclass so xcode (and the compiler) help the developer avoid the previously possible runtime exception.

In addition, a new init method is added that takes the source layer and passes that on to mbgl::style:Layer (but only on classes where this is possible in core). This allows an application developer to style a specific source layer (i.e. the contour lines of the mapbox terrain vector source).

Fixes #6250
Fixes #6252

@frederoni @1ec5 do you think this approach is reasonable or did I miss a requirement that the initializers should be in the protocol and optional?

Todo:

  • confirm tests work
  • test that contour example works
  • review from @1ec5 @frederoni
  • clean up commented code
  • clean up code generation script

@boundsj boundsj added iOS Mapbox Maps SDK for iOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold runtime styling labels Sep 7, 2016
@boundsj boundsj added this to the ios-v3.4.0 milestone Sep 7, 2016
@boundsj boundsj self-assigned this Sep 7, 2016
@mention-bot
Copy link

@boundsj, thanks for your PR! By analyzing this pull request, we identified @1ec5, @incanus and @jfirebaugh to be potential reviewers.

@boundsj
Copy link
Contributor Author

boundsj commented Sep 7, 2016

With this change's -[MGLFillStyleLayer initWithLayerIdentifier:sourceIdentifier:sourceLayer:] it is now possible to style a specific layer for a source, like:

NSURL *url = [[NSURL alloc] initWithString:@"mapbox://mapbox.mapbox-terrain-v2"];
MGLVectorSource *vectorSource = [[MGLVectorSource alloc] initWithSourceIdentifier:@"terrain-data" URL:url];
[self.mapView.style addSource:vectorSource];

// style the "contour" source layer specifically
MGLLineStyleLayer *lineLayer = [[MGLLineStyleLayer alloc] initWithLayerIdentifier:@"terrain-data" sourceIdentifier:@"terrain-data" sourceLayer:@"contour"];

NSUInteger lineJoineValue = (NSUInteger)MGLLineStyleLayerLineJoinRound;
    [lineLayer setLineJoin:[NSValue value:&lineJoineValue withObjCType:@encode(MGLLineStyleLayerLineJoin)]];
NSUInteger lineCapValue = (NSUInteger)MGLLineStyleLayerLineCapRound;
[lineLayer setLineCap:[NSValue value:&lineCapValue withObjCType:@encode(MGLLineStyleLayerLineCap)]];
[lineLayer setLineColor:[UIColor redColor]];
[self.mapView.style addLayer:lineLayer];

screen shot 2016-09-06 at 9 59 37 pm

global.initLayer = function (layerType) {
if (layerType == "background") {
return `_layer = new mbgl::style::${camelize(layerType)}Layer(layerIdentifier.UTF8String);`
return `_layer = new mbgl::style::${camelize(layerType)}Layer(layerIdentifier.UTF8String);`
Copy link
Contributor

@frederoni frederoni Sep 7, 2016

Choose a reason for hiding this comment

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

We're leaking this layer and all other mbgl::style::layers. Should we delete the ref in -dealloc or rewrite the getters and setters to make them access the layer in a lazy way?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to take a step back and address this leak in #6254. Until then, any patches will only add to the confusion.

@1ec5
Copy link
Contributor

1ec5 commented Sep 7, 2016

How does the developer discover the layers of a source? Maybe we need a new method on MGLSource for getting an MGLSourceLayer by identifier or a property that returns all the source layers.

@boundsj
Copy link
Contributor Author

boundsj commented Sep 7, 2016

@1ec5 how do developers do source layer discovery on JS and Android? I don't see any mention of it in examples like the GL JS example for adding a third party vector source or in this Android demo app example.

Source discovery sounds great but also like a different feature. So far, I just assumed that we expect developers to read the documentation of the source (i.e. https://www.mapbox.com/vector-tiles/mapbox-terrain/)

@jfirebaugh
Copy link
Contributor

You should never use new or delete directly in C++ code, always std::make_unique or std::make_shared. Layer ownership is not shared, so std::make_unique<LayerType>(...), then std::move ownership into Map#addLayer when the layer is added. You will likely want to retain a raw pointer for the MGLStyleLayer's use, and for consistency with style::Layer*s obtained via Map#getLayer.

@boundsj
Copy link
Contributor Author

boundsj commented Sep 7, 2016

I added #6285 as a placeholder for adding source layer discovery at some point.

Regarding #6269 (comment) and #6269 (comment) let's take @1ec5's advice and handle that in #6254

This PR should be ready to go now. oops I need to fix a broken macos test first!

@boundsj boundsj added ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold and removed ✓ ready for review ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Sep 7, 2016
@boundsj
Copy link
Contributor Author

boundsj commented Sep 8, 2016

Ok I think we are good here. Merging.

Previously, we declared MGLStyleLayer initializers in the MGLStyleLayer
protocol as optional methods. This made it easy to opt in and out
of initializers that did or or did not make sense for the subclass.
However, this approach was dangerous since it was easy for an
application developer to initialize an MGLStyleLayer subclass with an
init method that was actually not implemented in that class causing
an exception at runtime.

This commit moves the init methods that each subclass supports to
each subclass so xcode (and the compiler) help the developer avoid
the previously possible runtime exception.

In addition, a new init method is added that takes the source layer
and passes that on to `mbgl::style:Layer` (but only on classes
where this is possible in core). This allows an application developer
to style a specific source layer (i.e. the contour lines of the mapbox
terrain vector source).

Finally, this refactors MGLStyleLayer classes to use an MGLSource instead of a
string identifier for the source when initializing the style.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS runtime styling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants