Review Request: KPixmapSequence: painting spinners made easy

[Christoph Feck]

> On 2009-08-23 08:28:10, Christoph Feck wrote:
> > trunk/KDE/kdelibs/kdeui/util/kpixmapsequencewidget.h, line 43
> > <http://reviewboard.kde.org/r/1345/diff/4/?file=10361#file10361line43>
> >
> >     maybe implement sizeHint for this class
> 
>  wrote:
>     not really necessary due to the fixed size, right?

Setting a fixed size on a widget does not make sizeHint() return that size. This was the reason for Konqueror's throbber being too big.


> On 2009-08-23 08:28:10, Christoph Feck wrote:
> > trunk/KDE/kdelibs/kdeui/util/kpixmapsequenceoverlaypainter.h, line 97
> > <http://reviewboard.kde.org/r/1345/diff/4/?file=10359#file10359line97>
> >
> >     Instead of an offset, maybe use a bounds rectangle to indicate where to paint to optionally allow scaled painting.
> 
>  wrote:
>     so you mean a rectangle and an optional scaling flag?

void setPosition(const QRect &rect = QRect(), Qt::Alignment align = Qt::AlignCenter);

If empty rect is passed, use widget's rect. If image does not fit in rect, it is downscaled automatically; upscaling is not done. This is the same behaviour as Qt icon rendering functions.

Maybe use separate position and alignment setters.


> On 2009-08-23 08:28:10, Christoph Feck wrote:
> > trunk/KDE/kdelibs/kdeui/util/kpixmapsequence.h, line 96
> > <http://reviewboard.kde.org/r/1345/diff/4/?file=10357#file10357line96>
> >
> >     support for NxM tiling is not implemented here, should use frameSize parameter
> 
>  wrote:
>     how about just removing the non-const methods?

How about removing the static functions? My vision for the API is this:

// creates empty sequence, set frameSize and pixmap later using setSequencePixmap() or load()
KPixmapSequence();

// create sequence from an XDG icon name
explicit KPixmapSequence(const QString &name, int iconSize = KIconLoader::SizeSmall);

// an enum is used to select some standard sequences (such as the kde-spinner)
KPixmapSequence(StandardPixmapSequence sequence);

Having this non "explicit" would allow us to use the enum on the classes that accept a KPixmapSequence. This would also solve the double loading problem.

// creates sequence from a combined pixmap. 
explicit KPixmapSequence(const QPixmap &pixmap, const QSize &frameSize = QSize());

You can just use QPixmap(const QString &fileName) to load from a file.

// change pixmap/frameSize
setSequencePixmap(const QPixmap &pixmap, const QSize &frameSize = QSize())

If frameSize is empty, the pixmap's width will be used as the size.


On 2009-08-23 08:28:10, Sebastian Trueg wrote:
> > 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.
> 
> Sebastian Trueg wrote:
>     The speed in QMovie is a percentage of the original movie speed. We do not have an original movie speed, except if we let one fall out of the sky. 
>     IMHO it makes more sense to add support for QMovie to the overlay painter instead of the sequence. The interval could be ignored in that case or something.

Sorry, my comment was confusing. With the current version you would be able to set different speeds for different widgets, because each has its own "interval" property. If that property was moved to the sequence, you would lose this ability. For this case, an additional "speed" property could be added to the painter/widget classes.

About QMovie support: I was thinking about using your new class inside KAnimatedWidget, so that the pixmap splitting logic wasn't duplicated. But KAnimatedWidget could handle the QMovie case separately; making KPixmapSequence a QObject is probably overkill. So ignore my comment about QMovie.

This reviewboard sucks, I cannot provide diffs for proposed changes, making me look too lazy to implement my suggestions. The growing edit boxes add to the sucking experience.


- Christoph


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


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