<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 6th, 2012, 7:07 p.m., <b>Matěj Laitl</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;">The last revision looks all-fine to me. When commiting, I would also apply following last-bits patch:
====================================================================
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 87dc1eb..58b3732 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -224,6 +224,9 @@ if( WITH_PLAYER )
add_subdirectory( src )
add_subdirectory( external )
+ # following line is here (and not near TAGLIB_MOD_FOUND) because there may be no MacroLogFeature without kdelibs
+ macro_log_feature( TAGLIB_MOD_FOUND "TagLib" "Support for tags in mod, s3m, it and xm files" "http://developer.kde.org/~wheeler/taglib.html" FALSE "1.8" "" )
+
macro_display_feature_log()
#Do not remove or modify these. The release script substitutes in for these
diff --git a/ChangeLog b/ChangeLog
index 755858a..4df073d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -4,6 +4,8 @@ Amarok ChangeLog
Version 2.6-Beta 1
FEATURES:
+ * Support for reading and writing tags from/to mod, s3m, it and xm files
+ thanks to Mathias Stephan Panzenböck.
* Support for embedded album covers in non-collection tracks and
in USB Mass Storage collection.
* Hold the Shift key when dragging tracks to collections to move them
====================================================================
I wanted to test this before pushing, but I faced 2 problems:
* TagLib 1.8 was not yet released and I didn't feel like compiling it by hand
* I found it hard to get example files of all the formats. Do you have some? It would be ideal if you had one small file of each format that would have a license compatible with Amarok so that they can be added to Amarok sources. Then you could write a test for these to ensure tag reading works in all future versions.
I think this patch _should_ be merged one day, but IMO we should wait for:
a) TagLib 1.8 release
b) ability to easily test the feature. Ideally if example files are added to Amarok sources and test-case is written, see the tests/ Amarok subdirectory. (Don't forget to enable KDE4_BUILD_TESTS cmake var; tests are then run using `make test`)</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;">As I patched TagLib I also added some basic tests and with them some small example files, so you can take these: https://github.com/taglib/taglib/tree/master/tests/data I created the files with the original trackers or with MilkyTracker and MODPlug Tracker (not sure which file I created with which program). They don't really contain any music, only some empty samples. If you want some real-life example files take a look at http://www.chiptune.com/ or http://keygenmusic.net/. There is no license information on the files there, but they are certainly free.</pre>
<br />
<p>- Mathias Stephan</p>
<br />
<p>On February 4th, 2012, 4:32 p.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 Feb. 4, 2012, 4:32 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 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/FileTypeResolver.cpp <span style="color: grey">(e69a514)</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>