[Panel-devel] LayoutAnimator, Phase/Animator

[Aaron J. Seigo]

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