Review Request: WIP - Dedicated equalizer controller

[Ryan McCoskrie]

> On Sept. 20, 2012, 3:21 p.m., Matěj Laitl wrote:
> > src/EqualizerController.h, lines 37-44
> > <http://git.reviewboard.kde.org/r/106511/diff/1/?file=86406#file86406line37>
> >
> >     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;
> >     }

I'm going to have to address this comment as if it where several. Here goes.
1 (No documentation):
    Just updated the patch to fix that. Hope it is nice and clear but I suspect not.
2 (Purpose):
    I'm still vaguely intending on writing a script that changes the equaliser preset when the track changes, which means adding in support
    for scripting the equaliser. At the moment that is impossible since the presets are kept in a private member of the equaliser dialogue.
    Much of the getting and setting is done in there as well. The obvious solution was to make both the dialogue and scripting interface
    front ends on a single controller.
3 (EqualizerPresets vs. EqualizerPreset)
    Like I mentioned before one of my next steps is to completely remove EqualizerPresets after
    reimplementing its behaviour in EqualizerController.
    Your suggestion for an EqualizerPreset class could really clean up this interface and give a good incentive to further clean up
    the equaliser dialogue. I would probably write it a little differently however.
4 (*Num classes)
    They seemed necessary at the time due to AmarokConfig doing everything according to index numbers. But I could probaby remove
    these without causing any inconvenience.
5 (Renamings)
    Consider it done.
6 (const makes no sense)
    I was putting const in pretty much everywhere else. Also thinking in Scala.
7 (Why bool?)
    Because the equaliser dialogue wouldn't compile without getting a bool.
8 (What does restore do?)
    I'm still not certain. The dialogue needed it but if it does what I think it does from the updated patch* then it probably
    isn't needed as the current behaviour of the dialogue seems to obsolete it.


*Which I just realised only covers one .h file


- Ryan


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


On Sept. 20, 2012, 11:24 p.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, 11:24 p.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/EqualizerController.h PRE-CREATION 
> 
> 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/20120921/d18f31e2/attachment-0001.html>