Differential D23538 Diff 66005 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),
30​ m_animatedImage(nullptr),
29 m_transition(transition), 31​ m_transition(transition),
30 m_animationStep(0), 32​ m_animationStep(0),
31 m_sizeHint() 33​ m_sizeHint()
32{ 34​{
33 setMinimumWidth(KIconLoader::SizeEnormous); 35​ setMinimumWidth(KIconLoader::SizeEnormous);
34 setMinimumHeight(KIconLoader::SizeEnormous); 36​ setMinimumHeight(KIconLoader::SizeEnormous);
35 37​
36 m_animation.setDuration(150); 38​ m_animation.setDuration(150);
37 m_animation.setCurveShape(QTimeLine::LinearCurve); 39​ m_animation.setCurveShape(QTimeLine::LinearCurve);
38 40​
39 if (m_transition != NoTransition) { 41​ if (m_transition != NoTransition) {
40 connect(&m_animation, &QTimeLine::valueChanged, this, QOverload<>::of(&PixmapViewer::update)); 42​ connect(&m_animation, &QTimeLine::valueChanged, this, QOverload<>::of(&PixmapViewer::update));
41 connect(&m_animation, &QTimeLine::finished, this, &PixmapViewer::checkPendingPixmaps); 43​ connect(&m_animation, &QTimeLine::finished, this, &PixmapViewer::checkPendingPixmaps);
42 } 44​ }
43} 45​}
44 46​
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…
45PixmapViewer::~PixmapViewer() 47​PixmapViewer::~PixmapViewer()
46{ 48​{
47} 49​}
broulikUnsubmitted
Done

Not neccessary as you parented it to this

broulik: Not neccessary as you parented it to `this`
48 50​
49void PixmapViewer::setPixmap(const QPixmap& pixmap) 51​void PixmapViewer::setPixmap(const QPixmap& pixmap)
50{ 52​{
51 if (pixmap.isNull()) { 53​ if (pixmap.isNull()) {
52 return; 54​ return;
53 } 55​ }
54 56​
57​ // Avoid flicker with static pixmap if an animated image is running
58​ if (m_animatedImage && m_animatedImage->state() == QMovie::Running) {
59​ return;
60​ }
61​
55 if ((m_transition != NoTransition) && (m_animation.state() == QTimeLine::Running)) { 62​ if ((m_transition != NoTransition) && (m_animation.state() == QTimeLine::Running)) {
56 m_pendingPixmaps.enqueue(pixmap); 63​ m_pendingPixmaps.enqueue(pixmap);
57 if (m_pendingPixmaps.count() > 5) { 64​ if (m_pendingPixmaps.count() > 5) {
58 // don't queue more than 5 pixmaps 65​ // don't queue more than 5 pixmaps
59 m_pendingPixmaps.takeFirst(); 66​ m_pendingPixmaps.takeFirst();
60 } 67​ }
61 return; 68​ return;
62 } 69​ }
63 70​
64 m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap; 71​ m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap;
65 m_pixmap = pixmap; 72​ m_pixmap = pixmap;
66 update(); 73​ update();
67 74​
68 const bool animate = (m_transition != NoTransition) && 75​ const bool animateTransition = (m_transition != NoTransition) &&
69 (m_pixmap.size() != m_oldPixmap.size()); 76​ (m_pixmap.size() != m_oldPixmap.size());
70 if (animate) { 77​ if (animateTransition) {
71 m_animation.start(); 78​ m_animation.start();
79​ } else if (hasAnimatedImage()) {
80​ // If there is no transition animation but an animatedImage
81​ // and it is not already running, start animating now
82​ if (m_animatedImage->state() != QMovie::Running) {
83​ m_animatedImage->setScaledSize(m_pixmap.size());
84​ m_animatedImage->start();
85​ }
72 } 86​ }
73} 87​}
74 88​
75void PixmapViewer::setSizeHint(const QSize& size) 89​void PixmapViewer::setSizeHint(const QSize& size)
76{ 90​{
91​ if (m_animatedImage && size != m_sizeHint) {
92​ m_animatedImage->stop();
93​ }
94​
77 m_sizeHint = size; 95​ m_sizeHint = size;
78 updateGeometry(); 96​ updateGeometry();
79} 97​}
80 98​
81QSize PixmapViewer::sizeHint() const 99​QSize PixmapViewer::sizeHint() const
82{ 100​{
83 return m_sizeHint; 101​ return m_sizeHint;
84} 102​}
85 103​
104​void PixmapViewer::setAnimatedImageFileName(const QString &fileName)
105​{
106​ if (!m_animatedImage) {
107​ m_animatedImage = new QMovie(this);
108​ connect(m_animatedImage, &QMovie::frameChanged, this, &PixmapViewer::updateAnimatedImageFrame);
109​ }
110​
111​ if (m_animatedImage->fileName() != fileName) {
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…
112​ m_animatedImage->stop();
113​ m_animatedImage->setFileName(fileName);
114​ }
115​}
116​
117​
118​QString PixmapViewer::animatedImageFileName() const
119​{
ngrahamUnsubmitted
Done

return QString() instead of QString::null

ngraham: return `QString()` instead of `QString::null`
120​ if (!m_animatedImage) {
121​ return QString();
122​ }
123​ return m_animatedImage->fileName();
124​}
125​
126​bool PixmapViewer::hasAnimatedImage()
127​{
128​ return (m_animatedImage && m_animatedImage->isValid() && m_animatedImage->frameCount() > 1);
129​}
130​
86void PixmapViewer::paintEvent(QPaintEvent* event) 131​void PixmapViewer::paintEvent(QPaintEvent* event)
87{ 132​{
88 QWidget::paintEvent(event); 133​ QWidget::paintEvent(event);
89 134​
90 QPainter painter(this); 135​ QPainter painter(this);
91 136​
92 if (m_transition != NoTransition) { 137​ 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(); 138​ const float value = m_animation.currentValue();
94 const int scaledWidth = static_cast<int>((m_oldPixmap.width() * (1.0 - value)) + (m_pixmap.width() * value)); 139​ 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)); 140​ const int scaledHeight = static_cast<int>((m_oldPixmap.height() * (1.0 - value)) + (m_pixmap.height() * value));
96 141​
97 const bool useOldPixmap = (m_transition == SizeTransition) && 142​ const bool useOldPixmap = (m_transition == SizeTransition) &&
98 (m_oldPixmap.width() > m_pixmap.width()); 143​ (m_oldPixmap.width() > m_pixmap.width());
99 const QPixmap& largePixmap = useOldPixmap ? m_oldPixmap : m_pixmap; 144​ const QPixmap& largePixmap = useOldPixmap ? m_oldPixmap : m_pixmap;
100 if (!largePixmap.isNull()) { 145​ if (!largePixmap.isNull()) {
Show All 12 Lines
113void PixmapViewer::checkPendingPixmaps() 158​void PixmapViewer::checkPendingPixmaps()
114{ 159​{
115 if (!m_pendingPixmaps.isEmpty()) { 160​ if (!m_pendingPixmaps.isEmpty()) {
116 QPixmap pixmap = m_pendingPixmaps.dequeue(); 161​ QPixmap pixmap = m_pendingPixmaps.dequeue();
117 m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap; 162​ m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap;
118 m_pixmap = pixmap; 163​ m_pixmap = pixmap;
119 update(); 164​ update();
120 m_animation.start(); 165​ m_animation.start();
166​ } else if (hasAnimatedImage()) {
ngrahamUnsubmitted
Done

ditto

ngraham: ditto
167​ m_animatedImage->setScaledSize(m_pixmap.size());
168​ m_animatedImage->start();
121 } else { 169​ } else {
122 m_oldPixmap = m_pixmap; 170​ m_oldPixmap = m_pixmap;
123 } 171​ }
124} 172​}
125 173​
174​void PixmapViewer::updateAnimatedImageFrame()
175​{
176​ Q_ASSERT (m_animatedImage);
177​
178​ m_pixmap = m_animatedImage->currentPixmap();
179​ update();
180​}
181​
182​void PixmapViewer::clearAnimatedImage()
183​{
184​ if (m_animatedImage) {
185​ m_animatedImage->stop();
186​ delete m_animatedImage;
187​ m_animatedImage = nullptr;
188​ }
189​}
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?
190​
191​const QStringList PixmapViewer::animatedMimeTypes()
192​{
193​ return QStringList() << QStringLiteral("image/gif")
194​ << QStringLiteral("image/webp")
195​ << QStringLiteral("video/x-mng");
196​}
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…