----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108995/#review28117 -----------------------------------------------------------
Hi, thanks for the patch and sorry for delays while reviewing it, I was preoccupied by other things lately. (Edward, Ralf, Teo, Bart, Sam, Sven: I'm not the only one who can review requests!) Please resolve the remarks below before the patch can be merged. src/EngineController.cpp <http://git.reviewboard.kde.org/r/108995/#comment21035> I think it is better to check whether mEqPar has 10 or 11 items. This is not issue of this patch, but I would also welcome if some of the variables got renamed, for example mEqPar should be equalizerParameters etc, no need for cryptic abbreviations and "m" prefix. src/EngineController.cpp <http://git.reviewboard.kde.org/r/108995/#comment21036> The assignment to scaledVal has no effect here, just the mEqParNewIt.next() has. src/dialogs/EqualizerDialog.h <http://git.reviewboard.kde.org/r/108995/#comment21039> While you're at it, please rename attribute names to match Amarok coding style, e.g.: m_valueScale m_bands m_bandValues (note the removed s to make them more english) m_bandLabels src/dialogs/EqualizerDialog.cpp <http://git.reviewboard.kde.org/r/108995/#comment21038> Ugly. Instead please: a) check whether meqBandFrq (horrible variable name btw, not your fault) has 10 items or 11. b) don't append preamp to mBands, mBandsValues, mBandsLabels in case it is not available. - Matěj Laitl On Feb. 18, 2013, 6:08 a.m., Harsh Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/108995/ > ----------------------------------------------------------- > > (Updated Feb. 18, 2013, 6:08 a.m.) > > > Review request for Amarok. > > > Description > ------- > > 1. Disabled Pre-amplifier in equalizer if it is not supported by phonon. > 2. Fixed top and bottom labels of first slider. Earlier band name label was > at the top and slider value label was at the bottom for first slider. > 3. Removed an extra semicolon EqualizerDialog.h . > Note : I have made an assumption that if at all preamp is present then it > will the first element of Effect Parameter list. > > > This addresses bug 301311. > https://bugs.kde.org/show_bug.cgi?id=301311 > > > Diffs > ----- > > src/dialogs/EqualizerDialog.ui 43b0187 > src/EngineController.cpp 3577acf > src/dialogs/EqualizerDialog.h fd9032b > src/dialogs/EqualizerDialog.cpp 63d8209 > > Diff: http://git.reviewboard.kde.org/r/108995/diff/ > > > Testing > ------- > > All unit test cases passed. > Note : I have tested it with gstreamer only. Xine phonon keep crashing on my > PC. > > > File Attachments > ---------------- > > Equalizer snapshot > http://git.reviewboard.kde.org/media/uploaded/files/2013/02/17/equalizer.png > > > Thanks, > > Harsh Gupta > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel