Review Request: KPixmapSequence: painting spinners made easy

[Christoph Feck]

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


Hi, nice idea. Here are some comments from my side.



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

    The description seems to suggest that you pass a directory, and all images inside this directory are used as the sequence's images.
    
    Clarify the description, and maybe rename "path" to "filePath".



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

    support for NxM tiling is not implemented here, should use frameSize parameter



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

    passing a null pixmap causes a division by zero exception



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

    end() the painter before storing the pixmap



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

    check for invalid pixmap to avoid the division by zero exception



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

    end() the painter before storing the pixmap



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

    spelling



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

    The problem is that it always loads the spinner sequence first. You maybe should have this logic moved to the constructor of the sequence to avoid loading two files.



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

    Instead of an offset, maybe use a bounds rectangle to indicate where to paint to optionally allow scaled painting.



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

    use the installed event filter on the widget to be notified of hide/show events to stop/restart the timer



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

    maybe implement sizeHint for this class



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

    spelling :)


I am not sure if "interval" should be a property of KPixmapSequence. If setting the speed on KPixmapSequence is limiting, you could optionally have a "speed" property (as QMovie) to alter the sequence's speed in the other classes.

Maybe even refactor KPixmapSequence to be able to load QMovie and be usable as the "backend" for KAnimatedButton.

- Christoph


On 2009-08-20 15:58:39, Sebastian Trueg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1345/
> -----------------------------------------------------------
> 
> (Updated 2009-08-20 15:58:39)
> 
> 
> 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/kpixmapsequence.h 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/kpixmapsequenceoverlaypainter.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/util/kpixmapsequencewidget.h PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/util/kpixmapsequencewidget.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/1345/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sebastian
> 
>