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

Revamp MGLStyleLayer, MGLSource inheritance #6588

Merged
merged 4 commits into from
Oct 6, 2016
Merged

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Oct 5, 2016

This PR cleans up the runtime styling API’s class hierarchy and completes the documentation thereof.

The MGLStyleLayer protocol has been removed, because almost none of its members was actually implemented in every class that adopted the protocol, and because all the concrete classes ultimately derived from the same base class. The unused mapView backpointer property has been removed with no replacement, although we’ll eventually have to bring it back – privately – for #6254. MGLBaseStyleLayer has been renamed to MGLStyleLayer. New intermediate abstract classes, MGLForegroundStyleLayer and MGLVectorStyleLayer, cover subsets of style layer classes with like functionality. To summarize, the style layer class hierarchy looks like this now (classes in italics are abstract):

  • MGLStyleLayer (private properties in a Private category)
    • MGLBackgroundStyleLayer
    • MGLForegroundStyleLayer
      • MGLRasterStyleLayer
      • MGLVectorStyleLayer
        • MGLCircleStyleLayer
        • MGLFillStyleLayer
        • MGLLineStyleLayer
        • MGLSymbolStyleLayer

Each MGLBaseStyleLayer initializer and the corresponding properties have been pushed down to an abstract subclass such that the initializer makes sense for all concrete subclasses. Additional initializers and the predicate property have been pushed up to MGLVectorStyleLayer to eliminate duplication among the concrete subclasses. These initializers are all marked as designated initializers, and exceptions prevent developers from misusing them.

The word “source” or “layer” before “identifier” has been removed wherever the type of identifier is apparent. Extra MGLGeoJSONSource initializer variants have been removed in favor of nullable parameters (overlapping a little with #6524 for #6302).

Copious documentation comments have been added for source and style layer classes, including several previously undocumented methods and properties. In particular, the documentation now includes important preconditions and postconditions, as well as details about the subset of the predicate API that we support. Pragma marks break up the jazzy documentation pages into sections. Unfortunately, the fancy, marked-up documentation comments show up as plain text in Xcode 8: rdar://problem/28628571. At least they look good in jazzy:

predicate

Fixes #6406, fixes #6407, fixes #6582. Working towards #5970.

/cc @frederoni @incanus @boundsj

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS refactor macOS Mapbox Maps SDK for macOS runtime styling labels Oct 5, 2016
@1ec5 1ec5 added this to the ios-v3.4.0 milestone Oct 5, 2016
@1ec5 1ec5 self-assigned this Oct 5, 2016
@mention-bot
Copy link

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

@implementation MGLForegroundStyleLayer

- (instancetype)initWithIdentifier:(NSString *)identifier source:(MGLSource *)source {
if (self = [super initWithIdentifier:identifier]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this initializer raise an exception?

Copy link
Contributor Author

@1ec5 1ec5 Oct 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is the designated initializer called by the concrete classes’ designated initializers. If the developer erroneously creates an MGLForegroundStyleLayer directly using this initializer, they’ll run into an exception as soon as they try to add the style layer to the style because the layer property will be undefined.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That makes perfect sense for the abstract classes.

@class MGLSource;

/**
`MGLSourcedStyleLayer` is an abstract superclass for style layers whose content
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MGLForegroundStyleLayer is ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

NS_ASSUME_NONNULL_BEGIN

/**
`MGLSourcedStyleLayer` is an abstract superclass for style layers whose content
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MGLVectorStyleLayer is ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@1ec5 1ec5 force-pushed the 1ec5-style-inheritance-6406 branch from 0e80f50 to 390c2e4 Compare October 6, 2016 03:45
@1ec5 1ec5 added the beta blocker Blocks the next beta release label Oct 6, 2016
@boundsj
Copy link
Contributor

boundsj commented Oct 6, 2016

Would it make sense to override initWithIdentifier:source: in all of the concrete subclasses of MGLForegroundStyleLayer and raise an exception so that something like [[MGLFillStyleLayer alloc] initWithIdentifier:@"some-id"]; fails faster? I'm thinking about the edge case where the developer initializes the concrete layer subclass "correctly" but creates a ⏲ 💣 if the instance is stored and used later on. Alternatively, I suppose we could document this edge case in the MGLStyleLayer header but it seems like it might be confusing since initWithIdentifier:source: is perfectly fine for MGLBackgroundStyleLayer.

Copy link
Contributor

@boundsj boundsj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 👍 @1ec5 and the documentation is 💯. I would not let #6588 delay this from landing but it might be worth considering or documenting at some point.

@1ec5
Copy link
Contributor Author

1ec5 commented Oct 6, 2016

Would it make sense to override initWithIdentifier:source: in all of the concrete subclasses of MGLForegroundStyleLayer and raise an exception so that something like [[MGLFillStyleLayer alloc] initWithIdentifier:@"some-id"]; fails faster? I'm thinking about the edge case where the developer initializes the concrete layer subclass "correctly" but creates a ⏲ 💣 if the instance is stored and used later on.

Since -initWithIdentifier:source: is the designated initializer for MGLForegroundStyleLayer, calling -[MGLFillStyleLayer initWithIdentifier:] results in a compile-time error or warning.

Unfortunately, we can’t do anything about the developer calling -[MGLForegroundStyleLayer initWithIdentifier:source:], other than the ⏲💣 behavior noted above. This is an intrinsic limitation to abstract superclasses in Objective-C and the only real downside to using abstract superclasses instead of protocols. Because -initWithIdentifier:source: is the designated initializer, the concrete subclasses’ implementations of that method must call the abstract superclass’s implementation. The same pitfall exists for developers who attempt to initialize an NSControl or UIControl using -initWithFrame:. The only difference is that we don’t currently support custom subclasses of MGLStyleLayer. (If in the future we do support something along those lines, we’d document the methods that superclasses would want to override.)

Alternatively, I suppose we could document this edge case in the MGLStyleLayer header but it seems like it might be confusing since initWithIdentifier:source: is perfectly fine for MGLBackgroundStyleLayer.

The comments for all the abstract superclasses already say that the developer must initialize a concrete subclass, not the abstract superclass. I reiterate the point in the exception messages.

1ec5 added 3 commits October 6, 2016 15:19
Removed the MGLStyleLayer protocol, because almost none of its members was actually implemented in every class that adopted the protocol. Removed the unused mapView backpointer property with no replacement. Renamed MGLBaseStyleLayer to MGLStyleLayer. Created the intermediate abstract classes MGLForegroundStyleLayer and MGLVectorStyleLayer to cover subsets of style layer classes with like functionality.

Moved each MGLBaseStyleLayer initializer and the corresponding properties down to an abstract subclass such that the initializer makes sense for all concrete subclasses. Moved more initializers and the predicate property up to MGLVectorStyleLayer to eliminate duplication among the concrete subclasses. Marked these initializers as designated initializers.

Removed “source” or “layer” before identifier wherever the type of identifier is apparent. Removed extra MGLGeoJSONSource initializer variants in favor of nullable parameters.

Added copious documentation comments for source and style layer classes, including several previously undocumented methods and properties. In particular, some preconditions and postconditions have been documented. Added pragma marks to break up the jazzy documentation pages into sections. Reformatted exceptions for consistency.
@1ec5 1ec5 force-pushed the 1ec5-style-inheritance-6406 branch from 390c2e4 to 43658db Compare October 6, 2016 22:19
@1ec5 1ec5 merged commit 0bb46ea into master Oct 6, 2016
@1ec5 1ec5 deleted the 1ec5-style-inheritance-6406 branch October 6, 2016 23:24
@1ec5
Copy link
Contributor Author

1ec5 commented Oct 6, 2016

Since -initWithIdentifier:source: is the designated initializer for MGLForegroundStyleLayer, calling -[MGLFillStyleLayer initWithIdentifier:] results in a compile-time error or warning.

I was mistaken: NS_DESIGNATED_INITIALIZER only forces the subclass’s implementation to call the superclass’s, but the non-designated initializer becomes a convenience initializer rather than becoming unavailable. In 0bb46ea, I explicitly marked the incomplete initializers unavailable to prevent the very likely case of a developer calling -[MGLFillStyleLayer init].

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