Differential D23538 Diff 64955 src/panels/information/pixmapviewer.cpp

Changeset View

Standalone View

View Options

src/panels/information/pixmapviewer.cpp

Show All 17 Lines
18 ***************************************************************************/ 18​ ***************************************************************************/
19 19​
20#include "pixmapviewer.h" 20​#include "pixmapviewer.h"
21 21​
22#include <KIconLoader> 22​#include <KIconLoader>
23 23​
24#include <QPainter> 24​#include <QPainter>
25#include <QStyle> 25​#include <QStyle>
26​#include <QMovie>
elvisangelaccioUnsubmitted
Done

Please keep the list of includes sorted.

elvisangelaccio: Please keep the list of includes sorted.
26 27​
27PixmapViewer::PixmapViewer(QWidget* parent, Transition transition) : 28​PixmapViewer::PixmapViewer(QWidget* parent, Transition transition) :
28 QWidget(parent), 29​ QWidget(parent),
29 m_transition(transition), 30​ m_transition(transition),
30 m_animationStep(0), 31​ m_animationStep(0),
31 m_sizeHint() 32​ m_sizeHint()
32{ 33​{
33 setMinimumWidth(KIconLoader::SizeEnormous); 34​ setMinimumWidth(KIconLoader::SizeEnormous);
34 setMinimumHeight(KIconLoader::SizeEnormous); 35​ setMinimumHeight(KIconLoader::SizeEnormous);
35 36​
36 m_animation.setDuration(150); 37​ m_animation.setDuration(150);
37 m_animation.setCurveShape(QTimeLine::LinearCurve); 38​ m_animation.setCurveShape(QTimeLine::LinearCurve);
38 39​
39 if (m_transition != NoTransition) { 40​ if (m_transition != NoTransition) {
40 connect(&m_animation, &QTimeLine::valueChanged, this, QOverload<>::of(&PixmapViewer::update)); 41​ connect(&m_animation, &QTimeLine::valueChanged, this, QOverload<>::of(&PixmapViewer::update));
41 connect(&m_animation, &QTimeLine::finished, this, &PixmapViewer::checkPendingPixmaps); 42​ connect(&m_animation, &QTimeLine::finished, this, &PixmapViewer::checkPendingPixmaps);
42 } 43​ }
44​
45​ m_animatedImage = nullptr;
ngrahamUnsubmitted
Done

No need to create a new QMovieObject unless there's actually an animated image loaded; I'd create it on demand and then handle the case where m_movie is null.

ngraham: No need to create a new QMovieObject unless there's actually an animated image loaded; I'd…
iasensioAuthorUnsubmitted
Done

Should it also be destroyed on animation stopping? Or only if a different non-animated item gets loaded? Or that would be an extra penalty in creation/destruction.

iasensio: Should it also be destroyed on animation stopping? Or only if a different non-animated item…
mevenUnsubmitted
Done

I don't think it is necessary.
Once this has been initiated we have good chances that it will serve again.
And keeping it is just a little memory occupied while destroying will use some CPU and some more if and when the animatedImage is used again.

You can replace m_animatedImage = nullptr; here with a declaration in the constructor :

PixmapViewer::PixmapViewer(QWidget* parent, Transition transition) :
    QWidget(parent),
    m_transition(transition),
    m_animationStep(0),
    m_sizeHint(),
    m_animatedImage(nullptr)

I am not even sure it is necessary to declare it at all.

meven: I don't think it is necessary. Once this has been initiated we have good chances that it will…
43} 46​}
44 47​
45PixmapViewer::~PixmapViewer() 48​PixmapViewer::~PixmapViewer()
46{ 49​{
47} 50​}
broulikUnsubmitted
Done

Not neccessary as you parented it to this

broulik: Not neccessary as you parented it to `this`
48 51​
49void PixmapViewer::setPixmap(const QPixmap& pixmap) 52​void PixmapViewer::setPixmap(const QPixmap& pixmap)
50{ 53​{
51 if (pixmap.isNull()) { 54​ if (pixmap.isNull()) {
52 return; 55​ return;
53 } 56​ }
54 57​
55 if ((m_transition != NoTransition) && (m_animation.state() == QTimeLine::Running)) { 58​ if ((m_transition != NoTransition) && (m_animation.state() == QTimeLine::Running)) {
Show All 22 Lines79{
78 updateGeometry(); 81​ updateGeometry();
79} 82​}
80 83​
81QSize PixmapViewer::sizeHint() const 84​QSize PixmapViewer::sizeHint() const
82{ 85​{
83 return m_sizeHint; 86​ return m_sizeHint;
84} 87​}
85 88​
89​void PixmapViewer::setAnimatedImageFileName(const QString &fileName)
90​{
91​ if (!m_animatedImage) {
92​ m_animatedImage = new QMovie(this);
93​ connect(m_animatedImage, &QMovie::frameChanged, this, &PixmapViewer::updateAnimatedImageFrame);
94​ }
95​
96​ m_animatedImage->stop();
mevenUnsubmitted
Done

After @ngraham last comment
Just check the fileName has changed before stopping and re-setting the field

meven: After @ngraham last comment Just check the fileName has changed before stopping and re-setting…
97​ m_animatedImage->setFileName(fileName);
98​}
99​
100​
101​QString PixmapViewer::animatedImageFileName() const
102​{
103​ if (!m_animatedImage) {
104​ return QString();
ngrahamUnsubmitted
Done

return QString() instead of QString::null

ngraham: return `QString()` instead of `QString::null`
105​ }
106​ return m_animatedImage->fileName();
107​}
108​
86void PixmapViewer::paintEvent(QPaintEvent* event) 109​void PixmapViewer::paintEvent(QPaintEvent* event)
87{ 110​{
88 QWidget::paintEvent(event); 111​ QWidget::paintEvent(event);
89 112​
90 QPainter painter(this); 113​ QPainter painter(this);
91 114​
92 if (m_transition != NoTransition) { 115​ if (m_transition != NoTransition || (m_animatedImage && m_animatedImage->state() != QMovie::Running)) {
ngrahamUnsubmitted
Done

Need to move the check for m_animatedImage separately since if it doesn't already exist, the call to m_animatedImage->state() will crash

ngraham: Need to move the check for `m_animatedImage` separately since if it doesn't already exist, the…
iasensioAuthorUnsubmitted
Not Done

It's a common pattern in plain C. If m_animatedImage is evaluated false, it doesn't check the rest of the condition. It doesn't crash now for me but I'm guessing that it's not a good pattern in C++. Any ideas on how to do it while keeping it simple?
Thanks for the patience 😃

iasensio: It's a common pattern in plain C. If `m_animatedImage` is evaluated `false`, it doesn't check…
ngrahamUnsubmitted
Not Done

You're probably right and I'm being dense

ngraham: You're probably right and I'm being dense
93 const float value = m_animation.currentValue(); 116​ const float value = m_animation.currentValue();
94 const int scaledWidth = static_cast<int>((m_oldPixmap.width() * (1.0 - value)) + (m_pixmap.width() * value)); 117​ const int scaledWidth = static_cast<int>((m_oldPixmap.width() * (1.0 - value)) + (m_pixmap.width() * value));
95 const int scaledHeight = static_cast<int>((m_oldPixmap.height() * (1.0 - value)) + (m_pixmap.height() * value)); 118​ const int scaledHeight = static_cast<int>((m_oldPixmap.height() * (1.0 - value)) + (m_pixmap.height() * value));
96 119​
97 const bool useOldPixmap = (m_transition == SizeTransition) && 120​ const bool useOldPixmap = (m_transition == SizeTransition) &&
98 (m_oldPixmap.width() > m_pixmap.width()); 121​ (m_oldPixmap.width() > m_pixmap.width());
99 const QPixmap& largePixmap = useOldPixmap ? m_oldPixmap : m_pixmap; 122​ const QPixmap& largePixmap = useOldPixmap ? m_oldPixmap : m_pixmap;
100 if (!largePixmap.isNull()) { 123​ if (!largePixmap.isNull()) {
Show All 12 Lines
113void PixmapViewer::checkPendingPixmaps() 136​void PixmapViewer::checkPendingPixmaps()
114{ 137​{
115 if (!m_pendingPixmaps.isEmpty()) { 138​ if (!m_pendingPixmaps.isEmpty()) {
116 QPixmap pixmap = m_pendingPixmaps.dequeue(); 139​ QPixmap pixmap = m_pendingPixmaps.dequeue();
117 m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap; 140​ m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap;
118 m_pixmap = pixmap; 141​ m_pixmap = pixmap;
119 update(); 142​ update();
120 m_animation.start(); 143​ m_animation.start();
144​ } else if (m_animatedImage && m_animatedImage->isValid() && m_animatedImage->frameCount() > 1) {
ngrahamUnsubmitted
Done

ditto

ngraham: ditto
145​ m_animatedImage->setScaledSize(m_pixmap.size());
146​ m_animatedImage->start();
121 } else { 147​ } else {
122 m_oldPixmap = m_pixmap; 148​ m_oldPixmap = m_pixmap;
123 } 149​ }
124} 150​}
125 151​
152​void PixmapViewer::updateAnimatedImageFrame()
153​{
154​ Q_ASSERT (m_animatedImage);
155​
156​ m_pixmap = m_animatedImage->currentPixmap();
157​ update();
158​}
159​
160​void PixmapViewer::stopAnimatedImage()
161​{
162​ if (m_animatedImage) {
163​ m_animatedImage->stop();
164​ }
165​}
elvisangelaccioUnsubmitted
Not Done

I'd try to avoid to hardcode the list of supported mimetypes here. QMovie has QMovie::supportedFormats() which tells us the list of supported image formats.

We could create a QImageReader of the selected image and then check its QImageReader::format().

elvisangelaccio: I'd try to avoid to hardcode the list of supported mimetypes here. QMovie has `QMovie…