> 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
>
>
_______________________________________________
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
