<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/101598/">http://git.reviewboard.kde.org/r/101598/</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;">This change looks good, once minor problems are resolved I think this should be pushed.</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/101598/diff/3/?file=48464#file48464line358" style="color: black; font-weight: bold; text-decoration: underline;">shared/tag_helpers/TagHelper.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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; ">Meta::Tag::selectHelper( const TagLib::FileRef fileref, bool forceCreation )</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">358</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">else</span> <span class="k">if</span><span class="p">(</span> <span class="o">!</span><span class="n">fileref</span><span class="p">.</span><span class="n">isNull</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;">This change doesn't seem to be directly related to support for mod, s3m.. files. Could you provide rationale for it and the effects it has? It doesn't look bad, but I think it could break filetype-form-extension guessing in MetaTagLib.cpp:269-274. It would be great if you could test whether this is true or not.</pre>
</div>
<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;">Second small hiccup is that you did not touch Meta::Tag::FileTypeResolver::createFile(). You therefore depend on TagLib's basic filetype guessing from extension. You may want to add cases for audio/x-{mod,s3m,it,xm} mime-types (not for file extensions, these are already handled by TagLib) there to allow more sophisticated KDE's mime type subsystem to identify the files. Then tag reading should work from at least it, s3m and xm files even if you rename them to have no extension. (YMMV, but my /usr/share/mime/magic has entries for these)</pre>
<p>- Matěj</p>
<br />
<p>On January 31st, 2012, 2:12 a.m., Mathias Stephan Panzenböck 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 Mathias Stephan Panzenböck.</div>
<p style="color: grey;"><i>Updated Jan. 31, 2012, 2:12 a.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 adds read/write tag support for mod, s3m, it and xm files if taglib supports it. All new parts are #ifdefed so it won't break if an old taglib version is used.
I wrote a patch for taglib that enables support for these file formats and it will be included in taglib 1.8.0. Here is the pull request:
https://github.com/taglib/taglib/pull/4</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=90524">90524</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>CMakeLists.txt <span style="color: grey">(d47c28b)</span></li>
<li>config-amarok.h.cmake <span style="color: grey">(2d25cc7)</span></li>
<li>shared/FileType.h <span style="color: grey">(5c8081f)</span></li>
<li>shared/FileType.cpp <span style="color: grey">(a6e25d5)</span></li>
<li>shared/tag_helpers/TagHelper.cpp <span style="color: grey">(4c0fb2b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/101598/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>