----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
