D23538: [dolphin] Animate gifs on preview
Nathaniel Graham
noreply at phabricator.kde.org
Thu Aug 29 03:42:45 BST 2019
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>
More information about the kfm-devel
mailing list