<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/109283/">http://git.reviewboard.kde.org/r/109283/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 5th, 2013, 1:41 a.m. UTC, <b>Dan Meltzer</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hi. All sorts of things going on here. My biggest question is that it seems as if you are designing this to only allow one label to be set as the "banned" label. Why not a list of labels? Also, keep your patch on topic. Formatting changes aren't a bad thing, but they make it hard to see what you are actually changing.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> My biggest question is that it seems as if you are designing this to only allow one label to be set as the "banned" label. Why not a list of labels?
I don't think that multiple labels are necessary (and that's how the suggestion how the implement it on the bug was formulated). Intended use case is that user creates a label extra for that (e.g. "noscrobble") and assigns it to all tracks. (mass-assigning labels works fine in Amarok, and you can filter by existing labels)
[the things below are just replies to comments above, will do a review too]</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 5th, 2013, 1:41 a.m. UTC, <b>Dan Meltzer</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/109283/diff/1/?file=116979#file116979line35" style="color: black; font-weight: bold; text-decoration: underline;">src/core/collections/QueryMaker.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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">namespace Collections {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">35</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">enum</span> <span class="n">AlbumQueryMode</span> <span class="p">{</span> <span class="n">AllAlbums</span> <span class="o">=</span> <span class="mi">0</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">35</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">enum</span> <span class="n">AlbumQueryMode</span> <span class="p">{</span> <span class="n">AllAlbums</span> <span class="o">=</span> <span class="mi">0</span><span class="p"><span class="hl">,</span></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;">No need to reformat this in this patch. It just makes it more confusing what's going on.</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;">Right, moreover this change goes against Amarok coding style (we don't alight values to be in the same column).</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 5th, 2013, 1:41 a.m. UTC, <b>Dan Meltzer</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/109283/diff/1/?file=116985#file116985line280" style="color: black; font-weight: bold; text-decoration: underline;">src/services/lastfm/ScrobblerAdapter.cpp</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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">ScrobblerAdapter::announceTrackCorrections( const lastfm::Track &track )</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">280</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">bool</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;">Is it your intention to only allow one label to be skipped for scrobbling? If so, why? Shouldn't it be a list of banned labels that we are comparing against? </pre>
</blockquote>
<p>On March 5th, 2013, 5:44 a.m. UTC, <b>Vedant Agarwala</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I used the QComboBox as per Matej's comment (#21) in https://bugs.kde.org/show_bug.cgi?id=140198
If needed I can easily change it to a QListWidget, that will resolve this issue.</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;">Yes, this was the intention.</pre>
<br />
<p>- Matěj</p>
<br />
<p>On March 4th, 2013, 7:31 p.m. UTC, Vedant Agarwala wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok.</div>
<div>By Vedant Agarwala.</div>
<p style="color: grey;"><i>Updated March 4, 2013, 7:31 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;">Added a check-box to disable scobbling for tracks with a particular label. The label can be selected from a drop-down list or entered manually. Any track that contains this label is skipped from being submitted to last.fm for being scrobbled.</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;">Builds and runs successfully.</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/core/collections/QueryMaker.h <span style="color: grey">(a30b94f)</span></li>
<li>src/services/lastfm/LastFmConfigWidget.ui <span style="color: grey">(5c5e51b)</span></li>
<li>src/services/lastfm/LastFmServiceConfig.h <span style="color: grey">(3f33b72)</span></li>
<li>src/services/lastfm/LastFmServiceSettings.h <span style="color: grey">(41b2ead)</span></li>
<li>src/services/lastfm/LastFmServiceSettings.cpp <span style="color: grey">(f6e1564)</span></li>
<li>src/services/lastfm/ScrobblerAdapter.h <span style="color: grey">(ea74196)</span></li>
<li>src/services/lastfm/ScrobblerAdapter.cpp <span style="color: grey">(b1a09f8)</span></li>
<li>src/statsyncing/Process.cpp <span style="color: grey">(c42fdc4)</span></li>
<li>src/statsyncing/ScrobblingService.h <span style="color: grey">(971edd7)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109283/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>