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

[Matěj Laitl]

> On Feb. 26, 2013, 10:55 a.m., Matěj Laitl wrote:
> > src/dialogs/EqualizerDialog.cpp, lines 99-119
> > <http://git.reviewboard.kde.org/r/108995/diff/1/?file=114229#file114229line99>
> >
> >     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.
> 
> Harsh Gupta wrote:
>     Reason for appending preamp : Since number of sliders are hard coded in UI ( 11 sliders ) I thought each slider should have some label.
>     Anyway I will remove preamp label and make first 10 sliders active while last slider is disabled.

> Reason for appending preamp : Since number of sliders are hard coded in UI ( 11 sliders ) I thought each slider should have some label.

True. Lets just fill in the label "Preamp" for the first slider directly in the .ui file, that way you don't need another special case in the .cpp file

> Anyway I will remove preamp label and make first 10 sliders active while last slider is disabled.

This is not what I meant/intended. Not including preamp in mBands, mBandsValues, mBandsLabels (and setting it disabled) if equalizer has 10 parameters should just skip it in logic functions in EqualizerDialog.cpp, but shouldn't affect the UI (apart from the fact the preamp slider won't react to changes). That way you don't have to query whether the slider is enabled in another EqualizerDialog.cpp methods.


- Matěj


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


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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130226/d6e40dfc/attachment-0001.html>