GNOME Bugzilla – Bug 649040
Add volume control to qt-gstreamer example player
Last modified: 2011-05-09 09:55:57 UTC
Created attachment 186945 [details] [review] Add volume control to the example player As volume control has been added some time ago to the bindings, it would be a pity not to show how to use it. The attached patch compensates for the lack.
Review of attachment 186945 [details] [review]: ::: 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... @@ -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 :) ::: 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. @@ -98,2 +100,3 @@ } +int Player::getVolume() Qt style: This method should be called just volume(), not getVolume(). @@ -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 ) @@ -100,0 +102,19 @@ +int Player::getVolume() +{ + if (m_pipeline) { ... 16 more ... coding style: as above, indent with 4 spaces and use braces in if(svp).
Created attachment 186946 [details] [review] revised version of the patch
(In reply to comment #1) > Review of attachment 186945 [details] [review]: > > ::: 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!
Review of attachment 186946 [details] [review]: Ok, thanks. It's good to be commited to master, if you also fix those 3 details below. You can commit directly to master if you want, or let me commit it tomorrow. ::: examples/player/player.cpp @@ -98,2 +99,3 @@ } +int Player::volume() Oh, almost forgot... also make this function const: int Player::volume() const (the declaration in player.h needs to change as well) @@ -100,0 +101,12 @@ +int Player::volume() +{ + if (m_pipeline) { ... 9 more ... Indentation not completely fixed here (and the brace 2 lines above) @@ -100,0 +101,23 @@ +int Player::volume() +{ + if (m_pipeline) { ... 20 more ... Here too
Created attachment 186971 [details] [review] beautified patch here it is, just to be sure I've passed it through astyle, keeping only the modifications it introduced in the new methods.
Created attachment 186979 [details] [review] greatest and latest, with proper fullscreen handling
Thanks, commited.