Changeset View
Standalone View
src/panels/information/pixmapviewer.cpp
Show All 15 Lines | |||||
16 | * Free Software Foundation, Inc., * | 16 | * Free Software Foundation, Inc., * | ||
17 | * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA * | 17 | * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA * | ||
18 | ***************************************************************************/ | 18 | ***************************************************************************/ | ||
19 | 19 | | |||
20 | #include "pixmapviewer.h" | 20 | #include "pixmapviewer.h" | ||
21 | 21 | | |||
22 | #include <KIconLoader> | 22 | #include <KIconLoader> | ||
23 | 23 | | |||
24 | #include <QImageReader> | ||||
25 | #include <QMovie> | ||||
24 | #include <QPainter> | 26 | #include <QPainter> | ||
25 | #include <QStyle> | 27 | #include <QStyle> | ||
26 | 28 | | |||
elvisangelaccio: Please keep the list of includes sorted. | |||||
27 | PixmapViewer::PixmapViewer(QWidget* parent, Transition transition) : | 29 | PixmapViewer::PixmapViewer(QWidget* parent, Transition transition) : | ||
28 | QWidget(parent), | 30 | QWidget(parent), | ||
31 | m_animatedImage(nullptr), | ||||
29 | m_transition(transition), | 32 | m_transition(transition), | ||
30 | m_animationStep(0), | 33 | m_animationStep(0), | ||
31 | m_sizeHint() | 34 | m_sizeHint(), | ||
35 | m_hasAnimatedImage(false) | ||||
32 | { | 36 | { | ||
33 | setMinimumWidth(KIconLoader::SizeEnormous); | 37 | setMinimumWidth(KIconLoader::SizeEnormous); | ||
34 | setMinimumHeight(KIconLoader::SizeEnormous); | 38 | setMinimumHeight(KIconLoader::SizeEnormous); | ||
35 | 39 | | |||
36 | m_animation.setDuration(150); | 40 | m_animation.setDuration(150); | ||
37 | m_animation.setCurveShape(QTimeLine::LinearCurve); | 41 | m_animation.setCurveShape(QTimeLine::LinearCurve); | ||
38 | 42 | | |||
39 | if (m_transition != NoTransition) { | 43 | if (m_transition != NoTransition) { | ||
40 | connect(&m_animation, &QTimeLine::valueChanged, this, QOverload<>::of(&PixmapViewer::update)); | 44 | connect(&m_animation, &QTimeLine::valueChanged, this, QOverload<>::of(&PixmapViewer::update)); | ||
41 | connect(&m_animation, &QTimeLine::finished, this, &PixmapViewer::checkPendingPixmaps); | 45 | connect(&m_animation, &QTimeLine::finished, this, &PixmapViewer::checkPendingPixmaps); | ||
42 | } | 46 | } | ||
43 | } | 47 | } | ||
44 | 48 | | |||
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() | 49 | PixmapViewer::~PixmapViewer() | ||
46 | { | 50 | { | ||
47 | } | 51 | } | ||
broulik: Not neccessary as you parented it to `this` | |||||
48 | 52 | | |||
49 | void 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 | | |||
59 | // Avoid flicker with static pixmap if an animated image is running | ||||
60 | if (m_animatedImage && m_animatedImage->state() == QMovie::Running) { | ||||
61 | return; | ||||
62 | } | ||||
63 | | ||||
55 | if ((m_transition != NoTransition) && (m_animation.state() == QTimeLine::Running)) { | 64 | if ((m_transition != NoTransition) && (m_animation.state() == QTimeLine::Running)) { | ||
56 | m_pendingPixmaps.enqueue(pixmap); | 65 | m_pendingPixmaps.enqueue(pixmap); | ||
57 | if (m_pendingPixmaps.count() > 5) { | 66 | if (m_pendingPixmaps.count() > 5) { | ||
58 | // don't queue more than 5 pixmaps | 67 | // don't queue more than 5 pixmaps | ||
59 | m_pendingPixmaps.takeFirst(); | 68 | m_pendingPixmaps.takeFirst(); | ||
60 | } | 69 | } | ||
61 | return; | 70 | return; | ||
62 | } | 71 | } | ||
63 | 72 | | |||
64 | m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap; | 73 | m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap; | ||
65 | m_pixmap = pixmap; | 74 | m_pixmap = pixmap; | ||
66 | update(); | 75 | update(); | ||
67 | 76 | | |||
68 | const bool animate = (m_transition != NoTransition) && | 77 | const bool animateTransition = (m_transition != NoTransition) && | ||
69 | (m_pixmap.size() != m_oldPixmap.size()); | 78 | (m_pixmap.size() != m_oldPixmap.size()); | ||
70 | if (animate) { | 79 | if (animateTransition) { | ||
71 | m_animation.start(); | 80 | m_animation.start(); | ||
81 | } else if (m_hasAnimatedImage) { | ||||
82 | // If there is no transition animation but an animatedImage | ||||
83 | // and it is not already running, start animating now | ||||
84 | if (m_animatedImage->state() != QMovie::Running) { | ||||
85 | m_animatedImage->setScaledSize(m_pixmap.size()); | ||||
86 | m_animatedImage->start(); | ||||
87 | } | ||||
72 | } | 88 | } | ||
73 | } | 89 | } | ||
74 | 90 | | |||
75 | void PixmapViewer::setSizeHint(const QSize& size) | 91 | void PixmapViewer::setSizeHint(const QSize& size) | ||
76 | { | 92 | { | ||
93 | if (m_animatedImage && size != m_sizeHint) { | ||||
94 | m_animatedImage->stop(); | ||||
95 | } | ||||
96 | | ||||
77 | m_sizeHint = size; | 97 | m_sizeHint = size; | ||
78 | updateGeometry(); | 98 | updateGeometry(); | ||
79 | } | 99 | } | ||
80 | 100 | | |||
81 | QSize PixmapViewer::sizeHint() const | 101 | QSize PixmapViewer::sizeHint() const | ||
82 | { | 102 | { | ||
83 | return m_sizeHint; | 103 | return m_sizeHint; | ||
84 | } | 104 | } | ||
85 | 105 | | |||
106 | void PixmapViewer::setAnimatedImageFileName(const QString &fileName) | ||||
107 | { | ||||
108 | if (!m_animatedImage) { | ||||
109 | m_animatedImage = new QMovie(this); | ||||
110 | connect(m_animatedImage, &QMovie::frameChanged, this, &PixmapViewer::updateAnimatedImageFrame); | ||||
111 | } | ||||
112 | | ||||
113 | 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… | |||||
114 | m_animatedImage->stop(); | ||||
115 | m_animatedImage->setFileName(fileName); | ||||
116 | } | ||||
117 | | ||||
118 | m_hasAnimatedImage = m_animatedImage->isValid() && (m_animatedImage->frameCount() > 1); | ||||
119 | } | ||||
120 | | ||||
121 | | ||||
ngraham: return `QString()` instead of `QString::null` | |||||
122 | QString PixmapViewer::animatedImageFileName() const | ||||
123 | { | ||||
124 | if (!m_hasAnimatedImage) { | ||||
125 | return QString(); | ||||
126 | } | ||||
127 | return m_animatedImage->fileName(); | ||||
128 | } | ||||
129 | | ||||
86 | void PixmapViewer::paintEvent(QPaintEvent* event) | 130 | void PixmapViewer::paintEvent(QPaintEvent* event) | ||
87 | { | 131 | { | ||
88 | QWidget::paintEvent(event); | 132 | QWidget::paintEvent(event); | ||
89 | 133 | | |||
90 | QPainter painter(this); | 134 | QPainter painter(this); | ||
91 | 135 | | |||
92 | if (m_transition != NoTransition) { | 136 | if (m_transition != NoTransition || (m_hasAnimatedImage && 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(); | 137 | const float value = m_animation.currentValue(); | ||
94 | const int scaledWidth = static_cast<int>((m_oldPixmap.width() * (1.0 - value)) + (m_pixmap.width() * value)); | 138 | 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)); | 139 | const int scaledHeight = static_cast<int>((m_oldPixmap.height() * (1.0 - value)) + (m_pixmap.height() * value)); | ||
96 | 140 | | |||
97 | const bool useOldPixmap = (m_transition == SizeTransition) && | 141 | const bool useOldPixmap = (m_transition == SizeTransition) && | ||
98 | (m_oldPixmap.width() > m_pixmap.width()); | 142 | (m_oldPixmap.width() > m_pixmap.width()); | ||
99 | const QPixmap& largePixmap = useOldPixmap ? m_oldPixmap : m_pixmap; | 143 | const QPixmap& largePixmap = useOldPixmap ? m_oldPixmap : m_pixmap; | ||
100 | if (!largePixmap.isNull()) { | 144 | if (!largePixmap.isNull()) { | ||
Show All 12 Lines | |||||
113 | void PixmapViewer::checkPendingPixmaps() | 157 | void PixmapViewer::checkPendingPixmaps() | ||
114 | { | 158 | { | ||
115 | if (!m_pendingPixmaps.isEmpty()) { | 159 | if (!m_pendingPixmaps.isEmpty()) { | ||
116 | QPixmap pixmap = m_pendingPixmaps.dequeue(); | 160 | QPixmap pixmap = m_pendingPixmaps.dequeue(); | ||
117 | m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap; | 161 | m_oldPixmap = m_pixmap.isNull() ? pixmap : m_pixmap; | ||
118 | m_pixmap = pixmap; | 162 | m_pixmap = pixmap; | ||
119 | update(); | 163 | update(); | ||
120 | m_animation.start(); | 164 | m_animation.start(); | ||
165 | } else if (m_hasAnimatedImage) { | ||||
ngraham: ditto | |||||
166 | m_animatedImage->setScaledSize(m_pixmap.size()); | ||||
167 | m_animatedImage->start(); | ||||
121 | } else { | 168 | } else { | ||
122 | m_oldPixmap = m_pixmap; | 169 | m_oldPixmap = m_pixmap; | ||
123 | } | 170 | } | ||
124 | } | 171 | } | ||
125 | 172 | | |||
173 | void PixmapViewer::updateAnimatedImageFrame() | ||||
174 | { | ||||
175 | Q_ASSERT (m_animatedImage); | ||||
176 | | ||||
177 | m_pixmap = m_animatedImage->currentPixmap(); | ||||
178 | update(); | ||||
179 | } | ||||
180 | | ||||
181 | void PixmapViewer::stopAnimatedImage() | ||||
182 | { | ||||
183 | if (m_hasAnimatedImage) { | ||||
184 | m_animatedImage->stop(); | ||||
185 | m_hasAnimatedImage = false; | ||||
186 | } | ||||
187 | } | ||||
188 | | ||||
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? | |||||
189 | bool PixmapViewer::isAnimatedImage(const QString &fileName) | ||||
190 | { | ||||
191 | const QByteArray imageFormat = QImageReader::imageFormat(fileName); | ||||
192 | return !imageFormat.isEmpty() && QMovie::supportedFormats().contains(imageFormat); | ||||
193 | } |
Please keep the list of includes sorted.