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

Source and layer ownership is inconsistent (from style versus from developer) #6254

Closed
1ec5 opened this issue Sep 6, 2016 · 5 comments
Closed
Labels
iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor release blocker Blocks the next final release runtime styling
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Sep 6, 2016

MGLSource and the MGLStyleLayer classes manage their underlying C++ objects inconsistently depending on the task. For example, when getting an MGLSource object from an MGLStyle, the MGLSource object holds a strong reference to an mbgl::style::Source object in its source property. On the other hand, when the developer initializes an MGLSource object from scratch in order to add it to an MGLMapView, -[MGLSource mbglSource] creates the mbgl::style::Source object lazily when adding it to the style and never holds a reference to it. If the developer attempts to get an MGLSource from the style and add it back to the style, a brand-new mbgl::style::Source object is created and added to the map, discarding any modifications that the developer may have made on the MGLSource object.

We should remove -[MGLSource mbglSource]. MGLSource should create the mbgl::style::Source upfront in its initializers. (There should be an additional private initializer that accepts an mbgl::style::Source.) This means all the source options need to be immutable. Currently, the only mutable source option is the sourceIdentifier property, but I think that should be read-only anyways.

The catch is that a source added to the map really shouldn’t be owned by the MGLSource object. Could we treat the source property as holding only a “pending” source and always access the mbgl::style::Source object via mbgl::Map::getSource()? What happens if the developer attempts to add the same MGLSource to the MGLStyles of two different MGLMapViews?

/cc @frederoni @boundsj

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS refactor macOS Mapbox Maps SDK for macOS runtime styling labels Sep 6, 2016
@1ec5 1ec5 added this to the ios-v3.4.0 milestone Sep 6, 2016
@jfirebaugh
Copy link
Contributor

To expand on #6269 (comment), objects wrapping mbgl::style::Layer and mbgl::style::Source domain objects need to have two internal members:

  • A std::unique_ptr<>, which is present only for objects that were created independently (not obtained by Map#getLayer or Map#getSource) and have not yet been added via Map#addLayer or Map#addSource.
  • A raw pointer to the mbgl object, which is always initialized, either to the result of Map#getLayer or Map#getSource, or for independently created objects, to the pointer value held in the unique_ptr. In the latter case, this raw pointer value stays even after ownership of the object is transferred via Map#addLayer or Map#addSource.

This is the approach that the Android bindings follow.

@jfirebaugh
Copy link
Contributor

Note that after a source or layer is removed from the style, you don't get ownership of that object back, and any raw pointers you retained are invalid. This has implications for documentation of the SDK bindings for the removal methods.

@1ec5
Copy link
Contributor Author

1ec5 commented Sep 7, 2016

By the way, a somewhat orthogonal idea: for MGLStyleLayers obtained from the MGLStyle, the only state MGLStyleLayer maintains would be the layer identifier, and it always uses that identifier to obtain the mbgl::style::Layer and manipulate it locally. This sort of proxy pattern is common in frameworks like Scripting Bridge.

@1ec5
Copy link
Contributor Author

1ec5 commented Sep 26, 2016

Textbook example of what developers face with the current inconsistency: #6460.

@boundsj
Copy link
Contributor

boundsj commented Nov 7, 2016

The layer portion of this was fixed by #6904 and the source portion in #6793.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor release blocker Blocks the next final release runtime styling
Projects
None yet
Development

No branches or pull requests

3 participants