Review Request: Plasma + Qt Kinetic GSoC Project - Attempt 2

[Richard Moore]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1512/#review2234
-----------------------------------------------------------



/trunk/KDE/kdelibs/plasma/animations/animation.h
<http://reviewboard.kde.org/r/1512/#comment1521>

    Not really clear to me what this function does.



/trunk/KDE/kdelibs/plasma/animations/animation.h
<http://reviewboard.kde.org/r/1512/#comment1522>

    More protected data members.



/trunk/KDE/kdelibs/plasma/animations/expand.h
<http://reviewboard.kde.org/r/1512/#comment1523>

    Move private data members into the dpointer



/trunk/KDE/kdelibs/plasma/animations/fade.h
<http://reviewboard.kde.org/r/1512/#comment1524>

    Shouldn't this be called something like FadeAnimation. Fade is too general.



/trunk/KDE/kdelibs/plasma/animations/grow.h
<http://reviewboard.kde.org/r/1512/#comment1525>

    Grow -> GrowAnimation



/trunk/KDE/kdelibs/plasma/animations/slide.cpp
<http://reviewboard.kde.org/r/1512/#comment1526>

    This looks like a setter but is named like a getter.



/trunk/KDE/kdelibs/plasma/deprecated/animator.cpp
<http://reviewboard.kde.org/r/1512/#comment1527>

    This class shouldn't be public


- Richard


On 2009-09-02 06:30:59, makmanalp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1512/
> -----------------------------------------------------------
> 
> (Updated 2009-09-02 06:30:59)
> 
> 
> Review request for Plasma, Aaron Seigo and Alexis Menard.
> 
> 
> Summary
> -------
> 
> Okay, this is the "oh cr*p Alexis is leaving Tokamak" version, which means I'm missing a couple of stuff but I want to get it in in time. Sorry about this, but I have school now so I can't give my attention to this as much as I could. What's missing is:
> 
> - Didn't dptr the private members yet because I got stuck trying to figure out how I'd override render() (which would be in AnimationPrivate) in the subclasses such as Grow etc. I guess I could have a function with the same name in AnimationDerivedPrivate but it wouldn't exactly be overriding?
> - Didn't add factory methods for each animation type yet.
> - Didn't map the existing enums to new stuff (eg AppearAnimation etc)
> - Didn't figure out checking animation status / stop and pause etc.
> 
> Now that that's over, here's the good news:
> 
> - Oodles of general tidying up:
>     - Moved everything into animations/ so it's neater.
>     - Added license headers to everything.
>     - Tidied up includes.
>     - Split each class into its own file.
>     - Added missing getters.
>     - Consted as much as I can.
>     - No more unnecessary this->es.
> - BaseAnimationElement is now AbstractAnimation.
> - Moved the duration variable (m_duration) down the hierarchy into Animation because setting duration for groups is meaningless.
> - setObject() is called setWidget() now and is moved up the hierarchy into AbstractAnimation since it was doing the same for both Animation and AnimationGroup.
> - render() is no longer what it used to be, it's now a protected pure virtual function in Animation that the subclasses must override. getQtAnimation() does some common checking and then calls the render() of that subclass. getQtAnimation() is the exposed interface.
> - getQtAnimation() takes a parent object to pass to Animations and AnimationGroups to use when generating the QPropertyAnimations and QAnimationGroups. When getQtAnimation() is used on AnimationGroups, the generated sub-animations are now owned by the generated group.
> - The MovementDirection enum is now called AnimationDirection and is in plasma.h
> 
> This is the essence of all changes. Documentation might be lagging behind, I'll try to clean it up later on and comments on what's wrong are much appreciated.
> 
> Thanks a lot in advance!
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/plasma/CMakeLists.txt 1018731 
>   /trunk/KDE/kdelibs/plasma/animations/abstractanimation.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/abstractanimation.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/animation.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/animation.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/animationgroup.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/animationgroup.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/expand.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/expand.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/fade.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/fade.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/grow.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/grow.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/slide.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animations/slide.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animator.h 1018731 
>   /trunk/KDE/kdelibs/plasma/animator.cpp 1018731 
>   /trunk/KDE/kdelibs/plasma/deprecated/animator.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/plasma.h 1018731 
> 
> Diff: http://reviewboard.kde.org/r/1512/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> makmanalp
> 
>