Changeset View
Standalone View
src/panels/information/informationpanelcontent.cpp
Show All 40 Lines | |||||
41 | 41 | | |||
42 | #include <QLabel> | 42 | #include <QLabel> | ||
43 | #include <QDialogButtonBox> | 43 | #include <QDialogButtonBox> | ||
44 | #include <QScrollArea> | 44 | #include <QScrollArea> | ||
45 | #include <QTextLayout> | 45 | #include <QTextLayout> | ||
46 | #include <QTimer> | 46 | #include <QTimer> | ||
47 | #include <QVBoxLayout> | 47 | #include <QVBoxLayout> | ||
48 | #include <QStyle> | 48 | #include <QStyle> | ||
49 | #include <QMovie> | ||||
49 | 50 | | |||
50 | #include "dolphin_informationpanelsettings.h" | 51 | #include "dolphin_informationpanelsettings.h" | ||
51 | #include "phononwidget.h" | 52 | #include "phononwidget.h" | ||
52 | #include "pixmapviewer.h" | 53 | #include "pixmapviewer.h" | ||
53 | 54 | | |||
54 | InformationPanelContent::InformationPanelContent(QWidget* parent) : | 55 | InformationPanelContent::InformationPanelContent(QWidget* parent) : | ||
55 | QWidget(parent), | 56 | QWidget(parent), | ||
56 | m_item(), | 57 | m_item(), | ||
Show All 28 Lines | 66 | { | |||
85 | 86 | | |||
86 | m_phononWidget = new PhononWidget(parent); | 87 | m_phononWidget = new PhononWidget(parent); | ||
87 | m_phononWidget->hide(); | 88 | m_phononWidget->hide(); | ||
88 | m_phononWidget->setMinimumWidth(minPreviewWidth); | 89 | m_phononWidget->setMinimumWidth(minPreviewWidth); | ||
89 | m_phononWidget->setAutoPlay(InformationPanelSettings::previewsAutoPlay()); | 90 | m_phononWidget->setAutoPlay(InformationPanelSettings::previewsAutoPlay()); | ||
90 | connect(m_phononWidget, &PhononWidget::hasVideoChanged, | 91 | connect(m_phononWidget, &PhononWidget::hasVideoChanged, | ||
91 | this, &InformationPanelContent::slotHasVideoChanged); | 92 | this, &InformationPanelContent::slotHasVideoChanged); | ||
92 | 93 | | |||
94 | // mimetypes that can be animated by QMovie (gif/mng/webp) | ||||
95 | for ( const auto format : QMovie::supportedFormats() ) { | ||||
broulik: Is there a way to get the mime types instead? Format doesn't neccessarily match the part after… | |||||
Not necessarily, but it is usual. Currently, the supported formats are just gif, mng and webp, which match their mimetypes. Since there are only 3 of them, maybe we can also simply harcode them, but this would automatically adapt if a new format gets popular and it's included by Qt. iasensio: Not necessarily, but it is usual. Currently, the supported formats are just `gif`, `mng` and… | |||||
I think the current approach is sane. Using std::transform gets messy since there's a type change involved (QByteArray to QString), and it would be less readable than the current approach too. ngraham: I think the current approach is sane. Using `std::transform` gets messy since there's a type… | |||||
The mimetype for MNG images is video/x-mng. not image/mng. pino: The mimetype for MNG images is video/x-mng. not image/mng.
In general, there's nothing that… | |||||
96 | m_animatedMimeTypes << QString("image/%1").arg(QString(format)); | ||||
97 | } | ||||
98 | | ||||
93 | // name | 99 | // name | ||
94 | m_nameLabel = new QLabel(parent); | 100 | m_nameLabel = new QLabel(parent); | ||
95 | QFont font = m_nameLabel->font(); | 101 | QFont font = m_nameLabel->font(); | ||
96 | font.setBold(true); | 102 | font.setBold(true); | ||
97 | m_nameLabel->setFont(font); | 103 | m_nameLabel->setFont(font); | ||
98 | m_nameLabel->setTextFormat(Qt::PlainText); | 104 | m_nameLabel->setTextFormat(Qt::PlainText); | ||
99 | m_nameLabel->setAlignment(Qt::AlignHCenter); | 105 | m_nameLabel->setAlignment(Qt::AlignHCenter); | ||
100 | m_nameLabel->setSizePolicy(QSizePolicy::Ignored, QSizePolicy::Fixed); | 106 | m_nameLabel->setSizePolicy(QSizePolicy::Ignored, QSizePolicy::Fixed); | ||
▲ Show 20 Lines • Show All 56 Lines • ▼ Show 20 Line(s) | |||||
157 | } | 163 | } | ||
158 | 164 | | |||
159 | void InformationPanelContent::showItem(const KFileItem& item) | 165 | void InformationPanelContent::showItem(const KFileItem& item) | ||
160 | { | 166 | { | ||
161 | if (item != m_item) { | 167 | if (item != m_item) { | ||
162 | m_item = item; | 168 | m_item = item; | ||
163 | 169 | | |||
164 | refreshMetaData(); | 170 | refreshMetaData(); | ||
165 | } | 171 | } | ||
This change will result in the preview being refreshed a lot more, even if it's not an animated image. Can you find a way to do what you're trying to do without changing this? ngraham: This change will result in the preview being refreshed a lot more, even if it's not an animated… | |||||
166 | refreshPreview(); | 172 | refreshPreview(); | ||
Please remove this comment, as D23668 will make it redundant. elvisangelaccio: Please remove this comment, as D23668 will make it redundant. | |||||
167 | } | 173 | } | ||
168 | 174 | | |||
169 | void InformationPanelContent::refreshPreview() | 175 | void InformationPanelContent::refreshPreview() | ||
170 | { | 176 | { | ||
171 | // If there is a preview job, kill it to prevent that we have jobs for | 177 | // If there is a preview job, kill it to prevent that we have jobs for | ||
172 | // multiple items running, and thus a race condition (bug 250787). | 178 | // multiple items running, and thus a race condition (bug 250787). | ||
173 | if (m_previewJob) { | 179 | if (m_previewJob) { | ||
174 | m_previewJob->kill(); | 180 | m_previewJob->kill(); | ||
175 | } | 181 | } | ||
176 | 182 | | |||
183 | if (!m_preview->movieFileName().isEmpty()) { | ||||
184 | m_preview->setMovieFileName(QString::null); | ||||
185 | } | ||||
186 | | ||||
177 | setNameLabelText(m_item.text()); | 187 | setNameLabelText(m_item.text()); | ||
178 | if (InformationPanelSettings::previewsShown()) { | 188 | if (InformationPanelSettings::previewsShown()) { | ||
179 | 189 | | |||
180 | const QUrl itemUrl = m_item.url(); | 190 | const QUrl itemUrl = m_item.url(); | ||
181 | const bool isSearchUrl = itemUrl.scheme().contains(QStringLiteral("search")) && m_item.localPath().isEmpty(); | 191 | const bool isSearchUrl = itemUrl.scheme().contains(QStringLiteral("search")) && m_item.localPath().isEmpty(); | ||
182 | if (isSearchUrl) { | 192 | if (isSearchUrl) { | ||
183 | m_preview->show(); | 193 | m_preview->show(); | ||
184 | 194 | | |||
Show All 23 Lines | 200 | } else { | |||
208 | if (m_previewJob->uiDelegate()) { | 218 | if (m_previewJob->uiDelegate()) { | ||
209 | KJobWidgets::setWindow(m_previewJob, this); | 219 | KJobWidgets::setWindow(m_previewJob, this); | ||
210 | } | 220 | } | ||
211 | 221 | | |||
212 | connect(m_previewJob.data(), &KIO::PreviewJob::gotPreview, | 222 | connect(m_previewJob.data(), &KIO::PreviewJob::gotPreview, | ||
213 | this, &InformationPanelContent::showPreview); | 223 | this, &InformationPanelContent::showPreview); | ||
214 | connect(m_previewJob.data(), &KIO::PreviewJob::failed, | 224 | connect(m_previewJob.data(), &KIO::PreviewJob::failed, | ||
215 | this, &InformationPanelContent::showIcon); | 225 | this, &InformationPanelContent::showIcon); | ||
216 | 226 | | |||
elvisangelaccio: Unrelated whitespace change | |||||
217 | const QString mimeType = m_item.mimetype(); | 227 | const QString mimeType = m_item.mimetype(); | ||
218 | const bool isVideo = mimeType.startsWith(QLatin1String("video/")); | 228 | const bool isVideo = mimeType.startsWith(QLatin1String("video/")); | ||
A small optimization : invert the two condition, isAnimatedImage has already been computed and if false, mimeType.startsWith(QLatin1String("video/")) won't be run at all. meven: A small optimization : invert the two condition, isAnimatedImage has already been computed and… | |||||
219 | const bool usePhonon = mimeType.startsWith(QLatin1String("audio/")) || isVideo; | 229 | const bool usePhonon = mimeType.startsWith(QLatin1String("audio/")) || isVideo; | ||
220 | 230 | | |||
231 | if (m_animatedMimeTypes.contains(mimeType)) { | ||||
232 | m_preview->setMovieFileName(itemUrl.toLocalFile()); | ||||
233 | } | ||||
You can add an else if, animatedMimeTypes and usePhonon can't be true at the same time. meven: You can add an `else if`, animatedMimeTypes and usePhonon can't be true at the same time.
Let's… | |||||
It is mutually exclusive with usePhonon but not with the else clause afterwards. I could move this block to there, if that makes it more clear. iasensio: It is mutually exclusive with `usePhonon` but not with the `else` clause afterwards. I could… | |||||
No the next else after if (usePhonon) { if for if (InformationPanelSettings::previewsShown()) {. meven: No the next else after `if (usePhonon) {` if for `if (InformationPanelSettings::previewsShown… | |||||
Sorry, but I don't get it. I mean the else in line 278 (270 before the patch). I hope to have rebased correctly. iasensio: Sorry, but I don't get it. I mean the `else` in line 278 (270 before the patch). I hope to have… | |||||
Sorry my previous comment was not very clear. Currently you have : if (isAnimatedImaget) {} if (isPhonon){} I suggest to have if (isAnimatedImaget) {} else if (isPhonon){} If you look at the code carefully you will see if (isPhonon){} has no else block. meven: Sorry my previous comment was not very clear.
Currently you have :
```
if (isAnimatedImaget)… | |||||
234 | | ||||
221 | if (usePhonon) { | 235 | if (usePhonon) { | ||
222 | 236 | | |||
223 | if (InformationPanelSettings::previewsAutoPlay() && isVideo) { | 237 | if (InformationPanelSettings::previewsAutoPlay() && isVideo) { | ||
224 | // hides the preview now to avoid flickering when the autoplay video starts | 238 | // hides the preview now to avoid flickering when the autoplay video starts | ||
225 | m_preview->hide(); | 239 | m_preview->hide(); | ||
226 | } else { | 240 | } else { | ||
227 | // the video won't play before the preview is displayed | 241 | // the video won't play before the preview is displayed | ||
228 | m_preview->show(); | 242 | m_preview->show(); | ||
▲ Show 20 Lines • Show All 85 Lines • ▼ Show 20 Line(s) | 325 | { | |||
314 | KIconLoader::global()->drawOverlays(item.overlays(), pixmap, KIconLoader::Desktop); | 328 | KIconLoader::global()->drawOverlays(item.overlays(), pixmap, KIconLoader::Desktop); | ||
315 | m_preview->setPixmap(pixmap); | 329 | m_preview->setPixmap(pixmap); | ||
316 | } | 330 | } | ||
317 | 331 | | |||
318 | void InformationPanelContent::showPreview(const KFileItem& item, | 332 | void InformationPanelContent::showPreview(const KFileItem& item, | ||
319 | const QPixmap& pixmap) | 333 | const QPixmap& pixmap) | ||
320 | { | 334 | { | ||
321 | m_outdatedPreviewTimer->stop(); | 335 | m_outdatedPreviewTimer->stop(); | ||
322 | Q_UNUSED(item); | | |||
meven: Please re-add this, it saves compile warning | |||||
Sorry if I'm wrong, but it shouldn't give any warning, since item is used below in the function. iasensio: Sorry if I'm wrong, but it shouldn't give any warning, since `item` is used below in the… | |||||
meven: Indeed my bad | |||||
elvisangelaccio: Then this is an unrelated change, I'll fix in on master. | |||||
323 | 336 | | |||
324 | QPixmap p = pixmap; | 337 | QPixmap p = pixmap; | ||
325 | KIconLoader::global()->drawOverlays(item.overlays(), p, KIconLoader::Desktop); | 338 | KIconLoader::global()->drawOverlays(item.overlays(), p, KIconLoader::Desktop); | ||
326 | m_preview->setPixmap(p); | 339 | m_preview->setPixmap(p); | ||
327 | } | 340 | } | ||
328 | 341 | | |||
329 | void InformationPanelContent::markOutdatedPreview() | 342 | void InformationPanelContent::markOutdatedPreview() | ||
330 | { | 343 | { | ||
▲ Show 20 Lines • Show All 75 Lines • Show Last 20 Lines |
Is there a way to get the mime types instead? Format doesn't neccessarily match the part after "image/", does it?
You can also probably use a std::transform to turn that one list into the other one