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/#review10279
-----------------------------------------------------------


This change looks good, once minor problems are resolved I think this should be pushed.


shared/tag_helpers/TagHelper.cpp
<http://git.reviewboard.kde.org/r/101598/#comment8457>

    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.


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)

- Matěj Laitl


On Jan. 31, 2012, 2:12 a.m., Mathias Stephan Panzenböck wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101598/
> -----------------------------------------------------------
> 
> (Updated Jan. 31, 2012, 2:12 a.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/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/20120202/5d3e5cac/attachment-0001.html>