Layer ownership refactor #6904
Layer ownership refactor #6904
Conversation
fabian-guerra
added
iOS
|
@fabian-guerra, thanks for your PR! By analyzing the history of the files in this pull request, we identified @boundsj, @1ec5 and @incanus to be potential reviewers. |
| @@ -1,5 +1,5 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <document type="com.apple.InterfaceBuilder3.CocoaTouch.Storyboard.XIB" version="3.0" toolsVersion="11201" systemVersion="16A323" targetRuntime="iOS.CocoaTouch" propertyAccessControl="none" useAutolayout="YES" useTraitCollections="YES" colorMatched="YES" initialViewController="PSe-Ot-7Ff"> | |||
| <?xml version="1.0" encoding="UTF-8" standalone="no"?> | |||
There was a problem hiding this comment.
I didn't meant to merge this storyboard.
fabian-guerra
force-pushed
the
fabian-guerra-layer-ownership-refactor
branch
from
3970310
to
4903b19
Compare
…GLForegroundStyleLayer subclasses
| @@ -96,6 +96,9 @@ typedef NS_ENUM(NSUInteger, MGLFillTranslateAnchor) { | |||
| */ | |||
| @property (nonatomic, null_resettable) MGLStyleValue<NSString *> *fillPattern; | |||
|
|
|||
|
|
|||
| - (void)addToMapView:(MGLMapView *)mapView; | |||
There was a problem hiding this comment.
This method needs to be private. We want developers to continue to call -[MGLStyle addLayer:] or -[MGLStyle insertLayer:belowLayer:] as appropriate. To avoid having to create a separate private header for each layer subclass, declare it in the private header for MGLStyle.
There was a problem hiding this comment.
Er, I meant the private header for MGLStyleLayer, MGLStyleLayer_Private.h.
| @@ -2,6 +2,7 @@ | |||
| // Edit platform/darwin/scripts/generate-style-code.js, then run `make style-code-darwin`. | |||
|
|
|||
| #import "MGLSource.h" | |||
| #import "MGLMapView_Private.h" | |||
There was a problem hiding this comment.
These changes will eventually need to happen in all the style layer subclasses. We have a script (mentioned above) that automatically generates these headers. So when you're ready, update the templates that the script uses (the .ejs files in this directory) and run make style-code-darwin. Don't do that until you've updated the templates with your changes, though!
There was a problem hiding this comment.
I have updated the script to import MGLMapView_Private.h
…dated to import MGLMapView_Private header
|
Yesterday when I was profiling I noticed a small memory leak, with the changes we have implemented we no longer have leaks in StyleLayer classes.
|
There was a problem hiding this comment.
@fabian-guerra I think this is looking good! Adding a few suggestions here but nothing major.
| <% } else { -%> | ||
| - (instancetype)initWithIdentifier:(NSString *)identifier source:(MGLSource *)source | ||
| { | ||
| if (self = [super initWithIdentifier:identifier source:source]) { | ||
| _layer = new mbgl::style::<%- camelize(type) %>Layer(identifier.UTF8String, source.identifier.UTF8String); | ||
| [self commonInit:identifier source:source]; |
There was a problem hiding this comment.
Not a big deal but the commonInit pattern is typically used when there is some shared logic that needs to be called by several different initializers and/or something that happens in init also happens latter on. In this case it might be clearer to just do the work in initWithIdentifier:source: itself.
| @@ -5,9 +5,20 @@ | |||
|
|
|||
| #include <mbgl/style/layer.hpp> | |||
|
|
|||
| @class MGLMapView; | |||
|
|
|||
| @interface MGLStyleLayer (Private) | |||
|
|
|||
| @property (nonatomic, readwrite, copy) NSString *identifier; | |||
| @property (nonatomic) mbgl::style::Layer *layer; | |||
There was a problem hiding this comment.
For future reference, I think we should add documentation for this property like we did in MGLSource_Private.h for the rawSource property. When coming back to this code (or looking at it for the first time) it is not immediately obvious that this is a raw pointer that is useable even after layer ownership is transferred in addToMapView:. Also, for consistency, it might be worth considering renaming this to rawLayer (or naming [MGLSource rawSource] to [MGLSource source] (again)).
…add StyleLayers to map
boundsj
removed
the
⚠️ DO NOT MERGE
Fixes #6254
cc @boundsj @1ec5