Baby steps towards an odeon-like animation system. First victim: Drawer.

Baby steps towards an odeon-like animation system. First victim: Drawer.

This introduces an AnimationPerformance class, which is intended to manage an
animation (or its reverse), with the ability to manually control the timeline
or to apply a force to advance the animation with a diminishing speed.

I'm having trouble fitting the odeon model to Sky. Odeon has a lot of nice
properties, but fundamentally operates on UINodes, which contain all the
properties to be animated. Sky, on the other hand, has no such universal
properties. Instead, each Widget assembles itself how it sees fit.

So my current plan is to let AnimationPerformance own a generic set of
AnimatedVariables. You pass it a bag of things, say position and opacity, as
AnimatedVariables. It updates them based on the animation, and they each have
a way to build a widget based on their current state.

R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/d3b7661cf2cc8257f4d95cada175c5a9e2119499

[Hixie, 2015-06-25 21:00:58]

ianh@google.com changed reviewers:
+ ianh@google.com

[Hixie, 2015-06-25 21:00:59]

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
File sky/sdk/lib/widgets/material.dart (right):

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:15: // Ugly code to linearly interpolate 2
BoxShadows.
This logic should go in ../painting/ somewhere.

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:35: List<BoxShadow> getShadow(double t) {
computeShadow(), maybe? getShadow() sounds cheaper than this is.

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:40: return shadows[level2];
this line confuses me

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:56: this.level = new
AnimatedValue(level.toDouble());
Doing this every time Material() is constructed (multiple times per frame) seems
expensive, especially since you only actually need the AnimatedValue in the
instance that keeps surviving.

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:62: Widget child;
maintain the same order here as in the constructor and syncFields

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:78: if (level.value != source.level.value) {
(no {} around one-line blocks)

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:79:
level.animateTo(source.level.value.toDouble(), 200.0);
the 200.0 should use a constant from theme/ or something. On Android in
particular we will eventually want to get this from system animation speed
settings.

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:87: // TODO(ianh): we should make this animate
level changes and color changes
remove the TODO if you're doing it :-D

[Matt Perry, 2015-06-25 22:31:46]

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
File sky/sdk/lib/widgets/material.dart (right):

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:35: List<BoxShadow> getShadow(double t) {
On 2015/06/25 21:00:59, Hixie wrote:
> computeShadow(), maybe? getShadow() sounds cheaper than this is.

Done.

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:40: return shadows[level2];
On 2015/06/25 21:00:59, Hixie wrote:
> this line confuses me

clarified and added a comment.

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:56: this.level = new
AnimatedValue(level.toDouble());
On 2015/06/25 21:00:59, Hixie wrote:
> Doing this every time Material() is constructed (multiple times per frame)
seems
> expensive, especially since you only actually need the AnimatedValue in the
> instance that keeps surviving.

I'm doing this here because animate() requires that the Component is not yet
mounted. I guess I could change that requirement, but I don't know what
implications that might have.

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:62: Widget child;
On 2015/06/25 21:00:58, Hixie wrote:
> maintain the same order here as in the constructor and syncFields

Done.

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:78: if (level.value != source.level.value) {
On 2015/06/25 21:00:59, Hixie wrote:
> (no {} around one-line blocks)

Done.

https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:87: // TODO(ianh): we should make this animate
level changes and color changes
On 2015/06/25 21:00:59, Hixie wrote:
> remove the TODO if you're doing it :-D

Done.

[Hixie, 2015-06-25 22:42:16]

>
https://codereview.chromium.org/1211603003/diff/40001/sky/sdk/lib/widgets/mat...
> sky/sdk/lib/widgets/material.dart:56: this.level = new
AnimatedValue(level.toDouble());
> On 2015/06/25 21:00:59, Hixie wrote:
> > Doing this every time Material() is constructed (multiple times per frame)
seems
> > expensive, especially since you only actually need the AnimatedValue in the
> > instance that keeps surviving.
> 
> I'm doing this here because animate() requires that the Component is not yet
mounted. I guess I could change that requirement, but I don't know what
implications that might have.

Can you do it during mount, maybe?

[Hixie, 2015-06-25 22:42:20]

https://codereview.chromium.org/1211603003/diff/60001/sky/sdk/lib/widgets/mat...
File sky/sdk/lib/widgets/material.dart (right):

https://codereview.chromium.org/1211603003/diff/60001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:38: if (level < 1.0)  // shadows[1] is the
first shadow
Can't we animate from no shadow to a shadow?

[Matt Perry, 2015-06-30 22:09:11]

OK, I think this is ready for review. I added a way for AnimatedVariables to
build widgets for you.

I'm not quite happy with the fact that we have AnimationPerformance and
AnimatedValue and they don't talk to each other. I want to reconcile that soon.

https://codereview.chromium.org/1211603003/diff/60001/sky/sdk/lib/widgets/mat...
File sky/sdk/lib/widgets/material.dart (right):

https://codereview.chromium.org/1211603003/diff/60001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:38: if (level < 1.0)  // shadows[1] is the
first shadow
On 2015/06/25 22:42:19, Hixie wrote:
> Can't we animate from no shadow to a shadow?

We could, but that's extra code and we don't need it atm.

[abarth-chromium, 2015-06-30 22:26:07]

abarth@chromium.org changed reviewers:
+ abarth@chromium.org

[abarth-chromium, 2015-06-30 22:26:09]

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
File sky/sdk/lib/animation/animation_performance.dart (right):

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:11: void setPercentage(double
t);
Does t go from 0 to 100 or from 0 to 1?  If its the latter, "percentage"
probably isn't the best term.  Maybe "fraction"?

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:16: T begin, end;
final?

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:17: Curve curve;
final?

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:43: double durationMS;
Maybe this should be of type Duration?

https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/dart:core.Dur...

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:58: bool get isPresented =>
timeline.value == 1;
0.0 and 1.0

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:74: // time.
Have you looked at ParticleInBoxWithFriction from mechanics.dart?

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:80: if (distance > 0)
0.0

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:89: if (remainingDistance != 0)
0.0

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/base/lerp.dart
File sky/sdk/lib/base/lerp.dart (right):

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/base/lerp.d...
sky/sdk/lib/base/lerp.dart:17: lerpNum(a.dy, b.dy, t));
Should these functions be on Color and Offset, respectively?

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/painting/bo...
File sky/sdk/lib/painting/box_painter.dart (right):

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/painting/bo...
sky/sdk/lib/painting/box_painter.dart:75: static BoxShadow lerp(BoxShadow a,
BoxShadow b, double t) =>
This should be a constructor:

BoxShadow.lerp(BoxShadow a, BoxShadow b, double t) { ... }

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/widgets/ani...
File sky/sdk/lib/widgets/animated_component.dart (right):

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/widgets/ani...
sky/sdk/lib/widgets/animated_component.dart:75: }
Why not make an AnimatedTransform that inherits from Component that does this
work?

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/widgets/mat...
File sky/sdk/lib/widgets/material.dart (right):

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:20: List<BoxShadow> computeShadow(double
level) {
We should probably name this function with an _ to make it private to this
library.

Also, the word "compute" doesn't mean anything.  Maybe just _getShadow ?

[abarth-chromium, 2015-06-30 22:26:45]

I like the direction this is going.  The comments above are all pretty minor.

[Hixie, 2015-06-30 22:52:26]

On 2015/06/30 at 22:09:11, mpcomplete wrote:
> OK, I think this is ready for review. I added a way for AnimatedVariables to
build widgets for you.
> 
> I'm not quite happy with the fact that we have AnimationPerformance and
AnimatedValue and they don't talk to each other. I want to reconcile that soon.
> 
>
https://codereview.chromium.org/1211603003/diff/60001/sky/sdk/lib/widgets/mat...
> File sky/sdk/lib/widgets/material.dart (right):
> 
>
https://codereview.chromium.org/1211603003/diff/60001/sky/sdk/lib/widgets/mat...
> sky/sdk/lib/widgets/material.dart:38: if (level < 1.0)  // shadows[1] is the
first shadow
> On 2015/06/25 22:42:19, Hixie wrote:
> > Can't we animate from no shadow to a shadow?
> 
> We could, but that's extra code and we don't need it atm.

Seems reasonable. Make sure we have a TODO or a bug to track this.

[Hixie, 2015-06-30 23:11:57]

I like the general approach, I think. I don't yet fully understand how it'll
turn out.

https://codereview.chromium.org/1211603003/diff/100001/sky/engine/core/painti...
File sky/engine/core/painting/Offset.dart (right):

https://codereview.chromium.org/1211603003/diff/100001/sky/engine/core/painti...
sky/engine/core/painting/Offset.dart:24: Offset operator *(double scale) => new
Offset(dx * scale, dy * scale);
If you add *, also add /, ~/, and %. See Size.dart.

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
File sky/sdk/lib/animation/animation_performance.dart (right):

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:11: void setPercentage(double
t);
If the range is 0.0..1.0, it should probably be called Fraction rather than
Percentage.

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:34: // manipulating
|percentage|, or |fling| the timeline causing a physics-based
Is the idea that for each possible physical simulation, there'll be a method
here to do it?

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:43: double durationMS;
Or use a Duration?

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:57: bool get isDismissed =>
timeline.value == 0;
0.0

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:58: bool get isPresented =>
timeline.value == 1;
1.0

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:66: }
present and dismiss don't sound like opposites.
Maybe "runForward" and "runBackward"? or playForward, playBackward (matches
"stop", feels more "media"-like)

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:84: // TODO(mpcomplete): fix
animateTo to better handle resuming an animation,
"better" how?

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:86: void _animateTo(double
target) {
move this closer to present/dismiss, since they're the only callers.

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/painting/b...
File sky/sdk/lib/painting/box_painter.dart (right):

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/painting/b...
sky/sdk/lib/painting/box_painter.dart:75: static BoxShadow lerp(BoxShadow a,
BoxShadow b, double t) =>
Why BoxShadow.lerp() but lerpColor()?

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/widgets/an...
File sky/sdk/lib/widgets/animated_component.dart (right):

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/widgets/an...
sky/sdk/lib/widgets/animated_component.dart:64: // widget.
I don't really understand how this will look.

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/widgets/dr...
File sky/sdk/lib/widgets/drawer.dart (right):

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/widgets/dr...
sky/sdk/lib/widgets/drawer.dart:125: double scaler =
controller.performance.percentage + 1;
1.0

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/widgets/dr...
sky/sdk/lib/widgets/drawer.dart:133: var content = controller.position.build(
Widget, not var. (Right? I'm not quite sure I understand what build() returns
here.)

[Matt Perry, 2015-07-01 18:17:44]

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
File sky/sdk/lib/animation/animation_performance.dart (right):

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:11: void setPercentage(double
t);
On 2015/06/30 22:26:09, abarth-chromium wrote:
> Does t go from 0 to 100 or from 0 to 1?  If its the latter, "percentage"
> probably isn't the best term.  Maybe "fraction"?

Done.

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:16: T begin, end;
On 2015/06/30 22:26:08, abarth-chromium wrote:
> final?

Done.

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:17: Curve curve;
On 2015/06/30 22:26:08, abarth-chromium wrote:
> final?

Done.

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:43: double durationMS;
On 2015/06/30 22:26:08, abarth-chromium wrote:
> Maybe this should be of type Duration?
> 
>
https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/dart:core.Dur...

Done.

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:58: bool get isPresented =>
timeline.value == 1;
On 2015/06/30 22:26:09, abarth-chromium wrote:
> 0.0 and 1.0

Done.

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:74: // time.
On 2015/06/30 22:26:09, abarth-chromium wrote:
> Have you looked at ParticleInBoxWithFriction from mechanics.dart?

Yeah, that looks promising. I'd have to hook it up to timeline.

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:80: if (distance > 0)
On 2015/06/30 22:26:08, abarth-chromium wrote:
> 0.0

Done.

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/animation/a...
sky/sdk/lib/animation/animation_performance.dart:89: if (remainingDistance != 0)
On 2015/06/30 22:26:08, abarth-chromium wrote:
> 0.0

Done.

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/base/lerp.dart
File sky/sdk/lib/base/lerp.dart (right):

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/base/lerp.d...
sky/sdk/lib/base/lerp.dart:17: lerpNum(a.dy, b.dy, t));
On 2015/06/30 22:26:09, abarth-chromium wrote:
> Should these functions be on Color and Offset, respectively?

Yeah I dunno.. lerp doesn't feel important enough to warrant a class method. I
should probably make it a global for BoxShadow too.

(Effective C++ recommends anything that doesn't require access to private member
vars shouldn't be a class method.)

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/painting/bo...
File sky/sdk/lib/painting/box_painter.dart (right):

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/painting/bo...
sky/sdk/lib/painting/box_painter.dart:75: static BoxShadow lerp(BoxShadow a,
BoxShadow b, double t) =>
On 2015/06/30 22:26:09, abarth-chromium wrote:
> This should be a constructor:
> 
> BoxShadow.lerp(BoxShadow a, BoxShadow b, double t) { ... }
> 
> 

Interesting... Is the idea that any class method that returns a new instance of
the class should be a constructor? This doesn't feel "constructory" to me. It's
just a method that happens to return another object.

In any case, I'm going to make it a global function in this file, to follow the
pattern in lerp.dart.

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/widgets/ani...
File sky/sdk/lib/widgets/animated_component.dart (right):

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/widgets/ani...
sky/sdk/lib/widgets/animated_component.dart:75: }
On 2015/06/30 22:26:09, abarth-chromium wrote:
> Why not make an AnimatedTransform that inherits from Component that does this
> work?

My goal is to make it so the user doesn't have to worry about what hierarchy the
animation system creates. So I envision something like this:

performance.variable = new AnimatedCollection([
  new AnimatedPosition(startPos, endPos)..duringTimeline(0.0, 0.75),
  new AnimatedFade(0.0, 1.0),
  new AnimatedScale(0.8, 1.0)]);

which would mean "move from startPos to endPos during the first 75% of the
timeline, fade-in from 0 to 100%, and scale from 80% to 100%". Then later, you
just call:

performance.variable.build(...)

and it constructs whatever hierarchy it needs.

Does that make sense?

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/widgets/mat...
File sky/sdk/lib/widgets/material.dart (right):

https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/widgets/mat...
sky/sdk/lib/widgets/material.dart:20: List<BoxShadow> computeShadow(double
level) {
On 2015/06/30 22:26:09, abarth-chromium wrote:
> We should probably name this function with an _ to make it private to this
> library.

Done.

> Also, the word "compute" doesn't mean anything.  Maybe just _getShadow ?

I had "get" and Ian recommended "compute" because it's not cheap :) I don't have
a strong opinion.

https://codereview.chromium.org/1211603003/diff/100001/sky/engine/core/painti...
File sky/engine/core/painting/Offset.dart (right):

https://codereview.chromium.org/1211603003/diff/100001/sky/engine/core/painti...
sky/engine/core/painting/Offset.dart:24: Offset operator *(double scale) => new
Offset(dx * scale, dy * scale);
On 2015/06/30 23:11:56, Hixie wrote:
> If you add *, also add /, ~/, and %. See Size.dart.

Done.

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
File sky/sdk/lib/animation/animation_performance.dart (right):

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:34: // manipulating
|percentage|, or |fling| the timeline causing a physics-based
On 2015/06/30 23:11:56, Hixie wrote:
> Is the idea that for each possible physical simulation, there'll be a method
> here to do it?

Dunno. odeon has "disengageWithForce", which is what I was planning to mimic
with "fling". Not sure what else we need here.

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:66: }
On 2015/06/30 23:11:56, Hixie wrote:
> present and dismiss don't sound like opposites.
> Maybe "runForward" and "runBackward"? or playForward, playBackward (matches
> "stop", feels more "media"-like)

Went with "play" and "reverse".

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:84: // TODO(mpcomplete): fix
animateTo to better handle resuming an animation,
On 2015/06/30 23:11:56, Hixie wrote:
> "better" how?

Hmm.. I forgot what I meant by this. I'll take it out.

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/animation/...
sky/sdk/lib/animation/animation_performance.dart:86: void _animateTo(double
target) {
On 2015/06/30 23:11:56, Hixie wrote:
> move this closer to present/dismiss, since they're the only callers.

Done.

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/painting/b...
File sky/sdk/lib/painting/box_painter.dart (right):

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/painting/b...
sky/sdk/lib/painting/box_painter.dart:75: static BoxShadow lerp(BoxShadow a,
BoxShadow b, double t) =>
On 2015/06/30 23:11:56, Hixie wrote:
> Why BoxShadow.lerp() but lerpColor()?

Good question. Made it a global.

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/widgets/an...
File sky/sdk/lib/widgets/animated_component.dart (right):

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/widgets/an...
sky/sdk/lib/widgets/animated_component.dart:64: // widget.
On 2015/06/30 23:11:56, Hixie wrote:
> I don't really understand how this will look.

See my reply to abarth's comment in this file. Let me know if that clears it up.

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/widgets/dr...
File sky/sdk/lib/widgets/drawer.dart (right):

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/widgets/dr...
sky/sdk/lib/widgets/drawer.dart:125: double scaler =
controller.performance.percentage + 1;
On 2015/06/30 23:11:56, Hixie wrote:
> 1.0

Done.

https://codereview.chromium.org/1211603003/diff/100001/sky/sdk/lib/widgets/dr...
sky/sdk/lib/widgets/drawer.dart:133: var content = controller.position.build(
On 2015/06/30 23:11:56, Hixie wrote:
> Widget, not var. (Right? I'm not quite sure I understand what build() returns
> here.)

Done.

[abarth-chromium, 2015-07-01 19:40:14]

On 2015/07/01 at 18:17:44, mpcomplete wrote:
>
https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/base/lerp.d...
> sky/sdk/lib/base/lerp.dart:17: lerpNum(a.dy, b.dy, t));
> On 2015/06/30 22:26:09, abarth-chromium wrote:
> > Should these functions be on Color and Offset, respectively?
> 
> Yeah I dunno.. lerp doesn't feel important enough to warrant a class method. I
should probably make it a global for BoxShadow too.
> 
> (Effective C++ recommends anything that doesn't require access to private
member vars shouldn't be a class method.)

Dart is different than C++ because Dart doesn't support overloading.  In C++,
you could call each of these functions |lerp| and have the compiler pick the
right one based on the type of the arguments.  In dart, you need to write
lerpColor, which means you need to mention the class name anyway.  Writing this
as Color.lerp would seem better structured, but then you have a static class
function that returns a new instance, which is really what a constructor is.

I'm not sure it's a big deal either way, but using a named constructor seems
like the most idiomatic thing for Dart.

> Interesting... Is the idea that any class method that returns a new instance
of the class should be a constructor?

It does't have to be, but if you're a class method that returns a new instance,
that's just the same thing as a named constructor.

> This doesn't feel "constructory" to me. It's just a method that happens to
return another object.

We're getting down to aesthetics, but what this function does is construct a new
Color object from two other Color objects and a fraction.

> In any case, I'm going to make it a global function in this file, to follow
the pattern in lerp.dart.

That's fine.  We can always change our minds later if we decide the other way is
pretty.

>
https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/widgets/ani...
> File sky/sdk/lib/widgets/animated_component.dart (right):
> 
>
https://codereview.chromium.org/1211603003/diff/80001/sky/sdk/lib/widgets/ani...
> sky/sdk/lib/widgets/animated_component.dart:75: }
> On 2015/06/30 22:26:09, abarth-chromium wrote:
> > Why not make an AnimatedTransform that inherits from Component that does
this
> > work?
> 
> My goal is to make it so the user doesn't have to worry about what hierarchy
the animation system creates. So I envision something like this:
> 
> performance.variable = new AnimatedCollection([
>   new AnimatedPosition(startPos, endPos)..duringTimeline(0.0, 0.75),
>   new AnimatedFade(0.0, 1.0),
>   new AnimatedScale(0.8, 1.0)]);
> 
> which would mean "move from startPos to endPos during the first 75% of the
timeline, fade-in from 0 to 100%, and scale from 80% to 100%". Then later, you
just call:
> 
> performance.variable.build(...)
> 
> and it constructs whatever hierarchy it needs.
> 
> Does that make sense?

Yeah, that's neat.

> > Also, the word "compute" doesn't mean anything.  Maybe just _getShadow ?
> 
> I had "get" and Ian recommended "compute" because it's not cheap :) I don't
have a strong opinion.

Ok.

[abarth-chromium, 2015-07-01 19:44:09]

lgtm

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/base/lerp....
File sky/sdk/lib/base/lerp.dart (right):

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/base/lerp....
sky/sdk/lib/base/lerp.dart:13: lerpNum(a.blue, b.blue, t).toInt());
We've been using { } rather than => when the body takes up more than one line of
text.

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/an...
File sky/sdk/lib/widgets/animated_component.dart (right):

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/an...
sky/sdk/lib/widgets/animated_component.dart:70: Widget build({Widget child}) {
Maybe remove the { }.  It seems unlikely this function will take any more
parameters in the future.

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/dr...
File sky/sdk/lib/widgets/drawer.dart (right):

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/dr...
sky/sdk/lib/widgets/drawer.dart:49: new Point(-_kWidth, 0.0), new Point(0.0,
0.0), curve: _kAnimationCurve);
s/new Point(0.0, 0.0)/Point.origin/

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/ma...
File sky/sdk/lib/widgets/material.dart (right):

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/ma...
sky/sdk/lib/widgets/material.dart:19: List<BoxShadow> computeShadow(double
level) {
s/computeShadow/_computeShadow/

[Matt Perry, 2015-07-01 20:05:40]

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/base/lerp....
File sky/sdk/lib/base/lerp.dart (right):

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/base/lerp....
sky/sdk/lib/base/lerp.dart:13: lerpNum(a.blue, b.blue, t).toInt());
On 2015/07/01 19:44:08, abarth-chromium wrote:
> We've been using { } rather than => when the body takes up more than one line
of
> text.

Done.

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/an...
File sky/sdk/lib/widgets/animated_component.dart (right):

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/an...
sky/sdk/lib/widgets/animated_component.dart:70: Widget build({Widget child}) {
On 2015/07/01 19:44:08, abarth-chromium wrote:
> Maybe remove the { }.  It seems unlikely this function will take any more
> parameters in the future.

Done.

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/dr...
File sky/sdk/lib/widgets/drawer.dart (right):

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/dr...
sky/sdk/lib/widgets/drawer.dart:49: new Point(-_kWidth, 0.0), new Point(0.0,
0.0), curve: _kAnimationCurve);
On 2015/07/01 19:44:09, abarth-chromium wrote:
> s/new Point(0.0, 0.0)/Point.origin/

Done.

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/ma...
File sky/sdk/lib/widgets/material.dart (right):

https://codereview.chromium.org/1211603003/diff/120001/sky/sdk/lib/widgets/ma...
sky/sdk/lib/widgets/material.dart:19: List<BoxShadow> computeShadow(double
level) {
On 2015/07/01 19:44:09, abarth-chromium wrote:
> s/computeShadow/_computeShadow/

Done.

[Matt Perry, 2015-07-01 20:38:24]

Committed patchset #8 (id:140001) manually as
d3b7661cf2cc8257f4d95cada175c5a9e2119499 (presubmit successful).