Implement v2 APIs #22

jverkoey · 2017-12-01T20:55:03Z

In short: this PR drops the redundant "motion" prefix on various types and aligns naming with system terminology where possible and re-implements the APIs as Objective-C APIs, closing #5.

Old API	New API	Rationale
MotionTiming	AnimationTraits	This structure is intended to describe animations only, not motion in general.
MotionCurve	TimingCurve	This brings the API name closer to the similarly-purposed `CAMediaTimingFunction`. MotionCurve could also be easily confused with motion through x/y space rather than through time (e.g. ArcMove), which will be problematic as we start defining paths of motion through space.
MotionRepetition	RepetitionTraits	This aligns the naming with AnimationTraits.

All prior APIs have been deleted, including the macro-based APIs and any active deprecations. This is a major change and will bump the release to v2.

A migration script has been included to ease migration of existing code.

romoore · 2017-12-03T02:17:03Z

src/MDMAnimationTraits.m

+
+const MDMAnimationTraits MDMAnimationTraitsSystemModalMovement = {
+  .duration = 0.500, .timingCurve = {
+    .type = MDMTimingCurveTypeSpring, .data = { 3, 1000, 500, 0 }


Nit/Follow-up: Naming the data?

I'm not sure what or how we would name this - did you have something in mind?

I don't. They just look like a series of NSInteger(uint32_t, int16_t, double?) values. I know they make a timing curve, but looking at the code I have no idea what they're doing.

Ah, I had assumed you meant renaming the data parameter. I'll give these values const names in the scope here.

romoore · 2017-12-03T02:17:52Z

src/MDMTimingCurve.h

+  /**
+   The value will be instantly set with no animation.
+   */
+  MDMTimingCurveTypeInstant,


Please assign an explicit numeric value to each of these.

romoore · 2017-12-03T02:19:10Z

src/MDMTimingCurve.h

+  /**
+   The data values corresponding with this curve.
+   */
+  CGFloat data[4];


Why do all timing curves have 4*sizeof(CGFloat) worth of data? Should this not be a pointer to another type?

Given that these are C-structs, a pointer to another type would be require an extra hurdle to jump over when initializing these values. In practice the timing curves supported by Core Animation only need four pieces of data so this array has been fairly sufficient.

romoore · 2017-12-03T02:20:24Z

src/MDMTimingCurve.h

+ */
+// clang-format off
+FOUNDATION_EXTERN
+MDMTimingCurve MDMTimingCurveMakeBezier(CGFloat p1x, CGFloat p1y, CGFloat p2x, CGFloat p2y)


Why not a pair of CGPoint? Why 4 explicit CGFloat? It seems that if they represent 2 points, it's best to use CGPoint to provide additional context. Also consider naming "p2x, p2y, p3x, p3y" to match the method documentation.

This is in keeping consistent with the CAMediaTimingFunction API.

romoore · 2017-12-03T02:22:03Z

src/MDMTimingCurve.m

-MDMMotionCurve MDMMotionCurveMakeBezier(CGFloat p1x, CGFloat p1y, CGFloat p2x, CGFloat p2y) {
-  return _MDMBezier(p1x, p1y, p2x, p2y);
+const MDMTimingCurve MDMTimingCurveLinear = {
+  .type = MDMTimingCurveTypeBezier, .data = { 0, 0, 1, 1 }


Same request for named structures if possible later.

romoore · 2017-12-03T02:23:49Z

src/MDMTimingCurve.h

+ Named indices for the bezier traits curve's data array.
+ */
+typedef NS_ENUM(NSUInteger, MDMTimingCurveBezierDataIndex) {
+  MDMTimingCurveBezierDataIndexP1X,


Please provide explicit numeric values for these and below. This seems especially valuable since they will function as indices into zero-based arrays.

Updated.

jverkoey · 2017-12-04T14:35:14Z

Based on discussion with @romoore offline and with #5 in mind, I'm going to use this API v2 opportunity to implement the new APIs as ObjC types so that we don't have to do so in a v3.

jverkoey · 2017-12-04T16:12:20Z

API is now Objective-C'ized.

Needs re-review.

romoore · 2017-12-04T18:27:44Z

scripts/v1_to_v2.sh

+}
+
+replace_all() {
+  replace_objc $1


Please place quotes around the expanding variable so that paths with spaces (or special characters) won't break. "$1"

romoore · 2017-12-04T18:32:53Z

scripts/v1_to_v2.sh

+  replace_swift $1
+}
+
+# replace_all "s/MotionCurveMakeSpring(mass/TimingCurve(springWithMass/g"


I would suggest either grouping all the commented-out replacements at the bottom (with a comment) or remove them entirely.

romoore · 2017-12-04T18:34:39Z

src/MDMAnimationTraits.h

+@interface MDMAnimationTraits: NSObject
+
+/**
+ Initializes the instance with the provided duration and default iOS ease in/out cubic bezier.


Nit: Consider referencing the actual constant function.

romoore · 2017-12-04T18:41:27Z

src/MDMAnimationTraits.h

+ Initializes the instance with the provided duration, delay, and default iOS ease in/out cubic
+ bezier.
+
+ @param delay The amount of time, in seconds, to wait before starting the animation.


Is any non-positive delay treated as "instantaneous" or would the animation be reversed?

Negative delay would shift the animation backward in time. E.g if animating linearly from 0 to 1 over 10 seconds, a delay of -5 would start the animation at 0.5 and animate for 5 seconds before completing.

And a negative duration would animate backwards until the duration is met?

It appears that Core Animation will default to a duration of 0.25 when the duration is <= 0. Wonderful! material-motion/motion-animator-objc#87 filed to track.

romoore · 2017-12-04T18:53:28Z

tests/unit/MDMRepetitionOverTimeTests.swift

+    XCTAssertFalse(repetition.autoreverses)
+  }
+
+  func testInitializationWithNumberOfRepetitionsAndAutoreversed() {


The name says "number of repetitions" but the code says duration. Wrong name?

romoore · 2017-12-04T18:54:34Z

src/MDMAnimationTraits.h

+ @param duration The animation will occur over this length of time, in seconds, after the delay time
+ has passed.
+ */
+- (nonnull instancetype)initWithDelay:(NSTimeInterval)delay duration:(NSTimeInterval)duration;


Nit: This should probably be "initWithDuration:delay:" to match the convenience constructor above. Same for the two below.

Have flip-flopped on this. Conceptually the delay happens before the duration, so I was attempting to make it easier to scan and understand blocks of animation traits. I was finding it harder to read the blocks when delay was the second argument. For example:

- (MDMAnimationTraits *)chipWidth { return [[MDMAnimationTraits alloc] initWithDelay:0.000 duration:0.285 timingCurve:StandardTimingCurve()]; } - (MDMAnimationTraits *)chipHeight { return [[MDMAnimationTraits alloc] initWithDelay:0.015 duration:0.360 timingCurve:StandardTimingCurve()]; } - (MDMAnimationTraits *)chipY { return [[MDMAnimationTraits alloc] initWithDelay:0.015 duration:0.360 timingCurve:StandardTimingCurve()]; } - (MDMAnimationTraits *)chipContentOpacity { return [[MDMAnimationTraits alloc] initWithDelay:0.000 duration:0.075 timingCurve:LinearTimingCurve()]; } - (MDMAnimationTraits *)headerContentOpacity { return [[MDMAnimationTraits alloc] initWithDelay:0.075 duration:0.150 timingCurve:LinearTimingCurve()]; } - (MDMAnimationTraits *)navigationBarY { return [[MDMAnimationTraits alloc] initWithDelay:0.015 duration:0.360 timingCurve:StandardTimingCurve()]; }

That makes sense. Not a blocking issue, just jumped out at me when I was reading.

jverkoey requested review from samnm and romoore December 1, 2017 20:55

jverkoey mentioned this pull request Dec 1, 2017

Implement the v2 interchange. material-motion/motion-animator-objc#86

Closed

romoore previously requested changes Dec 3, 2017

View reviewed changes

romoore previously approved these changes Dec 4, 2017

View reviewed changes

jverkoey changed the title ~~Implement v2 APIs~~ [WIP] Implement v2 APIs Dec 4, 2017

jverkoey changed the title ~~[WIP] Implement v2 APIs~~ Implement v2 APIs Dec 4, 2017

Jeff Verkoeyen added 10 commits December 4, 2017 13:14

Add v2 apis.

0f6bf48

Add repetition renaming.

a334d4d

Fix more tests.

1865087

Delete unused header.

6630061

More to script.

55001b4

Review feedback.

151d00c

API name.

b0c260b

Docs.

8c32394

Implement the APIs in Objective-C. Increase testing coverage.

4aceb00

Reordering.

a400a25

jverkoey force-pushed the v2 branch from 86cdf2b to a400a25 Compare December 4, 2017 18:14

Fix tests.

771d712

romoore suggested changes Dec 4, 2017

View reviewed changes

Jeff Verkoeyen added 2 commits December 4, 2017 14:30

Quotes.

c4a4cc2

Review feedback.

6898bac

romoore approved these changes Dec 4, 2017

View reviewed changes

jverkoey merged commit 3a28e22 into develop Dec 4, 2017

jverkoey deleted the v2 branch December 4, 2017 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement v2 APIs #22

Implement v2 APIs #22

jverkoey commented Dec 1, 2017 •

edited

Loading

romoore Dec 3, 2017

jverkoey Dec 3, 2017

romoore Dec 4, 2017

jverkoey Dec 4, 2017 •

edited

Loading

romoore Dec 3, 2017

jverkoey Dec 3, 2017

romoore Dec 3, 2017

jverkoey Dec 3, 2017

romoore Dec 4, 2017

romoore Dec 3, 2017

jverkoey Dec 3, 2017 •

edited

Loading

romoore Dec 4, 2017

romoore Dec 3, 2017

romoore Dec 3, 2017

jverkoey Dec 3, 2017

jverkoey commented Dec 4, 2017

jverkoey commented Dec 4, 2017

romoore Dec 4, 2017

jverkoey Dec 4, 2017

romoore Dec 4, 2017

jverkoey Dec 4, 2017

romoore Dec 4, 2017

jverkoey Dec 4, 2017

romoore Dec 4, 2017

jverkoey Dec 4, 2017 •

edited

Loading

romoore Dec 4, 2017

jverkoey Dec 4, 2017

romoore Dec 4, 2017

jverkoey Dec 4, 2017

romoore Dec 4, 2017

jverkoey Dec 4, 2017 •

edited

Loading

romoore Dec 4, 2017

Implement v2 APIs #22

Implement v2 APIs #22

Conversation

jverkoey commented Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jverkoey Dec 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jverkoey Dec 3, 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

jverkoey commented Dec 4, 2017

jverkoey commented Dec 4, 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

Choose a reason for hiding this comment

jverkoey Dec 4, 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

Choose a reason for hiding this comment

jverkoey Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jverkoey commented Dec 1, 2017 •

edited

Loading

jverkoey Dec 4, 2017 •

edited

Loading

jverkoey Dec 3, 2017 •

edited

Loading

jverkoey Dec 4, 2017 •

edited

Loading

jverkoey Dec 4, 2017 •

edited

Loading