Differential D23538 Diff 64859 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_movie = new QMovie(this);
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…
46​ connect(m_movie, &QMovie::frameChanged, this, &PixmapViewer::updateAnimationFrame);
43} 47​}
44 48​
45PixmapViewer::~PixmapViewer() 49​PixmapViewer::~PixmapViewer()
46{ 50​{
51​ delete m_movie;
broulikUnsubmitted
Done

Not neccessary as you parented it to this

broulik: Not neccessary as you parented it to `this`
47} 52​}
48 53​
49void PixmapViewer::setPixmap(const QPixmap& pixmap) 54​void PixmapViewer::setPixmap(const QPixmap& pixmap)
50{ 55​{
51 if (pixmap.isNull()) { 56​ if (pixmap.isNull()) {
52 return; 57​ return;
53 } 58​ }
54 59​
Show All 23 Lines81{
78 updateGeometry(); 83​ updateGeometry();
79} 84​}
80 85​
81QSize PixmapViewer::sizeHint() const 86​QSize PixmapViewer::sizeHint() const
82{ 87​{
83 return m_sizeHint; 88​ return m_sizeHint;
84} 89​}
85 90​
91​void PixmapViewer::setMovieFileName(const QString &fileName)
92​{
93​ m_movie->stop();
94​ m_movie->setFileName(fileName);
95​}
96​
97​QString PixmapViewer::movieFileName() const
98​{
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…
99​ return m_movie->fileName();
100​}
101​
86void PixmapViewer::paintEvent(QPaintEvent* event) 102​void PixmapViewer::paintEvent(QPaintEvent* event)
87{ 103​{
88 QWidget::paintEvent(event); 104​ QWidget::paintEvent(event);
89 105​
90 QPainter painter(this); 106​ QPainter painter(this);
ngrahamUnsubmitted
Done

return QString() instead of QString::null

ngraham: return `QString()` instead of `QString::null`
91 107​
92 if (m_transition != NoTransition) { 108​ if (m_transition != NoTransition || m_movie->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(); 109​ const float value = m_animation.currentValue();
94 const int scaledWidth = static_cast<int>((m_oldPixmap.width() * (1.0 - value)) + (m_pixmap.width() * value)); 110​ 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)); 111​ const int scaledHeight = static_cast<int>((m_oldPixmap.height() * (1.0 - value)) + (m_pixmap.height() * value));
96 112​
97 const bool useOldPixmap = (m_transition == SizeTransition) && 113​ const bool useOldPixmap = (m_transition == SizeTransition) &&
98 (m_oldPixmap.width() > m_pixmap.width()); 114​ (m_oldPixmap.width() > m_pixmap.width());
99 const QPixmap& largePixmap = useOldPixmap ? m_oldPixmap : m_pixmap; 115​ const QPixmap& largePixmap = useOldPixmap ? m_oldPixmap : m_pixmap;
100 if (!largePixmap.isNull()) { 116​ if (!largePixmap.isNull()) {
Show All 12 Lines
113void PixmapViewer::checkPendingPixmaps() 129​void PixmapViewer::checkPendingPixmaps()
114{ 130​{
115 if (!m_pendingPixmaps.isEmpty()) { 131​ if (!m_pendingPixmaps.isEmpty()) {
116 QPixmap pixmap = m_pendingPixmaps.dequeue(); 132​ QPixmap pixmap = m_pendingPixmaps.dequeue();
117 m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap; 133​ m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap;
118 m_pixmap = pixmap; 134​ m_pixmap = pixmap;
119 update(); 135​ update();
120 m_animation.start(); 136​ m_animation.start();
137​ } else if (m_movie->isValid() && m_movie->frameCount() > 1) {
ngrahamUnsubmitted
Done

ditto

ngraham: ditto
138​ m_movie->setScaledSize(m_pixmap.size());
139​ m_movie->start();
121 } else { 140​ } else {
122 m_oldPixmap = m_pixmap; 141​ m_oldPixmap = m_pixmap;
123 } 142​ }
124} 143​}
125 144​
145​void PixmapViewer::updateAnimationFrame()
146​{
147​ m_pixmap = m_movie->currentPixmap();
148​ update();
149​}
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…
elvisangelaccioUnsubmitted
Done

Is it really necessary to delete the QMovie instance and create another one every time?

elvisangelaccio: Is it really necessary to delete the QMovie instance and create another one every time?