[Panel-devel] LayoutAnimator, Phase/Animator
Aaron J. Seigo
aseigo at kde.org
Thu Sep 13 08:13:43 CEST 2007
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
-------------- 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/4d7de742/attachment.pgp
More information about the Panel-devel
mailing list