Review Request: tag reading support for mod, s3m, it and xm files

[Matěj Laitl]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101598/#review10375
-----------------------------------------------------------


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`)

- Matěj Laitl


On Feb. 4, 2012, 4:32 p.m., Mathias Stephan Panzenböck wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101598/
> -----------------------------------------------------------
> 
> (Updated Feb. 4, 2012, 4:32 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> 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
> 
> 
> This addresses bug 90524.
>     https://bugs.kde.org/show_bug.cgi?id=90524
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt d47c28b 
>   config-amarok.h.cmake 2d25cc7 
>   shared/FileType.h 5c8081f 
>   shared/FileType.cpp a6e25d5 
>   shared/FileTypeResolver.cpp e69a514 
>   shared/tag_helpers/TagHelper.cpp 4c0fb2b 
> 
> Diff: http://git.reviewboard.kde.org/r/101598/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Mathias Stephan Panzenböck
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120206/d1870fe7/attachment.html>