Changeset View
Standalone View
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> | ||||
elvisangelaccio: Please keep the list of includes sorted. | |||||
26 | 27 | | |||
27 | PixmapViewer::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 | | |||
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… | |||||
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… | |||||
I don't think it is necessary. 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… | |||||
45 | PixmapViewer::~PixmapViewer() | 47 | PixmapViewer::~PixmapViewer() | ||
46 | { | 48 | { | ||
47 | } | 49 | } | ||
broulik: Not neccessary as you parented it to `this` | |||||
48 | 50 | | |||
49 | void 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 | | |||
75 | void 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 | | |||
81 | QSize 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) { | ||||
After @ngraham last comment 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 | { | ||||
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 | | ||||
86 | void 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)) { | ||
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… | |||||
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? iasensio: It's a common pattern in plain C. If `m_animatedImage` is evaluated `false`, it doesn't check… | |||||
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 | |||||
113 | void 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()) { | ||||
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::stopAnimatedImage() | ||||
183 | { | ||||
184 | if (m_animatedImage) { | ||||
185 | m_animatedImage->stop(); | ||||
186 | delete m_animatedImage; | ||||
187 | m_animatedImage = nullptr; | ||||
188 | } | ||||
189 | } | ||||
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 | } | ||||
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… |
Please keep the list of includes sorted.