D23538: [dolphin] Animate gifs on preview

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> meven wrote in informationpanelcontent.cpp:322
> Indeed my bad

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

> informationpanelcontent.cpp:175
>      }
> +    // Refresh even on same item to handle resizing events
>      refreshPreview();

Please remove this comment, as D23668 <https://phabricator.kde.org/D23668> will make it redundant.

> informationpanelcontent.cpp:224
>      bool usePhonon = false;
> +
>      setNameLabelText(m_item.text());

Unrelated whitespace change

> pixmapviewer.cpp:26
>  #include <QStyle>
> +#include <QMovie>
>  

Please keep the list of includes sorted.

> pixmapviewer.cpp:182-189
> +void PixmapViewer::clearAnimatedImage()
> +{
> +    if (m_animatedImage) {
> +        m_animatedImage->stop();
> +        delete m_animatedImage;
> +        m_animatedImage = nullptr;
> +    }

Is it really necessary to delete the QMovie instance and create another one every time?

> pixmapviewer.cpp:191-196
> +const QStringList PixmapViewer::animatedMimeTypes()
> +{
> +    return QStringList() << QStringLiteral("image/gif")
> +                         << QStringLiteral("image/webp")
> +                         << QStringLiteral("video/x-mng");
> +}

I'd try to avoid to hardcode the list of supported mimetypes here. QMovie has `QMovie::supportedFormats()` which tells us the list of supported image formats.

We could create a `QImageReader` of the selected image and then check its `QImageReader::format()`.

REPOSITORY
  R318 Dolphin

REVISION DETAIL
  https://phabricator.kde.org/D23538

To: iasensio, #dolphin, #vdg, ngraham, elvisangelaccio
Cc: pino, fuksitter, meven, broulik, kfm-devel, iasensio, fprice, MrPepe, fbampaloukas, alexde, Codezela, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20190914/b7345720/attachment.htm>