D7539: Dolphin - Add autoplay feature for media files

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Thanks for the contribution. Some comments after a quick test:
  
  1. If I select two ore more audio files, the autoplay starts for the first selected file. This is probably unexpected.
  2. Not sure if the new option really belongs to the Navigation section of the settings dialog.
  
  See also the inline comments.

INLINE COMMENTS

> phononwidget.cpp:23
> +#include "dolphin_generalsettings.h"
> +#include "dolphinmainwindow.h"
>  

Not needed.

> phononwidget.cpp:86
> +        
> +        //TODO: autoplay starts (related to qtimer) even if the target file has changed before the timer has run out
> +        //TODO: stop/destroy the preview once user does not highlight the item/Dolphin main window or use the window

When does this happen?
Btw it could be solved by storing the file size when creating the timer, and checking the new file size in the `play()` slot.

> phononwidget.cpp:88
> +        //TODO: stop/destroy the preview once user does not highlight the item/Dolphin main window or use the window
> +        //TODO: the autoplay feature should only apply to audio files, exclude video files (as it works in Caja/Nautilus)
> +        

Why? I would expect audio and video files to behave the same way.

> phononwidget.cpp:95
> +        if(GeneralSettings::autoPlayMediaFiles()) { //If the this radiobutton is checked, then
> +                connect(m_autoplaytimer, SIGNAL(timeout()), this, SLOT(play()));
> +                m_autoplaytimer->setSingleShot(true); // Play only once, otherwise this loops in 1 sec cycles

Use the new syntax:

  connect(m_autoplaytimer, &QTimer::timeout, this, &PhononWidget::play)

Also, the whole if block has a wrong indentation (8 spaces instead of 4).

> navigationsettingspage.cpp:47
>  
> +    m_autoPlayMediaFiles = new QCheckBox(i18nc("@option:check", "Play media files automatically"), vBox);
> +    vBoxLayout->addWidget(m_autoPlayMediaFiles);

It should probably tell *how* the autoplay is started (selection/hovering).

REPOSITORY
  R318 Dolphin

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

To: pekkah, #dolphin, #kde_applications
Cc: elvisangelaccio, #dolphin, pekkah, navarromorales, firef, andrebarros, alexeymin, genaxxx, emmanuelp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20170825/9f74cc88/attachment.htm>