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