[Bug 649040] Add volume control to qt-gstreamer example player

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=649040
  GStreamer | qt-gstreamer | 0.10.33

--- Comment #3 from Marco Ballesio <gibrovacco at gmail.com> 2011-04-30 18:51:15 UTC ---
(In reply to comment #1)
> Review of attachment 186945 [details]:
> 
> ::: examples/player/mediaapp.cpp
> @@ -195,1 +198,5 @@
> 
> +    m_volumeSlider = new QSlider(Qt::Horizontal);
> +    m_volumeSlider->setTickPosition(QSlider::TicksLeft);
> +    m_volumeSlider->setTickInterval(2);
> +    m_volumeSlider->setMaximum(10);
> 
> Is just 10 levels of volume enough? I would expect something like 100...

it's a matter of taste.. I tend to prefer cleaner UIs with few messages with
the lower layers. Moreover the player of my cellphone has actually just about
10 steps.

> 
> @@ -196,0 +199,7 @@
> +    m_volumeSlider = new QSlider(Qt::Horizontal);
> +    m_volumeSlider->setTickPosition(QSlider::TicksLeft);
> +    m_volumeSlider->setTickInterval(2);
> ... 4 more ...
> 
> I think it's fine to set the maximum size of a widget :)

agreed. Comment removed.

> 
> ::: examples/player/player.cpp
> @@ +29,3 @@
>  #include <QGst/Event>
> +#include <QGst/StreamVolume>
> +#include <QGst/enums.h>
> 
> Including enums.h is not required. It is included implicitly from global.h. In
> general, all the *.h headers are considered private and should not be included
> directly. One should only use the camel-case ones, just like with Qt.
> 

strange, it didn't compile at first but now it works fine. Removed.

> @@ -98,2 +100,3 @@
>  }
> 
> +int Player::getVolume()
> 
> Qt style: This method should be called just volume(), not getVolume().
> 

done. It would be good to have a grand-unified-style across languages.. a task
for when I'll dominate the world :)

> @@ -100,0 +102,9 @@
> +int Player::getVolume()
> +{
> +  if (m_pipeline) {
> ... 6 more ...
> 
> coding style: All this function should be re-indented to 4-space indentation,
> like the rest of the file. Also, the if (svp) statement should have braces as
> well (see http://techbase.kde.org/Policies/Kdelibs_Coding_Style#Braces )
> 

done. Style is fun!

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.