[darwin] Remove unnecessary MGLTileSource initializers #8633
Conversation
| After initializing and configuring the source, add it to a map view’s style | ||
| using the `-[MGLStyle addSource:]` method. | ||
|
|
||
| #### Tile URL templates |
There was a problem hiding this comment.
Redeclaring -initWithIdentifier:tileURLTemplates:options: in the abstract superclass gave us a common place to put this substantial amount of documentation. It’s important for the developer to know that the identically or similarly named initializers in the subclasses behave similarly – or even to think of MGLRasterSource and MGLVectorSource as sharing the same initializers – even if they don’t share the same implementation in mbgl under the hood (but the developer doesn’t need to know that).
If we need to eliminate the superclass method, we should at least factor this documentation out into a jazzy guide and link to it from each identical method instead of duplicating it. It would take the developer awhile to determine that both copies are the same.
There was a problem hiding this comment.
There's currently no indication in the documentation of how to initialize MGLVectorSource. I think this is an improvement, despite the duplication.
There was a problem hiding this comment.
There’s the line @interface MGLVectorSource : MGLTileSource at the top, which links to the documentation for MGLTileSource. However, that page says “Do not create instances of this class directly”, which the developer may misinterpret to mean “don’t use these methods at all”. This is a shortcoming of our documentation for abstract classes in general (and of some of Apple’s own abstract class documentation).
There was a problem hiding this comment.
This PR is technically sound. #8634 and #8635 will clean up the duplication introduced by it. Technically, removing this public method is a backwards-incompatible change. However, it’s extremely unlikely that a developer would’ve called it directly, since MGLTileSource is an abstract class. The most likely usage scenario isn’t particularly likely:
MGLTileSource *tileSource = [[isRaster ? MGLRasterSource : MGLVectorSource alloc] initWithIdentifier:@"tiles" tileURLTemplates:@[@"…"] options:nil];We could add a blurb to the iOS and macOS release notes about this change, as a courtesy just in case.
| After initializing and configuring the source, add it to a map view’s style | ||
| using the `-[MGLStyle addSource:]` method. | ||
|
|
||
| #### Tile URL templates |
1ec5
added
documentation
iOS
| auto source = std::make_unique<mbgl::style::RasterSource>(identifier.UTF8String, | ||
| configurationURL.mgl_URLByStandardizingScheme.absoluteString.UTF8String, | ||
| uint16_t(round(tileSize))); | ||
| return [super initWithPendingSource:std::move(source)]; |
There was a problem hiding this comment.
Initializers should always follow the pattern if (self = [super …]) { … } return self;, even if it looks redundant.
There was a problem hiding this comment.
Even if the body of the if is empty? What's the reason for that?
There was a problem hiding this comment.
If you wanted to, you could say return self = [super …], but the if is a convention that makes it easier to add and initialize ivars later on. The important part is assigning self and returning it; otherwise, ARC is free to assume that the object returned by the super call is no longer in use and can release the object. (Along these lines, if you’re partway through the if block and discover that the object can’t be initialized for some reason, setting self to nil has the same effect of releasing the object.)
| Initializes and returns a source with an owning pointer to the backing store, | ||
| unassociated from a style. | ||
| */ | ||
| - (instancetype)initWithPendingSource:(std::unique_ptr<mbgl::style::Source>)pendingSource; |
There was a problem hiding this comment.
This method doesn’t appear to be implemented in MGLSource.mm. Calling it will result in a crash.
Introduce `[MGLSource initWithPendingSource:]`, which allows it to track the owned pendingSource pointer and implement -addToMapView: and -removeFromMapView: without any casts.
jfirebaugh
force-pushed
the
darwin-source-initializers
branch
from
d3fba5e to
9b60317
Compare
|
Rolling this into #8626. |
Both
-initWithIdentifier:configurationURL:and-initWithIdentifier:tileURLTemplates:options:threw away all but the first argument and called the superclass-initWithIdentifier. Working versions of these initializers are provided by the subclassesMGLRasterSourceandMGLVectorSource, so there's no need to define stubs in the base class.