Differential D23538 Diff 64872 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​{
47} 51​}
broulikUnsubmitted
Done

Not neccessary as you parented it to this

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

return QString() instead of QString::null

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

ditto

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