Review Request 113389: GSoC 2013 Revamping Scripting - Part 5/6 : Misc shared/

[Matěj Laitl]

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


Not yet a "Skip it" from me, please answer some important questions below. I guess this patch could be renamed to "Add support for reading lyrics from tags", right?


shared/MetaTagLib.h
<http://git.reviewboard.kde.org/r/113389/#comment31476>

    I don't understand the need for this method. Isn't readTags() supposed to read lyrics and return them under Meta::valLyrics key?
    
    What does this method return? Maps what to what? It would need documentation.



shared/MetaTagLib.cpp
<http://git.reviewboard.kde.org/r/113389/#comment31477>

    You leak tagHelper in this case, I propose using QScopedPointer.
    
    Also, I totally don't get the purpose - if ID3v2TagHelper needs special casing, it needs to be *inside* it.



shared/MetaValues.h
<http://git.reviewboard.kde.org/r/113389/#comment31478>

    No, please add valLyrics after valImage with value (1LL << 40) + 3;



shared/tag_helpers/ID3v2TagHelper.h
<http://git.reviewboard.kde.org/r/113389/#comment31479>

    why the need for a custom method?


- Matěj Laitl


On Oct. 22, 2013, 6:06 p.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113389/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2013, 6:06 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Repository: amarok
> 
> 
> Description
> -------
> 
> Changes made to the tagging framework in shared/.
> 
> 
> Diffs
> -----
> 
>   shared/MetaTagLib.h 2de3be2 
>   shared/MetaTagLib.cpp d42dd32 
>   shared/MetaValues.h 4b003ed 
>   shared/tag_helpers/APETagHelper.cpp ba39a10 
>   shared/tag_helpers/ASFTagHelper.cpp 3439845 
>   shared/tag_helpers/ID3v2TagHelper.h a9c9ab9 
>   shared/tag_helpers/ID3v2TagHelper.cpp 03a3836 
>   shared/tag_helpers/MP4TagHelper.cpp 23d8879 
>   shared/tag_helpers/VorbisCommentTagHelper.cpp 8ff52c5 
> 
> Diff: http://git.reviewboard.kde.org/r/113389/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20131117/9a6424f4/attachment-0001.html>