[vlc-devel] VLC MiniMode Widget: SVG/CSS based Media Playback Notification and Controller

[Jean-Baptiste Kempf]

Hello,

On Fri, Jun 05, 2009 at 12:40:37AM +0200, Rohit Yadav wrote :
> -  PKG_CHECK_MODULES(QT4, [QtCore QtGui >= 4.4.0], [
> +  PKG_CHECK_MODULES(QT4, [QtCore QtGui QtSvg >= 4.4.0], [

  Notice to other developers: this introduces QtSvg dependency.

> --- vlc/modules/gui/qt4/components/interface_widgets.cpp	2009-06-02 15:36:10.000000000 +0200
> +++ vlc.new/modules/gui/qt4/components/interface_widgets.cpp	2009-06-04 23:08:09.000000000 +0200
I'd rather have it in a different file, if possible.

> +    /* Toggle control */
> +    b_toggle = false; /* Show Pause, 0 = pause, 1 = play */
> +    b_lock = false;   /* Show Unlocked, Lock = 1, Unlock = 0 */
Alignment?

> +    this->setWindowTitle( "VLC MiniMode" );
> +    this->setAcceptDrops( true ); /* Enable Drag n Drop Files to Play! */

> +    /* Add Widgets to 'controls' container: Play/Pause, Previous, Next */
> +    QHBoxLayout *controls = new QHBoxLayout();
> +    QVBoxLayout *controlDock = new QVBoxLayout();
Why not a QGridLayout?

> +    /* Construct Meta Data Information */
> +    qs_title  = "VLC MiniMode Widget";
Name isn't good. "VLC" should be enough.

> +    qs_artist = "The VideoLAN Team";
Same. "VideoLAN".

> +    qs_album  = "VLC Widget Project, Copyleft 2009";
...

> +    playbackControl->setStyleSheet(
> +        " QSlider::groove:horizontal"
> +            "{ background: #5f5f5f; height: 1px; margin: 0px 0; }"
> +        " QSlider::handle:horizontal"
> +            "{ image: url(:/svgs/knob.svg); width: 16px; height: 16px; margin: -4px 0; }"
> +        " QSlider::add-page:horizontal"
> +            "{ background: #5f5f5f; }"
> +        " QSlider::sub-page:horizontal"
> +            "{ background: #1987de; /* Cool Blue */}" );
> +    volumeControl->setStyleSheet(
> +        " QSlider::groove:vertical"
> +            "{ background: #5f5f5f; width: 2px; }"
> +        " QSlider::handle:vertical"
> +            "{ image: url(:/svgs/knob.svg); width: 12px; height: 12px; margin: 0 -4px; }"
> +        " QSlider::sub-page:vertical"
> +            "{ background: #5f5f5f; width: 2px;}"
> +        " QSlider::add-page:vertical"
> +            "{ background: #fe1987; width: 2px;  /* Posh Pink */}" );
Do we really need CSS, when using SvG?

> +    QVBoxLayout *volumeLockLayout = new QVBoxLayout();
Same as above.

> +    timer->setInterval( 5000 );
Use the same as FSC configuration variable.

> +    connect( timer, SIGNAL( timeout() ), this, SLOT( hideWidget() ) );
CONNECT?

> +    if( data == NULL )
> +      return false;
indentation.

> +    /* FIXED: Artist and Album names updation */
Fix comment.

> +        timer->setSingleShot( false );
> +        connect( timer, SIGNAL( timeout() ), this, SLOT( hideWidget() ) );
As above.

> +/* Volume Control Interfaces */
> +#define MNW_MAX_VOLUME 200
> +void MiniModeWidget::notifierVolumeChanged( int i_vol )
> +{
> +    int i_res = ( i_vol )  * ( AOUT_VOLUME_MAX / 2 ) / MNW_MAX_VOLUME;
> +    aout_VolumeSet( p_intf, i_res );
> +}
This should go to Action_Manager.hpp

> +    if( b_toggle == true )
       if( b_toggle ) is enough

> +    if( b_lock == true )
  same.

> +        if( event->button() == Qt::RightButton )
> +        {
> +            title->setText( "About \"VLC MiniMode\"" );
> +            artist->setText( "Created by Rohit Yadav" );
> +            album->setText( "Copyleft 2009" );
I dislike this.

> +void MiniModeWidget::dropEvent( QDropEvent *event )
DO NOT duplicate code. Find a way to pass it to MI.

> +    if( b_opt == true )
Same.

Best Regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/