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

Changeset View

Standalone View

View Options

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​
54InformationPanelContent::InformationPanelContent(QWidget* parent) : 55​InformationPanelContent::InformationPanelContent(QWidget* parent) :
55 QWidget(parent), 56​ QWidget(parent),
56 m_item(), 57​ m_item(),
Show All 28 Lines66{
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() ) {
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…
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​
159void 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​ }
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(); 172​ refreshPreview();
elvisangelaccioUnsubmitted
Done

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​
169void 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 Lines200 } 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​
elvisangelaccioUnsubmitted
Done

Unrelated whitespace change

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/"));
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…
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​ }
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)…
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​
318void 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); ​
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 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​
329void InformationPanelContent::markOutdatedPreview() 342​void InformationPanelContent::markOutdatedPreview()
330{ 343​{
▲ Show 20 Lines • Show All 75 Lines • Show Last 20 Lines