Implicit animations through Layer.

Adds the ability to set an Animation on Layer so that future changes to the layer result in animating the property. See code in desktop_window_view.cc and window.cc for usage.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103282

[Ben Goodger (Google), 2011-09-27 22:38:04]

lgtm

http://codereview.chromium.org/7972023/diff/6001/ui/aura/window.h
File ui/aura/window.h (right):

http://codereview.chromium.org/7972023/diff/6001/ui/aura/window.h#newcode81
ui/aura/window.h:81: void SetBounds(const gfx::Rect& new_bounds, int anim_ms);
Make a SetBoundsAnimated that takes anim_ms and have SetBounds just take new
bounds.

[commit-bot: I haz the power, 2011-09-27 22:38:17]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/7972023/6001

[commit-bot: I haz the power, 2011-09-27 22:38:20]

Can't apply patch for file ui/gfx/compositor/layer_animator.cc.
While running patch -p1 --forward --force;
patching file ui/gfx/compositor/layer_animator.cc
Hunk #4 FAILED at 72.
1 out of 4 hunks FAILED -- saving rejects to file
ui/gfx/compositor/layer_animator.cc.rej

[Ben Goodger (Google), 2011-09-27 22:38:30]

DOH

I clicked the wrong pixel.

[Ben Goodger (Google), 2011-09-27 22:41:28]

To be clear, the lgtm was unintentional.

-Ben

On Tue, Sep 27, 2011 at 3:38 PM, <ben@chromium.org> wrote:

> DOH
>
> I clicked the wrong pixel.
>
>
http://codereview.chromium.**org/7972023/<http://codereview.chromium.org/7972...
>

[sky, 2011-09-27 23:30:24]

Updated.

[Ian Vollick, 2011-09-28 15:24:31]

Hi Scott,

Please correct me if I'm wrong, but if I understand correctly, if you set an
animation on a layer, until that animation completes, all calls to SetTransform,
SetBounds and SetOpacity will result in animations, and all of the animations
will finish at the same time. Also, if the original animation is 75% complete,
the new transitions will jump to 75% complete when they start animating (since
they all use the same animation->GetCurrentValue()). It doesn't seem possible to
set a layer property immediately while the animation is running or start an
overlapping animation of another property. It also seems quite possible that
animations may be accidentally started with this design.

The problems that could be caused by setting properties during an animation
could be mitigated by caching pending property changes and processing them
afterwards. Or instead of enabling animation on layers by setting an animation,
maybe layers could use the approach you took with views and have a property
setter?

Ian

http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
File ui/aura_shell/examples/window_type_launcher.cc (right):

http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
ui/aura_shell/examples/window_type_launcher.cc:57:
static_cast<float>(total_width);
Will the padding get scaled by x_scale? If not, I think you want
containing_width = x_scale * total_window_width + padding ==> x_scale =
(containing_width - padding) / total_window_width.

http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
ui/aura_shell/examples/window_type_launcher.cc:66: (containing_size.width() * -
total_width * *x_scale) / 2));
Should this be containing_size.width() - total_width? Also if the padding's not
scaled, then this math will also need to be updated.

http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
ui/aura_shell/examples/window_type_launcher.cc:77:
static_cast<float>(current_bounds.width() + kPadding) * *x_scale);
If padding's not scaled, this is another thing to update.

[sky, 2011-09-28 16:01:59]

On Wed, Sep 28, 2011 at 8:24 AM,  <vollick@chromium.org> wrote:
> Hi Scott,
>
> Please correct me if I'm wrong, but if I understand correctly, if you set an
> animation on a layer, until that animation completes, all calls to
> SetTransform,
> SetBounds and SetOpacity will result in animations, and all of the
> animations
> will finish at the same time. Also, if the original animation is 75%
> complete,
> the new transitions will jump to 75% complete when they start animating
> (since
> they all use the same animation->GetCurrentValue()). It doesn't seem
> possible to
> set a layer property immediately while the animation is running or start an
> overlapping animation of another property. It also seems quite possible that
> animations may be accidentally started with this design.

That is correct. As I said, this patch gives 'trivial' animation
support. What should have once an animation is in progress and a
property changes?

. Stop animations and immediately go to new value.
. Queue up change and animate there are current animation completes.
. Reroute animation to new value.

I can think of cases where you want each of these. For example, if
there is some window move animation going on and the user happens to
grab the window and move it, then you want all animations to stop.
Screen rotation code wants the queue approach. And tab animations
generally get rerouted to the new value.

We'll likely need a way to specify the behavior that should happen.

> The problems that could be caused by setting properties during an animation
> could be mitigated by caching pending property changes and processing them
> afterwards. Or instead of enabling animation on layers by setting an
> animation,
> maybe layers could use the approach you took with views and have a property
> setter?

  -Scott

>
> Ian
>
>
>
http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
> File ui/aura_shell/examples/window_type_launcher.cc (right):
>
>
http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
> ui/aura_shell/examples/window_type_launcher.cc:57:
> static_cast<float>(total_width);
> Will the padding get scaled by x_scale? If not, I think you want
> containing_width = x_scale * total_window_width + padding ==> x_scale =
> (containing_width - padding) / total_window_width.
>
>
http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
> ui/aura_shell/examples/window_type_launcher.cc:66:
> (containing_size.width() * - total_width * *x_scale) / 2));
> Should this be containing_size.width() - total_width? Also if the
> padding's not scaled, then this math will also need to be updated.
>
>
http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
> ui/aura_shell/examples/window_type_launcher.cc:77:
> static_cast<float>(current_bounds.width() + kPadding) * *x_scale);
> If padding's not scaled, this is another thing to update.
>
> http://codereview.chromium.org/7972023/
>

[Ian Vollick, 2011-09-28 17:18:08]

On 2011/09/28 16:01:59, sky wrote:
> On Wed, Sep 28, 2011 at 8:24 AM,  <mailto:vollick@chromium.org> wrote:
> > Hi Scott,
> >
> > Please correct me if I'm wrong, but if I understand correctly, if you set an
> > animation on a layer, until that animation completes, all calls to
> > SetTransform,
> > SetBounds and SetOpacity will result in animations, and all of the
> > animations
> > will finish at the same time. Also, if the original animation is 75%
> > complete,
> > the new transitions will jump to 75% complete when they start animating
> > (since
> > they all use the same animation->GetCurrentValue()). It doesn't seem
> > possible to
> > set a layer property immediately while the animation is running or start an
> > overlapping animation of another property. It also seems quite possible that
> > animations may be accidentally started with this design.
> 
> That is correct. As I said, this patch gives 'trivial' animation
> support. What should have once an animation is in progress and a
> property changes?
> 
> . Stop animations and immediately go to new value.
> . Queue up change and animate there are current animation completes.
> . Reroute animation to new value.
> 
> I can think of cases where you want each of these. For example, if
> there is some window move animation going on and the user happens to
> grab the window and move it, then you want all animations to stop.
> Screen rotation code wants the queue approach. And tab animations
> generally get rerouted to the new value.
> 
> We'll likely need a way to specify the behavior that should happen.

It is definitely ambiguous. I'm in favor of a trivial approach, but I'm a bit
concerned that the new layer API could be misused and result in glitchy or
inconsistent transitions. Used properly, the design should work fine, so I don't
have strong feelings, but both of the first two options you suggested seem
simple to code and use and might be little safer.

Ian

> 
> > The problems that could be caused by setting properties during an animation
> > could be mitigated by caching pending property changes and processing them
> > afterwards. Or instead of enabling animation on layers by setting an
> > animation,
> > maybe layers could use the approach you took with views and have a property
> > setter?
> 
>   -Scott
> 
> >
> > Ian
> >
> >
> >
>
http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
> > File ui/aura_shell/examples/window_type_launcher.cc (right):
> >
> >
>
http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
> > ui/aura_shell/examples/window_type_launcher.cc:57:
> > static_cast<float>(total_width);
> > Will the padding get scaled by x_scale? If not, I think you want
> > containing_width = x_scale * total_window_width + padding ==> x_scale =
> > (containing_width - padding) / total_window_width.
> >
> >
>
http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
> > ui/aura_shell/examples/window_type_launcher.cc:66:
> > (containing_size.width() * - total_width * *x_scale) / 2));
> > Should this be containing_size.width() - total_width? Also if the
> > padding's not scaled, then this math will also need to be updated.
> >
> >
>
http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/wind...
> > ui/aura_shell/examples/window_type_launcher.cc:77:
> > static_cast<float>(current_bounds.width() + kPadding) * *x_scale);
> > If padding's not scaled, this is another thing to update.
> >
> > http://codereview.chromium.org/7972023/
> >

[Ben Goodger (Google), 2011-09-28 17:41:10]

See my comment in the other email I sent to you directly.

I was thinking the pattern we'd end up with is:

layer->ConfigureAnimation(state_1);
layer->SetBounds(step_1_bounds);
layer->ConfigureAnimation(state_2);
layer->SetBounds(step_2_bounds);
layer->SetOpacity(step_2_opacity);

Each call to ConfigureAnimation enqueues a new animation state, which could
include a property setter.

The animation for the state at the head of the queue is started
automatically.

-Ben

On Wed, Sep 28, 2011 at 10:18 AM, <vollick@chromium.org> wrote:

> On 2011/09/28 16:01:59, sky wrote:
>
>  On Wed, Sep 28, 2011 at 8:24 AM,  <mailto:vollick@chromium.org> wrote:
>> > Hi Scott,
>> >
>> > Please correct me if I'm wrong, but if I understand correctly, if you
>> set an
>> > animation on a layer, until that animation completes, all calls to
>> > SetTransform,
>> > SetBounds and SetOpacity will result in animations, and all of the
>> > animations
>> > will finish at the same time. Also, if the original animation is 75%
>> > complete,
>> > the new transitions will jump to 75% complete when they start animating
>> > (since
>> > they all use the same animation->GetCurrentValue()). It doesn't seem
>> > possible to
>> > set a layer property immediately while the animation is running or start
>> an
>> > overlapping animation of another property. It also seems quite possible
>> that
>> > animations may be accidentally started with this design.
>>
>
>  That is correct. As I said, this patch gives 'trivial' animation
>> support. What should have once an animation is in progress and a
>> property changes?
>>
>
>  . Stop animations and immediately go to new value.
>> . Queue up change and animate there are current animation completes.
>> . Reroute animation to new value.
>>
>
>  I can think of cases where you want each of these. For example, if
>> there is some window move animation going on and the user happens to
>> grab the window and move it, then you want all animations to stop.
>> Screen rotation code wants the queue approach. And tab animations
>> generally get rerouted to the new value.
>>
>
>  We'll likely need a way to specify the behavior that should happen.
>>
>
> It is definitely ambiguous. I'm in favor of a trivial approach, but I'm a
> bit
> concerned that the new layer API could be misused and result in glitchy or
> inconsistent transitions. Used properly, the design should work fine, so I
> don't
> have strong feelings, but both of the first two options you suggested seem
> simple to code and use and might be little safer.
>
> Ian
>
>
>
>  > The problems that could be caused by setting properties during an
>> animation
>> > could be mitigated by caching pending property changes and processing
>> them
>> > afterwards. Or instead of enabling animation on layers by setting an
>> > animation,
>> > maybe layers could use the approach you took with views and have a
>> property
>> > setter?
>>
>
>    -Scott
>>
>
>  >
>> > Ian
>> >
>> >
>> >
>>
>
> http://codereview.chromium.**org/7972023/diff/12001/ui/**
>
aura_shell/examples/window_**type_launcher.cc<http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/window_type_launcher.cc>
>
>> > File ui/aura_shell/examples/window_**type_launcher.cc (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/7972023/diff/12001/ui/**
>
aura_shell/examples/window_**type_launcher.cc#newcode57<http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/window_type_launcher.cc#newcode57>
>
>> > ui/aura_shell/examples/window_**type_launcher.cc:57:
>> > static_cast<float>(total_**width);
>> > Will the padding get scaled by x_scale? If not, I think you want
>> > containing_width = x_scale * total_window_width + padding ==> x_scale =
>> > (containing_width - padding) / total_window_width.
>> >
>> >
>>
>
> http://codereview.chromium.**org/7972023/diff/12001/ui/**
>
aura_shell/examples/window_**type_launcher.cc#newcode66<http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/window_type_launcher.cc#newcode66>
>
>> > ui/aura_shell/examples/window_**type_launcher.cc:66:
>> > (containing_size.width() * - total_width * *x_scale) / 2));
>> > Should this be containing_size.width() - total_width? Also if the
>> > padding's not scaled, then this math will also need to be updated.
>> >
>> >
>>
>
> http://codereview.chromium.**org/7972023/diff/12001/ui/**
>
aura_shell/examples/window_**type_launcher.cc#newcode77<http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/window_type_launcher.cc#newcode77>
>
>> > ui/aura_shell/examples/window_**type_launcher.cc:77:
>> > static_cast<float>(current_**bounds.width() + kPadding) * *x_scale);
>> > If padding's not scaled, this is another thing to update.
>> >
>> >
http://codereview.chromium.**org/7972023/<http://codereview.chromium.org/7972...
>> >
>>
>
>
>
>
http://codereview.chromium.**org/7972023/<http://codereview.chromium.org/7972...
>

[vollick1, 2011-09-28 17:59:17]

Cool, SGTM.

On Wed, Sep 28, 2011 at 1:41 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> See my comment in the other email I sent to you directly.
>
> I was thinking the pattern we'd end up with is:
>
> layer->ConfigureAnimation(state_1);
> layer->SetBounds(step_1_bounds);
> layer->ConfigureAnimation(state_2);
> layer->SetBounds(step_2_bounds);
> layer->SetOpacity(step_2_opacity);
>
> Each call to ConfigureAnimation enqueues a new animation state, which could
> include a property setter.
>
> The animation for the state at the head of the queue is started
> automatically.
>
> -Ben
>
>
> On Wed, Sep 28, 2011 at 10:18 AM, <vollick@chromium.org> wrote:
>
>> On 2011/09/28 16:01:59, sky wrote:
>>
>>  On Wed, Sep 28, 2011 at 8:24 AM,  <mailto:vollick@chromium.org> wrote:
>>> > Hi Scott,
>>> >
>>> > Please correct me if I'm wrong, but if I understand correctly, if you
>>> set an
>>> > animation on a layer, until that animation completes, all calls to
>>> > SetTransform,
>>> > SetBounds and SetOpacity will result in animations, and all of the
>>> > animations
>>> > will finish at the same time. Also, if the original animation is 75%
>>> > complete,
>>> > the new transitions will jump to 75% complete when they start animating
>>> > (since
>>> > they all use the same animation->GetCurrentValue()). It doesn't seem
>>> > possible to
>>> > set a layer property immediately while the animation is running or
>>> start an
>>> > overlapping animation of another property. It also seems quite possible
>>> that
>>> > animations may be accidentally started with this design.
>>>
>>
>>  That is correct. As I said, this patch gives 'trivial' animation
>>> support. What should have once an animation is in progress and a
>>> property changes?
>>>
>>
>>  . Stop animations and immediately go to new value.
>>> . Queue up change and animate there are current animation completes.
>>> . Reroute animation to new value.
>>>
>>
>>  I can think of cases where you want each of these. For example, if
>>> there is some window move animation going on and the user happens to
>>> grab the window and move it, then you want all animations to stop.
>>> Screen rotation code wants the queue approach. And tab animations
>>> generally get rerouted to the new value.
>>>
>>
>>  We'll likely need a way to specify the behavior that should happen.
>>>
>>
>> It is definitely ambiguous. I'm in favor of a trivial approach, but I'm a
>> bit
>> concerned that the new layer API could be misused and result in glitchy or
>> inconsistent transitions. Used properly, the design should work fine, so I
>> don't
>> have strong feelings, but both of the first two options you suggested seem
>> simple to code and use and might be little safer.
>>
>> Ian
>>
>>
>>
>>  > The problems that could be caused by setting properties during an
>>> animation
>>> > could be mitigated by caching pending property changes and processing
>>> them
>>> > afterwards. Or instead of enabling animation on layers by setting an
>>> > animation,
>>> > maybe layers could use the approach you took with views and have a
>>> property
>>> > setter?
>>>
>>
>>    -Scott
>>>
>>
>>  >
>>> > Ian
>>> >
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.**org/7972023/diff/12001/ui/**
>>
aura_shell/examples/window_**type_launcher.cc<http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/window_type_launcher.cc>
>>
>>> > File ui/aura_shell/examples/window_**type_launcher.cc (right):
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.**org/7972023/diff/12001/ui/**
>>
aura_shell/examples/window_**type_launcher.cc#newcode57<http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/window_type_launcher.cc#newcode57>
>>
>>> > ui/aura_shell/examples/window_**type_launcher.cc:57:
>>> > static_cast<float>(total_**width);
>>> > Will the padding get scaled by x_scale? If not, I think you want
>>> > containing_width = x_scale * total_window_width + padding ==> x_scale =
>>> > (containing_width - padding) / total_window_width.
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.**org/7972023/diff/12001/ui/**
>>
aura_shell/examples/window_**type_launcher.cc#newcode66<http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/window_type_launcher.cc#newcode66>
>>
>>> > ui/aura_shell/examples/window_**type_launcher.cc:66:
>>> > (containing_size.width() * - total_width * *x_scale) / 2));
>>> > Should this be containing_size.width() - total_width? Also if the
>>> > padding's not scaled, then this math will also need to be updated.
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.**org/7972023/diff/12001/ui/**
>>
aura_shell/examples/window_**type_launcher.cc#newcode77<http://codereview.chromium.org/7972023/diff/12001/ui/aura_shell/examples/window_type_launcher.cc#newcode77>
>>
>>> > ui/aura_shell/examples/window_**type_launcher.cc:77:
>>> > static_cast<float>(current_**bounds.width() + kPadding) * *x_scale);
>>> > If padding's not scaled, this is another thing to update.
>>> >
>>> >
http://codereview.chromium.**org/7972023/<http://codereview.chromium.org/7972...
>>> >
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/7972023/<http://codereview.chromium.org/7972...
>>
>
>

[sky, 2011-09-28 18:21:38]

Updated to cancel animations if the animation has ticked once and SetBounds
invoked.

I need to add tests, but I'll do that in a separate patch as I want to do a bit
of surgery to layer_unittests that would make this too unwieldy.

[Ben Goodger (Google), 2011-09-28 19:13:33]

LGTM as an incremental step.

[Ian Vollick, 2011-09-29 14:56:51]

On 2011/09/28 19:13:33, Ben Goodger (Google) wrote:
> LGTM as an incremental step.

This also LGTM as an incremental step.