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

Changeset View

Standalone View

View Options

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);
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…
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 54 Lines • ▼ Show 20 Line(s)
157} 157​}
158 158​
159void 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 refreshMetaData(); 164​ refreshMetaData();
165 } ​
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…
166 refreshPreview(); 165​ refreshPreview();
167} 166​ }
167​}
elvisangelaccioUnsubmitted
Done

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

elvisangelaccio: Please remove this comment, as D23668 will make it redundant.
168 168​
169void InformationPanelContent::refreshPreview() 169​void InformationPanelContent::refreshPreview()
170{ 170​{
171 // If there is a preview job, kill it to prevent that we have jobs for 171​ // 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). 172​ // multiple items running, and thus a race condition (bug 250787).
173 if (m_previewJob) { 173​ if (m_previewJob) {
174 m_previewJob->kill(); 174​ m_previewJob->kill();
175 } 175​ }
176 176​
177​ m_preview->stopAnimatedImage();
178​
177 setNameLabelText(m_item.text()); 179​ setNameLabelText(m_item.text());
178 if (InformationPanelSettings::previewsShown()) { 180​ if (InformationPanelSettings::previewsShown()) {
179 181​
180 const QUrl itemUrl = m_item.url(); 182​ const QUrl itemUrl = m_item.url();
181 const bool isSearchUrl = itemUrl.scheme().contains(QStringLiteral("search")) && m_item.localPath().isEmpty(); 183​ const bool isSearchUrl = itemUrl.scheme().contains(QStringLiteral("search")) && m_item.localPath().isEmpty();
182 if (isSearchUrl) { 184​ if (isSearchUrl) {
183 m_preview->show(); 185​ m_preview->show();
184 186​
Show All 23 Lines192 } 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​
elvisangelaccioUnsubmitted
Done

Unrelated whitespace change

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);
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…
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​ }
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)…
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​
246void InformationPanelContent::configureShownProperties() 254​void InformationPanelContent::configureShownProperties()
247{ 255​{
248 m_configureLabel->setVisible(true); 256​ m_configureLabel->setVisible(true);
Show All 11 Lines
260void 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 Lines321{
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​
318void 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); ​
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.
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​
329void InformationPanelContent::markOutdatedPreview() 338​void InformationPanelContent::markOutdatedPreview()
330{ 339​{
▲ Show 20 Lines • Show All 75 Lines • Show Last 20 Lines