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

MGLStyleLayer classes should conform to NSCopying #7600

Open
1ec5 opened this issue Jan 5, 2017 · 6 comments
Open

MGLStyleLayer classes should conform to NSCopying #7600

1ec5 opened this issue Jan 5, 2017 · 6 comments
Labels
feature GL JS parity For feature parity with Mapbox GL JS gl-ios iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Jan 5, 2017

We should make it easy for a developer to duplicate a style layer. Currently, this is a tedious process that requires hard-coding a method call for each property in the style specification. The most natural way to streamline copying would be to have the concrete subclasses of MGLStyleLayer conform to the NSCopying protocol.

Copying a style layer would not copy the raw pointer to the underlying mbgl object but would create a pending layer object and set all its layout and paint attributes without any developer intervention. We could implement copying support in mbgl, or we could implement it entirely at the SDK level. Either way, we can take advantage of the codegen script to avoid having to hard-code the list of properties ourselves. If we do implement the feature in mbgl, we could take advantage of the JSON serialization that would be added in #7563 to conform to NSSecureCoding at the same time.

I’d consider both NSCopying and NSSecureCoding compliance to be a GL JS parity feature, because it’s quite easy with GL JS to call getLayer() and use a common JavaScript library to clone the returned object or stringify the JSON object.

/cc @frederoni @jfirebaugh

@1ec5 1ec5 added feature GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling labels Jan 5, 2017
@1ec5 1ec5 added this to the ios-v3.5.0 milestone Jan 5, 2017
@jfirebaugh
Copy link
Contributor

The equivalent mbgl core method is Layer::Impl::copy. Note that this method requires id and sourceID parameters (ignore the ref parameter, that's going away soon). These parameters reflect the fact that id and sourceID are immutable once a layer is constructed. Can the NSCopying protocol faithfully reflect this as well?

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 5, 2017

Yes, -[NSCopying copyWithZone:] and -[NSObject copy] are intended to make an exact functional duplicate of the object, so we’d implement -copyWithZone: by passing in self.identifier as the ID and self.sourceIdentifier as the source ID.

Note that this paradigm isn’t necessarily mutually exclusive with the idea that layers must have unique IDs. A developer might reasonably want to use -copy in the following way:

MGLSymbolStyleLayer *parkLayer = [[mainMapView.style layerWithIdentifier:@"parks"] copy];
[sideMapView.style addLayer:parkLayer];

Although that’s sort of awkward, since the SDK’s public interface halfheartedly pretends that a layer points to a source object as opposed to a source identifier. (MGLForegroundSource’s initializers require an MGLSource object, as a way to encourage the developer to create the layer after the source, but has only a sourceIdentifier property rather than a source property, so that we can avoid a backpointer to MGLMapView.)

In any event, the primary use case of duplicating a layer for use within the same map view would require a distinct identifier to be passed in. The typical way to express these semantics is with a “with” method: -[MGLStyleLayer styleLayerWithIdentifier:] (not to be confused with -initWithIdentifier:] or -[MGLStyle layerWithIdentifier:]), which we’d refine for Swift as withIdentifier(_:). So NSCopying would be orthogonal to satisfying this primary use case.

@frederoni
Copy link
Contributor

frederoni commented Jan 5, 2017

Great idea.

Note that Studio just appends "copy" when duplicating a layer and subsequently starts incrementing. This feels natural when copying uniquely identified objects, copying files in Finder being the first that comes to mind. Although -[MGLStyleLayer styleLayerWithIdentifier:] is self-explanatory, we could implement both without much fuss.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 5, 2017

We could implement some sort of conflict avoidance inside the copy method, but that runs counter to the spirit of NSCopying, where the copy would be object-equal with the original object (assuming we override -isEqual:). As long as we provide a -styleLayerWithIdentifier: method, conflict avoidance within -copyWithZone: should be unnecessary.

@1ec5 1ec5 modified the milestones: ios-v3.5.0, ios-v3.6.0 Mar 9, 2017
@boundsj boundsj modified the milestones: ios-future, ios-v3.6.0 Mar 31, 2017
@stale stale bot added the archived Archived because of inactivity label Nov 10, 2018
@stale
Copy link

stale bot commented Nov 26, 2018

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this as completed Nov 26, 2018
@friedbunny friedbunny reopened this Nov 26, 2018
@stale stale bot removed the archived Archived because of inactivity label Nov 26, 2018
@stale stale bot added the archived Archived because of inactivity label May 25, 2019
@stale
Copy link

stale bot commented May 25, 2019

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this as completed May 25, 2019
@friedbunny friedbunny reopened this May 28, 2019
@stale stale bot removed the archived Archived because of inactivity label May 28, 2019
@jmkiley jmkiley added the gl-ios label Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature GL JS parity For feature parity with Mapbox GL JS gl-ios iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling
Projects
None yet
Development

No branches or pull requests

6 participants