Make source ownership consistent and make MGLGeoJSONSource's content properties mutable

[boundsj]

WIP for source portion of: #6254
Fixes #6159

[mention-bot]

@boundsj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @1ec5, @frederoni and @ituaijagbone to be potential reviewers.

[1ec5]

Per #6254 (comment), MGLSource should follow the same invalidation pattern that we use in MGLOfflinePack, another class that wraps but does not own a C++ object.

[1ec5]

The distinction between source and rawSource will need to be documented well, because it isn’t at all clear to anyone encountering this code for the first time.

[1ec5]

#6254 (comment) seems like a more straightforward alternative to source. The proxy approach explicitly ties an MGLSource instance to a single MGLMapView instance – a dependency that implicitly exists in this PR due to the unique_ptr.

We could even avoid the invalidation pattern, if desired: if the developer calls -[MGLGeoJSONSource setFeatures:] but -[MGLStyle sourceForIdentifier:] comes up empty-handed, that means the source either has yet to be added or has been removed from the style; we can fall back to building up a pendingSource internally that can be added to the style later.

[boundsj]

Thanks @1ec5. Putting aside the lack of clarity about rawPointer and invalidation for the moment, I think a common scenario that the PR in its current form (actually with a few typo fixes but ignoring that, too, for the moment) handles well is:

MGLGeoJSONSource *source = [[MGLGeoJSONSource alloc] initWithIdentifier:@"mutable-data-source-features-id" features:@[smallBoxFeature] options:nil];
[self.mapView.style addSource:source];

MGLFillStyleLayer *layer = [[MGLFillStyleLayer alloc] initWithIdentifier:@"mutable-data-layer-features-id" source:source];
MGLStyleValue *fillColor = [MGLStyleValue<UIColor *> valueWithRawValue:[UIColor redColor]];
layer.fillColor = fillColor;
[self.mapView.style addLayer:layer];

///
// sometime later...
///
source.features = @[largeBoxFeature];

In this scenario, a source is added to (and forever tied to as you've noted) the map. At any point, the features (or URL or data) of this MGLGeoJSONSource can be mutated without having to ask the map view to give it back again. In the current beta, you must also ask the map for the source but only because the underlying std::move wipes out the reference and there is no notion of a "raw pointer" in the beta. I think we've already seen issues related to the requirement of the map as the middleman causing confusion. Also, I don't think the current PR rules out avoiding the invalidation pattern. If a developer calls setFeatures: and rawSource is null (because the MGL source has not been added or was removed at some point) then the features (or URL or data) can simply trigger the creation of a new "pending" source value (pending until it is moved to the mbgl map that is).

I agree that this PR won't work for multiple map instances. I guess my question is would it be better to optimize for that up front with the identifier based approached that you outlined? Or would it be better to optimize for the (probably more common) single map view approach and note in the documentation that developers must create sources per map/view?

[1ec5]

I agree that this PR won't work for multiple map instances.

My suggestion wouldn’t work for multiple map instances too – in fact, it makes the dependency more explicit, because the source would hold a weak pointer to an MGLMapView instead of some mbgl::Source object that just happens to be owned by the MGLMapView via mbgl::Map and mbgl::Style.

The rawSource/source approach is fine for now, but it should be documented so we understand the difference when we come back to this code later. My suggestion about making MGLSource a proxy for mbgl::Source can be the subject of a separate PR, if we decide to go that route.

[boundsj]

Thanks @1ec5 and sorry for the misunderstanding. I will continue with this approach for now and I've also brought over changes from #6777. I've updated the title and description to reflect that.

[boundsj]

This is ready for review. It makes MGLGeoJSONSource content properties (features, geoJSONData, and URL) settable. It also refactors the way MGLSource objects manage their reference to their mbgl source. Through the introduction of a "raw" pointer to keep a consistent reference, a rename of source to pendingSource, and comments regarding these internal properties, it clarifies how the transfer of ownership of the mbgl source works and makes the API more forgiving to application developers without the need to introduce an invalidation pattern in the MGLSource object.

Notes:

• Setting MGLGeoJSONSource URL has no visible effect currently since Mutable geojson source #6777 is not in this PR. This will be fixed when we merge master into the iOS release branch.
• Qt tests are failing since we don't have Qt macOS fixes #6804. This will be fixed when we merge master into the iOS release branch.

cc @1ec5 @incanus @frederoni

[1ec5]

Looks good to me.

@jfirebaugh, can you double-check that these changes match what you suggested in #6254 (comment)?

[jfirebaugh]

Is it possible to put the std::move here, rather than in the pendingSource property accessor? That would make it clearer where transfer of ownership is actually occurring.

[boundsj]

Thanks @jfirebaugh. a43911e clarifies the transfer of ownership at the call site but might introduce other readability issues. I don't think we can do this with the @Property since it represents a single set/return signature for the value. I just did it this way (adding setter/getter) to make it work and to get a sense of how it would look. Maybe there is a better, alternative approach that allows the std::move to stay at the actual ownership transfer location? cc @1ec5

If not, I'm tempted to revert a43911e unless you feel strongly about it.

[1ec5]

Here’s an idea that still requires indirection but limits the gymnastics to well-worn techniques:

• Remove the _pendingSource ivar from MGLSource and add it as an ivar in each concrete subclass instead. All the current calls to -setPendingSource: become direct assignments to _pendingSource.
• Have each concrete subclass of MGLSource implement an -addToMapView: method that takes a reference to an mbgl::Map and calls addSource() on it, passing in std::move(_pendingSource).

Do you anticipate any other reasons we’d need to get the pending source like that?

[boundsj]

I went with deleting a43911e @1ec5. IMO, the simplicity of working with the pendingSource as a property outweighs the clarity benefit in the one place where it is actually transferred.

Relatedly:

Do you anticipate any other reasons we’d need to get the pending source like that?

I don't think there will be other reasons to get the pending source. The platform code should always deal with MGLSource's public API (the content properties) and those use the raw pointer internally. The pending source pointer exists only to be transferred to the mbgl object.

[1ec5]

The pending source pointer exists only to be transferred to the mbgl object.

To me, that suggests we may want to go with the approach I outlined above. Not only does it make it clear where ownership is transferred, but it also prevents misuse of the pendingSource pointer. Currently you can use that property however you like.

[boundsj]

That's true. I'll go with your approach then.

[boundsj]

Fixed in 19dc2cb

[boundsj]

The two issues noted above related to commits missing from master should be resolved since this was rebased on top of release-ios-v3.4.0 and master was merged into it.

[1ec5]

-[MGLSource initWithIdentifier:] should implement a private -commonInit stub so that -[MGLSource initWithIdentifier:] should call it and consolidate all these disparate calls. Each MGLSource subclass can still override the method to do things specific to the particular kind of mbgl::style::Source it’s working with.

[boundsj]

That would be a nice refactor. However, commonInit relies on having properties like URL and geoJSONData and features set before it runs. If MGLSource's initWithIdentifier: calls commonInit before the subclass init can set the requisite properties, then unexpected results will occur. I can imagine some ways to work around this but I think the disparate calls may actually be the best way to go, for now.

#6793 (comment)

[1ec5]

Either revert a43911e or implement #6793 (comment).

a43911e has been reverted.

[nitrag]

Is this setup such that a new features set can be added without having to reload the entire source when new data is detected? (Thinking about performance). I'm interested in having one GeoJSON layer and adding additional GeoJSON features on-the-fly/later on.

...Or is this a feature request?

[1ec5]

The MGLGeoJSONSource.features array is now read-write, which means you can copy it, mutate the copy, and set features to the copy in order to add features on the fly. From your perspective, the source never gets torn down and recreated. The same is true of the geoJSONData property should you find that format more convenient than MGLFeature.

There is overhead associated with either workflow, namely that the same features need to get round-tripped between C++ and Objective-C representations. We could alleviate that overhead by making the features array mutable (i.e., NSMutableArray and mutable KVO methods like -addFeature:). But this and any other enhancements should be tracked in separate issues.

[nitrag]

Hmm.... I'm getting nil when calling .geoJSONData or .features what am I doing wrong?

func mapViewDidFinishLoadingMap(_ mapView: MGLMapView) {
   ...
   let geoJSONURL = Bundle.main.url(forResource: "example", withExtension: "geojson")!
   //would be nice if we could create MGLGeoJSONSource without any data to start with
   self.trailData = MGLGeoJSONSource(identifier: "trailData", url: geoJSONURL, options: options)
   mapView.style().add(trailData!)
   print("Features: ", self.trailData?.features)
}
Features: nil

[1ec5]

If you use the MGLGeoJSONSource(identifier:url:options:) initializer, then the URL is loaded asynchronously and, indeed, we don’t set features after that point. This is documented behavior. (mbgl would need to inform the SDK that the URL has loaded.) Can you file a separate feature request about features being nil in this case?

would be nice if we could create MGLGeoJSONSource without any data to start with

Can you use MGLGeoJSONSource(identifier:features:options:) with an empty features array?

[nitrag]

Aha ok thanks for that explanation, it's working!

//mapDidFinishLoading

    //thanks for the tip on setting empty 'features' array!
    self.trailData = MGLGeoJSONSource(identifier: "trailData", features: [MGLFeature](), options: options)
    mapView.style().add(trailData!)

...

//later, via a manual click event
    do {
        let jsonPathUrl = Bundle.main.url(forResource: "single_feature", withExtension: "geojson")
        let jsonData = try Data(contentsOf: jsonPathUrl!)
        self.trailData?.geoJSONData = jsonData
        print("Features: ", self.trailData?.features)  //<--successfully prints the feature!
    }catch {
        print("GeoJSON parsing failed")
    }

For the merging/mutating of the NSData I'm using the following (which could probably be improved):

if let newData = self.mergeGeoJSONfeatures(old: self.trailData?.geoJSONData, new: jsonData) {
    self.trailData?.geoJSONData = newData
}else{
    print("Returned nil from merge, possible no change needed")
}

import SwiftyJSON
func mergeGeoJSONfeatures(old: Data?, new: Data?) -> Data? {
    //return other if one is nil
    if(old == nil && new != nil){
        return new!
    }else if(old != nil && new == nil){
        return nil
    }
    let oldJSON = JSON(data: old!)
    var newJSON = JSON(data: new!)
    guard (newJSON["features"].arrayObject != nil) else {
        print("Can't merge empty features!")
        return nil
    }
    //Merge
    newJSON["features"].arrayObject = oldJSON["features"].arrayObject! + newJSON["features"].arrayObject!
    if let result = try? newJSON.rawData() {
        return result
    }else{
        print("Error converting Merged data back to NSData!")
    }
    return nil
}

``