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

Runtime styling API for iOS/macOS #5727

Merged
merged 59 commits into from
Aug 11, 2016
Merged

Runtime styling API for iOS/macOS #5727

merged 59 commits into from
Aug 11, 2016

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Jul 19, 2016

WIP #5626

Runtime styling for iOS and macOS is taking shape. Here's an overview if what's done, what's left to do and what's TBD.

  • Sources
    • Vector
    • Raster
    • GeoJSON
    • TileJSON
  • Layers
    • Background
    • Fill
    • Line
    • Symbol
    • Raster
    • Circle
    • Custom
  • Types
    • Color
    • Enum (missing function getter)
    • String
    • Boolean
    • Number
    • Array
    • Function
    • Filter
  • Documentation
    • Paint and layout properties (parsed from style spec and not particularly idiomatic)
    • Classes and methods

@frederoni frederoni added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jul 19, 2016
@frederoni frederoni added this to the ios-v3.4.0 milestone Jul 19, 2016
@mention-bot
Copy link

mention-bot commented Jul 19, 2016

@frederoni, thanks for your PR! By analyzing the annotation information on this pull request, we identified @1ec5, @friedbunny, @boundsj and @incanus to be potential reviewers

- (void)testRuntimeStyling
{
MGLFillStyleLayer *waterLayer = (MGLFillStyleLayer *)[self.mapView.style layerWithIdentifier:@"water"];
waterLayer.fillColor = [UIColor redColor];
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to make this a toggle.

@friedbunny
Copy link
Contributor

Is the convention that we’ll run make style-code and the various make style-code-{platform} manually when the spec changes?

/cc @jfirebaugh

@frederoni
Copy link
Contributor Author

frederoni commented Jul 20, 2016

@friedbunny Android is generating style code for each build #5734-184. It's convenient but easier for bugs to slip through. Eventually, we could generate tests from the style spec and when we have that in place, I wouldn't mind to hook it up in a similar way.

ios and macos style layers is currently generated by style-code-ios but since that part of the implementation lives in platform/darwin I guess style-code-darwin would make more sense?

@1ec5
Copy link
Contributor

1ec5 commented Jul 20, 2016

We should place generated files in their own directory.

Instead of (or in addition to) a make rule for generating the source files, how about a target and scheme in the iOS and macOS workspaces?

return true;
case 'array':
return true;
case 'enum': // (enum will be converted to NSString hence isObject(enum) == true)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we’re going with the strongly typed design proposed in #5626, each enumeration type in the style specification would ideally be mapped to a C enum (NS_ENUM) rather than NSString.

@1ec5
Copy link
Contributor

1ec5 commented Jul 21, 2016

Most if not all of the paint properties are defined in the style specification as being “optional”. This presents a problem for our Cocoa translation, because the most natural types for the specification’s Enum, Boolean, and Number types (enum, BOOL, and CGFloat, respectively) don’t offer a natural way to indicate the absence of a value.

For Enum, if we go with the approach in #5727 (comment), we can define a “None” value in each enumeration. But for Boolean and Number, I think we may need to use NSNumber instead of C types. As I pointed out in #5727 (comment), NSNumber would also ensure consistency with the case of an Array of Numbers or Booleans, since NSArray can’t directly contain C types.

We’ll also need to audit the headers for nullability and mark the optional properties as being nullable.

@interface MGL<%- camelize(type) %>StyleLayer : MGLStyleLayer

<% if (layoutProperties.length) { -%>
// Layout properties
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Use #pragma mark Layout Properties so jazzy can create a new section for the symbols below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Better yet, “Accessing the Layout Attributes”, since we follow the Apple documentation convention of labeling each section in the documentation with a task. Since “property” has a more generic meaning in Objective-C and Swift, “attribute” is usually used to describe the kind of property that affects presentation etc. We use “attribute” in MGLFeature, for example.

// This file is generated.
// Edit platform/darwin/scripts/generate-style-code.js, then run `make style-code-darwin`.

#import <Mapbox/Mapbox.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Importing Mapbox.h isn’t necessary and it breaks jazzy doc generation.

mapbox-gl-native/platform/darwin/src/MGLLineStyleLayer.h:4:9: fatal error: 'Mapbox/Mapbox.h' file not found

@friedbunny
Copy link
Contributor

MGLStyleLayer and friends aren’t being included in the jazzy docs:

screen shot 2016-08-10 at 3 33 54 pm

@@ -0,0 +1,10 @@
#import <Mapbox/Mapbox.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

This header should import MGLSource.h, not Mapbox.h.

Copy link
Contributor

@friedbunny friedbunny Aug 10, 2016

Choose a reason for hiding this comment

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

Looks like removing Mapbox.h also requires either UIKit or CoreGraphics to be added. Because this is in /darwin, we can’t rely on UIKit being available here.

#import "MGLSource.h"
#import <CoreGraphics/CoreGraphics.h>

@1ec5
Copy link
Contributor

1ec5 commented Aug 11, 2016

Filed lots of tail work for what’s been implemented so far: #5943 #5947 #5948 #5949 #5950 #5951 #5952 #5953 #5954 #5955 #5956 #5957 #5958 #5959 #5960 #5961 #5962.

#5626 remains open for the remaining unimplemented portions of the runtime styling API.

@1ec5 1ec5 removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Aug 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants