Differential D23538 Diff 64959 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)) {
56 m_pendingPixmaps.enqueue(pixmap); 59​ m_pendingPixmaps.enqueue(pixmap);
57 if (m_pendingPixmaps.count() > 5) { 60​ if (m_pendingPixmaps.count() > 5) {
58 // don't queue more than 5 pixmaps 61​ // don't queue more than 5 pixmaps
59 m_pendingPixmaps.takeFirst(); 62​ m_pendingPixmaps.takeFirst();
60 } 63​ }
61 return; 64​ return;
62 } 65​ }
63 66​
64 m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap; 67​ m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap;
65 m_pixmap = pixmap; 68​ m_pixmap = pixmap;
66 update(); 69​ update();
67 70​
68 const bool animate = (m_transition != NoTransition) && 71​ const bool animateTransition = (m_transition != NoTransition) &&
69 (m_pixmap.size() != m_oldPixmap.size()); 72​ (m_pixmap.size() != m_oldPixmap.size());
70 if (animate) { 73​ if (animateTransition) {
71 m_animation.start(); 74​ m_animation.start();
75​ } else if (hasAnimatedImage()) {
76​ // If there is no transition animation but an animatedImage, start animating now
77​ m_animatedImage->setScaledSize(m_pixmap.size());
78​ m_animatedImage->start();
72 } 79​ }
73} 80​}
74 81​
75void PixmapViewer::setSizeHint(const QSize& size) 82​void PixmapViewer::setSizeHint(const QSize& size)
76{ 83​{
77 m_sizeHint = size; 84​ m_sizeHint = size;
78 updateGeometry(); 85​ updateGeometry();
79} 86​}
80 87​
81QSize PixmapViewer::sizeHint() const 88​QSize PixmapViewer::sizeHint() const
82{ 89​{
83 return m_sizeHint; 90​ return m_sizeHint;
84} 91​}
85 92​
93​void PixmapViewer::setAnimatedImageFileName(const QString &fileName)
94​{
95​ if (!m_animatedImage) {
96​ m_animatedImage = new QMovie(this);
97​ connect(m_animatedImage, &QMovie::frameChanged, this, &PixmapViewer::updateAnimatedImageFrame);
98​ }
99​
100​ 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…
101​ m_animatedImage->setFileName(fileName);
102​}
103​
104​
105​QString PixmapViewer::animatedImageFileName() const
106​{
107​ if (!m_animatedImage) {
108​ return QString();
ngrahamUnsubmitted
Done

return QString() instead of QString::null

ngraham: return `QString()` instead of `QString::null`
109​ }
110​ return m_animatedImage->fileName();
111​}
112​
113​bool PixmapViewer::hasAnimatedImage()
114​{
115​ return (m_animatedImage && m_animatedImage->isValid() && m_animatedImage->frameCount() > 1);
116​}
117​
86void PixmapViewer::paintEvent(QPaintEvent* event) 118​void PixmapViewer::paintEvent(QPaintEvent* event)
87{ 119​{
88 QWidget::paintEvent(event); 120​ QWidget::paintEvent(event);
89 121​
90 QPainter painter(this); 122​ QPainter painter(this);
91 123​
92 if (m_transition != NoTransition) { 124​ 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(); 125​ const float value = m_animation.currentValue();
94 const int scaledWidth = static_cast<int>((m_oldPixmap.width() * (1.0 - value)) + (m_pixmap.width() * value)); 126​ 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)); 127​ const int scaledHeight = static_cast<int>((m_oldPixmap.height() * (1.0 - value)) + (m_pixmap.height() * value));
96 128​
97 const bool useOldPixmap = (m_transition == SizeTransition) && 129​ const bool useOldPixmap = (m_transition == SizeTransition) &&
98 (m_oldPixmap.width() > m_pixmap.width()); 130​ (m_oldPixmap.width() > m_pixmap.width());
99 const QPixmap& largePixmap = useOldPixmap ? m_oldPixmap : m_pixmap; 131​ const QPixmap& largePixmap = useOldPixmap ? m_oldPixmap : m_pixmap;
100 if (!largePixmap.isNull()) { 132​ if (!largePixmap.isNull()) {
Show All 12 Lines
113void PixmapViewer::checkPendingPixmaps() 145​void PixmapViewer::checkPendingPixmaps()
114{ 146​{
115 if (!m_pendingPixmaps.isEmpty()) { 147​ if (!m_pendingPixmaps.isEmpty()) {
116 QPixmap pixmap = m_pendingPixmaps.dequeue(); 148​ QPixmap pixmap = m_pendingPixmaps.dequeue();
117 m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap; 149​ m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap;
118 m_pixmap = pixmap; 150​ m_pixmap = pixmap;
119 update(); 151​ update();
120 m_animation.start(); 152​ m_animation.start();
153​ } else if (hasAnimatedImage()) {
ngrahamUnsubmitted
Done

ditto

ngraham: ditto
154​ m_animatedImage->setScaledSize(m_pixmap.size());
155​ m_animatedImage->start();
121 } else { 156​ } else {
122 m_oldPixmap = m_pixmap; 157​ m_oldPixmap = m_pixmap;
123 } 158​ }
124} 159​}
125 160​
161​void PixmapViewer::updateAnimatedImageFrame()
162​{
163​ Q_ASSERT (m_animatedImage);
164​
165​ m_pixmap = m_animatedImage->currentPixmap();
166​ update();
167​}
168​
169​void PixmapViewer::stopAnimatedImage()
170​{
171​ if (m_animatedImage) {
172​ m_animatedImage->stop();
173​ delete m_animatedImage;
174​ m_animatedImage = nullptr;
175​ }
176​}
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?