Allows observers to be notified when layer animations complete.

Allows observers to be notified when layer animations complete.

Depends on http://codereview.chromium.org/8362006/

BUG=101595
TEST=None

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

[sky, 2011-10-26 21:11:06]

I tend to think that a consumer of this API I care about when the animation I
started is either done or canceled. Having to determine which animationelement
corresponds to the animation I started is a bit tricky.

[vollick1, 2011-10-27 15:57:15]

You're right, it is a bit awkward. Really, the animator operates in terms of
sequences not elements. I am tempted to remove the "Element" methods from
the animator API (ScheduleAnimationElement, etc), to force consumers to work
with sequences, too. I think that would make it easier for a consumer to
determine when their sequence ends. Another option would be to change the
AnimationObserver interface to include OnLayerAnimationElementEnded, but I'm
not as excited about this option because it would bloat a fair number
classes that implement AnimationObserver via LayerDelegate.

I don't think that there's a really nice way to determine when an implicit
animation ends. I could have the Set*** methods return a pointer to the
scheduled sequence, but that feels a bit ugly. Maybe it's ok though?

What do you think?

Ian

On Wed, Oct 26, 2011 at 5:11 PM, <sky@chromium.org> wrote:

> I tend to think that a consumer of this API I care about when the animation
> I
> started is either done or canceled. Having to determine which
> animationelement
> corresponds to the animation I started is a bit tricky.
>
>
http://codereview.chromium.**org/8395046/<http://codereview.chromium.org/8395...
>

[sky, 2011-10-27 17:01:49]

I think the sequence observer is the one folks care about too. Maybe
ScopedSettings should have the ability to take a
LayerAnimationSequenceObserver (maybe this name should be more
generic, but you get the idea). If an observer is supplied it's set as
observer on the last layer animation sequence that gets created.

  -Scott

On Thu, Oct 27, 2011 at 8:57 AM, Ian Vollick <vollick@google.com> wrote:
> You're right, it is a bit awkward. Really, the animator operates in terms of
> sequences not elements. I am tempted to remove the "Element" methods from
> the animator API (ScheduleAnimationElement, etc), to force consumers to work
> with sequences, too. I think that would make it easier for a consumer to
> determine when their sequence ends. Another option would be to change the
> AnimationObserver interface to include OnLayerAnimationElementEnded, but I'm
> not as excited about this option because it would bloat a fair number
> classes that implement AnimationObserver via LayerDelegate.
> I don't think that there's a really nice way to determine when an implicit
> animation ends. I could have the Set*** methods return a pointer to the
> scheduled sequence, but that feels a bit ugly. Maybe it's ok though?
> What do you think?
>
> Ian
>
> On Wed, Oct 26, 2011 at 5:11 PM, <sky@chromium.org> wrote:
>>
>> I tend to think that a consumer of this API I care about when the
>> animation I
>> started is either done or canceled. Having to determine which
>> animationelement
>> corresponds to the animation I started is a bit tricky.
>>
>> http://codereview.chromium.org/8395046/
>
>

[vollick1, 2011-10-27 17:23:53]

That makes sense. Here's how I plan to move ahead (please let me know if
I've misunderstood your suggestion).

   - LayerAnimationSequence will now have an observer list (the observers
   who care about this sequence finishing). Although this will require a bit
   more storage, it allows observers to care about only certain sequences.
   - LayerAnimator will still have an observer list (the observers who care
   about any animation finishing). The animator's observers are associated with
   every sequence that's run.
   - I will add ScopedSettings::AddObserver which will add to the animators
   observer list (and remove it when destructed). These observers will be
   associated with any implicit animation that's created while the scoped
   settings are live.

Example of usage:

// explicit animation
LayerAnimationSequence* seq = new MinimizeSequence();
seq->AddObserver(this);
layer()->GetAnimator()->ScheduleAnimation(seq);

// implicit animation
{
  ScopedSettings settings(layer()->GetAnimator());
  settings.AddObserver(this);
  // We will only listen for the animations caused by the following.
  layer()->SetBounds(new_bounds);
  layer()->SetTransform(new_transform);
}

Ian

On Thu, Oct 27, 2011 at 1:01 PM, Scott Violet <sky@chromium.org> wrote:

> I think the sequence observer is the one folks care about too. Maybe
> ScopedSettings should have the ability to take a
> LayerAnimationSequenceObserver (maybe this name should be more
> generic, but you get the idea). If an observer is supplied it's set as
> observer on the last layer animation sequence that gets created.
>
>  -Scott
>
> On Thu, Oct 27, 2011 at 8:57 AM, Ian Vollick <vollick@google.com> wrote:
> > You're right, it is a bit awkward. Really, the animator operates in terms
> of
> > sequences not elements. I am tempted to remove the "Element" methods from
> > the animator API (ScheduleAnimationElement, etc), to force consumers to
> work
> > with sequences, too. I think that would make it easier for a consumer to
> > determine when their sequence ends. Another option would be to change the
> > AnimationObserver interface to include OnLayerAnimationElementEnded, but
> I'm
> > not as excited about this option because it would bloat a fair number
> > classes that implement AnimationObserver via LayerDelegate.
> > I don't think that there's a really nice way to determine when an
> implicit
> > animation ends. I could have the Set*** methods return a pointer to the
> > scheduled sequence, but that feels a bit ugly. Maybe it's ok though?
> > What do you think?
> >
> > Ian
> >
> > On Wed, Oct 26, 2011 at 5:11 PM, <sky@chromium.org> wrote:
> >>
> >> I tend to think that a consumer of this API I care about when the
> >> animation I
> >> started is either done or canceled. Having to determine which
> >> animationelement
> >> corresponds to the animation I started is a bit tricky.
> >>
> >> http://codereview.chromium.org/8395046/
> >
> >
>

[sky, 2011-10-27 17:38:28]

SGTM

  -Scott

On Thu, Oct 27, 2011 at 10:22 AM, Ian Vollick <vollick@google.com> wrote:
> That makes sense. Here's how I plan to move ahead (please let me know if
> I've misunderstood your suggestion).
>
> LayerAnimationSequence will now have an observer list (the observers who
> care about this sequence finishing). Although this will require a bit more
> storage, it allows observers to care about only certain sequences.
> LayerAnimator will still have an observer list (the observers who care about
> any animation finishing). The animator's observers are associated with every
> sequence that's run.
> I will add ScopedSettings::AddObserver which will add to the animators
> observer list (and remove it when destructed). These observers will be
> associated with any implicit animation that's created while the scoped
> settings are live.
>
> Example of usage:
> // explicit animation
> LayerAnimationSequence* seq = new MinimizeSequence();
> seq->AddObserver(this);
> layer()->GetAnimator()->ScheduleAnimation(seq);
> // implicit animation
> {
>   ScopedSettings settings(layer()->GetAnimator());
>   settings.AddObserver(this);
>   // We will only listen for the animations caused by the following.
>   layer()->SetBounds(new_bounds);
>   layer()->SetTransform(new_transform);
> }
> Ian
> On Thu, Oct 27, 2011 at 1:01 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> I think the sequence observer is the one folks care about too. Maybe
>> ScopedSettings should have the ability to take a
>> LayerAnimationSequenceObserver (maybe this name should be more
>> generic, but you get the idea). If an observer is supplied it's set as
>> observer on the last layer animation sequence that gets created.
>>
>>  -Scott
>>
>> On Thu, Oct 27, 2011 at 8:57 AM, Ian Vollick <vollick@google.com> wrote:
>> > You're right, it is a bit awkward. Really, the animator operates in
>> > terms of
>> > sequences not elements. I am tempted to remove the "Element" methods
>> > from
>> > the animator API (ScheduleAnimationElement, etc), to force consumers to
>> > work
>> > with sequences, too. I think that would make it easier for a consumer to
>> > determine when their sequence ends. Another option would be to change
>> > the
>> > AnimationObserver interface to include OnLayerAnimationElementEnded, but
>> > I'm
>> > not as excited about this option because it would bloat a fair number
>> > classes that implement AnimationObserver via LayerDelegate.
>> > I don't think that there's a really nice way to determine when an
>> > implicit
>> > animation ends. I could have the Set*** methods return a pointer to the
>> > scheduled sequence, but that feels a bit ugly. Maybe it's ok though?
>> > What do you think?
>> >
>> > Ian
>> >
>> > On Wed, Oct 26, 2011 at 5:11 PM, <sky@chromium.org> wrote:
>> >>
>> >> I tend to think that a consumer of this API I care about when the
>> >> animation I
>> >> started is either done or canceled. Having to determine which
>> >> animationelement
>> >> corresponds to the animation I started is a bit tricky.
>> >>
>> >> http://codereview.chromium.org/8395046/
>> >
>> >
>
>

[Ian Vollick, 2011-10-27 18:43:25]

I've implemented the plan. I've also made it so that LayerDelegates are not
LayerAnimationObservers. This was the case so the aura desktop could listen for
screen rotations ending. The desktop now observes screen rotations directly.
Most of the modified files are the removal of empty ui::LayerAnimationObserver
implementations. I was also able to remove LayerAnimatorDelegate (which was both
a LayerAnimationDelegate and a LayerAnimationObserver) in favor of
LayerAnimationDelegate.

Ian

On 2011/10/27 17:38:28, sky wrote:
> SGTM
> 
>   -Scott
> 
> On Thu, Oct 27, 2011 at 10:22 AM, Ian Vollick <mailto:vollick@google.com>
wrote:
> > That makes sense. Here's how I plan to move ahead (please let me know if
> > I've misunderstood your suggestion).
> >
> > LayerAnimationSequence will now have an observer list (the observers who
> > care about this sequence finishing). Although this will require a bit more
> > storage, it allows observers to care about only certain sequences.
> > LayerAnimator will still have an observer list (the observers who care about
> > any animation finishing).&nbsp;The animator's observers are associated with
every
> > sequence that's run.
> > I will add ScopedSettings::AddObserver which will add to the animators
> > observer list (and remove it when destructed). These observers will be
> > associated with any implicit animation that's created while the scoped
> > settings are live.
> >
> > Example of usage:
> > // explicit animation
> > LayerAnimationSequence* seq = new MinimizeSequence();
> > seq->AddObserver(this);
> > layer()->GetAnimator()->ScheduleAnimation(seq);
> > // implicit animation
> > {
> > &nbsp; ScopedSettings settings(layer()->GetAnimator());
> > &nbsp; settings.AddObserver(this);
> > &nbsp; // We will only listen for the animations caused by the following.
> > &nbsp; layer()->SetBounds(new_bounds);
> > &nbsp; layer()->SetTransform(new_transform);
> > }
> > Ian

[sky, 2011-10-27 19:29:19]

Make sure you add some test coverage too.

http://codereview.chromium.org/8395046/diff/10001/ui/aura/desktop.cc
File ui/aura/desktop.cc (right):

http://codereview.chromium.org/8395046/diff/10001/ui/aura/desktop.cc#newcode281
ui/aura/desktop.cc:281: #if !defined(NDEBUG)
I don't like this method here. It results in Desktop implementing an interface
and cluttering the API. Could we rip it out?

http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/dummy_lay...
File ui/gfx/compositor/dummy_layer_animation_delegate.h (right):

http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/dummy_lay...
ui/gfx/compositor/dummy_layer_animation_delegate.h:19: class COMPOSITOR_EXPORT
DummyLayerAnimationDelegate
Rename this TestLayerAnimationDelegate

http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_observer.h (right):

http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_observer.h:22: // if this becomes necessary,
this would be the appropriate place to add
Remove this comment.

http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animator.cc (right):

http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:10: #include "base/observer_list.h"
don't need this since in header.

http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:211: animator_->AddObserver(observer);
I believe this means the observer is notified of every sequence created.
Shouldn't it just be the last one that gets created?

http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:304:
NotifyAnimationEnded(running_animations_[i].sequence);
Can't progress recognize it's at the end and notify observers itself?

http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animator.h (right):

http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.h:246: // These parties are notified when layer
animations end.
parties -> observers

[Ian Vollick, 2011-10-28 13:26:26]

On 2011/10/27 19:29:19, sky wrote:
> Make sure you add some test coverage too.
Done.

> 
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/dummy_lay...
> File ui/gfx/compositor/dummy_layer_animation_delegate.h (right):
> 
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/dummy_lay...
> ui/gfx/compositor/dummy_layer_animation_delegate.h:19: class COMPOSITOR_EXPORT
> DummyLayerAnimationDelegate
> Rename this TestLayerAnimationDelegate
Done.

> 
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
> File ui/gfx/compositor/layer_animation_observer.h (right):
> 
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animation_observer.h:22: // if this becomes necessary,
> this would be the appropriate place to add
> Remove this comment.
Done.

> 
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
> File ui/gfx/compositor/layer_animator.cc (right):
> 
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:10: #include "base/observer_list.h"
> don't need this since in header.
Removed.

> 
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:211: animator_->AddObserver(observer);
> I believe this means the observer is notified of every sequence created.
> Shouldn't it just be the last one that gets created?
If you add an observer using your scoped settings and start two implicit
animations (bounds and transform) I wanted the observer to be notified when
either completes. Since there's no guarantee that the last animation created
will be the last to complete, I'd hesitate to only notify when the last created
animation completes.

> 
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.cc:304:
> NotifyAnimationEnded(running_animations_[i].sequence);
> Can't progress recognize it's at the end and notify observers itself?
Yes it can and should. Thank you for catching that. Fixed.

> 
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
> File ui/gfx/compositor/layer_animator.h (right):
> 
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
> ui/gfx/compositor/layer_animator.h:246: // These parties are notified when
layer
> animations end.
> parties -> observers
Done.

[sky, 2011-10-28 17:37:17]

On Fri, Oct 28, 2011 at 6:26 AM,  <vollick@chromium.org> wrote:
> On 2011/10/27 19:29:19, sky wrote:
>>
>> Make sure you add some test coverage too.
>
> Done.
>
>
>
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/dummy_lay...
>>
>> File ui/gfx/compositor/dummy_layer_animation_delegate.h (right):
>
>
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/dummy_lay...
>>
>> ui/gfx/compositor/dummy_layer_animation_delegate.h:19: class
>> COMPOSITOR_EXPORT
>> DummyLayerAnimationDelegate
>> Rename this TestLayerAnimationDelegate
>
> Done.
>
>
>
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
>>
>> File ui/gfx/compositor/layer_animation_observer.h (right):
>
>
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
>>
>> ui/gfx/compositor/layer_animation_observer.h:22: // if this becomes
>> necessary,
>> this would be the appropriate place to add
>> Remove this comment.
>
> Done.
>
>
>
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
>>
>> File ui/gfx/compositor/layer_animator.cc (right):
>
>
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
>>
>> ui/gfx/compositor/layer_animator.cc:10: #include "base/observer_list.h"
>> don't need this since in header.
>
> Removed.
>
>
>
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
>>
>> ui/gfx/compositor/layer_animator.cc:211: animator_->AddObserver(observer);
>> I believe this means the observer is notified of every sequence created.
>> Shouldn't it just be the last one that gets created?
>
> If you add an observer using your scoped settings and start two implicit
> animations (bounds and transform) I wanted the observer to be notified when
> either completes.

Yes, that sounds good. And it'll only be notified once, right? I was
more concerned with the situation of multiple bounds animations being
created, in which case observers only want to know when the last one
is done, and not for each one.

  -Scott

>  Since there's no guarantee that the last animation created
> will be the last to complete, I'd hesitate to only notify when the last
> created
> animation completes.
>
>
>
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
>>
>> ui/gfx/compositor/layer_animator.cc:304:
>> NotifyAnimationEnded(running_animations_[i].sequence);
>> Can't progress recognize it's at the end and notify observers itself?
>
> Yes it can and should. Thank you for catching that. Fixed.
>
>
>
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
>>
>> File ui/gfx/compositor/layer_animator.h (right):
>
>
>
http://codereview.chromium.org/8395046/diff/10001/ui/gfx/compositor/layer_ani...
>>
>> ui/gfx/compositor/layer_animator.h:246: // These parties are notified when
>
> layer
>>
>> animations end.
>> parties -> observers
>
> Done.
>
>
> http://codereview.chromium.org/8395046/
>

[Ian Vollick, 2011-10-28 20:22:30]

> >> ui/gfx/compositor/layer_animator.cc:211: animator_->AddObserver(observer);
> >> I believe this means the observer is notified of every sequence created.
> >> Shouldn't it just be the last one that gets created?
> >
> > If you add an observer using your scoped settings and start two implicit
> > animations (bounds and transform) I wanted the observer to be notified when
> > either completes.
> 
> Yes, that sounds good. And it'll only be notified once, right? I was
> more concerned with the situation of multiple bounds animations being
> created, in which case observers only want to know when the last one
> is done, and not for each one.

I'd hoped that you'd get notified of all animations that are started while
you're observing*, but I can see how it would make it tricky to determine if all
the implicit animations are done. I have a solution, though. I've made it so
that animation observers are notified whenever an animation is scheduled,
aborted, or ended. That should make it easy to wait for the last implicit
animation, but still allows you to be notified whenever any observed animation
completes. I've added unit test coverage for the OnLayerAnimationScheduled and
OnLayerAnimationAborted.

Ian

// *implicit animation
{
  ScopedSettings settings(layer()->GetAnimator());
  settings.AddObserver(this);
  // Will get notified about both bounds animations
  layer()->SetBounds(bounds_a);
  layer()->SetBounds(bounds_b);
}

[sky, 2011-10-28 22:11:38]

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_observer.h (right):

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_observer.h:20: const LayerAnimationSequence*
sequence) = 0;
Most folks are only going to care about ended. Because of this I tend to think
all of these should be non-pure methods.

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_sequence.cc (right):

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_sequence.cc:113: if
(!observers_.HasObserver(observer))
Don't do this, there's a DCHECK that you don't add an observer twice in
observer_list.

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animation_sequence.h (right):

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animation_sequence.h:108: // These parties are notified
when layer animations end.
parties -> observers

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/layer_ani...
File ui/gfx/compositor/layer_animator.cc (right):

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:199: if (!observers_.HasObserver(observer))
remove this.

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:204: observers_.RemoveObserver(observer);
Doesn't this need to remove it from any sequences too?

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/layer_ani...
ui/gfx/compositor/layer_animator.cc:215:
std::set<LayerAnimationObserver*>::const_iterator i = observers_.begin();
move into loop.

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/test_laye...
File ui/gfx/compositor/test_layer_animation_delegate.h (right):

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/test_laye...
ui/gfx/compositor/test_layer_animation_delegate.h:23:
TestLayerAnimationDelegate(const LayerAnimationDelegate& other);
explicit

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/test_laye...
File ui/gfx/compositor/test_layer_animation_observer.cc (right):

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/test_laye...
ui/gfx/compositor/test_layer_animation_observer.cc:7: #include <cstddef>
Is this include really needed?

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/test_laye...
File ui/gfx/compositor/test_layer_animation_observer.h (right):

http://codereview.chromium.org/8395046/diff/11003/ui/gfx/compositor/test_laye...
ui/gfx/compositor/test_layer_animation_observer.h:33: 
nit: remove line.

[sky, 2011-10-28 22:13:25]

On Fri, Oct 28, 2011 at 1:22 PM,  <vollick@chromium.org> wrote:
>> >> ui/gfx/compositor/layer_animator.cc:211:
>> >> animator_->AddObserver(observer);
>> >> I believe this means the observer is notified of every sequence
>> >> created.
>> >> Shouldn't it just be the last one that gets created?
>> >
>> > If you add an observer using your scoped settings and start two implicit
>> > animations (bounds and transform) I wanted the observer to be notified
>> > when
>> > either completes.
>
>> Yes, that sounds good. And it'll only be notified once, right? I was
>> more concerned with the situation of multiple bounds animations being
>> created, in which case observers only want to know when the last one
>> is done, and not for each one.
>
> I'd hoped that you'd get notified of all animations that are started while
> you're observing*, but I can see how it would make it tricky to determine if
> all
> the implicit animations are done. I have a solution, though. I've made it so
> that animation observers are notified whenever an animation is scheduled,
> aborted, or ended. That should make it easy to wait for the last implicit
> animation, but still allows you to be notified whenever any observed
> animation
> completes.

I'm not sure that makes it any easier, it means I have to maintain
some count which is easily forgotten. Luckily I don't think most folks
will create a sequence so hopefully isn't much of an issue.

  -Scott

>  I've added unit test coverage for the OnLayerAnimationScheduled
> and
> OnLayerAnimationAborted.
>
> Ian
>
> // *implicit animation
> {
>  ScopedSettings settings(layer()->GetAnimator());
>  settings.AddObserver(this);
>  // Will get notified about both bounds animations
>  layer()->SetBounds(bounds_a);
>  layer()->SetBounds(bounds_b);
> }
>
>
>
>
> http://codereview.chromium.org/8395046/
>

[vollick1, 2011-10-29 01:17:33]

>
> >> >> animator_->AddObserver(observer);
> >> >> I believe this means the observer is notified of every sequence
> >> >> created.
> >> >> Shouldn't it just be the last one that gets created?
> >> >
> >> > If you add an observer using your scoped settings and start two
> implicit
> >> > animations (bounds and transform) I wanted the observer to be notified
> >> > when
> >> > either completes.
> >
> >> Yes, that sounds good. And it'll only be notified once, right? I was
> >> more concerned with the situation of multiple bounds animations being
> >> created, in which case observers only want to know when the last one
> >> is done, and not for each one.
> >
> > I'd hoped that you'd get notified of all animations that are started
> while
> > you're observing*, but I can see how it would make it tricky to determine
> if
> > all
> > the implicit animations are done. I have a solution, though. I've made it
> so
> > that animation observers are notified whenever an animation is scheduled,
> > aborted, or ended. That should make it easy to wait for the last implicit
> > animation, but still allows you to be notified whenever any observed
> > animation
> > completes.
>
> I'm not sure that makes it any easier, it means I have to maintain
> some count which is easily forgotten. Luckily I don't think most folks
> will create a sequence so hopefully isn't much of an issue.


It's true, you would have to maintain some state to know when you're seeing
the 'last' sequence. It's not ideal. It would be better if
OnLayerAnimationEnded was called only once with the sequence you care about
(and perhaps with NULL if no implicit animation is created?), but it seems
tricky to know when it's the 'last' animation with animations targeting
different properties. I've thought of two solutions.

   1. I could make it simple to determine the 'last' sequence by changing
   the observer API so that you observe only a particular property.
   2. I could create an ImplicitAnimationObserver (maybe with a different
   name) that is associated with a ScopedSettings and whose
   OnImplicitAnimationEnded is called only when all the animations caused by
   the ScopedSettings are done.

What do you think is the best way forward?

Thanks,

Ian

[sky, 2011-10-31 14:06:21]

 Fri, Oct 28, 2011 at 6:17 PM, Ian Vollick <vollick@google.com> wrote:
>> >> >> animator_->AddObserver(observer);
>> >> >> I believe this means the observer is notified of every sequence
>> >> >> created.
>> >> >> Shouldn't it just be the last one that gets created?
>> >> >
>> >> > If you add an observer using your scoped settings and start two
>> >> > implicit
>> >> > animations (bounds and transform) I wanted the observer to be
>> >> > notified
>> >> > when
>> >> > either completes.
>> >
>> >> Yes, that sounds good. And it'll only be notified once, right? I was
>> >> more concerned with the situation of multiple bounds animations being
>> >> created, in which case observers only want to know when the last one
>> >> is done, and not for each one.
>> >
>> > I'd hoped that you'd get notified of all animations that are started
>> > while
>> > you're observing*, but I can see how it would make it tricky to
>> > determine if
>> > all
>> > the implicit animations are done. I have a solution, though. I've made
>> > it so
>> > that animation observers are notified whenever an animation is
>> > scheduled,
>> > aborted, or ended. That should make it easy to wait for the last
>> > implicit
>> > animation, but still allows you to be notified whenever any observed
>> > animation
>> > completes.
>>
>> I'm not sure that makes it any easier, it means I have to maintain
>> some count which is easily forgotten. Luckily I don't think most folks
>> will create a sequence so hopefully isn't much of an issue.
>
> It's true, you would have to maintain some state to know when you're seeing
> the 'last' sequence. It's not ideal. It would be better if
> OnLayerAnimationEnded was called only once with the sequence you care about
> (and perhaps with NULL if no implicit animation is created?), but it seems
> tricky to know when it's the 'last' animation with animations targeting
> different properties. I've thought of two solutions.
>
> I could make it simple to determine the 'last' sequence by changing the
> observer API so that you observe only a particular property.
> I could create an ImplicitAnimationObserver (maybe with a different name)
> that is associated with a ScopedSettings and whose OnImplicitAnimationEnded
> is called only when all the animations caused by the ScopedSettings are
> done.
>
> What do you think is the best way forward?
> Thanks,
> Ian

I wouldn't worry about it for now. Wait until it becomes an issue.

  -Scott

[sky, 2011-10-31 20:11:13]

LGTM