----------------------------------------------------------- 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 > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
