D12918: Add filmstrip embed configuration

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> ffmpegthumbnailer.cpp:22
> +#include <QCheckBox>
> +#include <klocalizedstring.h>
>  

`<KLocalizedString>`

> ffmpegthumbnailer.cpp:36
> +    FFMpegThumbnailerSettings* settings = FFMpegThumbnailerSettings::self();
> +    settings->load();
> +    if (settings->filmstrip()) {

Is this really necessary? Doesn't `self()` internally call `read()` ?

> ffmpegthumbnailer.cpp:66
> +{
> +    QCheckBox *filmstripCheckBox = new QCheckBox(i18nc("@option:check", "Embed filmstrip effect in video thumbnails"));
> +    filmstripCheckBox->setChecked(FFMpegThumbnailerSettings::filmstrip());

"in video thubmnails" is redundant, imho

> ffmpegthumbnailer.h:35
> +    virtual QWidget* createConfigurationWidget();
> +    virtual void writeConfiguration( const QWidget* configurationWidget );
>  private:

Please remove spaces after/before ( )

Also, both methods should be marked with `override`.

REPOSITORY
  R343 FFmpeg Thumbnailers

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

To: tcarmichael, #dolphin, ngraham
Cc: elvisangelaccio, cfeck
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20180526/3fdef305/attachment.htm>