[Panel-devel] LayoutAnimator, Phase/Animator

[Aaron J. Seigo]

On Thursday 13 September 2007, Robert Knight wrote:
> > 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.

yes, that's a description of what LayoutAnimator does, but it doesn't explain 
why it is separate from Animator =)

> > - 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.

that isn't what i mean either =) the enums are fine, but it should be possible 
to provide different implementations for "fade in and move", for instance. 
this is what Phase/Animator provides.

> > 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.

but is it guaranteed to be initialized to 0?i know some special variables such 
as statics get cutesy initialization guarantees, but i'm not sure that's the 
case with a QHash<int, int>?

> > 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

the ItemAnimation only animates one item; it isn't an animation manager, 
however =)

> 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.

my concerns are two-fold:

- it exposes an implementation detail (a QTimeLine is used) making it 
unpossible (i love that non-word) to change the implementation later if 
desired
- use of a single QTimeLine will lead to bugs and/or limitations (as i already 
outlined in the previous email =)

> > 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.

ah, it is indeed using QPointer. ok, that's one concern down. the animations 
just stopping is still of concern, though the lesser of the two indeed.

> > 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.

having one tick is good for performance, bad for presentation. we came to a 
best-of-both-worlds solution in Animator where we use one tick but allow 
per-anim curves, durations and framerates

> > 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).

yes.. and again i think that Animator provides a decent way to deal with this 
by letting the consumer define the effect they want when setting up the 
animation.

that said, Animator can be improved in a few ways, such as chaining of 
animations and i still need to implement the math for the different curve 
shapes ... 

but i really think it's the sane way to go to have one location for animations 
that can be reimplemented easily (including by plugins) so that the animation 
manager defines the complexity of the effects not the consumer (think about 
this: how do we turn off layout animations for low powered systems? this is 
trivial with Animator)

it also gives is a one-tick animation system.

i don't think we need to get rid of the code in LayoutAnimator, it just needs 
to be merged into Animator. that said, i don't think this is a vital task, 
even for 4.0, but it would be very smart of us to make sure LayoutAnimator 
does not live any longer than it needs to.

> > - 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.

the biggest offenses that i'm seeing (and not just in your code, but in so 
much of the stuff people are committing recently):

- foreach (foo, bar) not foreach(foo, bar); it should look like flow control 
not a function/method call
- if (foo) not if ( foo ); the spaces inside ()s are gratuitous
- always use {}s, even on single liners

-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/panel-devel/attachments/20070913/6fb8eb74/attachment.pgp