[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: KPixmapSequence: painting spinners made easy
From:       "Christoph Feck" <christoph () maxiom ! de>
Date:       2009-08-23 8:28:03
Message-ID: 20090823082803.988.47337 () localhost
[Download RAW message or body]


-----------------------------------------------------------
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
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]