Add back MGLStyleFunction and re-document MGLStyleValue #7943

boundsj · 2017-02-04T01:06:29Z

Addresses MGLStyleValue documentation todos noted in #7924:

Update class documentation for MGLStyleValue (as a class cluster it represents something different now)
+[MGLStyleValue valueWithInterpolationMode:cameraStops:options:]
+[MGLStyleValue valueWithInterpolationMode:sourceStops:attributeName:options:]
+[MGLStyleValue valueWithInterpolationMode:compositeStops:attributeName:options:]
MGLCameraStyleFunction (and all methods and properties)
MGLSourceStyleFunction (and all methods and properties)
MGLCompositeStyleFunction (and all methods and properties)

mention-bot · 2017-02-04T01:06:30Z

@boundsj, thanks for your PR! By analyzing this pull request, we identified @1ec5 and @friedbunny to be potential reviewers.

boundsj · 2017-02-04T01:30:36Z

This PR adds back MGLStyleFunction so that style functions can be considered more abstractly in code and documentation.

A constraint here is that we don't want to break backwards compatibility with 3.4.x's MGLStyleFunction and code that may have expected not only to reference the class but also its interpolationBase and stops properties. Concurrently, we want MGLStyleFunction to be a base class for all of the concrete function classes and not a kind of wrapper for MGLCameraStyleFunction specifically.

To do this I:

Removed -@compatibility_alias MGLStyleFunction MGLCameraStyleFunction
Added back a MGLStyleFunction but documented it as an abstract base class that is not to be instantiated directly or subclassed
Added interpolationBase and stops properties to MGLStyleFunction so it is fully backwards compatible

The main tension point was the stops property combined with lightwieght generics: Since MGLStyleFunction needs stops to be backwards compatible but not all of the concrete function subclasses share the same generic annotation types, I intentionally added a plain NSDictionary stops property to the abstract MGLStyleFunction with covariants in each of the subclasses with the fully expressed, lightweight generic annotations. I think this allows for full backwards compatibility, stays out of the way when dealing with the subclasses, and still allows for (in Swift) iterating over functions, cast in the abstract, and casting stops even for the most complex (composite) case:

// circleStyleLayer.circleRadius  previously set to a `MGLCompositeStyleFunction`
if let returnedCircleRadius = circleStyleLayer.circleRadius as? MGLStyleFunction<NSNumber> {
  if let returnedStops = returnedCircleRadius.stops as NSDictionary? as? [NSNumber: [NSNumber: MGLStyleValue<NSNumber>]] {
    let lhs: MGLStyleValue<NSNumber> = returnedStops[0]!.values.first!
    let rhs: MGLStyleValue<NSNumber> = radiusCompositeExponentialOrIntervalStops[0]!.values.first!
                XCTAssertEqual(lhs, rhs)
   }
}

1ec5 · 2017-02-05T22:52:41Z

platform/darwin/src/MGLStyleValue.h

+ @return An `MGLCompositeStyleFunction` object with the given interpolation mode,
+ composite stops, attribute name, and options.
+ */
+ (instancetype)valueWithInterpolationMode:(MGLInterpolationMode)interpolationMode compositeStops:(NS_DICTIONARY_OF(id, NS_DICTIONARY_OF(id, MGLStyleValue<T> *) *) *)compositeStops attributeName:(NSString *)attributeName options:(nullable NS_DICTIONARY_OF(MGLStyleFunctionOption, id) *)options;


If each stop is “interpreted as a source function”, should the type of compositeStops be NSDictionary<id, NSDictionary<id, MGLSourceStyleFunction<T> *> *> *?

This was me doing a poor job of trying to clarify the opposite: "composite" in this case means a composite of stops dictionaries not functions (see discussion in #7596 (comment)). In d46559a, I punted on trying to explain this in the class factory method in favor of the explanation in the MGLCompositeStyleFunction stops property

1ec5 · 2017-02-05T22:54:24Z

platform/darwin/src/MGLStyleValue.h

-// TODO: API docs
+/**
+ The exponential interpolation base of the function’s interpolation curve.
+ */


Document the fact that interpolationBase may not apply to all style functions.

1ec5 · 2017-02-05T22:55:35Z

platform/darwin/src/MGLStyleValue.h

-// TODO: API docs
+/**
+ The exponential interpolation base of the function’s interpolation curve.
+ */
 @property (nonatomic) CGFloat interpolationBase;

 @end


For backwards compatibility, MGLStyleFunction also needs the initializers that it had in v3.4.0. These initializers can be marked as deprecated.

This will probably look a bit strange no matter what we do. I think 406e647 is a workable solution until a semver bump when we can more officially make this class abstract.

The initializers have been brought back and an actual instance of MGLStyleFunction is created and returned if you use them. The conversion methods have been updated to coerce MGLStyleFunction instances into MGLCameraStyleFunction instances with the appropriate interpolation mode. However, documentation attempts to point developers to the MGLStyleValue class factory methods to avoid all of this.

1ec5 · 2017-02-05T22:56:45Z

platform/darwin/src/MGLStyleValue.h

+ An `MGLCameraStyleFunction` is a value function defining a style value that changes
+ as the zoom level changes. The layout and paint attribute properties of an
+ `MGLStyleLayer` object can be set to `MGLCameraStyleFunction` objects. Use a zoom
+ level function to create the illusion of depth and control data density.


“Zoom level functions” are now known as “camera functions”.

1ec5 · 2017-02-05T23:22:05Z

I intentionally added a plain NSDictionary stops property to the abstract MGLStyleFunction with covariants in each of the subclasses with the fully expressed, lightweight generic annotations. I think this allows for full backwards compatibility, stays out of the way when dealing with the subclasses, and still allows for (in Swift) iterating over functions, cast in the abstract, and casting stops even for the most complex (composite) case

Note that this approach technically doesn’t guarantee full backwards compatibility with Swift code:

let circleRadius = circleStyleLayer.circleRadius as! MGLStyleFunction<NSNumber>
circleStyleLayer.circleRadius = circleRadius.stops[14]

must now be converted to at least something like this:

let circleRadius = circleStyleLayer.circleRadius as! MGLStyleFunction<NSNumber>
circleStyleLayer.circleRadius = (circleRadius.stops as! [NSNumber: MGLStyleFunction<NSNumber>])[14]

This isn’t a problem in Objective-C, where you can use NSDictionary * as a more specific type without casting. We’ve carved out an exception to semantic versioning for Swift bridging changes, although it’s a last resort.

1ec5 · 2017-02-06T21:13:47Z

platform/darwin/src/MGLStyleValue.h

+ @return An `MGLCameraStyleFunction` object with the given interpolation mode,
+ camera stops, and options.
+ */
+ (instancetype)valueWithInterpolationMode:(MGLInterpolationMode)interpolationMode cameraStops:(NS_DICTIONARY_OF(id, MGLStyleValue<T> *) *)cameraStops options:(nullable NS_DICTIONARY_OF(MGLStyleFunctionOption, id) *)options;


Remember to use the NS_…_OF macros until we drop iOS 8 support.

It looks like we forgot to do this in the 3.4.0 release. In fbb8068 and 3b90d85 I updated the deprecated methods and everything in the resurrected MGLStyleFunction to use the macro. The only use of NSDictionary (with no lightweight generic attributes) in MGLStyleValue now is for the stops property of MGLStyleFunction for reasons already discussed in this PR.

1ec5

MGLStyleFunction is going to be a bit messy, sort of like CLRegion (versus CLCircularRegion), for awhile, but I think we have the start of a workable solution.

1ec5 · 2017-02-07T20:17:18Z

platform/darwin/src/MGLStyleValue.h

+/**
+ The modes used to interpolate property values between map zoom level changes or
+ over a range of feature attribute values.
+ */
 @property (nonatomic) MGLInterpolationMode interpolationMode;


Could we avoid effectively adding an interpolationMode property to MGLStyleFunction that would only sometimes be applicable?

In ee89e94 I removed it in MGLStyleFunction and added it back to the concrete subclasses. This sort of makes sense because an MGLStyleFunction technically does not (did not) have an interpolation mode. In the future, we should add the property back to MGLStyleFunction because every actual function class does have an interpolation mode.

every actual function class does have an interpolation mode

Ah, I misunderstood. In that case, it’s fine (and preferable) to have it in MGLStyleFunction to avoid duplication.

Yeah. For now, it'll be a bit of a lie, but still reasonable.

MGLStyleFunction's interpolation mode will be exponential even though it did not have such a concept before in 3.4.x. However, any MGLStyleFunction instance that is put to use in 3.5.0 is treated like a camera function with an exponential interpolation mode so I feel ok about that. I'll put it back on MGLStyleFunction.

1ec5 · 2017-02-07T20:19:29Z

platform/darwin/src/MGLStyleValue.h

+ @param stops A dictionary associating zoom levels with style values.
+ @return An `MGLStyleFunction` object with the given interpolation base and stops.
+ */
+- (instancetype)initWithInterpolationBase:(CGFloat)interpolationBase stops:(NS_DICTIONARY_OF(NSNumber *, MGLStyleValue<T> *) *)stops NS_DESIGNATED_INITIALIZER;


If you mark this as a designated initializer, does that mean all the subclass initializers have to call this method? Or would removing NS_DESIGNATED_INITIALIZER be considered a breaking change?

Ideally, in a future release, we’d end up making all -init… methods private (as would be expected of class clusters). So we should make all the new -init… methods (like in MGLCameraStyleFunction) private and leave this one public but deprecated, for backwards compatibility.

If you mark this as a designated initializer, does that mean all the subclass initializers have to call this method?

Yes

Or would removing NS_DESIGNATED_INITIALIZER be considered a breaking change?

I don't think so. We never suggested that developers should subclass MGLStyleFunction.

In 7cbeba7, I exposed only MGLStyleFunction's initializer publicly but
deprecated it and removed its designated initializer status. Internally this allows us to ignore the deprecated initializer in subclasses.

1ec5 · 2017-02-07T20:20:10Z

platform/darwin/src/MGLStyleValue.mm

+}
+
+ (instancetype)functionWithInterpolationBase:(CGFloat)interpolationBase stops:(NSDictionary *)stops {
+ return [[self alloc] initWithInterpolationBase:interpolationBase stops:stops];


Can we initialize an MGLCameraStyleFunction here instead (and in +functionWithStops:, but not -init or -initWithInterpolationBase:stops:)? If there are warnings associated with the designated initializer, we can suppress the warnings.

Done 2f414f2

1ec5 · 2017-02-07T20:24:42Z

platform/darwin/src/MGLStyleValue.mm

+}
+
+- (NSUInteger)hash {
+ return self.stops.hash + self.interpolationBase;


Use @(self.interpolationBase).hash, not interpolationBase itself, to further minimize collisions. There are similar problems with interpolationMode in the subclasses. Feel free to take care of this in a separate PR.

Done 1b67069

Expose only MGLStyleFunction's initializer publicly but deprecate it and do not mark it as designated.

1ec5

Sorry for flip-flopping on interpolationMode; based on your explanation in #7943 (comment), it does make sense to declare it on MGLStyleFunction and not each of its subclasses.

Otherwise, this PR looks good to go.

This reverts commit ee89e94.

nitrag · 2017-04-26T20:21:52Z

I'm super lost.

let carLayer = MGLSymbolStyleLayer(identifier: "cars-layer", source: carSource)
style.setImage(UIImage(named: "car-red")!, forName: "car-red")
style.setImage(UIImage(named: "car-blue")!, forName: "car-blue")
style.setImage(UIImage(named: "car-silver")!, forName: "car-silver")
style.setImage(UIImage(named: "car-green")!, forName: "car-green")
style.setImage(UIImage(named: "car-orange")!, forName: "car-orange")
carLayer.iconImageName = MGLSourceStyleFunction(
    interpolationMode: .identity,
    sourceStops: nil,
    attributeName: "car-color",
    options: [.defaultValue: MGLStyleValue<NSString>(rawValue: "car-silver")]
)
carLayer.iconImageName = MGLCameraStyleFunction(
    interpolationMode: .identity,
    sourceStops: nil,
    attributeName: "car-color",
    options: [.defaultValue: MGLStyleValue<NSString>(rawValue: "car-silver")]
)

regardless of which one I choose:
'NSInvalidArgumentException', reason: 'This property can only be set to camera functions. Use +[MGLStyleValue valueWithInterpolationMode:cameraStops:options:] instead.'

but I need attributeName to tell it what field? No??

jfirebaugh · 2017-04-26T21:41:53Z

Data-driven styling for iconImageName is not in any released iOS SDK version yet. It'll be in 3.6.0.

If you're ever in doubt about SDK compatibility, https://www.mapbox.com/mapbox-gl-js/style-spec/ has support matrixes for every property (albeit you'll need to translate from SDK property name to style spec property name).

nitrag · 2017-04-26T22:26:27Z

Ahhhhhh. I'll remember this for the future. Thanks!

1ec5 · 2017-04-26T23:32:41Z

The documentation for MGLSymbolStyleLayer.iconImageName in v3.5.2 says it supports camera functions but not source functions. We use the same automated process to generate the property’s implementation, documentation, and the exception message you ran into, so you can be sure of its accuracy.

albeit you'll need to translate from SDK property name to style spec property name

The iOS SDK’s documentation comes with a full correspondence table.

wr6524798 · 2017-04-27T02:48:28Z

please, when I use Data-driven styling for iconImageName,the Application crash, I want use Data-driven styling for iconImageName,when the 3.6.0. release?

1ec5 · 2017-04-27T03:30:57Z

@wr6524798, this closed pull request isn’t the best place for new bug reports. Please open a new issue about the crash you’re seeing, and be sure to include the crash trace so we can properly diagnose and prioritize the problem. Thank you!

[ios, macos] Add back MGLStyleFunction and re-document MGLStyleValue

6aea7f1

boundsj added documentation iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor runtime styling labels Feb 4, 2017

boundsj added this to the ios-v3.5.0 milestone Feb 4, 2017

boundsj self-assigned this Feb 4, 2017

boundsj requested a review from 1ec5 February 4, 2017 01:06

1ec5 reviewed Feb 5, 2017

View reviewed changes

boundsj added 5 commits February 6, 2017 09:30

wip: clarify composite function stops

d46559a

wip: clarify interpolation base

1fa2bb7

wip: clarify that stops can be nil in function base class

fe99147

wip: s/zoom level function/camera function

646b0d3

wip: implement MGLStyleFunction initializers

406e647

1ec5 reviewed Feb 6, 2017

View reviewed changes

boundsj added 2 commits February 6, 2017 15:33

wip: Update deprecated methods to use NS_..._OF macro

fbb8068

wip: use NS_..._OF macro

3b90d85

1ec5 reviewed Feb 7, 2017

View reviewed changes

boundsj added 4 commits February 7, 2017 13:02

wip: Rightsize initializer surface area

7cbeba7

Expose only MGLStyleFunction's initializer publicly but deprecate it and do not mark it as designated.

wip: Move interpolation mode to concrete subclasses

ee89e94

wip: Make MGLStyleFunction conveniences return camera function

2f414f2

wip: Fix hash implementations with more hash

1b67069

boundsj mentioned this pull request Feb 7, 2017

[ios, macos] Document DDS enumerations #7931

Merged

8 tasks

1ec5 approved these changes Feb 8, 2017

View reviewed changes

Revert "wip: Move interpolation mode to concrete subclasses"

9a6395f

This reverts commit ee89e94.

boundsj merged commit 06a8ed7 into master Feb 8, 2017

boundsj deleted the boundsj-add-function-base-class branch February 8, 2017 20:27

boundsj mentioned this pull request Feb 8, 2017

Add documentation for data-driven styling #7924

Closed

18 tasks

1ec5 mentioned this pull request Feb 10, 2017

Mark MGLStyleFunction as abstract in documentation navigation #8013

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add back MGLStyleFunction and re-document MGLStyleValue #7943

Add back MGLStyleFunction and re-document MGLStyleValue #7943

boundsj commented Feb 4, 2017 •

edited

Loading

mention-bot commented Feb 4, 2017

boundsj commented Feb 4, 2017

1ec5 Feb 5, 2017

boundsj Feb 6, 2017

1ec5 Feb 5, 2017

boundsj Feb 6, 2017

1ec5 Feb 5, 2017

boundsj Feb 6, 2017

1ec5 Feb 5, 2017

1ec5 commented Feb 5, 2017

1ec5 Feb 6, 2017

boundsj Feb 6, 2017

1ec5 left a comment •

edited

Loading

1ec5 Feb 7, 2017

boundsj Feb 7, 2017

1ec5 Feb 8, 2017

boundsj Feb 8, 2017

1ec5 Feb 7, 2017

1ec5 Feb 7, 2017

boundsj Feb 7, 2017

1ec5 Feb 7, 2017

boundsj Feb 7, 2017

1ec5 Feb 7, 2017

boundsj Feb 7, 2017

1ec5 left a comment

nitrag commented Apr 26, 2017

jfirebaugh commented Apr 26, 2017

nitrag commented Apr 26, 2017

1ec5 commented Apr 26, 2017

wr6524798 commented Apr 27, 2017

1ec5 commented Apr 27, 2017

Add back MGLStyleFunction and re-document MGLStyleValue #7943

Add back MGLStyleFunction and re-document MGLStyleValue #7943

Conversation

boundsj commented Feb 4, 2017 • edited Loading

mention-bot commented Feb 4, 2017

boundsj commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Feb 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

nitrag commented Apr 26, 2017

jfirebaugh commented Apr 26, 2017

nitrag commented Apr 26, 2017

1ec5 commented Apr 26, 2017

wr6524798 commented Apr 27, 2017

1ec5 commented Apr 27, 2017

boundsj commented Feb 4, 2017 •

edited

Loading

1ec5 left a comment •

edited

Loading