[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:       "Sebastian Trueg" <trueg () kde ! org>
Date:       2009-08-24 10:18:37
Message-ID: 20090824101837.26527.54953 () localhost
[Download RAW message or body]



> 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?
> 
> wrote:
> 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.

I like the only-constructors approach.
I don't even think the setSequencePixmap is necessary. After all you would barely \
need to change the used pixmap. And if you do, just create a new one. The overhead is \
minimal and it keeps the API clean. As for the enum: so far we only have two such \
sequences in KDE: th one used by konqueror which will probably not be used by anyone \
else and the one I use which the overlay painter defaults to. Thus, I am not sure if \
the enum is really worth it. But: it would be nice once there is more choice and it \
can easily be added at a later point in time. Loading from icon name: here I see one \
problem: we need to specify both iconSize and frameSize in the constructor. That \
seems a but clumsy, doesn't it? And the code would not differ much:

   KPixmapSequence s1("process_working", QSize(16,16), 16 );

vs.

   KPixmapSequence s2(KIcon("process_working").pixmap(16,16), QSize(16,16));


> 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?
> 
> wrote:
> 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. 

ok, done.


> 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?
> 
> wrote:
> 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.
> 

OK, but then a relative offset from the bottom-right for example is much more \
complicated for the developer since they have to connect to sizeEvents to update the \
rect. And the only situation where you would actually need downscaling is when the \
widget is too small. That could be done automatically. Another possibility would be \
to *add* the rect, i.e. have both an optional rect and an optional relative placement \
point.


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. 
> Christoph Feck wrote:
> 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.

so you want a sequence to have its own interval which can be modified by a relative \
speed in the overlay painter?


- Sebastian


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


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