Per-attribute transition properties on MGLStyleLayer #8225

fabian-guerra · 2017-02-28T17:56:57Z

mention-bot · 2017-02-28T18:50:03Z

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

incanus

Should this PR be based against the 3.5.0 release branch since it builds upon data-driven styling?

incanus · 2017-02-28T22:47:27Z

platform/darwin/src/MGLTypes.h

@@ -85,6 +85,21 @@ typedef NS_OPTIONS(NSUInteger, MGLMapDebugMaskOptions) {
 #endif
 };

+/**
+ A structure containing information about a transition values.


Remove values.

incanus · 2017-02-28T22:47:56Z

platform/darwin/src/MGLTypes.h

+    NSTimeInterval duration;
+
+    /**
+     The delay in seconds to before applying any changes to the style URL or to layout and paint attributes.


Remove to in to before.

incanus · 2017-02-28T22:49:04Z

platform/darwin/src/MGLSymbolStyleLayer.mm

+{
+    NSString *camelCaseKey;
+    if ([key length] > 1) {
+        camelCaseKey = [NSString stringWithFormat:@"%@%@",[[key substringToIndex:1] uppercaseString],[key substringFromIndex:1]];


Code style: spaces after ,.

incanus · 2017-02-28T22:52:44Z

platform/darwin/src/MGLStyleLayer.mm.ejs

@@ -14,6 +14,8 @@
 #import "MGLStyleValue_Private.h"
 #import "MGL<%- camelize(type) %>StyleLayer.h"

+#import "NSDate+MGLAdditions.h"
+


Put this further up with the others, next to #import "NSPredicate+MGLAdditions.h", a similar category use.

incanus · 2017-02-28T22:54:27Z

platform/darwin/src/MGLStyle.h

@@ -30,6 +30,7 @@ NS_ASSUME_NONNULL_BEGIN
 */
 static MGL_EXPORT const NSInteger MGLStyleDefaultVersion = 9;

+


Extraneous whitespace.

incanus · 2017-02-28T22:55:07Z

platform/darwin/src/MGLStyle.h

@@ -194,6 +195,11 @@ MGL_EXPORT
 @property (nonatomic, strong) NS_SET_OF(__kindof MGLSource *) *sources;

 /**
+ Transition values.


Should describe it a bit more here, maybe:

Values describing animated transitions to styling changes, either to the style URL or to individual properties.

incanus · 2017-02-28T22:56:36Z

platform/darwin/src/MGLStyle.mm

+- (void)setTransition:(MGLTransition)transition
+{
+    [self setTransitionDuration:transition.duration];
+    [self setTransitionDelay:transition.delay];


We should probably get rid of the separate internal -setTransitionDuration: and -setTransitionDelay: since externally we only set full transition structures containing both values.

incanus · 2017-02-28T22:59:19Z

platform/darwin/src/MGLStyleLayer.mm.ejs

+
+        [setPropertyInvocation setArgument:&transition atIndex:2];
+
+        [setPropertyInvocation performSelector:@selector(invoke)];


Couldn't this be simply -[NSObject performSelector:withObject:] since it's only one argument, passing the transition wrapped in NSValue?

@incanus using an NSValue I will have to wrap it then unwrap in set***Transition:, also I will have to add pragma marks to avoid the "PerformSelect may cause a leak because...".

I think this approach is more straightforward, what are your thoughts?

boundsj

As you continue with this PR, please also add appropriate tests (generated with MGLStyleLayerTests.mm.ejs) and the obligatory changelog entry 😄

boundsj · 2017-02-28T23:43:58Z

platform/darwin/src/MGLStyleLayer.mm.ejs

+    NSString *setPropertyTransitionString = [NSString stringWithFormat:@"mbx_set%@Transition:", camelCaseKey];
+    SEL setPropertyTransitionSelector = NSSelectorFromString(setPropertyTransitionString);
+
+    if ([self respondsToSelector:setPropertyTransitionSelector]) {


What is the main motivation to introduce dynamic method invocation for transitions?

Most of the complexity here is about parsing the string and forming the private method name to match the actual implementation and that introduces new types of edge cases that we did not previously need to handle. For example, what if the client app passes in a nonsensical string at runtime? So far, I think this implementation would fail silently. Should it log a warning, raise an exception, etc.? Do we expect to offer a string parameter based getter for each transition, too?

Since the style layer implementation is generated and in the spirit of consistency with the existing API and its per property getters and setters, I suggest that we consider making the mbx_ methods in this implementation public and avoid the dynamic invocation altogether.

I was pointed to the original post in #8010 (comment) so this makes sense now. Thanks!

I still vote for explicit getters and setters. However, if we do go with the CALayer influenced API, it would be nice to try a refactor so that some or all of this string parsing and method invocation logic goes into the base MGLStyleLayer class.

boundsj · 2017-02-28T23:44:54Z

platform/darwin/src/MGLSymbolStyleLayer.h

@@ -1204,6 +1206,15 @@ MGL_EXPORT
 * `MGLCameraStyleFunction` with an interpolation mode of:
   * `MGLInterpolationModeExponential`
   * `MGLInterpolationModeInterval`
+ * `MGLSourceStyleFunction` with an interpolation mode of:


This is a byproduct of the style spec / gl native mismatch. All of these comments should be manually removed in this PR (or work against a style spec gitsha that does not cause this to happen to avoid the inconvenience)

Also, it is likely that #7939 will land before this PR and, I think, this problem will go away then once this branch is rebased on top of that change set.

Yes, if you rebase this PR on top of master as of now, f901e77 will fix this problem 👍

boundsj · 2017-02-28T23:48:09Z

@incanus Should this PR be based against the 3.5.0 release branch since it builds upon data-driven styling?

The 3.5.0 branch is actually currently master. We expect to make (and share with Android) a 3.5.0 release branch later today or tomorrow. We can point this PR to that branch once it exists.

1ec5 · 2017-03-03T03:05:30Z

platform/darwin/src/MGLStyleLayer.mm

+- (NSArray *)transitionKeys
+{
+    // This is overridden by subclasses
+    return nil;


Raise an MGLAbstractClassException. Search for that string for an example to follow.

1ec5 · 2017-03-03T03:17:43Z

platform/darwin/src/MGLBackgroundStyleLayer.mm

+    [transitionKeys addObject:@"backgroundOpacity"];
+    [transitionKeys addObject:@"backgroundPattern"];
+
+    return transitionKeys;


It would be more efficient to return an array literal.

1ec5 · 2017-03-03T03:18:34Z

platform/darwin/src/MGLBackgroundStyleLayer.mm

@@ -84,13 +85,33 @@ - (void)removeFromMapView:(MGLMapView *)mapView

 #pragma mark - Accessing the Paint Attributes

+- (NSArray *)transitionKeys


Since this method doesn’t have any dependencies on other instance methods, you can make this a class method. But I’m unclear on this method’s purpose. It appears to be unused.

This method returns the keys which you can add a transition

Ah, looking more closely, this method appears to be used only in tests (but is exposed publicly). As an alternative to this method, could we move the array of keys into the tests themselves?

1ec5 · 2017-03-04T21:38:37Z

Since this PR is scheduled for iOS SDK v3.5.0, please rebase and retarget it at the release-ios-v3.5.0-android-v5.0.0 branch. Thanks!

incanus

I'm also seeing extraneous commits like 33f1982 in the mix. Is this because you've brought over some changes from master as well @fabian-guerra?

incanus · 2017-03-07T00:42:12Z

platform/darwin/src/MGLStyleLayer.mm.ejs

+
+    return transitionKeys;
+}
+


Yeah, per https://github.com/mapbox/mapbox-gl-native/pull/8225/files#r104082076, you could do like this:

+ (NSArray *)transitionKeys { return @[ <% for ... @"<%- camelizeWithLeadingLowercase(property.name) %>", <% } -%> ]; }

@incanus I was using master but retarget to rebase release-ios-v3.5.0-android-v5.0.0

In the future, do git rebase -i newBranchName and remove things you don't want as part of the PR. Then force-push and finally retarget the PR.

boundsj · 2017-03-07T01:02:54Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/Mapbox.java

@@ -43,7 +43,7 @@ public static synchronized Mapbox getInstance(@NonNull Context context, @NonNull
    return INSTANCE;
  }

-  private Mapbox(@NonNull Context context, @NonNull String accessToken) {
+  Mapbox(@NonNull Context context, @NonNull String accessToken) {


This PR seems to have picked up additional, unrelated commits along the way when it was rebased like this one from #8228

incanus · 2017-03-07T01:11:28Z

Fixed #8225 (review) manually with a rebase dropping these oddball non-@fabian-guerra commits and force-pushed to fix 👍

1ec5

In addition to the comments below, please add an entry to the iOS and macOS changelogs about this enhancement. Thanks!

1ec5 · 2017-03-07T01:23:00Z

platform/darwin/src/MGLStyleLayer.h

+/**
+ Returns an array of strings that identify transition keys attached to the layer.
+ */
+ (NSArray *)transitionKeys;


Should we expose this method publicly? If so, let's call it transitionableAttributes and also make the return type a set of strings instead of an unqualified array.

This Method is intended to explicitly return which layer attributes are transitionable, we can as well add a note on each property, but I think this is a better way to let know devs about what they can change, also this can be used as helper to set the same transition property to all transitionable attributes

1ec5 · 2017-03-07T01:25:09Z

platform/darwin/src/MGLStyleLayer.h

@@ -69,6 +69,17 @@ MGL_EXPORT
 */
 @property (nonatomic, assign) float minimumZoomLevel;

+#pragma mark Layer Transitions


jazzy uses these marks to produce section headings in the generated documentation. For consistency with Apple's documentation, we label each mark according to a task. Here, we can say "Timing Changes to Attributes".

1ec5 · 2017-03-07T01:25:29Z

platform/darwin/src/MGLStyleLayer.h

@@ -69,6 +69,17 @@ MGL_EXPORT
 */
 @property (nonatomic, assign) float minimumZoomLevel;

+#pragma mark Layer Transitions
+
+- (void)setTransition:(MGLTransition)transition forKey:(NSString *)key;


Public methods need documentation comments.

I'm thinking we might want to call it -setTransition:forAttribute: to be more specific about the kind of key that's being transitioned.

1ec5 · 2017-03-07T01:29:00Z

platform/darwin/src/MGLStyleLayer.mm

+    return transition;
+}
+
+- (SEL)mbx_selectorForKey:(NSString *)key type:(NSString *)type hasParams:(BOOL)params


There's no need to prefix private methods with a class prefix the way we prefix category methods.

This method would be a bit simpler if we had two methods: one for getters and one for setters.

Since all the valid setter method names form a closed set, we should generate a lookup table mapping attributes to the corresponding setter name, then look up the passed-in attribute in that table to decide what to call. That way, we can raise an exception when the developer passes in an untransitionable attribute or a bogus attribute. Currently, doing that leads to a nasty crash with no useful information.

incanus

I'm now not positive that the getter use of -Warc-performSelector-leaks is safe, since we are creating a new NSValue inside of mbx_getFooTransition and we aren't telling ARC how to dispose of it (the use in setters is fine since they return void). So I think the overridden leak warning is a valid one.

i.e. What happens to the NSValue at https://github.com/mapbox/mapbox-gl-native/pull/8225/files#diff-24c0d8cf4ae8bed8e0bc00616e9aae68R89 after we return the wrapped transition?

Additionally, per NSValue docs:

This method has the same effect as valueWithBytes:objCType: and may be deprecated in a future release. You should use valueWithBytes:objCType: instead.

fabian-guerra · 2017-03-08T02:14:40Z

After talking to @1ec5 and @boundsj I realized that the trade-off for having a single method to set transition VS a straightforward approach was not enough to justify the added complexity.

Each transitionable style attribute has it's corresponding method.

1ec5 · 2017-03-08T07:10:18Z

platform/darwin/src/MGLBackgroundStyleLayer.h

@@ -69,6 +69,11 @@ MGL_EXPORT
 #endif

 /**
+ `backgroundColor` transition attributes.


The transition affecting any changes to this layer’s backgroundColor property.

This property corresponds to the background-color-transition property in the style JSON file format.

(We can't quite say "in the Mapbox Style Specification", as we do elsewhere, because of mapbox/mapbox-gl-js#4081.)

1ec5 · 2017-03-08T07:14:24Z

platform/darwin/test/MGLBackgroundStyleLayerTests.mm

@@ -59,6 +62,11 @@ - (void)testProperties {
        XCTAssertThrowsSpecificNamed(layer.backgroundColor = functionStyleValue, NSException, NSInvalidArgumentException, @"MGLStyleValue should raise an exception if it is applied to a property that cannot support it");
        functionStyleValue = [MGLStyleValue<MGLColor *> valueWithInterpolationMode:MGLInterpolationModeInterval compositeStops:@{@18: constantStyleValue} attributeName:@"" options:nil];
        XCTAssertThrowsSpecificNamed(layer.backgroundColor = functionStyleValue, NSException, NSInvalidArgumentException, @"MGLStyleValue should raise an exception if it is applied to a property that cannot support it");
+        // Transition property test
+        layer.backgroundColorTransition = transitionTest;
+        MGLTransition backgroundColorTransition = layer.backgroundColorTransition;


By setting and getting before asserting, we're verifying that the transition value round-trips. That's good, but it doesn't rule out the possibility that both the getter and setter are buggy. To rule that out, we'd need to assert after setting that the underlying mbgl::style::BackgroundLayer::getBackgroundColorTransition() is correct.

1ec5 · 2017-03-08T07:15:47Z

platform/darwin/src/MGLSymbolStyleLayer.h

@@ -1753,6 +1814,7 @@ MGL_EXPORT
 */
 @property (nonatomic, null_resettable) MGLStyleValue<NSValue *> *textTranslationAnchor;

+


Nit: stray line.

1ec5 · 2017-03-08T07:21:10Z

platform/darwin/src/MGLTypes.h

+     */
+    NSTimeInterval delay;
+} MGLTransition;
+


We should provide a factory method MGLTransitionMake(). We might also want to add transition-related methods to NSValue(MGLAdditions) to make it easier to work with MGLTransition instances when an object is needed.

1ec5 · 2017-03-08T07:23:32Z

platform/darwin/src/MGLTypes.h

+ */
+typedef struct MGLTransition {
+    /**
+     The duration in seconds to animate any changes to the style URL or to layout and paint attributes.


It's a bit of a nonsequitur to talk about style URLs and attributes here. This structure is generic enough that its documentation should be more self-contained. For example:

The amount of time the animation should take, not including the delay.

(It doesn't include the delay, does it?)

1ec5 · 2017-03-08T07:27:32Z

platform/darwin/src/MGLTypes.h

+    NSTimeInterval duration;
+
+    /**
+     The delay in seconds before applying any changes to the style URL or to layout and paint attributes.


The amount of time in seconds to wait before beginning the animation.

1ec5

Almost there!

1ec5 · 2017-03-09T19:26:46Z

platform/darwin/src/MGLBackgroundStyleLayer.h

@@ -69,6 +69,13 @@ MGL_EXPORT
 #endif

 /**
+ The transition affecting any changes to this layer’s `backgroundColor` property.
+
+ This property corresponds to the background-color-transition property in the style JSON file format.


Surround background-color-transition in backticks.

1ec5 · 2017-03-09T19:29:30Z

platform/darwin/test/MGLFillStyleLayerTests.mm

+        options.delay = MGLTimeIntervalFromDuration(toptions.delay.value_or(mbgl::Duration::zero()));
+        options.duration = MGLTimeIntervalFromDuration(toptions.duration.value_or(mbgl::Duration::zero()));
+        XCTAssertEqual(options.delay, transitionTest.delay);
+        XCTAssertEqual(options.duration, transitionTest.duration);


I would structure this a little differently:

Set fillColorTransition to transitionTest.

Call getFillColorTransition().

Assert toptions.delay && *toptions.delay == transitionTest.delay.

1ec5 · 2017-03-09T19:31:16Z

platform/darwin/src/MGLTypes.h

+
+/**
+ Creates a new `MGLTransition` from the given duration and delay.
+ */


Document each @param and the @return value.

1ec5 · 2017-03-09T19:32:02Z

platform/darwin/src/NSValue+MGLAdditions.h

@@ -70,6 +71,22 @@ NS_ASSUME_NONNULL_BEGIN
 */
 @property (readonly) MGLOfflinePackProgress MGLOfflinePackProgressValue;

+#pragma mark Working with Transition values


Typo: s/values/Values/

1ec5

Looking great.

1ec5 · 2017-03-10T02:11:41Z

platform/darwin/src/MGLTypes.h

+
+ @return Returns a `MGLTransition` struct containing the transition attributes.
+ */
+NS_INLINE MGLTransition MGLTransitionMake(NSTimeInterval duration, NSTimeInterval delay) {


I'm not entirely sure, but you may also need to MGL_EXPORT this function. Generally we have to export public, top-level symbols, but this one is inlined. @kkaefer would know for sure.

Looks good to me!

…alFromDuration

…c methods

…ptions

… methods to NSValue(MGLAdditions)

incanus · 2017-03-10T19:01:34Z

platform/darwin/src/NSValue+MGLAdditions.m

+    [self getValue:&transition];
+    return transition;
+}
+


Why have these utilities anymore? We don't use them.

I've been pointed at #8225 (comment). 👍🏻

incanus · 2017-03-10T21:17:21Z

🎉

fabian-guerra added iOS Mapbox Maps SDK for iOS runtime styling labels Feb 28, 2017

fabian-guerra added this to the ios-v3.5.0 milestone Feb 28, 2017

fabian-guerra self-assigned this Feb 28, 2017

fabian-guerra requested review from incanus, boundsj and 1ec5 February 28, 2017 17:56

incanus changed the title ~~Per-attribute transition properties on MGLStyleLaye~~ Per-attribute transition properties on MGLStyleLayer Feb 28, 2017

incanus suggested changes Feb 28, 2017

View reviewed changes

boundsj previously requested changes Feb 28, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-per-attribute-transition branch 2 times, most recently from bbdb9f4 to 27b6f8d Compare March 2, 2017 22:34

1ec5 suggested changes Mar 3, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-per-attribute-transition branch from 27b6f8d to f0adb64 Compare March 6, 2017 19:40

fabian-guerra changed the base branch from master to release-ios-v3.5.0-android-v5.0.0 March 6, 2017 21:16

incanus suggested changes Mar 7, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-per-attribute-transition branch from ba516bc to 95c452e Compare March 7, 2017 01:00

boundsj reviewed Mar 7, 2017

View reviewed changes

incanus force-pushed the fabian-per-attribute-transition branch from 95c452e to 7cedbff Compare March 7, 2017 01:10

fabian-guerra force-pushed the fabian-per-attribute-transition branch 2 times, most recently from a324ae8 to 2e51b1e Compare March 7, 2017 01:24

1ec5 suggested changes Mar 7, 2017

View reviewed changes

incanus reviewed Mar 7, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-per-attribute-transition branch from 2e51b1e to 1ffc2e6 Compare March 8, 2017 01:46

1ec5 reviewed Mar 8, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-per-attribute-transition branch from d85657b to 77dea62 Compare March 9, 2017 18:51

1ec5 reviewed Mar 9, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-per-attribute-transition branch from 77dea62 to a63be28 Compare March 10, 2017 01:12

1ec5 approved these changes Mar 10, 2017

View reviewed changes

fabian-guerra added 13 commits March 10, 2017 09:27

[ios, macos] new struct MGLTransition

b274294

[ios] MGLTransition struct moved to MGLTypes.h

3420382

[ios] MGLTransition implemented for style layer properties

8ad49c3

[ios, macos] added support for getting property transitions

c36ec2d

[ios, macos] updated to MGLDurationFromTimeInterval and MGLTimeInterv…

0b95cb9

…alFromDuration

[ios, macos] added transition properties test

c8945f9

[ios, macos] modified key strings imp

da415f1

[ios, macos] The impl for transition properties were changed to publi…

5a18098

…c methods

[ios, macos] Changelog update

a229846

[ios] Fixed layer space formatting

967a19a

[core] The name space was fixed for getters that returned TransitionO…

e6e2099

…ptions

[ios, macos] Added rawLayer transition property tests

36c1920

[ios] Added a factory method for MGLTransition and transition-related…

09f6f9f

… methods to NSValue(MGLAdditions)

fabian-guerra force-pushed the fabian-per-attribute-transition branch from a63be28 to 09f6f9f Compare March 10, 2017 17:30

[ios, macos] Copyedited changelog blurbs about transition properties

f796ced

incanus suggested changes Mar 10, 2017

View reviewed changes

incanus approved these changes Mar 10, 2017

View reviewed changes

fabian-guerra merged commit 20712b7 into release-ios-v3.5.0-android-v5.0.0 Mar 10, 2017

boundsj mentioned this pull request Mar 10, 2017

Per-attribute transition properties on MGLStyleLayer #8010

Closed

incanus deleted the fabian-per-attribute-transition branch March 10, 2017 21:17

		@@ -30,6 +30,7 @@ NS_ASSUME_NONNULL_BEGIN
		*/
		static MGL_EXPORT const NSInteger MGLStyleDefaultVersion = 9;


		[setPropertyInvocation setArgument:&transition atIndex:2];

		[setPropertyInvocation performSelector:@selector(invoke)];

		@@ -84,13 +85,33 @@ - (void)removeFromMapView:(MGLMapView *)mapView

		#pragma mark - Accessing the Paint Attributes

		- (NSArray *)transitionKeys

		@@ -1753,6 +1814,7 @@ MGL_EXPORT
		*/
		@property (nonatomic, null_resettable) MGLStyleValue<NSValue > textTranslationAnchor;

Per-attribute transition properties on MGLStyleLayer #8225

Per-attribute transition properties on MGLStyleLayer #8225

Conversation

fabian-guerra commented Feb 28, 2017

mention-bot commented Feb 28, 2017

incanus left a 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 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

boundsj commented Feb 28, 2017 • 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

1ec5 commented Mar 4, 2017

incanus left a 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

incanus commented Mar 7, 2017

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

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

incanus left a comment • edited Loading

Choose a reason for hiding this comment

fabian-guerra commented Mar 8, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incanus commented Mar 10, 2017

boundsj left a comment •

edited

Loading

boundsj commented Feb 28, 2017 •

edited

Loading

incanus left a comment •

edited

Loading