[Panel-devel] LayoutAnimator, Phase/Animator

[Robert Knight]

> why is LayoutAnimator separate from Animator?

LayoutAnimator works on LayoutItems as opposed to QGraphicsItems so it
can get and set an item's geometry during the animation via geometry()
/ setGeometry().  It also understands the states of an item in a
layout ( newly inserted , moved , removed , not in the layout ) and
allows different animations to be defined for transitions between
those states.

> - the inability to change the hardcoded animations at runtime, e.g. via a plugin

I used an enum because it was simpler and I think it covers most of
what people wanted to do.  It could be made to use plugins, although
catering for people who want to add lots of different layouting
effects is low on my list of priorities.

> the effects QHash is never initialized anywhere

If you ask QHash for an element which it does not have it will return
a default constructed value, in the case of an int this is 0, which is
the value of NoEffect.  To make this explicit, "= 0" could be added to
the declaration of NoEffect.

> that's an implementation detail that doesn't
> need to be exposed. the API says it's to change the duration of the
> animation; well, that could be accomplished through simple setters.

I used the QGraphicsItemAnimation API as inspiration.  In addition to
duration, QTimeLine also provides a curve shape.
I don't have a strong opinion on whether it would be better to use
QTimeLine or have a set of setter methods to define the duration and
curve shape.  If you would prefer setters and getters that can be
done.

> what happens if the timeline gets deleted behind our
> backs, since the animator doesn't own it but continues to use it?

The animation will just stop since the timeline is not emitting any
more valueChanged() signals.  The timeline is stored using a
QPointer<> so d->timeLine will be 0 after the timeline is destroyed.

> the use of a single timeline here is a bit on the broken side since it means
> using the same curve shape and time period for all anims that are going on

To clarify, yes, all items within a layout which has an animator will
be animated with the same curve shape and duration, which seemed
sensible to me.  I tested this class mainly using FlowLayout and up to
100 items, I thought it better not to have 100 timelines running at
once.

> it really should just be myLayout->setAnimated(true); and all of that hubbub
> should happen internal to the layout class

Somewhere the set of effects to be used for animation needs to be
defined, but I could define a reasonable default set in LayoutAnimator
( probably Fade in , Smooth move and resize , Fade out ) and have
setAnimated(true).

> - and because it's my favourite thing in the world to whinge about right now:
> wtf doesn't it adhere to the coding style? *bambi eyes*

I thought I was following the style of the rest of the code.  If not,
my bad, I can look up the style notes and fix it.

Regards,
Robert.



On 13/09/2007, Aaron J. Seigo <aseigo at kde.org> wrote:
> hi...
>
> why is LayoutAnimator separate from Animator?
>
> assuming there is a good answer for that (i'm assuming it's a combination
> of "it seemed we'd want one animator per layout" and "it was easier to do it
> this way"), there are some issues that need to be addressed:
>
> - the inability to change the hardcoded animations at runtime, e.g. via a
> plugin
>
> - the effects QHash is never initialized anywhere. i assume that if it works
> at all when setEffects isn't called externally is a fluke (with it being set
> to 0 which is NoEffect if we're lucky). there really ought to be a default
> initialization of the effects hash done.
>
> - the states changing stuff seems a bit on the dangerous side; e.g.
> itemAutoDeleter deletes the item passed in, so if setCurrentState gets
> called
> with state == DeadState we'll end up with problems. yes, it only gets called
> on animationFinished in this fashion ..... but that's an API that's just set
> up for failures. this is esp true since so many methods such as
> setCurrentState are public!
>
> - what is setTimeLine all about? that's an implementation detail that
> doesn't
> need to be exposed. the API says it's to change the duration of the
> animation; well, that could be accomplished through simple setters. but it
> concerns me that we then have code like this:
>
> 	if (animator() && animator()->timeLine()) {
> 		animator()->timeLine()->setCurrentTime(0);
> 		if (animator()->timeLine()->state() == QTimeLine::NotRunning) {
> 			animator()->timeLine->start();
>
> besides breaking a lot of encapsulation concepts all at once here, and
> making
> it all pretty brittle (what happens if the timeline gets deleted behind our
> backs, since the animator doesn't own it but continues to use it?), it also
> means that if multiple items were being animated in the layout that the
> animations of the ones already started will restart (by virtue of the
> setCurrentTime(0) call)
>
> - the use of a single timeline here is a bit on the broken side since it
> means
> using the same curve shape and time period for all anims that are going on,
> which seems wrong. it also leads to problems such as the setCurrentTime(0)
> problem above.
>
> - the example code shows 7 LOC. imho, if the separate LayoutAnimator class
> is
> kept as a seperate entity at all, it really should just be
> myLayout->setAnimated(true); and all of that hubbub should happen internal
> to
> the layout class. if one really must, they can access the resulting animator
> via myLayout->animator();
>
> - and because it's my favourite thing in the world to whinge about right
> now:
> wtf doesn't it adhere to the coding style? *bambi eyes*
>
> virtually all the above would be resolved by merging LayoutAnimator with
> Animator. i'm sure there's more as well as i have to get some sleep here
> rather than continue to read through this code...
>
> --
> Aaron J. Seigo
> humru othro a kohnu se
> GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43
>
> KDE core developer sponsored by Trolltech
>