<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/106511/">http://git.reviewboard.kde.org/r/106511/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 20th, 2012, 3:21 p.m., <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/106511/diff/1/?file=86406#file86406line37" style="color: black; font-weight: bold; text-decoration: underline;">src/EqualizerController.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">37</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">presetName</span><span class="p">()</span> <span class="k">const</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">38</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">int</span> <span class="n">presetNum</span><span class="p">()</span> <span class="k">const</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">39</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="n">setPreset</span><span class="p">(</span><span class="n">QString</span> <span class="n">name</span><span class="p">)</span> <span class="k">const</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">40</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="n">setPreset</span><span class="p">(</span><span class="kt">int</span> <span class="n">number</span><span class="p">)</span> <span class="k">const</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">41</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="n">EqualizerPresets</span> <span class="n">presets</span><span class="p">()</span> <span class="k">const</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">42</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="n">setPresetValues</span><span class="p">(</span><span class="k">const</span> <span class="n">QString</span> <span class="n">name</span><span class="p">,</span> <span class="k">const</span> <span class="n">QList</span><span class="o"><</span><span class="kt">int</span><span class="o">>&</span> <span class="n">values</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">43</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">bool</span> <span class="n">deletePreset</span><span class="p">(</span><span class="k">const</span> <span class="n">QString</span> <span class="n">name</span><span class="p">);</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">44</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">bool</span> <span class="n">restorePreset</span><span class="p">(</span><span class="k">const</span> <span class="n">QString</span> <span class="n">name</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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;
}</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
<br />
<p>- Ryan</p>
<br />
<p>On September 20th, 2012, 11:24 p.m., Ryan McCoskrie wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok.</div>
<div>By Ryan McCoskrie.</div>
<p style="color: grey;"><i>Updated Sept. 20, 2012, 11:24 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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()</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/EqualizerController.h <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106511/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>