D23538: [dolphin] Animate gifs on preview

[Nathaniel Graham]

ngraham added a comment.


  Nice work!
  
  `movieFileName()` and `setMovieFileName()` are somewhat ambiguous since the information panel can already play movies. Maybe change to `animatedImageFileName()` and `setAnimatedImageFileName()`; Same with `m_movie`. I'd call it `m_animatedImage`. I have a few more inline comments below:

INLINE COMMENTS

> iasensio wrote in informationpanelcontent.cpp:95
> 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.

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.

> pixmapviewer.cpp:45
> +
> +    m_movie = new QMovie(this);
> +    connect(m_movie, &QMovie::frameChanged, this, &PixmapViewer::updateAnimationFrame);

No need to create a new QMovieObject unless there's actually an animated image loaded; I'd create it on demand and then handle the case where `m_movie` is null.

REPOSITORY
  R318 Dolphin

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

To: iasensio, #dolphin, #vdg, ngraham, elvisangelaccio
Cc: meven, broulik, kfm-devel, vmarinescu, 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/20190829/1c6df2c8/attachment.htm>