Review Request: KPixmapSequence: painting spinners made easy

[Pino Toscano]

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


Few general notes:

a) please implement the tiled loading for XDG icons of the "animations" category as done in KAnimatedButton:
   this means not assuming that a pixmap is 1xN or Nx1, but NxM: in such case, you should check W and H to be exact multipliers of the wanted icon size; for example you can have a 88x44 pixmap representing a 8-step animation for the 22px size of an icon/animation.
   See the code in KAnimatedButton for an idea.

b) missing \since 4.4 in the apidox of the new classes

c) maybe add in KPixmapSequence the icon loading by icon name of the XDG icon theme (using KIconLoader loading from the "animations" category)?

d) still too many spaces around brackets :P

In general I like the idea, I did a small animated widget for Okular (see kdegraphics/okular/ui/animatedwidget.*) for doing this job, so count on Okular as possible user for this new class.


trunk/KDE/kdelibs/kdeui/util/kpixmapsequence.cpp
<http://reviewboard.kde.org/r/1345/#comment1372>

    Not sure about this.
    If you have let's say, 5 as frame count and ask for index=6, you would expect a null pixmap, not the 2nd of the sequence, IMHO.
    Thus a
      return d->mFrames.value(index);
    should do the job, in the case.



trunk/KDE/kdelibs/kdeui/util/kpixmapsequenceoverlaypainter.cpp
<http://reviewboard.kde.org/r/1345/#comment1373>

    Maybe a QBasicTimer would avoid a signal emission (and thus being slightly more efficient).



trunk/KDE/kdelibs/kdeui/util/kpixmapsequencewidget.cpp
<http://reviewboard.kde.org/r/1345/#comment1374>

    You should check for a valid sequence being passed, I think.


- Pino


On 2009-08-17 15:05:35, Sebastian Trueg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1345/
> -----------------------------------------------------------
> 
> (Updated 2009-08-17 15:05:35)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> Showing a spinner to indicate a work in progrss is a typical task. Gwenview does have a nice spinner when loading images. Aurelien and I extracted the code from Gwenview and molded it into three nice classes that allow to create spinners very easily in any situation. At the moment the classes are used in Gwenview and in Nepomuk.
> 
> KPixmapSequence: a simple container class that loads a sequence of pixmaps and provides the frames through a simple interface.
> KPixmapSequenceOverlayPainter: Installs an event filter to paint a KPixmapSequence onto any widget using Qt::Alignment or a relative placement.
> KPixmapSequenceWidget: A simple widget using the overlay painter to draw a spinner while the widget is visible.
> 
> We propose an addition to kdeui.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kdeui/util/kpixmapsequencewidget.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/util/kpixmapsequencewidget.h PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/util/kpixmapsequenceoverlaypainter.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/util/kpixmapsequence.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/util/kpixmapsequenceoverlaypainter.h PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/util/kpixmapsequence.h PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/CMakeLists.txt 1006831 
> 
> Diff: http://reviewboard.kde.org/r/1345/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sebastian
> 
>