Review Request: Plasma + QT Kinetic GSoC Project - Attempt 1

Mon Aug 17 21:59:54 CEST 2009

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

great first steps. there's some API clean up needed, though :)

i wonder if all the animation implementations like Fade, Move, etc need to be exported as public classes or if we can get away with a set of factory methods that return Plasma::AbstractAnimations? one big benefit to that approach is that the animations can be changed behind the scenes without disturbing code.

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1388>

    * one class per file
    * a proper license header is needed
    * please list #includes in alphabetical order
    * these files should probably all go into the animations/ sub directory to keep it all together.

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1382>

    if these are in the Plasma namespace, they belong in plasma.h; however, in this case i think that there should be an Animator class still.. see main comments..

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1387>

    following our naming scheme, this should be AbstractAnimationElement; that said, i wonder if AbastractAnimationElement and Animation couldn't be merged and AnimationGroup subclass Animation?

    either that or AbastractAnimationElement should not have things like the duration in it as setting the duration on a group is probably not meaningful (in which case it should be moved to Animation)

    so the setObject/object setter/getter pair, render and start would remain, the rest would move into Animation

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1375>

    if this is a public class (which it seems to be) it must have a dptr and no exposed data members.

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1380>

    perhaps setWidget, since that's what is being passed in? also, there should probably be a getter for this...

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1376>

    isParellel(), also should be const.

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1379>

    const

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1377>

    dptr.

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1393>

    i don't think a map or a hash is called for here; a simple QList should do? it provides counts, indexed access, removal, etc.

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1378>

    why is the dtor pure virtual? it should just be virtual?

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1383>

    int duration() const; getter is missing

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1384>

    should there be a way to check if the animation is running, paused or stopped?

    speaking of which, how does one pause/stop an animation? or is the idea to just delete the animation objects when that happens?

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1385>

    returning an object that belongs to the caller is just asking for memory leaks :/ i'd recommend taking a QObject *parent and passing that to the generated QPropertyAnimation

/trunk/KDE/kdelibs/plasma/animationelements.h
<http://reviewboard.kde.org/r/1320/#comment1386>

    this is already pure virtual in BaseAnimationElement, no?

    i'd actually suggest implementing it here and calling a protected method that subclasses could override. there's a common check for, e.g., m_object that could be put into the render() implementation, preventing such sanity checks from being repeated in each subclass.

/trunk/KDE/kdelibs/plasma/animationelements.cpp
<http://reviewboard.kde.org/r/1320/#comment1381>

    this-> is unecessary ...

/trunk/KDE/kdelibs/plasma/animationelements.cpp
<http://reviewboard.kde.org/r/1320/#comment1391>

    who owns/deletes these items? that's an ambiguity in this design currently that will lead to much pain and memory leaking, i fear.

    i think that when an element is added to a group, the group should take ownership of it.

/trunk/KDE/kdelibs/plasma/animationelements.cpp
<http://reviewboard.kde.org/r/1320/#comment1392>

    m_numAnims could just be the size of the collection of items. no need for a separate variable for that.

/trunk/KDE/kdelibs/plasma/animator.h
<http://reviewboard.kde.org/r/1320/#comment1389>

    could these be mapped to something using the new code? would ease porting

/trunk/KDE/kdelibs/plasma/animator.h
<http://reviewboard.kde.org/r/1320/#comment1390>

    ditto as with the Animation enum

- Aaron

On 2009-08-14 12:50:13, makmanalp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1320/
> -----------------------------------------------------------
> 
> (Updated 2009-08-14 12:50:13)
> 
> 
> Review request for Plasma, Aaron Seigo and Alexis Menard.
> 
> 
> Summary
> -------
> 
> Hey,
> Here's the current state of where I took my project. I'm sure that this isn't perfect enough to instantly merge into trunk, but it covers the requirements and works. I'll be working on it after GSoC ends until it gets in trunk and then I'll continue maintaining it, so I need the criticism to make it better and re-submit to review later.
> 
> Please read the FAQ [0] first, where I'll guide you though how everything works from start to finish. If you read that and check out the code at the same time, it'll save you a lot of time. Install instructions [1] are available so you can test. There's also a TODO [2] with some non-crucial stuff and some bugs (If you have any idea what's wrong with those, it'd be great to know). Finally, there's a sample plasmoid [3] but it's not nearly as good as it should be (sorry aseigo, couldn't get the chance for the cool demo). That's my next priority so we can show it off.
> 
> Oh, and I think I should mention that I moved animator.cpp to deprecated/animator.cpp and deprecated the old methods, it's not clear in the diffs.
> 
> [0] http://websvn.kde.org/branches/work/~makmanalp/FAQ?view=markup
> [1] http://websvn.kde.org/branches/work/~makmanalp/INSTALL?view=markup
> [2] http://websvn.kde.org/branches/work/~makmanalp/TODO?view=markup
> [3] http://websvn.kde.org/branches/work/~makmanalp/sample/
> 
> UPDATE: I generated docs: http://blackwater.constant.inople.net/gsocdoc/html/
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/plasma/CMakeLists.txt 1011336 
>   /trunk/KDE/kdelibs/plasma/animationelements.h PRE-CREATION 
>   /trunk/KDE/kdelibs/plasma/animationelements.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 1011336 
>   /trunk/KDE/kdelibs/plasma/animator.cpp 1011336 
>   /trunk/KDE/kdelibs/plasma/deprecated/animator.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/1320/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> makmanalp
> 
>