Differential D23538 Diff 65139 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->clearAnimatedImage();
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…
175​ // Refresh even on same item to handle resizing events
elvisangelaccioUnsubmitted
Done

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

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

Unrelated whitespace change

elvisangelaccio: Unrelated whitespace change
222 setNameLabelText(m_item.text()); 225​ setNameLabelText(m_item.text());
223 if (InformationPanelSettings::previewsShown()) { 226​ if (InformationPanelSettings::previewsShown()) {
224 227​
225 const QUrl itemUrl = m_item.url(); 228​ const QUrl itemUrl = m_item.url();
226 const bool isSearchUrl = itemUrl.scheme().contains(QStringLiteral("search")) && m_item.localPath().isEmpty(); 229​ const bool isSearchUrl = itemUrl.scheme().contains(QStringLiteral("search")) && m_item.localPath().isEmpty();
227 if (isSearchUrl) { 230​ if (isSearchUrl) {
228 m_preview->show(); 231​ m_preview->show();
229 232​
230 // in the case of a search-URL the URL is not readable for humans 233​ // 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) 234​ // (at least not useful to show in the Information Panel)
232 m_preview->setPixmap( 235​ m_preview->setPixmap(
233 QIcon::fromTheme(QStringLiteral("nepomuk")).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous) 236​ QIcon::fromTheme(QStringLiteral("nepomuk")).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous)
234 ); 237​ );
235 } else { 238​ } else {
236 239​
237 refreshPixmapView(); 240​ refreshPixmapView();
238 241​
239 const QString mimeType = m_item.mimetype(); 242​ const QString mimeType = m_item.mimetype();
240 m_isVideo = mimeType.startsWith(QLatin1String("video/")); 243​ const bool isAnimatedImage = m_preview->animatedMimeTypes().contains(mimeType);
244​ m_isVideo = mimeType.startsWith(QLatin1String("video/")) && !isAnimatedImage;
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/")); 245​ usePhonon = m_isVideo || mimeType.startsWith(QLatin1String("audio/"));
242 246​
247​ if (isAnimatedImage) {
248​ m_preview->setAnimatedImageFileName(itemUrl.toLocalFile());
249​ }
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)…
250​
243 if (usePhonon) { 251​ if (usePhonon) {
244 // change the cursor of the preview 252​ // change the cursor of the preview
245 m_preview->setCursor(Qt::PointingHandCursor); 253​ m_preview->setCursor(Qt::PointingHandCursor);
246 m_preview->installEventFilter(m_phononWidget); 254​ m_preview->installEventFilter(m_phononWidget);
247 255​
248 // if the video is playing, has been paused or stopped 256​ // if the video is playing, has been paused or stopped
249 // we don't need to update the preview/phonon widget states 257​ // we don't need to update the preview/phonon widget states
250 // unless the previewed file has changed, 258​ // unless the previewed file has changed,
Show All 20 Lines278 } else {
271 // When we don't need it, hide the phonon widget first to avoid flickering 279​ // When we don't need it, hide the phonon widget first to avoid flickering
272 m_phononWidget->hide(); 280​ m_phononWidget->hide();
273 m_preview->show(); 281​ m_preview->show();
274 m_preview->removeEventFilter(m_phononWidget); 282​ m_preview->removeEventFilter(m_phononWidget);
275 m_phononWidget->clearUrl(); 283​ m_phononWidget->clearUrl();
276 } 284​ }
277 } 285​ }
278 } else { 286​ } else {
287​ m_preview->clearAnimatedImage();
279 m_preview->hide(); 288​ m_preview->hide();
280 m_phononWidget->hide(); 289​ m_phononWidget->hide();
281 } 290​ }
282} 291​}
283 292​
284void InformationPanelContent::configureShownProperties() 293​void InformationPanelContent::configureShownProperties()
285{ 294​{
286 m_configureLabel->setVisible(true); 295​ m_configureLabel->setVisible(true);
Show All 11 Lines
298void InformationPanelContent::showItems(const KFileItemList& items) 307​void InformationPanelContent::showItems(const KFileItemList& items)
299{ 308​{
300 // If there is a preview job, kill it to prevent that we have jobs for 309​ // 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). 310​ // multiple items running, and thus a race condition (bug 250787).
302 if (m_previewJob) { 311​ if (m_previewJob) {
303 m_previewJob->kill(); 312​ m_previewJob->kill();
304 } 313​ }
305 314​
315​ m_preview->clearAnimatedImage();
316​
306 m_preview->setPixmap( 317​ m_preview->setPixmap(
307 QIcon::fromTheme(QStringLiteral("dialog-information")).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous) 318​ QIcon::fromTheme(QStringLiteral("dialog-information")).pixmap(KIconLoader::SizeEnormous, KIconLoader::SizeEnormous)
308 ); 319​ );
309 setNameLabelText(i18ncp("@label", "%1 item selected", "%1 items selected", items.count())); 320​ setNameLabelText(i18ncp("@label", "%1 item selected", "%1 items selected", items.count()));
310 321​
311 m_metaDataWidget->setItems(items); 322​ m_metaDataWidget->setItems(items);
312 323​
313 m_phononWidget->hide(); 324​ m_phononWidget->hide();
Show All 38 Lines360{
352 KIconLoader::global()->drawOverlays(item.overlays(), pixmap, KIconLoader::Desktop); 363​ KIconLoader::global()->drawOverlays(item.overlays(), pixmap, KIconLoader::Desktop);
353 m_preview->setPixmap(pixmap); 364​ m_preview->setPixmap(pixmap);
354} 365​}
355 366​
356void InformationPanelContent::showPreview(const KFileItem& item, 367​void InformationPanelContent::showPreview(const KFileItem& item,
357 const QPixmap& pixmap) 368​ const QPixmap& pixmap)
358{ 369​{
359 m_outdatedPreviewTimer->stop(); 370​ m_outdatedPreviewTimer->stop();
360 Q_UNUSED(item) ​
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 371​
362 QPixmap p = pixmap; 372​ QPixmap p = pixmap;
363 KIconLoader::global()->drawOverlays(item.overlays(), p, KIconLoader::Desktop); 373​ KIconLoader::global()->drawOverlays(item.overlays(), p, KIconLoader::Desktop);
364 374​
365 if (m_isVideo) { 375​ if (m_isVideo) {
366 // adds a play arrow 376​ // adds a play arrow
367 377​
368 // compute relative pixel positions 378​ // compute relative pixel positions
▲ Show 20 Lines • Show All 116 Lines • Show Last 20 Lines