Changeset View
Standalone View
src/panels/information/informationpanelcontent.cpp
Show First 20 Lines • Show All 85 Lines • ▼ Show 20 Line(s) | 65 | { | |||
---|---|---|---|---|---|
86 | m_phononWidget = new PhononWidget(parent); | 86 | m_phononWidget = new PhononWidget(parent); | ||
87 | m_phononWidget->hide(); | 87 | m_phononWidget->hide(); | ||
88 | m_phononWidget->setMinimumWidth(minPreviewWidth); | 88 | m_phononWidget->setMinimumWidth(minPreviewWidth); | ||
89 | m_phononWidget->setAutoPlay(InformationPanelSettings::previewsAutoPlay()); | 89 | m_phononWidget->setAutoPlay(InformationPanelSettings::previewsAutoPlay()); | ||
90 | connect(m_phononWidget, &PhononWidget::hasVideoChanged, | 90 | connect(m_phononWidget, &PhononWidget::hasVideoChanged, | ||
91 | this, &InformationPanelContent::slotHasVideoChanged); | 91 | this, &InformationPanelContent::slotHasVideoChanged); | ||
92 | 92 | | |||
93 | // name | 93 | // name | ||
94 | m_nameLabel = new QLabel(parent); | 94 | m_nameLabel = new QLabel(parent); | ||
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… | |||||
95 | QFont font = m_nameLabel->font(); | 95 | QFont font = m_nameLabel->font(); | ||
96 | font.setBold(true); | 96 | font.setBold(true); | ||
97 | m_nameLabel->setFont(font); | 97 | m_nameLabel->setFont(font); | ||
98 | m_nameLabel->setTextFormat(Qt::PlainText); | 98 | m_nameLabel->setTextFormat(Qt::PlainText); | ||
99 | m_nameLabel->setAlignment(Qt::AlignHCenter); | 99 | m_nameLabel->setAlignment(Qt::AlignHCenter); | ||
100 | m_nameLabel->setSizePolicy(QSizePolicy::Ignored, QSizePolicy::Fixed); | 100 | m_nameLabel->setSizePolicy(QSizePolicy::Ignored, QSizePolicy::Fixed); | ||
101 | 101 | | |||
102 | const bool previewsShown = InformationPanelSettings::previewsShown(); | 102 | const bool previewsShown = InformationPanelSettings::previewsShown(); | ||
▲ Show 20 Lines • Show All 53 Lines • ▼ Show 20 Line(s) | 155 | { | |||
156 | InformationPanelSettings::self()->save(); | 156 | InformationPanelSettings::self()->save(); | ||
157 | } | 157 | } | ||
158 | 158 | | |||
159 | void InformationPanelContent::showItem(const KFileItem& item) | 159 | void InformationPanelContent::showItem(const KFileItem& item) | ||
160 | { | 160 | { | ||
161 | if (item != m_item) { | 161 | if (item != m_item) { | ||
162 | m_item = item; | 162 | m_item = item; | ||
163 | 163 | | |||
164 | m_preview->stopAnimatedImage(); | ||||
164 | refreshMetaData(); | 165 | refreshMetaData(); | ||
165 | } | 166 | } | ||
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… | |||||
167 | // Refresh even on same item to handle resizing events | ||||
Please remove this comment, as D23668 will make it redundant. elvisangelaccio: Please remove this comment, as D23668 will make it redundant. | |||||
166 | refreshPreview(); | 168 | refreshPreview(); | ||
167 | } | 169 | } | ||
168 | 170 | | |||
169 | void InformationPanelContent::refreshPreview() | 171 | void InformationPanelContent::refreshPreview() | ||
170 | { | 172 | { | ||
171 | // If there is a preview job, kill it to prevent that we have jobs for | 173 | // 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). | 174 | // multiple items running, and thus a race condition (bug 250787). | ||
173 | if (m_previewJob) { | 175 | if (m_previewJob) { | ||
Show All 34 Lines | 192 | } else { | |||
208 | if (m_previewJob->uiDelegate()) { | 210 | if (m_previewJob->uiDelegate()) { | ||
209 | KJobWidgets::setWindow(m_previewJob, this); | 211 | KJobWidgets::setWindow(m_previewJob, this); | ||
210 | } | 212 | } | ||
211 | 213 | | |||
212 | connect(m_previewJob.data(), &KIO::PreviewJob::gotPreview, | 214 | connect(m_previewJob.data(), &KIO::PreviewJob::gotPreview, | ||
213 | this, &InformationPanelContent::showPreview); | 215 | this, &InformationPanelContent::showPreview); | ||
214 | connect(m_previewJob.data(), &KIO::PreviewJob::failed, | 216 | connect(m_previewJob.data(), &KIO::PreviewJob::failed, | ||
215 | this, &InformationPanelContent::showIcon); | 217 | this, &InformationPanelContent::showIcon); | ||
216 | 218 | | |||
elvisangelaccio: Unrelated whitespace change | |||||
217 | const QString mimeType = m_item.mimetype(); | 219 | const QString mimeType = m_item.mimetype(); | ||
218 | const bool isVideo = mimeType.startsWith(QLatin1String("video/")); | 220 | const bool isAnimatedImage = m_preview->animatedMimeTypes().contains(mimeType); | ||
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… | |||||
221 | const bool isVideo = mimeType.startsWith(QLatin1String("video/")) && !isAnimatedImage; | ||||
219 | const bool usePhonon = mimeType.startsWith(QLatin1String("audio/")) || isVideo; | 222 | const bool usePhonon = mimeType.startsWith(QLatin1String("audio/")) || isVideo; | ||
220 | 223 | | |||
224 | if (isAnimatedImage) { | ||||
225 | m_preview->setAnimatedImageFileName(itemUrl.toLocalFile()); | ||||
226 | } | ||||
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)… | |||||
227 | | ||||
221 | if (usePhonon) { | 228 | if (usePhonon) { | ||
222 | 229 | | |||
223 | if (InformationPanelSettings::previewsAutoPlay() && isVideo) { | 230 | if (InformationPanelSettings::previewsAutoPlay() && isVideo) { | ||
224 | // hides the preview now to avoid flickering when the autoplay video starts | 231 | // hides the preview now to avoid flickering when the autoplay video starts | ||
225 | m_preview->hide(); | 232 | m_preview->hide(); | ||
226 | } else { | 233 | } else { | ||
227 | // the video won't play before the preview is displayed | 234 | // the video won't play before the preview is displayed | ||
228 | m_preview->show(); | 235 | m_preview->show(); | ||
229 | } | 236 | } | ||
230 | 237 | | |||
231 | m_phononWidget->show(); | 238 | m_phononWidget->show(); | ||
232 | m_phononWidget->setUrl(m_item.targetUrl(), isVideo ? PhononWidget::MediaKind::Video : PhononWidget::MediaKind::Audio); | 239 | m_phononWidget->setUrl(m_item.targetUrl(), isVideo ? PhononWidget::MediaKind::Video : PhononWidget::MediaKind::Audio); | ||
233 | m_phononWidget->setVideoSize(m_preview->size()); | 240 | m_phononWidget->setVideoSize(m_preview->size()); | ||
234 | } else { | 241 | } else { | ||
235 | // When we don't need it, hide the phonon widget first to avoid flickering | 242 | // When we don't need it, hide the phonon widget first to avoid flickering | ||
236 | m_phononWidget->hide(); | 243 | m_phononWidget->hide(); | ||
237 | m_preview->show(); | 244 | m_preview->show(); | ||
238 | } | 245 | } | ||
239 | } | 246 | } | ||
240 | } else { | 247 | } else { | ||
248 | m_preview->stopAnimatedImage(); | ||||
241 | m_preview->hide(); | 249 | m_preview->hide(); | ||
242 | m_phononWidget->hide(); | 250 | m_phononWidget->hide(); | ||
243 | } | 251 | } | ||
244 | } | 252 | } | ||
245 | 253 | | |||
246 | void InformationPanelContent::configureShownProperties() | 254 | void InformationPanelContent::configureShownProperties() | ||
247 | { | 255 | { | ||
248 | m_configureLabel->setVisible(true); | 256 | m_configureLabel->setVisible(true); | ||
Show All 11 Lines | |||||
260 | void InformationPanelContent::showItems(const KFileItemList& items) | 268 | void InformationPanelContent::showItems(const KFileItemList& items) | ||
261 | { | 269 | { | ||
262 | // If there is a preview job, kill it to prevent that we have jobs for | 270 | // If there is a preview job, kill it to prevent that we have jobs for | ||
263 | // multiple items running, and thus a race condition (bug 250787). | 271 | // multiple items running, and thus a race condition (bug 250787). | ||
264 | if (m_previewJob) { | 272 | if (m_previewJob) { | ||
265 | m_previewJob->kill(); | 273 | m_previewJob->kill(); | ||
266 | } | 274 | } | ||
267 | 275 | | |||
276 | m_preview->stopAnimatedImage(); | ||||
277 | | ||||
268 | m_preview->setPixmap( | 278 | m_preview->setPixmap( | ||
269 | QIcon::fromTheme(QStringLiteral("dialog-information")).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous) | 279 | QIcon::fromTheme(QStringLiteral("dialog-information")).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous) | ||
270 | ); | 280 | ); | ||
271 | setNameLabelText(i18ncp("@label", "%1 item selected", "%1 items selected", items.count())); | 281 | setNameLabelText(i18ncp("@label", "%1 item selected", "%1 items selected", items.count())); | ||
272 | 282 | | |||
273 | m_metaDataWidget->setItems(items); | 283 | m_metaDataWidget->setItems(items); | ||
274 | 284 | | |||
275 | m_phononWidget->hide(); | 285 | m_phononWidget->hide(); | ||
Show All 38 Lines | 321 | { | |||
314 | KIconLoader::global()->drawOverlays(item.overlays(), pixmap, KIconLoader::Desktop); | 324 | KIconLoader::global()->drawOverlays(item.overlays(), pixmap, KIconLoader::Desktop); | ||
315 | m_preview->setPixmap(pixmap); | 325 | m_preview->setPixmap(pixmap); | ||
316 | } | 326 | } | ||
317 | 327 | | |||
318 | void InformationPanelContent::showPreview(const KFileItem& item, | 328 | void InformationPanelContent::showPreview(const KFileItem& item, | ||
319 | const QPixmap& pixmap) | 329 | const QPixmap& pixmap) | ||
320 | { | 330 | { | ||
321 | m_outdatedPreviewTimer->stop(); | 331 | 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 | 332 | | |||
324 | QPixmap p = pixmap; | 333 | QPixmap p = pixmap; | ||
325 | KIconLoader::global()->drawOverlays(item.overlays(), p, KIconLoader::Desktop); | 334 | KIconLoader::global()->drawOverlays(item.overlays(), p, KIconLoader::Desktop); | ||
326 | m_preview->setPixmap(p); | 335 | m_preview->setPixmap(p); | ||
327 | } | 336 | } | ||
328 | 337 | | |||
329 | void InformationPanelContent::markOutdatedPreview() | 338 | void InformationPanelContent::markOutdatedPreview() | ||
330 | { | 339 | { | ||
▲ 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