Refactor: use mbgl::style::conversion system to convert MGLStyleFunction to mbgl core types by anandthakker · Pull Request #8026 · mapbox/mapbox-gl-native

anandthakker · 2017-02-11T02:18:10Z

Quoting from #7944 (comment) (see the thread there for a bit more context):

Followup on #7944 (comment): As I started digging into resolving the compilation issues in the MGLCompositeStyleFunction => mbgl::style::CompositeFunction conversion today, it felt like I was solving the same problems that are being solved by mbgl::style::conversion::*.

I'm now thinking that we should accomplish the MGL => mbgl conversion — at least for property values — by having the darwin platform bindings implement the Value conversions required to leverage the existing mbgl::style::conversion system. This will hopefully mean less code/logic duplication and better maintainability.

The approach I'm going for here is for MGLStyleValueTransformer::toDataDrivenPropertyValue() to:

Use the new NSObject * MGLStyleValueTransformer::toRawStyleSpecValue(MGLStyleFunction<T> *value) method to convert an MGLStyleFunction to an plain NSObject-based hierarchy isomorphic to the style spec JSON for the given style function value.
Stuff that into a MGLConversionValue, whose job is simply to be a convenience wrapper for the mbgl::style::conversion overloads provided in darwin_conversion.hpp.
Having implemented these base overloads, we can now just call convert<mbgl::style::DataDrivenPropertyValue<MBGLType>>(value) and it "just works". (More on the conversion system here)

cc @1ec5 @boundsj - (note this is still WIP; happy to have early course corrections if you want to peek, but feel free to just ignore for now and I'll ping when it's ready)

mention-bot · 2017-02-11T02:18:12Z

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

anandthakker · 2017-02-11T02:21:33Z

Oh, another note: my intent was to keep this PR/refactor scoped to handling StyleFunction types (because those are the ones currently presenting difficulties / blocking my symbol DDS work). But once it's working, propagating this approach to the other MGL => mbgl conversions should be pretty much as simple as implementing toRawStyleSpecValue for each MGL type.

anandthakker · 2017-02-11T02:25:14Z

+template <class Fn>
+optional<Error> eachMember(const MGLConversionValue&, Fn&&) {
+    // TODO
+    // mbgl::Log::Warning(mbgl::Event::Android, "eachMember not implemented");


(note to self) Here, and in some other places, need to raise/log an error in whatever way is "right" for this SDK/project.

1ec5

This is a good idea for avoiding repetition and modularizing the code. The multistep process is a clever way to work around the fact that C++ method overloading is unable to account for Objective-C class inheritance.

Most of the feedback below pertains to Objective-C concepts. I encourage you to read the linked articles at some point, but a general introduction to the language would help to explain some of the quirkiness you see in any Objective-C++ codebase.

1ec5 · 2017-02-11T06:33:35Z

+//
+//  Created by Anand Thakker on 2/10/17.
+//  Copyright © 2017 Mapbox. All rights reserved.
+//


In this repository, we typically remove the comment block that Xcode adds by default. Authorship can be determined from commit history and copyright information is explained in excruciating detail in LICENSE.md.

1ec5 · 2017-02-11T06:34:20Z

+//
+
+#ifndef MGLConversionValue_h
+#define MGLConversionValue_h


Neither guard defines nor #pragma once is needed in Objective-C headers, because #import is smart enough to avoid including twice.

1ec5 · 2017-02-11T06:35:28Z

+
+class MGLConversionValue {
+public:
+    Value(NSObject * _value) : value(_value) {}


Nit: the predominant Objective-C style leaves no space between * and the identifier.

1ec5 · 2017-02-11T06:42:32Z

+    };
+
+private:
+    NSObject * value;


Use id rather than NSObject * when you don’t want to constrain the variable’s class at all. The compiler won’t bother performing type checking on id. Moreover, unlike void *, id must be an Objective-C object. As a result, as long as you use [bracket syntax] instead of dot.syntax, you can pass any message you like to the id without casting.

(Technically, id also allows the object to be an NSProxy; however, proxies are quite rare in practice.)

It seems like we do want to constrain the object's class here, though, since we're making assumptions about value being an instance of the NSObject class hierarchy -- no?

For all practical purposes, NSObject and NSProxy are the only two root classes in Objective-C. If you don't care about the difference between NSObject and NSProxy – you may never encounter any reason to care about this distinction – then id is tantamount to NSObject * except with the convenience of not having to cast to a more specific type.

Ah, I see 👍

1ec5 · 2017-02-11T06:45:50Z

+    Value(NSObject * _value) : value(_value) {}
+
+    bool isNull() const {
+        return value;


This seems to be returning true if the value is non-null. Moreover, in some existing code that converts from Foundation types to C++ types, we also treat NSNull as a null value.

1ec5 · 2017-02-11T07:30:43Z

 /* End PBXCopyFilesBuildPhase section */

 /* Begin PBXFileReference section */
+		17C4564F1E4E19BB0029C9B1 /* darwin_conversion.hpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.h; path = darwin_conversion.hpp; sourceTree = "<group>"; };


Remember to add this file to macos.xcodeproj as well, since all the runtime styling code is shared between both platforms.

1ec5 · 2017-02-11T07:31:50Z

+    return value.getLength();;
+}
+
+inline mbgl::android::Value arrayMember(const MGLConversionValue& value, std::size_t i) {


Did you mean mbgl::Value?

Heh, actually it should have been MGLConversionValue.

1ec5 · 2017-02-11T07:34:47Z

+    };
+
+    Value get(const char* key) const {
+        NSDictionary *dict = (NSDictionary *)value;


Is it guaranteed that this method will only be called if isObject() returns true? Even if so, it might be worth asserting that we’re dealing with an NSDictionary.

It's guaranteed in the spec, but agreed on asserting anyway.

1ec5 · 2017-02-11T07:39:11Z

+        return { value.toString() };
+    } else if (value.isNumber()) {
+        // Need to cast to a double here as the float is otherwise considered a bool...
+       return { (double) value.toNumber() };


In C++ and Objective-C++, the static_cast syntax allows the compiler to warn you when you try to cast to an unrelated type. A C-style cast, by contrast, allows you to cast to any type with abandon.

1ec5 · 2017-02-11T07:44:21Z

+        return { value.toString() };
+    } else if (value.isNumber()) {
+        // Need to cast to a double here as the float is otherwise considered a bool...
+       return { (double) value.toNumber() };


Could MGLConversionValue::toNumber() return a double in the first place, avoiding the need for this cast? Converting float to double often leaves unwanted artifacts at the smallest decimal places.

anandthakker · 2017-02-13T14:45:54Z

@1ec5 Thank you for this extremely helpful review-slash-Objective-C-lesson!

jfirebaugh · 2017-02-13T19:25:12Z

@@ -0,0 +1,68 @@
+#include <string>
+
+class MGLConversionValue {


What's the purpose of MGLConversionValue in this PR, as opposed to inlining each of its methods in the corresponding method in darwin_conversion.hpp? E.g.:

inline bool isUndefined(NSObject *value) { return !value || value == [NSNull null]; }

@jfirebaugh hah -- yeah, this is exactly the question I started asking myself this morning: #8026 (comment). I started out with this wrapper because I didn't know at the time that a fully/cleanly NSObject-based hierarchy would be possible. If you agree that the wrapper's unnecessary, I'd love to remove it.

Good point – we aren't actually using method overloading here, so the fact that both bool and float are encapsulated in NSNumber just means the implementation of isBool() and isNumber() would involve an ||.

So this file should be removed now, right?

anandthakker · 2017-02-13T23:34:39Z

@1ec5 @jfirebaugh okay, I think this should now be ready for a (re-)review. Two outstanding issues:

One failing unit test--MGLStyleValueTests::testFunctionsWithDataDrivenProperties--due to precision loss in color values -- this happens because, in order to use the conversion subsystem, we end up roundtripping the color value through an "rgb(...)" string -- so [..., 0.5, ...] becomes "rgb(..., 127.5, ...)" becomes [..., 0.498039,...].
Per Refactor: use mbgl::style::conversion system to convert MGLStyleFunction to mbgl core types #8026 (review), remove MGLConversionValue, as it is an unnecessary level of indirection. (I just tried this locally and am getting some mysterious error '__val__' is unavailable in ARC. Storing that on this temporary branch till tomorrow AM when I'll take another look.)

jfirebaugh · 2017-02-14T00:40:39Z

"rgb(...)" strings don't limit the precision of their components, do they? Seems like we should be able to fix the precision loss.

jfirebaugh · 2017-02-14T00:46:50Z

Oh, I see, the issue isn't the stringification, it's that the conversion back goes through CSSColorParser::Color which uses unsigned char components. We don't guarantee more precision than that in other parts of the rendering, so I think this is something that the test will have to adjust for.

1ec5 · 2017-02-14T01:05:26Z

some mysterious error 'val' is unavailable in ARC

ARC is unable to reason about references like NSObject *&. With an Objective-C object, you should only ever need to use * or more rarely **, never & or *&.

anandthakker · 2017-02-14T12:57:56Z

ARC is unable to reason about references like NSObject *&. With an Objective-C object, you should only ever need to use * or more rarely **, never & or *&.

Ah. Yeah, the *& did seem weird and unnecessary even when I was typing it. But, alas, changing the types of the function parameters to the more sensical NSObject * doesn't solve the problem -- I still get the ARC error, and I think it's because we need an optional<NSObject *> -- the error log references a line from std::experimental::optional that looks like:

    _LIBCPP_INLINE_VISIBILITY constexpr optional(const value_type& __v)

So it seems like trying to make an optional<NSObject *>, we're still stuck with NSObject *& under the hood.

@jfirebaugh @1ec5 does this mean we basically need to stick with having a wrapper class? Or is there another way out?

anandthakker · 2017-02-14T16:35:01Z

Added an accuracy threshold to the color equality checks per #8026 (comment). @1ec5 as those were my first lines of Swift, I would imagine that I've violated any number of conventions/idioms, heh.

1ec5 · 2017-02-14T18:11:15Z

So it seems like trying to make an optional<NSObject *>, we're still stuck with NSObject *& under the hood.

It’s already a pointer, so normally you’d the absence of a value with nil. If the conversion system is insisting on using optional, then yes, we probably need the wrapper class.

anandthakker · 2017-02-14T18:15:52Z

It’s already a pointer, so you can indicate the absence of a value with nil. If the conversion system is insisting on using optional, then yes, we probably need the wrapper class.

Yep, it expects optional<>s from some of the conversion functions: https://github.com/mapbox/mapbox-gl-native/blob/master/include/mbgl/style/conversion.hpp#L47-L52

1ec5 · 2017-02-14T18:14:28Z


-
+#if os(iOS) || os(watchOS) || os(tvOS)
+typealias ColorType = UIColor


In Objective-C code, we’ve been using MGLColor as the name of the platform-agnostic color type.

1ec5 · 2017-02-14T18:15:12Z

+#endif
+
 extension MGLStyleValueTests {
+    func assertColorsEqualWithAccuracy(_ actual: ColorType, _ expected: ColorType, accuracy: Float = 0.1) {


Just an observation, not a request for change: Swift lets you overload methods like in C++. So you could overload XCTAssertEqualWithAccuracy(_:_:accuracy:) with two MGLColor arguments. Copying the existing XCTAssertEqualWithAccuracy(_:_:accuracy:) signature, you’d wind up with:

func XCTAssertEqualWithAccuracy<T : FloatingPoint>(_ color1: MGLColor, _ color2: MGLColor, accuracy: T, _ message: @autoclosure () -> String = "", file: StaticString = #file, line: UInt = #line)

With this fancy signature, you could pass file and line into the calls to the normal XCTAssertEqualWithAccuracy(_:_:accuracy:) and get a failure message that refers to the original line number.

1ec5 · 2017-02-14T18:19:12Z


-
+#if os(iOS) || os(watchOS) || os(tvOS)
+typealias ColorType = UIColor


With this typealias in place, it might be possible to unconditionalize the giant #if block in testFunctionsWithDataDrivenProperties().

1ec5 · 2017-02-14T18:31:47Z

+#endif
+
 extension MGLStyleValueTests {
+    func assertColorsEqualWithAccuracy(_ actual: ColorType, _ expected: ColorType, accuracy: Float = 0.1) {


Is 0.1 all the accuracy we can guarantee?

Oh, whoops, I meant for that to be 0.01, although I guess 0.005 is maybe a better, as a roundish number close to 1/255.

= 1/255 also compiles.

1ec5 · 2017-02-14T18:32:53Z

+        if let actualConstant = actual as? MGLStyleConstantValue<ColorType> {
+            XCTAssertTrue(expected is MGLStyleConstantValue<ColorType>)
+            assertColorsEqualWithAccuracy(actualConstant.rawValue, (expected as! MGLStyleConstantValue<ColorType>).rawValue)
+            return


It looks like no code would get executed beyond this if statement, so no return statement is needed.

1ec5 · 2017-02-14T18:33:58Z

+            // TODO: assert default values are equal
+            if actualFunction.stops == nil {
+                XCTAssertNil(expectedFunction.stops)
+                return


Use a guard … else statement for an early return. (Accentuate the positive!) Better yet, use a guard let … else statement for an early return conditioned on the existence of a variable you want to make use of below.

Ooh, guard let is so nice!

1ec5 · 2017-02-14T21:57:25Z

@@ -0,0 +1,68 @@
+#include <string>


Also import any Objective-C headers you need, namely the Foundation/Foundation.h umbrella header. You’re getting away with omitting them right now because you happen to be importing this header after other headers in other files.

1ec5 · 2017-02-14T22:01:13Z

+        return [array count];
+    };
+
+    MGLConversionValue get(const std::size_t index ) const {


size_t isn’t explicitly defined to be the same size as NSUInteger (the type used to subscript NSArray below), even though I think that happens to be the case. To avoid even the potential of an out-of-bounds array access, you could assert that index is at most NSUIntegerMax.

1ec5 · 2017-02-14T22:03:39Z

 #import "NSValue+MGLStyleAttributeAdditions.h"
 #import "MGLTypes.h"

+#include "darwin_conversion.hpp"


The convention for our predominantly Objective-C targets is to use .h for Objective-C++ headers. We also use MGLCamelCase for these headers.

1ec5 · 2017-02-14T22:04:27Z

@@ -0,0 +1,132 @@
+#pragma once


As long as this header is #imported, no #pragma once guard is needed.

1ec5 · 2017-02-14T22:04:41Z

+#include <mbgl/util/logging.hpp>
+#include <mbgl/style/conversion.hpp>
+#include <mbgl/util/feature.hpp>
+#include <mbgl/util/optional.hpp>


Also import the Foundation/Foundation.h umbrella header.

1ec5 · 2017-02-14T22:06:52Z

+    }
+
+    NSObject* operator*() {
+        assert(this);


Use NSCAssert() instead of assert() for files that are part of the Objective-C targets. NSCAssert() provides better diagnostics via NSException and works with Xcode’s built-in Objective-C exception breakpoint.

1ec5 · 2017-02-14T22:07:44Z

+// due to things like `optional(const value_type& __v)`
+class OptionalNSObjectValue {
+public:
+    OptionalNSObjectValue(NSObject *_value) : value(_value) {}


Annotate value’s type as _Nullable and surround the header’s contents in NS_ASSUME_NONNULL_BEGIN and NS_ASSUME_NONNULL_END to enable better compiler/analyzer diagnostics.

1ec5 · 2017-02-14T22:09:42Z

+        return value;
+    }
+
+    NSObject* operator*() {


Nit: move the space to before the *.

1ec5 · 2017-02-14T22:13:17Z

+
+inline std::size_t arrayLength(const NSObject* value) {
+    NSCAssert([value isKindOfClass:[NSArray class]], @"Value must be an NSArray for getLength().");
+    NSArray * array = (NSArray *)value;


I’d still encourage you to explore the use of id instead of NSObject *. Saying “NSObject” here tells the compiler (and the code reviewer) that the function wants an NSObject specifically, not some subclass, which is why you end up having to cast all the time.

1ec5 · 2017-02-14T22:16:27Z

+        return { *toString(value) };
+    } else if (_isNumber(value)) {
+        // Need to cast to a double here as the float is otherwise considered a bool...
+       return { (double) *toNumber(value) };


Use static_cast instead of a C-style cast.

jfirebaugh · 2017-02-14T22:15:43Z

+class OptionalNSObjectValue {
+public:
+    OptionalNSObjectValue(NSObject *_value) : value(_value) {}
+    OptionalNSObjectValue() {}


Omit this constructor...

jfirebaugh · 2017-02-14T22:16:49Z

+    if (value && value != [NSNull null]) {
+        return { member };
+    } else {
+        return {};


...and return { nullptr }; here.

jfirebaugh · 2017-02-14T22:17:18Z

+}
+
+template <class Fn>
+optional<Error> eachMember(const NSObject*, Fn&&) {


Don't define this method at all. Then you'll get a compiler error if something tries to use it.

1ec5

This is looking pretty good, especially on the C++ side. assertColorValuesEqual(_:_:) could use some more massaging, but you’re almost over the finish line.

1ec5 · 2017-02-14T23:54:52Z

+
+// A minimal wrapper class conforming to the requirements for `objectMember(v, name)` (see mbgl/style/conversion.hpp)
+// This is necessary because using `NSObject*` as the value type in `optional<NSObject*>` causes problems for the ARC,
+// due to things like `optional(const value_type& __v)`


This looks like a documentation comment. You can use the /** … */ syntax for a multiline documentation comment or the /// … syntax for a single-line documentation comment.

1ec5 · 2017-02-14T23:56:07Z

+    NSCAssert([value isKindOfClass:[NSArray class]], @"Value must be an NSArray for getLength().");
+    NSArray *array = value;
+    auto length = [array count];
+    NSCAssert(length < std::numeric_limits<size_t>::max(), @"Array length out of bounds.");


To be pedantic, shouldn’t a length equal to the maximum representable length also be allowable? 😉

1ec5 · 2017-02-14T23:58:12Z

+inline OptionalNSObjectValue objectMember(const id value, const char *key) {
+    NSCAssert([value isKindOfClass:[NSDictionary class]], @"Value must be an NSDictionary for get(string).");
+    NSDictionary *dict = value;
+    NSObject *member = dict[@(key)];


Nit: subscript notation in this case is shorthand for a call to -[NSDictionary objectForKey:]. You can avoid creating a separate dict variable by calling -objectForKey: on the id-typed value.

1ec5 · 2017-02-14T23:59:01Z

+    NSCAssert([value isKindOfClass:[NSArray class]], @"Value must be an NSArray for get(int).");
+    NSCAssert(i < NSUIntegerMax, @"Index must be less than NSUIntegerMax");
+    NSArray *array = value;
+    return array[i];


Nit: subscript notation in this case is shorthand for a call to -[NSArray objectAtIndex:]. You can avoid creating a separate array variable by calling -objectAtIndex: on the id-typed value.

👍 @1ec5 I take it that the purpose of doing this would be for code conciseness, right?

Yes. It matters more for more complex functions where you’d otherwise potentially have to keep both variables in sync.

1ec5 · 2017-02-15T00:01:09Z

+
+inline optional<std::string> toString(const id value) {
+    if (_isString(value)) {
+        return std::string([((NSString *)value) UTF8String]);


Nit: extra parentheses around value make the line harder to read. Incidentally, you can also write:

std::string((const char *)[value UTF8String])

In the form you suggested, would static_cast be preferable? If not, why not?

1ec5 · 2017-02-15T00:03:23Z

+    }
+
+    func assertColorValuesEqual(_ actual: MGLStyleValue<MGLColor>, _ expected: MGLStyleValue<MGLColor>) {
+        if let actualConstant = actual as? MGLStyleConstantValue<MGLColor> {


This is fine, but FYI you can avoid having to come up with a unique name for the coerced value by shadowing the original variable’s name:

if let actual = actual as? MGLStyleConstantValue<MGLColor> …

1ec5 · 2017-02-15T00:04:27Z

+            let expectedStops = expectedFunction.stops!
+            XCTAssertEqual(actualStops.keys.count, expectedStops.keys.count)
+            let actualKeys = actualStops.keys.sorted(by: { String(describing: $0) < String(describing: $1) })
+            let expectedKeys = expectedStops.keys.sorted(by: { String(describing: $0) < String(describing: $1) })


You can assert that actualStops.keys == expectedStops.keys to check both the count and the existence of identical pairs at the same time.

1ec5 · 2017-02-15T00:05:05Z

+            }
+            let expectedStops = expectedFunction.stops!
+            XCTAssertEqual(actualStops.keys.count, expectedStops.keys.count)
+            let actualKeys = actualStops.keys.sorted(by: { String(describing: $0) < String(describing: $1) })


Normally, you can call the operator directly instead of writing a new closure:

let actualKeys = actualStops.keys.sorted(by: <)

This syntax relies on type inference to choose the right operator. I’m not sure if it’s a problem that the compiler thinks the keys are of type Any rather than Comparable.

The original keys are Strings in some cases and NSNumbers (or perhaps Floats?) in other cases. With NSNumber, you’d probably need .floatValue or at least as Float. With the current code, you’re going to wind up with some weird sorting in the case of NSNumber. For example, "100" < "2". Fortunately, there shouldn’t be a problem as long as both arrays are sorted consistently.

Yeah -- I originally tried just using < as the sort function, but it seemed to think that the keys were of type Any. I landed at the same conclusion as you: that the weird alphanumeric sort wouldn't matter here... but, alas, I think we should probably figure this key type thing out in more detail -- if the conversion stuff is turning numeric stop values into string values, that's a bug, and comparing stringified values of the keys like we're doing here would occlude it.

What if?

var actualKeys: [Any] if let keys = actualStops.keys as? [String] { actualKeys = keys.sorted(by: <) } else if let keys = actualStops.keys as? [Float] { actualKeys = keys.sorted(by: <) } else { XCTFail("Stop keys must be either Strings or Floats.") }

1ec5 · 2017-02-15T00:22:07Z

+            let actualKeys = actualStops.keys.sorted(by: { String(describing: $0) < String(describing: $1) })
+            let expectedKeys = expectedStops.keys.sorted(by: { String(describing: $0) < String(describing: $1) })
+
+            for (ak, ek) in zip(actualKeys, expectedKeys) {


The documentation for Array.values says:

When iterated over, values appear in this collection in the same order as they occur in the dictionary’s key-value pairs.

So it might be possible to say zip(actual.values, expected.values) to avoid the relative inefficiency of subscripting both dictionaries for each key.

It's not clear to me from that documentation that two different dictionaries' .values would be in a consistent order... indeed, why would they?

1ec5 · 2017-02-15T00:23:35Z

+            let expectedFunction = expected as! MGLStyleFunction<MGLColor>
+            XCTAssertEqual(actualFunction.interpolationBase, expectedFunction.interpolationBase)
+            XCTAssertEqual(actualFunction.interpolationMode, expectedFunction.interpolationMode)
+            // TODO: assert default values are equal


Does this still need to happen?

jfirebaugh · 2017-02-15T00:35:06Z

 template <class T, class V>
 Result<optional<T>> convertDefaultValue(const V& value) {
-    auto defaultValueValue = objectMember(value, "defaultValue");
+    auto defaultValueValue = objectMember(value, "default");


Can you investigate why this mistake didn't trigger test failures? Contrary to my earlier statement, we do have tests which I would have expected to fail due to this, added in mapbox/mapbox-gl-js@e328b80.

👍 yep, will do

@jfirebaugh The reason it didn't trigger failures is that the tests are marked ignored on native. I'll submit a PR to gl-js to un-ignore them.

anandthakker · 2017-02-15T18:07:22Z

@1ec5 updated to address the issues you raised, including an attempt to clean up the assertColorValuesEqual. Note that in the end, I found using .keys to require much more weird acrobatics than just switching on the type of the dict itself -- but lemme know what you think.

See mapbox/mapbox-gl-native#8026 (review)

Picks up mapbox/mapbox-gl-js#4212, which was rebased to include mapbox/mapbox-gl-js#4279 and mapbox/mapbox-gl-js#4280

anandthakker commented Feb 11, 2017

View reviewed changes

1ec5 suggested changes Feb 11, 2017

View reviewed changes

1ec5 added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS refactor runtime styling labels Feb 11, 2017

1ec5 added this to the ios-v3.5.0 milestone Feb 11, 2017

anandthakker force-pushed the mgl-function-converstion branch from f679f55 to 204ebb7 Compare February 13, 2017 18:13

jfirebaugh reviewed Feb 13, 2017

View reviewed changes

1ec5 reviewed Feb 14, 2017

View reviewed changes

jfirebaugh suggested changes Feb 14, 2017

View reviewed changes

1ec5 approved these changes Feb 15, 2017

View reviewed changes

jfirebaugh reviewed Feb 15, 2017

View reviewed changes

anandthakker mentioned this pull request Feb 15, 2017

[ios / Swift] Coerce zoom stops and exponential,interval stops to [Float:...] #8068

Closed

anandthakker added a commit to mapbox/mapbox-gl-js that referenced this pull request Feb 15, 2017

Enable regression tests for property function defaults

d6b48bc

See mapbox/mapbox-gl-native#8026 (review)

anandthakker mentioned this pull request Feb 15, 2017

Enable regression tests for property function defaults mapbox/mapbox-gl-js#4279

Merged

anandthakker added a commit to mapbox/mapbox-gl-js that referenced this pull request Feb 15, 2017

Enable regression tests for property function defaults (#4279)

e3f8d6e

See mapbox/mapbox-gl-native#8026 (review)

anandthakker added 22 commits February 15, 2017 16:22

Address assorted code review comments

03f0f93

Drop TODO question

a3d2558

Fix some more small errors

ee9fab2

Add missing include

19e8182

Add darwin_conversion and MGLConversionValue to projects/targets

99c36b7

Finish some TODOs

fac5aca

Fix a bunch of test failures

879aae4

[ios,macos] Handle [x,y] offset values in function stops conversion

817b115

Add relaxed color equality checking in swift test

b5bda3e

Use NSColor on macOS

9f8ea0a

More code review fixes

555d046

inline MGLConversionValue to simplify darwin_conversion

7c8d948

Make default color precision constant more meaningful

b7df7b3

Fix dict comparison (and bad/silent assertion)

18b340e

Remove unused file

8655269

Moar code review fixes

0a27c21

Bounds check for array index conversion

5da0cc0

Some more edits

d94041f

Clean up color value assertion, use floats for stops

c612529

Fix type names

dca9fea

Update gl-js submodule

bdc0ca4

Picks up mapbox/mapbox-gl-js#4212, which was rebased to include mapbox/mapbox-gl-js#4279 and mapbox/mapbox-gl-js#4280

Final cleanup up

9a1094b

anandthakker force-pushed the mgl-function-converstion branch from 5fd7cf5 to 9a1094b Compare February 15, 2017 21:22

Correction to gl-js submodule reference

f854ae3

jfirebaugh approved these changes Feb 15, 2017

View reviewed changes

anandthakker merged commit 685f237 into master Feb 15, 2017

This was referenced Feb 16, 2017

[core] Enable property functions for text-{field,transform} #7944

Merged

Refactor MGLStyleValueTransformer to use mbgl::style::conversion system for all value types. #8088

Closed

anandthakker deleted the mgl-function-converstion branch February 21, 2017 17:42

1ec5 mentioned this pull request Mar 3, 2017

[ios] MGLAnnotationView annotation property made writable #8139

Merged



		#if os(iOS) \|\| os(watchOS) \|\| os(tvOS)
		typealias ColorType = UIColor

Conversation

anandthakker commented Feb 11, 2017

Uh oh!

mention-bot commented Feb 11, 2017

Uh oh!

anandthakker commented Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anandthakker Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anandthakker commented Feb 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anandthakker commented Feb 13, 2017

Uh oh!

jfirebaugh commented Feb 14, 2017

Uh oh!

jfirebaugh commented Feb 14, 2017

Uh oh!

1ec5 commented Feb 14, 2017

Uh oh!

anandthakker commented Feb 14, 2017

Uh oh!

anandthakker commented Feb 14, 2017

Uh oh!

1ec5 commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anandthakker commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

anandthakker commented Feb 11, 2017 •

edited

Loading

anandthakker Feb 11, 2017 •

edited

Loading

1ec5 commented Feb 14, 2017 •

edited

Loading

anandthakker commented Feb 14, 2017 •

edited

Loading

1ec5 Feb 14, 2017 •

edited

Loading

1ec5 Feb 14, 2017 •

edited

Loading

anandthakker Feb 14, 2017 •

edited

Loading