Map Darwin style enumerations to mbgl equivalents #7061

boundsj · 2016-11-15T01:01:59Z

mention-bot · 2016-11-15T01:02:01Z

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

1ec5 · 2016-11-15T06:25:40Z

platform/darwin/src/MGLLineStyleLayer.mm

@@ -71,8 +71,8 @@ - (void)setLineJoin:(MGLStyleValue<NSValue *> *)lineJoin {
 }

 - (MGLStyleValue<NSValue *> *)lineJoin {
-    auto propertyValue = self.rawLayer->getLineJoin() ?: self.rawLayer->getDefaultLineJoin();
-    return MGLStyleValueTransformer<mbgl::style::LineJoinType, NSValue *>().toStyleValue(propertyValue);
+    auto propertyValue = self.rawLayer->getLineJoin() ?: self.rawLayer->getDefaultLineJoin();    


Nit: trailing whitespace.

1ec5 · 2016-11-15T06:27:18Z

platform/darwin/src/MGLStyleValue_Private.h

+
+    MGLStyleValue<ObjCType> *toLineJoinStyleValue(const mbgl::style::PropertyValue<MBGLType> &mbglValue) {
+        auto str = mbgl::Enum<MBGLType>::toString(mbglValue.asConstant());
+        NSString *enumString = [NSString stringWithCString:str encoding:NSUTF8StringEncoding];


Nit: this can be expressed as str.UTF8String.

1ec5 · 2016-11-15T06:35:17Z

platform/darwin/src/MGLStyleValue_Private.h

+        auto str = mbgl::Enum<MBGLType>::toString(mbglValue.asConstant());
+        NSString *enumString = [NSString stringWithCString:str encoding:NSUTF8StringEncoding];
+        NSUInteger enumValue;
+        if ([enumString isEqualToString:@"miter"]) {


Nice! Using mbgl::Enum is a lot cleaner than what I thought we’d have to resort to (the _names macros).

I think we could take further advantage of mbgl::Enum, using MBGL_DEFINE_ENUM() to map e.g. { MGLLineJoinMiter, "miter" } and calling toEnum() on str. (Bogus strings that are used internally to mbgl, such as fakeround, would trip an assertion.) That way, we can avoid the NSString conversion and these if statements, and we can generate everything using the style specification (in a separate file).

In terms of organization, these MBGL_DEFINE_ENUM()s can go in MGLLineStyleLayer.mm, for instance. We can make this toLineJoinStyleValue() method into a more generic toEnumStyleValue() by adding an ObjCEnumType template argument that we set to MGLLineJoin, for instance.

boundsj · 2016-11-20T02:21:02Z

@1ec5 thanks for the advice in #7061 (comment). Most of that is implemented now although I did not DRY it up yet with anything like toEnumStyleValue and templates. Other than that though, I think this is pretty close now.

The lookup table in https://github.com/mapbox/mapbox-gl-native/pull/7061/files#diff-01213d8ba02156a3771961755cf10994R12 is sort of unfortunate. But, since the MBGL enums are not generated and because several of them are coalesced into things like AlignmentType (with auto) it is challenging to generate code with the raw hooks we have at hand.

Anyway, this is working!

boundsj · 2016-11-20T15:05:22Z

platform/darwin/scripts/generate-style-code.js

@@ -9,6 +9,30 @@ const spec = _.merge(require('mapbox-gl-style-spec').latest, require('./style-sp
 const prefix = 'MGL';
 const suffix = 'StyleLayer';

+global.mbglTypeName = function(str) {


Looks like this can be replaced with global.mbglType()

boundsj · 2016-11-20T16:13:37Z

This will also fix #5970 (again)

boundsj · 2016-11-20T21:51:02Z

@1ec5 @frederoni @incanus please review

1ec5 · 2016-11-20T22:17:37Z

platform/darwin/src/MGLBackgroundStyleLayer.mm

@@ -10,6 +10,13 @@

 #include <mbgl/style/layers/background_layer.hpp>

+namespace mbgl {


Can we get rid of this block if there’s are no symbols to define in this translation unit?

1ec5 · 2016-11-20T22:18:10Z

platform/darwin/src/MGLCircleStyleLayer.mm

+
+    using namespace style;
+
+


Nit: extra blank line.

1ec5 · 2016-11-20T22:19:21Z

platform/darwin/src/MGLCircleStyleLayer.mm

@@ -10,6 +10,21 @@

 #include <mbgl/style/layers/circle_layer.hpp>

+namespace mbgl {
+
+    using namespace style;


Nothing in this block is using the mbgl::style namespace, so this statement can go away.

1ec5 · 2016-11-20T22:31:42Z

platform/darwin/src/MGLCircleStyleLayer.mm

+    }
+    id value = [(MGLStyleConstantValue<NSValue *> *)circleTranslateAnchor rawValue];
+    MGLCircleTranslateAnchor circleTranslateAnchorValue;
+    [value getValue:&circleTranslateAnchorValue];


In the previous code, if circleTranslateAnchor was nil, we’d reset the value in mbgl to an implicit default value. In this code, if circleTranslateAnchor is nil, circleTranslateAnchorValue will be MGLCircleTranslateAnchorMap, even if the implicit default would’ve been mbgl::style::TranslateAnchorType::Viewport.

1ec5 · 2016-11-20T22:39:18Z

platform/darwin/src/MGLCircleStyleLayer.mm

    return MGLStyleValueTransformer<std::array<float, 2>, NSValue *>().toStyleValue(propertyValue);
 }

 - (void)setCircleTranslateAnchor:(MGLStyleValue<NSValue *> *)circleTranslateAnchor {
-    auto mbglValue = MGLStyleValueTransformer<mbgl::style::TranslateAnchorType, NSValue *>().toPropertyValue(circleTranslateAnchor);
-    self.rawLayer->setCircleTranslateAnchor(mbglValue);
+    if ([circleTranslateAnchor isKindOfClass:[MGLStyleFunction class]]) {


This code should be brought back into MGLStyleValueTransformer and made generic. Bring back the enumeration specialization of toMGLRawStyleValue(), but add an additional, optional ObjCEnum template parameter to MGLStyleValueTransformer that you can plug into mbgl::Enum. For additional safety, I think you can also make the specialization conditional on ObjCEnum satisfying std::is_enum just like it required MBGLEnum to satisfy std::is_enum.

1ec5 · 2016-11-20T22:48:27Z

platform/darwin/src/NSValue+MGLStyleEnumAttributeAdditions.h

+*/
+ (instancetype)valueWithMGLCirclePitchScale:(MGLCirclePitchScale)type;
+
+


Each factory method should come with a readonly property that converts back into the raw type (example). This way you (and the developer) won’t ever have to use -getValue:.

1ec5 · 2016-11-20T22:49:21Z

platform/darwin/src/NSValue+MGLStyleEnumAttributeAdditions.h

+
+@interface NSValue (MGLStyleEnumAttributeAdditions)
+
+#pragma mark Runtime Styling enumerations


A documentation section name (that is, amark in a public header) should describe a task, in title case for consistency:

#pragma mark Working with Style Layer Attribute Values

1ec5 · 2016-11-20T22:52:33Z

platform/ios/app/MBXViewController.m

 - (void)styleFilteredLines
 {
+
+#warning remove this test stub


Optional: We currently lack a demo in iosapp that draws a route line. We could add one that sets the line join and line cap to something that looks better than the default.

1ec5 · 2016-11-20T22:53:13Z

platform/ios/ios.xcodeproj/project.pbxproj

@@ -127,6 +127,10 @@
 		4018B1C91CDC288A00F666AF /* MGLAnnotationView_Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 4018B1C31CDC277F00F666AF /* MGLAnnotationView_Private.h */; };
 		4018B1CA1CDC288E00F666AF /* MGLAnnotationView.h in Headers */ = {isa = PBXBuildFile; fileRef = 4018B1C51CDC277F00F666AF /* MGLAnnotationView.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		4018B1CB1CDC288E00F666AF /* MGLAnnotationView.h in Headers */ = {isa = PBXBuildFile; fileRef = 4018B1C51CDC277F00F666AF /* MGLAnnotationView.h */; settings = {ATTRIBUTES = (Public, ); }; };
+		4032C5BF1DE1FC780062E8BD /* NSValue+MGLStyleEnumAttributeAdditions.h in Headers */ = {isa = PBXBuildFile; fileRef = 4032C5B81DE1EE7D0062E8BD /* NSValue+MGLStyleEnumAttributeAdditions.h */; };


These headers should be public.

1ec5 · 2016-11-20T22:54:56Z

platform/ios/app/MBXViewController.m

@@ -5,6 +5,7 @@
 #import "MBXOfflinePacksTableViewController.h"
 #import "MBXAnnotationView.h"
 #import "MBXUserLocationAnnotationView.h"
+#import "NSValue+MGLStyleEnumAttributeAdditions.h"


Import this header in both SDKs’ umbrella headers instead of here, since only iosapp and macosapp have this file in their header search paths.

1ec5 · 2016-11-20T23:05:13Z

platform/darwin/src/MGLStyleValue_Private.h

+                auto str = mbgl::Enum<MBGLType>::toString(mbglStop.second);
+                MGLType mglType;
+                mglType = mbgl::Enum<MGLType>::toEnum(str).value_or(mglType);
+                stops[@(mbglStop.first)] = [MGLStyleValue valueWithRawValue:[NSValue value:&mglType withObjCType:@encode(NSUInteger)]];


Encode MGLType instead of NSUInteger, if possible.

1ec5 · 2016-11-20T23:07:21Z

platform/darwin/src/MGLStyleValue_Private.h

@@ -13,6 +15,35 @@

 #include <array>

+template <typename MBGLType, typename MGLType>
+class MGLStyleEnumerationValueTransformer {


Can this method live inside MGLStyleValueTransformer? Use optional template parameters on the class to allow both NSValue * and MGLLineCap to be specific, and use conditional template parameters on the enumeration-specific methods to enable the methods only for enumerations.

Correctly map SDK runtime styling enumerations to mbgl equivalents. Also, add category methods to NSValue so enums can be wrapped up with less of the details of how they are layed out in memory in Objective-C.

boundsj · 2016-11-22T19:33:50Z

This will also fix #7153

1ec5

Looks good other than a couple nits about whitespace and indentation.

1ec5 · 2016-11-22T18:01:13Z

platform/darwin/src/MGLBackgroundStyleLayer.mm

@@ -10,6 +10,7 @@

 #include <mbgl/style/layers/background_layer.hpp>

+


Nit: extra whitespace.

1ec5 · 2016-11-22T23:12:52Z

platform/darwin/src/MGLCircleStyleLayer.mm

-    auto mbglValue = MGLStyleValueTransformer<mbgl::style::TranslateAnchorType, NSValue *>().toPropertyValue(circleTranslateAnchor);
-    self.rawLayer->setCircleTranslateAnchor(mbglValue);
+    auto mbglValue = MGLStyleValueTransformer<mbgl::style::TranslateAnchorType,
+    NSValue *,


Nit: funky indentation. Can this all go on one line?

This fixes an issue where the documentation for all NSValue categories were described as `MGLGeometryAdditions`.

boundsj mentioned this pull request Nov 15, 2016

SDK runtime styling enumerations don’t match mbgl equivalents #6760

Closed

1ec5 suggested changes Nov 15, 2016

View reviewed changes

1ec5 added bug iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS release blocker Blocks the next final release runtime styling labels Nov 15, 2016

1ec5 added this to the ios-v3.4.0 milestone Nov 15, 2016

frederoni assigned boundsj Nov 15, 2016

boundsj added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Nov 15, 2016

boundsj force-pushed the boundsj-map-darwin-mbgl-style-enums branch from 19af5d1 to 72f009d Compare November 20, 2016 02:16

boundsj added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Nov 20, 2016

boundsj commented Nov 20, 2016

View reviewed changes

boundsj mentioned this pull request Nov 20, 2016

Add category convenience methods to NSNumber for style values (was: Runtime styling properties are too loosely typed) #5970

Closed

boundsj force-pushed the boundsj-map-darwin-mbgl-style-enums branch from c964aa9 to e330840 Compare November 20, 2016 21:49

1ec5 suggested changes Nov 20, 2016

View reviewed changes

[ios, macos] Refactor runtime styling enumeration properties

3745b04

Correctly map SDK runtime styling enumerations to mbgl equivalents. Also, add category methods to NSValue so enums can be wrapped up with less of the details of how they are layed out in memory in Objective-C.

boundsj force-pushed the boundsj-map-darwin-mbgl-style-enums branch from 10ee43b to b18b2bc Compare November 22, 2016 19:31

1ec5 approved these changes Nov 22, 2016

View reviewed changes

[ios, macos] Remove unused MGLGeometryAdditions category stub

5ae966a

This fixes an issue where the documentation for all NSValue categories were described as `MGLGeometryAdditions`.

boundsj force-pushed the boundsj-map-darwin-mbgl-style-enums branch from b18b2bc to 5ae966a Compare November 23, 2016 03:32

boundsj removed the ✓ ready for review label Nov 23, 2016

boundsj merged commit d6d16c1 into release-ios-v3.4.0 Nov 23, 2016

boundsj deleted the boundsj-map-darwin-mbgl-style-enums branch November 23, 2016 03:53

1ec5 mentioned this pull request Nov 23, 2016

NSValue (MGLStyleEnumAttributeAdditions) declares unprefixed properties #7168

Closed

boundsj mentioned this pull request Nov 23, 2016

All NSValue methods live in an MGLGeometryAdditions category in documentation #7153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map Darwin style enumerations to mbgl equivalents #7061

Map Darwin style enumerations to mbgl equivalents #7061

boundsj commented Nov 15, 2016

mention-bot commented Nov 15, 2016

1ec5 Nov 15, 2016

1ec5 Nov 15, 2016

1ec5 Nov 15, 2016 •

edited

Loading

1ec5 Nov 15, 2016

boundsj commented Nov 20, 2016

boundsj Nov 20, 2016

boundsj commented Nov 20, 2016

boundsj commented Nov 20, 2016

1ec5 Nov 20, 2016

1ec5 Nov 20, 2016

1ec5 Nov 20, 2016

1ec5 Nov 20, 2016

1ec5 Nov 20, 2016

1ec5 Nov 20, 2016

1ec5 Nov 20, 2016

1ec5 Nov 20, 2016

1ec5 Nov 20, 2016

1ec5 Nov 20, 2016

1ec5 Nov 20, 2016

1ec5 Nov 20, 2016

boundsj commented Nov 22, 2016

1ec5 left a comment

1ec5 Nov 22, 2016

1ec5 Nov 22, 2016

		@@ -10,6 +10,13 @@

		#include <mbgl/style/layers/background_layer.hpp>

		namespace mbgl {

		*/
		+ (instancetype)valueWithMGLCirclePitchScale:(MGLCirclePitchScale)type;


		@interface NSValue (MGLStyleEnumAttributeAdditions)

		#pragma mark Runtime Styling enumerations

		@@ -10,6 +10,7 @@

		#include <mbgl/style/layers/background_layer.hpp>

Map Darwin style enumerations to mbgl equivalents #7061

Map Darwin style enumerations to mbgl equivalents #7061

Conversation

boundsj commented Nov 15, 2016

mention-bot commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boundsj commented Nov 20, 2016

Choose a reason for hiding this comment

boundsj commented Nov 20, 2016

boundsj commented Nov 20, 2016

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

boundsj commented Nov 22, 2016

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Nov 15, 2016 •

edited

Loading