Review Request 108995: FIX Pre-amplifier in equalizer doesn't work

[Matěj Laitl]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108995/#review29959
-----------------------------------------------------------


Looks good, thanks! Just a remark or 2 below. Thanks also for the variable naming fixes.

We also encourage commits to be focused on just one thing. Could you please split this patch into 2, first one just changing the variable names of existing variables, second implementing the actual pre-amp change? I suggest you use git add -p (interactive adding files to git index) along with its s: split function and e: editing hunks to be added by hand.


src/EngineController.h
<http://git.reviewboard.kde.org/r/108995/#comment22340>

    Hmm, please call it EQUALIZER_BANDS_COUNT and set it to 10, rather than 11. Then adapt the code.
    
    Or rather, instead of macros, in C++ const variables are preferred. You should be able to achieve the same with:
    static const s_equalizerBandsCount = 10;



src/EngineController.cpp
<http://git.reviewboard.kde.org/r/108995/#comment22339>

    The equalizerParameters.isEmpty() is not needed here, the code would be no-op even if it is empty.


- Matěj Laitl


On March 14, 2013, 5:30 p.m., Harsh Gupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108995/
> -----------------------------------------------------------
> 
> (Updated March 14, 2013, 5:30 p.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/EngineController.h 5de4beb 
>   src/EngineController.cpp 58d7360 
>   src/dialogs/EqualizerDialog.h fd9032b 
>   src/dialogs/EqualizerDialog.cpp 7d62e10 
>   src/dialogs/EqualizerDialog.ui 43b0187 
> 
> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130327/36b40d8b/attachment.html>