Differential D23538 Diff 66588 src/panels/information/informationpanelcontent.cpp

Changeset View

Standalone View

View Options

src/panels/information/informationpanelcontent.cpp

Show First 20 Lines • Show All 93 Lines • ▼ Show 20 Line(s)73{
94 m_phononWidget = new PhononWidget(parent); 94​ m_phononWidget = new PhononWidget(parent);
95 m_phononWidget->hide(); 95​ m_phononWidget->hide();
96 m_phononWidget->setMinimumWidth(minPreviewWidth); 96​ m_phononWidget->setMinimumWidth(minPreviewWidth);
97 m_phononWidget->setAutoPlay(InformationPanelSettings::previewsAutoPlay()); 97​ m_phononWidget->setAutoPlay(InformationPanelSettings::previewsAutoPlay());
98 connect(m_phononWidget, &PhononWidget::hasVideoChanged, 98​ connect(m_phononWidget, &PhononWidget::hasVideoChanged,
99 this, &InformationPanelContent::slotHasVideoChanged); 99​ this, &InformationPanelContent::slotHasVideoChanged);
100 100​
101 // name 101​ // name
102 m_nameLabel = new QLabel(parent); 102​ m_nameLabel = new QLabel(parent);
broulikUnsubmitted
Not Done

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

broulik: Is there a way to get the mime types instead? Format doesn't neccessarily match the part after…
iasensioAuthorUnsubmitted
Not Done

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.
About the std::transform, I have tried but my C++ skills doesn't allow for a "simpler" way to do it.

iasensio: Not necessarily, but it is usual. Currently, the supported formats are just `gif`, `mng` and…
ngrahamUnsubmitted
Not Done

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…
pinoUnsubmitted
Done

The mimetype for MNG images is video/x-mng. not image/mng.
In general, there's nothing that ensures that for the "foo" QMovie type the corresponding mime is image/foo -- it could be very well image/x-foo for example (in case of a non-IANA mimetype).

pino: The mimetype for MNG images is video/x-mng. not image/mng. In general, there's nothing that…
103 QFont font = m_nameLabel->font(); 103​ QFont font = m_nameLabel->font();
104 font.setBold(true); 104​ font.setBold(true);
105 m_nameLabel->setFont(font); 105​ m_nameLabel->setFont(font);
106 m_nameLabel->setTextFormat(Qt::PlainText); 106​ m_nameLabel->setTextFormat(Qt::PlainText);
107 m_nameLabel->setAlignment(Qt::AlignHCenter); 107​ m_nameLabel->setAlignment(Qt::AlignHCenter);
108 m_nameLabel->setSizePolicy(QSizePolicy::Ignored, QSizePolicy::Fixed); 108​ m_nameLabel->setSizePolicy(QSizePolicy::Ignored, QSizePolicy::Fixed);
109 109​
110 const bool previewsShown = InformationPanelSettings::previewsShown(); 110​ const bool previewsShown = InformationPanelSettings::previewsShown();
▲ Show 20 Lines • Show All 53 Lines • ▼ Show 20 Line(s)163{
164 InformationPanelSettings::self()->save(); 164​ InformationPanelSettings::self()->save();
165} 165​}
166 166​
167void InformationPanelContent::showItem(const KFileItem& item) 167​void InformationPanelContent::showItem(const KFileItem& item)
168{ 168​{
169 if (item != m_item) { 169​ if (item != m_item) {
170 m_item = item; 170​ m_item = item;
171 171​
172​ m_preview->stopAnimatedImage();
172 refreshMetaData(); 173​ refreshMetaData();
173 } 174​ }
ngrahamUnsubmitted
Done

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…
174 refreshPreview(); 175​ refreshPreview();
elvisangelaccioUnsubmitted
Done

Please remove this comment, as D23668 will make it redundant.

elvisangelaccio: Please remove this comment, as D23668 will make it redundant.
175} 176​}
176 177​
177void InformationPanelContent::refreshPixmapView() 178​void InformationPanelContent::refreshPixmapView()
178{ 179​{
179 // If there is a preview job, kill it to prevent that we have jobs for 180​ // If there is a preview job, kill it to prevent that we have jobs for
180 // multiple items running, and thus a race condition (bug 250787). 181​ // multiple items running, and thus a race condition (bug 250787).
181 if (m_previewJob) { 182​ if (m_previewJob) {
182 m_previewJob->kill(); 183​ m_previewJob->kill();
Show All 31 Lines214{
214 // If there is a preview job, kill it to prevent that we have jobs for 215​ // If there is a preview job, kill it to prevent that we have jobs for
215 // multiple items running, and thus a race condition (bug 250787). 216​ // multiple items running, and thus a race condition (bug 250787).
216 if (m_previewJob) { 217​ if (m_previewJob) {
217 m_previewJob->kill(); 218​ m_previewJob->kill();
218 } 219​ }
219 220​
220 m_preview->setCursor(Qt::ArrowCursor); 221​ m_preview->setCursor(Qt::ArrowCursor);
221 bool usePhonon = false; 222​ bool usePhonon = false;
222 setNameLabelText(m_item.text()); 223​ setNameLabelText(m_item.text());
elvisangelaccioUnsubmitted
Done

Unrelated whitespace change

elvisangelaccio: Unrelated whitespace change
223 if (InformationPanelSettings::previewsShown()) { 224​ if (InformationPanelSettings::previewsShown()) {
224 225​
225 const QUrl itemUrl = m_item.url(); 226​ const QUrl itemUrl = m_item.url();
226 const bool isSearchUrl = itemUrl.scheme().contains(QLatin1String("search")) && m_item.localPath().isEmpty(); 227​ const bool isSearchUrl = itemUrl.scheme().contains(QLatin1String("search")) && m_item.localPath().isEmpty();
227 if (isSearchUrl) { 228​ if (isSearchUrl) {
228 m_preview->show(); 229​ m_preview->show();
229 230​
230 // in the case of a search-URL the URL is not readable for humans 231​ // in the case of a search-URL the URL is not readable for humans
231 // (at least not useful to show in the Information Panel) 232​ // (at least not useful to show in the Information Panel)
232 m_preview->setPixmap( 233​ m_preview->setPixmap(
233 QIcon::fromTheme(QStringLiteral("nepomuk")).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous) 234​ QIcon::fromTheme(QStringLiteral("nepomuk")).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous)
234 ); 235​ );
235 } else { 236​ } else {
236 237​
237 refreshPixmapView(); 238​ refreshPixmapView();
238 239​
239 const QString mimeType = m_item.mimetype(); 240​ const QString mimeType = m_item.mimetype();
240 m_isVideo = mimeType.startsWith(QLatin1String("video/")); 241​ const bool isAnimatedImage = m_preview->isAnimatedImage(itemUrl.toLocalFile());
242​ m_isVideo = !isAnimatedImage && mimeType.startsWith(QLatin1String("video/"));
mevenUnsubmitted
Done

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…
241 usePhonon = m_isVideo || mimeType.startsWith(QLatin1String("audio/")); 243​ usePhonon = m_isVideo || mimeType.startsWith(QLatin1String("audio/"));
242 244​
243 if (usePhonon) { 245​ if (usePhonon) {
244 // change the cursor of the preview 246​ // change the cursor of the preview
245 m_preview->setCursor(Qt::PointingHandCursor); 247​ m_preview->setCursor(Qt::PointingHandCursor);
mevenUnsubmitted
Not Done

You can add an else if, animatedMimeTypes and usePhonon can't be true at the same time.
Let's make it more explicit.

meven: You can add an `else if`, animatedMimeTypes and usePhonon can't be true at the same time. Let's…
iasensioAuthorUnsubmitted
Not Done

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…
mevenUnsubmitted
Not Done

No the next else after if (usePhonon) { if for if (InformationPanelSettings::previewsShown()) {.
The block if (usePhonon) { has no else block.

meven: No the next else after `if (usePhonon) {` if for `if (InformationPanelSettings::previewsShown…
iasensioAuthorUnsubmitted
Not Done

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…
mevenUnsubmitted
Not Done

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.
The following else (line 278 / 286) is the else block of if (InformationPanelSettings::previewsShown()) {

meven: Sorry my previous comment was not very clear. Currently you have : ``` if (isAnimatedImaget)…
246 m_preview->installEventFilter(m_phononWidget); 248​ m_preview->installEventFilter(m_phononWidget);
247 249​
248 // if the video is playing, has been paused or stopped 250​ // if the video is playing, has been paused or stopped
249 // we don't need to update the preview/phonon widget states 251​ // we don't need to update the preview/phonon widget states
250 // unless the previewed file has changed, 252​ // unless the previewed file has changed,
251 // or the setting previewshown has changed 253​ // or the setting previewshown has changed
252 if ((m_phononWidget->state() != Phonon::State::PlayingState && 254​ if ((m_phononWidget->state() != Phonon::State::PlayingState &&
253 m_phononWidget->state() != Phonon::State::PausedState && 255​ m_phononWidget->state() != Phonon::State::PausedState &&
Show All 9 Lines263 } else {
263 m_preview->show(); 265​ m_preview->show();
264 } 266​ }
265 267​
266 m_phononWidget->show(); 268​ m_phononWidget->show();
267 m_phononWidget->setUrl(m_item.targetUrl(), m_isVideo ? PhononWidget::MediaKind::Video : PhononWidget::MediaKind::Audio); 269​ m_phononWidget->setUrl(m_item.targetUrl(), m_isVideo ? PhononWidget::MediaKind::Video : PhononWidget::MediaKind::Audio);
268 adjustWidgetSizes(parentWidget()->width()); 270​ adjustWidgetSizes(parentWidget()->width());
269 } 271​ }
270 } else { 272​ } else {
273​ if (isAnimatedImage) {
274​ m_preview->setAnimatedImageFileName(itemUrl.toLocalFile());
275​ }
271 // When we don't need it, hide the phonon widget first to avoid flickering 276​ // When we don't need it, hide the phonon widget first to avoid flickering
272 m_phononWidget->hide(); 277​ m_phononWidget->hide();
273 m_preview->show(); 278​ m_preview->show();
274 m_preview->removeEventFilter(m_phononWidget); 279​ m_preview->removeEventFilter(m_phononWidget);
275 m_phononWidget->clearUrl(); 280​ m_phononWidget->clearUrl();
276 } 281​ }
277 } 282​ }
278 } else { 283​ } else {
284​ m_preview->stopAnimatedImage();
279 m_preview->hide(); 285​ m_preview->hide();
280 m_phononWidget->hide(); 286​ m_phononWidget->hide();
281 } 287​ }
282} 288​}
283 289​
284void InformationPanelContent::configureShownProperties() 290​void InformationPanelContent::configureShownProperties()
285{ 291​{
286 m_configureLabel->setVisible(true); 292​ m_configureLabel->setVisible(true);
Show All 11 Lines
298void InformationPanelContent::showItems(const KFileItemList& items) 304​void InformationPanelContent::showItems(const KFileItemList& items)
299{ 305​{
300 // If there is a preview job, kill it to prevent that we have jobs for 306​ // If there is a preview job, kill it to prevent that we have jobs for
301 // multiple items running, and thus a race condition (bug 250787). 307​ // multiple items running, and thus a race condition (bug 250787).
302 if (m_previewJob) { 308​ if (m_previewJob) {
303 m_previewJob->kill(); 309​ m_previewJob->kill();
304 } 310​ }
305 311​
312​ m_preview->stopAnimatedImage();
313​
306 m_preview->setPixmap( 314​ m_preview->setPixmap(
307 QIcon::fromTheme(QStringLiteral("dialog-information")).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous) 315​ QIcon::fromTheme(QStringLiteral("dialog-information")).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous)
308 ); 316​ );
309 setNameLabelText(i18ncp("@label", "%1 item selected", "%1 items selected", items.count())); 317​ setNameLabelText(i18ncp("@label", "%1 item selected", "%1 items selected", items.count()));
310 318​
311 m_metaDataWidget->setItems(items); 319​ m_metaDataWidget->setItems(items);
312 320​
313 m_phononWidget->hide(); 321​ m_phononWidget->hide();
Show All 37 Lines357{
351 QPixmap pixmap = QIcon::fromTheme(item.iconName()).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous); 359​ QPixmap pixmap = QIcon::fromTheme(item.iconName()).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous);
352 KIconLoader::global()->drawOverlays(item.overlays(), pixmap, KIconLoader::Desktop); 360​ KIconLoader::global()->drawOverlays(item.overlays(), pixmap, KIconLoader::Desktop);
353 m_preview->setPixmap(pixmap); 361​ m_preview->setPixmap(pixmap);
354} 362​}
355 363​
356void InformationPanelContent::showPreview(const KFileItem& item, 364​void InformationPanelContent::showPreview(const KFileItem& item,
357 const QPixmap& pixmap) 365​ const QPixmap& pixmap)
358{ 366​{
359 m_outdatedPreviewTimer->stop(); 367​ m_outdatedPreviewTimer->stop();
360 368​
mevenUnsubmitted
Not Done

Please re-add this, it saves compile warning

meven: Please re-add this, it saves compile warning
iasensioAuthorUnsubmitted
Not Done

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…
mevenUnsubmitted
Not Done

Indeed my bad

meven: Indeed my bad
elvisangelaccioUnsubmitted
Done

Then this is an unrelated change, I'll fix in on master.

elvisangelaccio: Then this is an unrelated change, I'll fix in on master.
361 QPixmap p = pixmap; 369​ QPixmap p = pixmap;
362 KIconLoader::global()->drawOverlays(item.overlays(), p, KIconLoader::Desktop); 370​ KIconLoader::global()->drawOverlays(item.overlays(), p, KIconLoader::Desktop);
363 371​
364 if (m_isVideo) { 372​ if (m_isVideo) {
365 // adds a play arrow 373​ // adds a play arrow
366 374​
367 // compute relative pixel positions 375​ // compute relative pixel positions
368 const int zeroX = static_cast<int>(p.width() / 2 - PLAY_ARROW_SIZE / 2 / devicePixelRatio()); 376​ const int zeroX = static_cast<int>(p.width() / 2 - PLAY_ARROW_SIZE / 2 / devicePixelRatio());
▲ Show 20 Lines • Show All 115 Lines • Show Last 20 Lines