<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/104513/">http://git.reviewboard.kde.org/r/104513/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Overall I think this is a lot of config for a pretty esoteric feature. It's nice to have embedded covers, but can't we implement a default that works for the 98%?</pre>
<br />
<table bgcolor="#f0f0f0" cellpadding="5" cellspacing="5" style="border: 1px solid #c0c0c0; margin-bottom: 10px">
<tr>
<td><a href="/r/104513/s/508/" style="color: black; font-weight: bold; font-size: 9pt;">cover_art_handling_2.png</a></td>
</tr>
<tr>
<td><a href="http://git.reviewboard.kde.org/r/104513/s/508/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/04/08/cover_art_handling_2_468_481_61_16.png" style="border: 1px black solid;" alt=""></a></td>
</tr>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Append is a bit technical, would use "Add new cover".</pre>
<br />
<table bgcolor="#f0f0f0" cellpadding="5" cellspacing="5" style="border: 1px solid #c0c0c0; margin-bottom: 10px">
<tr>
<td><a href="/r/104513/s/508/" style="color: black; font-weight: bold; font-size: 9pt;">cover_art_handling_2.png</a></td>
</tr>
<tr>
<td><a href="http://git.reviewboard.kde.org/r/104513/s/508/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/04/08/cover_art_handling_2_457_432_291_13.png" style="border: 1px black solid;" alt=""></a></td>
</tr>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">A combo-box is not ideal for <4 items to choose from. I would personally prefer a checkbox group.
But as you can see lower in the dialog for transcoding, it does save space in an already overcrowded page.</pre>
<br />
<div>
<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/104513/diff/1/?file=56167#file56167line366" style="color: black; font-weight: bold; text-decoration: underline;">shared/tag_helpers/ASFTagHelper.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; ">ASFTagHelper::hasEmbeddedCover() const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">361</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span> <span class="n">pict</span><span class="p">.</span><span class="n">size</span><span class="p">()</span> <span class="o"><</span> <span class="n"><span class="hl">maxSize</span></span> <span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">365</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span> <span class="n">pict</span><span class="p">.</span><span class="n">size</span><span class="p">()</span> <span class="o"><</span> <span class="mi"><span class="hl">1024</span></span> <span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You should probably use a static const or #define here. It's self-documenting if the name is good.
Alternatively you can add a line of comment explaining where the 1024 comes from.</pre>
</div>
<br />
<div>
<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/104513/diff/1/?file=56168#file56168line355" style="color: black; font-weight: bold; text-decoration: underline;">shared/tag_helpers/ID3v2TagHelper.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">ID3v2TagHelper::embeddedCover() const</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">QImage</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">347</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">ID3v2TagHelper</span><span class="o">::</span><span class="n">embeddedCover</span><span class="p">()</span> <span class="k">const</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">355</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">ID3v2TagHelper</span><span class="o">::</span><span class="n">embeddedCover</span><span class="p">()</span> <span class="k">const</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Is there any caching of this cover higher up in the stack?</pre>
</div>
<br />
<div>
<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/104513/diff/1/?file=56173#file56173line108" style="color: black; font-weight: bold; text-decoration: underline;">src/dialogs/CollectionSetup.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; ">class CollectionSetup : public KVBox</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">108</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span> <span class="n">mode</span> <span class="o">==</span> <span class="n">QLatin1String</span><span class="p">(</span> <span class="s">"None"</span> <span class="p">)</span> <span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You are using the same hardcoded value multiple times. Better use something cleaner where you define the strings only once.</pre>
</div>
<br />
<p>- Bart</p>
<br />
<p>On April 8th, 2012, 5:54 p.m., Daniel Faust 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 Daniel Faust.</div>
<p style="color: grey;"><i>Updated April 8, 2012, 5:54 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 includes a list of small changes that are supposed to give the user more control over the cover writing process and unify it for all file formats.
1. The user can set the maximum cover size in the config dialog instead of having a fixed 200px. (BR 279493)
2. The user can set how existing covers should be handled:
- 'Replace existing front covers' (almost the current behavior except for m4a files and some cases where more than one 'front' cover exists)
- 'Replace all existing covers'
- 'Append new cover' (actually it's a prepend, doesn't work with wma files, though; the current behavior for m4a files)
3. Unify the reading of covers. Instead of searching for the biggest cover like it was implemented for some file formats from now on the first 'front' cover will be taken. If there is no 'front' cover, the first 'other' cover will be taken (if present). The 1kb limit is still present.
4. Fix a potential bug where covers couldn't be found in mp3 files if the first cover was neither a 'front' cover nor 'other' or smaller than 1kb.</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;">I have tested writing covers with flac, mp3, and wma files.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=279493">279493</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>shared/tag_helpers/ASFTagHelper.cpp <span style="color: grey">(93e6031)</span></li>
<li>shared/tag_helpers/ID3v2TagHelper.cpp <span style="color: grey">(27e0cf0)</span></li>
<li>shared/tag_helpers/MP4TagHelper.cpp <span style="color: grey">(faeae0a)</span></li>
<li>shared/tag_helpers/VorbisCommentTagHelper.cpp <span style="color: grey">(1fbb437)</span></li>
<li>src/amarokconfig.kcfg <span style="color: grey">(5610c4a)</span></li>
<li>src/core-impl/collections/db/sql/SqlMeta.cpp <span style="color: grey">(e663adf)</span></li>
<li>src/dialogs/CollectionSetup.h <span style="color: grey">(3146f17)</span></li>
<li>src/dialogs/CollectionSetup.cpp <span style="color: grey">(f1b7850)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104513/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/104513/s/508/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/04/08/cover_art_handling_2_400x100.png" style="border: 1px black solid;" alt="" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>