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/#review19224
-----------------------------------------------------------


I tend to like the general idea of moving the publicly-available methods to handle equalizer to a component. On the other hand this is IMO a good opportunity to clean up the current design which I consider messy. I know that you've mainly just added an interface, so I will be mostly criticising existing code, but hey. :-)


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

    code style:
    a) first own header (in case of .cpp file), then Amaork headers using the ""-style, preferrably relative to [source-dir]/src, then KDE includes, then Qt includes. Include groups separated by blank lines



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

    This lacks documentation with some high-level overview and usage.



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

    If EquelizerController is accessible through Amarok::Components, don't implement the singleton pattern. The same applies to The::equalizer().
    
    You may want to wait however till we discuss how to deal with components and their lifetime in Randa.



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

    All methods lack documentation, which is unacceptable for a component.
    
    From the names it seems that EqualizerController would solve 2 purposes:
    
    a) getting and setting the currently used equalizer preset
    b) store for equalizer presets
    
    Is it a good idea to combine these into one controller? Maybe yes, but I'd like to hear the reasoning.
    
    I also dislike the whole EqulizerPresets class. Why there's no EqualizerPreset class that would contain the values and name? I think that it would make EqualizerController interface cleaner.
    
    Comments on individual methods:
     * int presetNum() const; void setPreset(int number) const: seems redundant with the name-base methods.
     * QString presetName() const; should be named currentPresetName()? Ideally this would be EqualizerPreset currentPreset() const;
     * void setPreset(QString name) const; should be named activatePreset? I would love it it would accept "const EqualizerPreset &preset" instead.
    
     * const EqualizerPresets presets() const; -> QList<EqualizerPreset> savedPresets() const;? Also returning const by value has no sense.
     * void setPresetValues(const QString name, const QList<int>& values); -> void savePreset( const EqualizerPreset &preset );?
     * bool deletePreset(const QString name); Why bool? Should it take name or const EqualizerPreset &preset as an argument?
     * bool restorePreset(const QString name); what does this do?
    
    My suggestion for the EqualizerPreset class:
    
    .h file:
    
    class EqualizerPreset
    {
        public:
            EqualizerPreset( const QString &name, const QList<int> &values )
        
            QString name() const;
            void setName();
    
            QList<int> values() const;
            void setValues( const QList<int> &values );
            void setValue( int index, int value );  // convenience
    
            operator==( const EqualizerPreset &other ) const;
    
        private:
            class Private;
            QSharedDataPointer<Private> d;
    }
    
    .cpp file:
    
    class EqualizerPreset::Private {
        public:
            QString name;
            QList<int> values;
    }



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

    Deprecated, see above.


- Matěj Laitl


On Sept. 20, 2012, 12:27 a.m., Ryan McCoskrie wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106511/
> -----------------------------------------------------------
> 
> (Updated Sept. 20, 2012, 12:27 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/20120920/13ff8e9a/attachment-0001.html>