Review Request: WIP - Dedicated equalizer controller

[Matěj Laitl]

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


Please finish the cleanups and other refactorings - the patch as it is looks like first part of something bigger that makes it a bit unclean.


src/EqualizerController.h
<http://git.reviewboard.kde.org/r/106511/#comment15562>

    Please remove the singleton pattern.



src/EqualizerController.h
<http://git.reviewboard.kde.org/r/106511/#comment15561>

    Whitespace on blank lines! We all hate the red colour! ;)



src/EqualizerController.h
<http://git.reviewboard.kde.org/r/106511/#comment15563>

    Please remove the singleton pattern.



src/EqualizerController.cpp
<http://git.reviewboard.kde.org/r/106511/#comment15564>

    First own include, then Amarok includes using #include "path/to/Something.h" style, then KDE includes, then Qt includes, then everything else, groups separated by one blank line.


- Matěj Laitl


On Sept. 21, 2012, 12:45 a.m., Ryan McCoskrie wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106511/
> -----------------------------------------------------------
> 
> (Updated Sept. 21, 2012, 12:45 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> This patch moves much of the Equalizer Dialogues internal behaviour into a dedicated object accessed via The::equalizer()*. Any component of Amarok looking to adjust equalizer levels (i.e. The equalizer scripting support I'll resubmit some day) should use this interface.
> 
> I'm considering as my next steps, merging the EqualizerPresets class with the EqualizerController class and possibly removing dependence on AmarokConfig to make the actual changes..
> 
> *The equalizer dialogue is accessed via The::equalizerDialog()
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 8596144 
>   src/EqualizerController.h PRE-CREATION 
>   src/EqualizerController.cpp PRE-CREATION 
>   src/MainWindow.cpp 0530fd7 
>   src/core/support/Components.h c38977c 
>   src/core/support/Components.cpp 22e5de0 
>   src/dialogs/EqualizerDialog.h 9dad055 
>   src/dialogs/EqualizerDialog.cpp 3a63fe6 
> 
> Diff: http://git.reviewboard.kde.org/r/106511/diff/
> 
> 
> Testing
> -------
> 
> Checked that Amarok compiles and that the equalizer dialogue still works. Found that enabling/dis-enabling the equalizer forces the track to freeze or restart. Pressing stop and then play will make it continue with the correct preset enabled.
> 
> 
> Thanks,
> 
> Ryan McCoskrie
> 
>

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