Refactor initializers and pointer management in MGLSource and MGLStyleLayer hierarchies

[jfirebaugh]

Introduce -[MGLSource initWithPendingSource:], allowing the base class to track the owned _pendingSource pointer and implement -addToMapView: and -removeFromMapView: without any casts.

Similarly, introduce -[MGLStyleLayer initWithPendingLayer:], allowing the base class to track the owned _pendingSource pointer and implement -addToMapView: and -removeFromMapView: without any casts. This fixes #7437, where -[MGLStyle layerFromMBGLLayer:] would wind up creating layers whose _rawLayer and _pendingLayer held different values.

Finally, implement object identity for MGLSource* and MGLStyleLayer*: all MGLSource pointers referencing the same logical source will now be object identical; similarly for MGLStyleLayer. This lays groundwork for #7375.

• Update iOS Xcode project
• Manually test MGLOpenGLStyleLayer
• Resolve compiler warnings related to designated/convenience intializers

[1ec5]

The MGLSource can always be obtained from the mbgl::style::Layer, so we could move -[MGLStyle sourceFromMBGLSource:] to +[MGLSource sourceWithRawSource:] in MGLSource_Private.h (alongside the existing -[MGLSource initWithRawSource:]). That would make -[MGLStyle layerFromMBGLLayer:] a bit cleaner, since we wouldn’t need to pass in an MGLSource every time we wrap mbgl::style::Layer.

Not a big deal, but possibly worth doing the next time we have to touch this code.

[jfirebaugh]

Not sure I follow. Are you suggesting to remove the source parameter from the various initWithRawLayers? How would the implementation of initWithRawLayer obtain a mbgl::style::Source * to pass to +[MGLSource sourceWithRawSource:]?

[1ec5]

Ah, you’re right – I mistakenly thought the mbgl::style::Layer has a method to get the mbgl::style::Source, but of course it all comes from the mbgl::Map by ID.

[1ec5]

Initially, I thought about declaring just -[MGLSource initWithRawLayer:source:] (taking mbgl::style::Layer in a private header and only declaring specialized methods like this in .m files. But this is safer, so 👍.

[1ec5]

This is because the undo action stores a reference to a copy of the style layer before it has been removed from the style, rather than the copy that is removed from the style. Only the latter is valid to add back to the style.

To elaborate, the call to -[NSArrayController objectsAtIndexes:] creates a series of MGLStyleLayer objects (via -[MGLStyle objectInLayersAtIndex:]), which continues to think each layer is still part of the style. Meanwhile, -[NSArrayController removeObjectsAtArrangedObjectIndexes:] causes a different series of MGLStyleLayer objects to be created (again via -[MGLStyle objectInLayersAtIndex:]).

We could fix this issue either by keeping a one-to-one relationship between Objective-C and C++ objects, as proposed in #7375 (comment) or #7375 (comment). In the meantime, a simpler fix would be to modify -[MapDocument deleteStyleLayersAtArrangedObjectIndexes:] to pass layers into -[NSArrayController removeObjects:] instead of -removeObjectsAtArrangedObjectIndexes:, ensuring that we only deal with one set of MGLStyleLayer objects the whole time.

[jfirebaugh]

modify -[MapDocument deleteStyleLayersAtArrangedObjectIndexes:] to pass layers into -[NSArrayController removeObjects:] instead of -removeObjectsAtArrangedObjectIndexes:, ensuring that we only deal with one set of MGLStyleLayer objects the whole time.

This doesn't seem to work. Presumably because styleLayersArrayController has MGLStyleLayer objects of different identity than those created by reversedLayers, nothing gets removed from the list.

I'll work on implementing the one-to-one relationship instead.

[jfirebaugh]

Updated the OP with the new, more complete, strategy here.

@1ec5 Can you re-review, and in particular help me with the compiler warnings about "Designated initializer invoked a non-designated initializer"? When I try to add NS_DESIGNATED_INITIALIZER where it looks like I'm supposed to (on the base class -initWith... methods), I get the error "'objc_designated_initializer' attribute only applies to init methods of interface or class extension declarations".

The PR changed substantially since the review, so I’ll need to look more closely at it.

[1ec5]

When I try to add NS_DESIGNATED_INITIALIZER where it looks like I'm supposed to (on the base class -initWith... methods), I get the error "'objc_designated_initializer' attribute only applies to init methods of interface or class extension declarations".

NS_DESIGNATED_INITIALIZER can only be used on method declarations in the main interface of a class or a class extension (in a .m or .mm file), whereas -initWithRawLayer: and -initWithPendingLayer: are declared on a category of MGLStyleLayer. In other words, the designated initializer must either be visible to the public or be internal to a class.

Can you re-review, and in particular help me with the compiler warnings about "Designated initializer invoked a non-designated initializer"?

One solution would be to add a separate _pendingLayer ivar to each concrete class and assign to that ivar within the concrete subclasses’ initializer implementations (i.e., -[MGLBackgroundStyleLayer initWithIdentifier:]). The superclass implementations would do little more than call the superclass initializers.

[1ec5]

Finally, implement object identity for MGLSource* and MGLStyleLayer*: all MGLSource pointers referencing the same logical source will now be object identical; similarly for MGLStyleLayer.

This doesn’t appear to be the case. If I add an MGLShapeSource to the style:

MGLPointFeature *point = [[MGLPointFeature alloc] init];
MGLShapeSource *shapeSource = [[MGLShapeSource alloc] initWithIdentifier:@"foo" shape:point options:nil];
[style addSource:shapeSource];

then, once shapeSource goes out of scope, attempt to get the source by its identifier:

MGLSource *source = [self.mapView.style sourceWithIdentifier:@"foo"];

source and shapeSource have different pointer values:

Printing description of shapeSource:
<MGLShapeSource: 0x60800005b4e0; identifier = foo; URL = (null); shape = <MGLPointFeature: 0x608000463e80; title = (null); subtitle = (null); coordinate = 0.000000, 0.000000>>
Printing description of source:
<MGLShapeSource: 0x60800025c140; identifier = foo; URL = (null); shape = (null)>

LayerWrapper holds a weak pointer to the MGLStyleLayer object. In the absence of some other strong reference to the object, the object will be deallocated as soon as we leave the scope in which the object is initialized (and the layer member of LayerWrapper will become nil). Later, when obtaining the layer by identifier, we automatically rewrap the mbgl::style::Layer in a new MGLStyleLayer object as before. We never reach this line that reuses the original MGLStyleLayer object.

[jfirebaugh]

One solution would be to add a separate _pendingLayer ivar to each concrete class and assign to that ivar within the concrete subclasses’ initializer implementations (i.e., -[MGLBackgroundStyleLayer initWithIdentifier:]).

But that's exactly what this PR is trying to undo and avoid, because it requires that every concrete class implement an essentially identical -addToMapView: and -removeFromMapView:.

Is there another solution? Why does this work fine for MGLSource?

This doesn’t appear to be the case...

Yes, this implementation only guarantees that any two simultaneously live references are identical. This is enough to fix the issue noted in #8626 (comment). If we need object identity even with no live references, we'll need a more complex scheme, where the the peer stores a weak pointer when pending but strong otherwise. This will also increase the baseline memory consumption.

[1ec5]

Is there another solution? Why does this work fine for MGLSource?

Solving this for MGLSource required eliminating all the initializers of intermediate classes (namely MGLTileSource). I guess we need to eliminate -[MGLForegroundStyleLayer initWithPendingLayer:source:] somehow?

[jfirebaugh]

I think I figured it out. The key seemed to be not the intermediate class initializers (the warning still occurred after 791654c), but rather providing -[MGLStyleLayer initWithIdentifier:], mirroring that initializer in MGLSource.

[jfirebaugh]

This results in stack overflow for MGLBackgroundStyleLayer, because the call to [self initWithIdentifier:identifier] in -[MGLStyleLayer initWithRawLayer:] winds up doing a virtual dispatch to e.g. -[MGLBackgroundStyleLayer initWithIdentifier:]. Is there a way to disable virtual dispatch for this particular call?

[1ec5]

Is there a way to disable virtual dispatch for this particular call?

Unfortunately, there isn’t. (I guess anything is possible with swizzling, but there be dragons.) super is the only way to completely avoid calling subclass initializers like that, but MGLStyleLayer’s superclass is NSObject. It is possible to suppress the warnings you were getting earlier with pragmas, but the warnings do point to a problem we’re facing.

According to Objective-C’s initializer rules, each convenience initializer must call a designated initializer on self, and each designated initializer must call a designated initializer on super. -[MGLStyleLayer initWithRawLayer:] is running against these rules by calling -initWithIdentifier: on self.

So I think it was correct that -[MGLStyleLayer initWithRawLayer:] previously called -[NSObject init] and incorrect that it now calls -[MGLStyleLayer initWithIdentifier:]. But I’m still unsure about the warnings.

[1ec5]

MGLStyle has code that calls e.g. -[MGLFillStyleLayer initWithRawLayer:]. The NS_DESIGNATED_INITIALIZER attribute on -[MGLFillStyleLayer initWithIdentifier:] means -[MGLFillStyleLayer initWithRawLayer:] would be expected to call -[MGLFillStyleLayer initWithIdentifier:]. Normally we’d fix that by redeclaring -initWithRawLayer: on MGLFillStyleLayer with an NS_DESIGNATED_INITIALIZER attribute, but we can’t, because the declaration would have to live in a category. We might end up having to remove all the NS_DESIGNATED_INITIALIZER attributes, which would certainly eliminate the warnings but also make it less clear to developers what the primary initializer would be for each class.

[jfirebaugh]

The root of the problem seems to be that designated initializers can't belong to a private category. That's really what we want: a set of initializers that are a) private except to concrete subclasses and b) must be called by concrete subclasses. If that was possible, we could find some way to satisfy the other Objective-C initializer rules (though they do seem unnecessarily restrictive to me).

If that's not possible, I say we just remove NS_DESIGNATED_INITIALIZER from style layer subclasses. Each concrete style layer class has exactly one initializer, so there shouldn't be any confusion around primary vs secondary.

[1ec5]

Thanks for this fix. Removing the designated initializer attributes is unfortunate but unavoidable.

For the most part, there should be little developer impact, but let's add a note to both changelogs about the bug fix and the fact that these initializers have been moved to the concrete classes.