From 3fa295d99ded94d90cabc0dfe7737b7e8f617d9d Mon Sep 17 00:00:00 2001 From: naota Date: Wed, 28 Nov 2012 07:54:08 +0900 Subject: [PATCH 1/8] Include sys/stat.h to define S_* properly Without including sys/stat.h, this file failed to build on FreeBSD with the following error. In file included from /var/tmp/portage/media-libs/taglib-1.8/work/taglib-1.8/tests/test_trueaudio.cpp:5:0: /var/tmp/portage/media-libs/taglib-1.8/work/taglib-1.8/tests/utils.h: In function 'std::string copyFile(const string&, const string&)': /var/tmp/portage/media-libs/taglib-1.8/work/taglib-1.8/tests/utils.h:36:62: error: 'S_IRUSR' was not declared in this scope /var/tmp/portage/media-libs/taglib-1.8/work/taglib-1.8/tests/utils.h:36:72: error: 'S_IWUSR' was not declared in this scope In file included from /var/tmp/portage/media-libs/taglib-1.8/work/taglib-1.8/tests/test_mpeg.cpp:6:0: /var/tmp/portage/media-libs/taglib-1.8/work/taglib-1.8/tests/utils.h: In function 'std::string copyFile(const string&, const string&)': /var/tmp/portage/media-libs/taglib-1.8/work/taglib-1.8/tests/utils.h:36:62: error: 'S_IRUSR' was not declared in this scope /var/tmp/portage/media-libs/taglib-1.8/work/taglib-1.8/tests/utils.h:36:72: error: 'S_IWUSR' was not declared in this scope gmake[2]: *** [tests/CMakeFiles/test_runner.dir/test_mpeg.cpp.o] Error 1 --- tests/utils.h | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/utils.h b/tests/utils.h index 39e15ce9..b69bfa50 100644 --- a/tests/utils.h +++ b/tests/utils.h @@ -7,6 +7,7 @@ #include #include #include +#include #endif #include #include From 9eb0f2941f266f856b729dc46a997bed69aefc6a Mon Sep 17 00:00:00 2001 From: Michael Helmling Date: Mon, 10 Dec 2012 19:55:23 +0100 Subject: [PATCH 2/8] Add a test case for the return value of setProperties() --- tests/test_flac.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/test_flac.cpp b/tests/test_flac.cpp index 992a02b8..f6a65e09 100644 --- a/tests/test_flac.cpp +++ b/tests/test_flac.cpp @@ -24,6 +24,7 @@ class TestFLAC : public CppUnit::TestFixture CPPUNIT_TEST(testRepeatedSave); CPPUNIT_TEST(testSaveMultipleValues); CPPUNIT_TEST(testDict); + CPPUNIT_TEST(testInvalid); CPPUNIT_TEST_SUITE_END(); public: @@ -231,6 +232,17 @@ public: CPPUNIT_ASSERT_EQUAL(String("artöst 2"), dict["ARTIST"][1]); } + void testInvalid() + { + ScopedFileCopy copy("silence-44-s", ".flac"); + PropertyMap map; + map["HÄÖ"] = String("bla"); + FLAC::File f(copy.fileName().c_str()); + PropertyMap invalid = f.setProperties(map); + CPPUNIT_ASSERT_EQUAL(uint(1), invalid.size()); + CPPUNIT_ASSERT_EQUAL(uint(0), f.properties().size()); + } + }; CPPUNIT_TEST_SUITE_REGISTRATION(TestFLAC); From c0ca5c97d51512a40b4f75f0daeea7ce44302ec9 Mon Sep 17 00:00:00 2001 From: Michael Helmling Date: Mon, 10 Dec 2012 20:56:16 +0100 Subject: [PATCH 3/8] Fix issue #88 by changing the behavior of setProperties(). For file types that support multiple tag standards (for example, FLAC files can have ID3v1, ID3v2, and Vorbis comments) setProperties is now called for all existing tags instead of only for the most recommended one. This fixes the problem that under some circumstances it was not possible to delete a value using setProperties() because upon save() the call to Tag::duplicate recovered that value from the ID3v1 tag. --- taglib/ape/apefile.cpp | 12 ++++++------ taglib/flac/flacfile.cpp | 16 ++++++++-------- taglib/mpc/mpcfile.cpp | 13 ++++++------- taglib/mpeg/mpegfile.cpp | 17 +++++++++-------- taglib/trueaudio/trueaudiofile.cpp | 12 ++++++------ taglib/wavpack/wavpackfile.cpp | 12 ++++++------ 6 files changed, 41 insertions(+), 41 deletions(-) diff --git a/taglib/ape/apefile.cpp b/taglib/ape/apefile.cpp index 2572a2e9..fae4ab03 100644 --- a/taglib/ape/apefile.cpp +++ b/taglib/ape/apefile.cpp @@ -131,12 +131,12 @@ void APE::File::removeUnsupportedProperties(const StringList &properties) PropertyMap APE::File::setProperties(const PropertyMap &properties) { - if(d->hasAPE) - return d->tag.access(ApeAPEIndex, false)->setProperties(properties); - else if(d->hasID3v1) - return d->tag.access(ApeID3v1Index, false)->setProperties(properties); - else - return d->tag.access(ApeAPEIndex, true)->setProperties(properties); + PropertyMap result; + if(d->hasID3v1) + result = d->tag.access(ApeID3v1Index, false)->setProperties(properties); + if(d->hasAPE || !d->hasID3v1) + result = d->tag.access(ApeAPEIndex, true)->setProperties(properties); + return result; } APE::Properties *APE::File::audioProperties() const diff --git a/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp index 0bfe84c3..26d79974 100644 --- a/taglib/flac/flacfile.cpp +++ b/taglib/flac/flacfile.cpp @@ -168,14 +168,14 @@ void FLAC::File::removeUnsupportedProperties(const StringList &unsupported) PropertyMap FLAC::File::setProperties(const PropertyMap &properties) { - if(d->hasXiphComment) - return d->tag.access(FlacXiphIndex, false)->setProperties(properties); - else if(d->hasID3v2) - return d->tag.access(FlacID3v2Index, false)->setProperties(properties); - else if(d->hasID3v1) - return d->tag.access(FlacID3v1Index, false)->setProperties(properties); - else - return d->tag.access(FlacXiphIndex, true)->setProperties(properties); + PropertyMap result; + if(d->hasID3v1) + result = d->tag.access(FlacID3v1Index, false)->setProperties(properties); + if(d->hasID3v2) + result = d->tag.access(FlacID3v2Index, false)->setProperties(properties); + if(d->hasXiphComment || !(d->hasID3v1 || d->hasID3v2)) + result = d->tag.access(FlacXiphIndex, true)->setProperties(properties); + return result; } FLAC::Properties *FLAC::File::audioProperties() const diff --git a/taglib/mpc/mpcfile.cpp b/taglib/mpc/mpcfile.cpp index 18f533f8..6d6b3f6b 100644 --- a/taglib/mpc/mpcfile.cpp +++ b/taglib/mpc/mpcfile.cpp @@ -135,15 +135,14 @@ void MPC::File::removeUnsupportedProperties(const StringList &properties) PropertyMap MPC::File::setProperties(const PropertyMap &properties) { - if(d->hasAPE) - return d->tag.access(MPCAPEIndex, false)->setProperties(properties); - else if(d->hasID3v1) - return d->tag.access(MPCID3v1Index, false)->setProperties(properties); - else - return d->tag.access(APE, true)->setProperties(properties); + PropertyMap result; + if(d->hasID3v1) + result = d->tag.access(MPCID3v1Index, false)->setProperties(properties); + if(d->hasAPE || !d->hasID3v1) + result = d->tag.access(MPCAPEIndex, true)->setProperties(properties); + return result; } - MPC::Properties *MPC::File::audioProperties() const { return d->properties; diff --git a/taglib/mpeg/mpegfile.cpp b/taglib/mpeg/mpegfile.cpp index ec094cdc..90754007 100644 --- a/taglib/mpeg/mpegfile.cpp +++ b/taglib/mpeg/mpegfile.cpp @@ -156,16 +156,17 @@ void MPEG::File::removeUnsupportedProperties(const StringList &properties) else if(d->hasID3v1) d->tag.access(ID3v1Index, false)->removeUnsupportedProperties(properties); } + PropertyMap MPEG::File::setProperties(const PropertyMap &properties) { - if(d->hasID3v2) - return d->tag.access(ID3v2Index, false)->setProperties(properties); - else if(d->hasAPE) - return d->tag.access(APEIndex, false)->setProperties(properties); - else if(d->hasID3v1) - return d->tag.access(ID3v1Index, false)->setProperties(properties); - else - return d->tag.access(ID3v2Index, true)->setProperties(properties); + PropertyMap result; + if(d->hasID3v1) + result = d->tag.access(ID3v1Index, false)->setProperties(properties); + if(d->hasAPE) + result = d->tag.access(APEIndex, false)->setProperties(properties); + if(d->hasID3v2 || !(d->hasID3v1 || d->hasAPE)) + result = d->tag.access(ID3v2Index, true)->setProperties(properties); + return result; } MPEG::Properties *MPEG::File::audioProperties() const diff --git a/taglib/trueaudio/trueaudiofile.cpp b/taglib/trueaudio/trueaudiofile.cpp index 9efc6e9d..6af72aaa 100644 --- a/taglib/trueaudio/trueaudiofile.cpp +++ b/taglib/trueaudio/trueaudiofile.cpp @@ -141,12 +141,12 @@ PropertyMap TrueAudio::File::properties() const PropertyMap TrueAudio::File::setProperties(const PropertyMap &properties) { - if(d->hasID3v2) - return d->tag.access(TrueAudioID3v2Index, false)->setProperties(properties); - else if(d->hasID3v1) - return d->tag.access(TrueAudioID3v1Index, false)->setProperties(properties); - else - return d->tag.access(TrueAudioID3v2Index, true)->setProperties(properties); + PropertyMap result; + if(d->hasID3v1) + result = d->tag.access(TrueAudioID3v1Index, false)->setProperties(properties); + if(d->hasID3v2 || !d->hasID3v1) + result =d->tag.access(TrueAudioID3v2Index, true)->setProperties(properties); + return result; } TrueAudio::Properties *TrueAudio::File::audioProperties() const diff --git a/taglib/wavpack/wavpackfile.cpp b/taglib/wavpack/wavpackfile.cpp index 2d1f8cd9..6cc86527 100644 --- a/taglib/wavpack/wavpackfile.cpp +++ b/taglib/wavpack/wavpackfile.cpp @@ -119,12 +119,12 @@ PropertyMap WavPack::File::properties() const PropertyMap WavPack::File::setProperties(const PropertyMap &properties) { - if(d->hasAPE) - return d->tag.access(WavAPEIndex, false)->setProperties(properties); - else if(d->hasID3v1) - return d->tag.access(WavID3v1Index, false)->setProperties(properties); - else - return d->tag.access(APE, true)->setProperties(properties); + PropertyMap result; + if(d->hasID3v1) + result = d->tag.access(WavID3v1Index, false)->setProperties(properties); + if(d->hasAPE || !d->hasID3v1) + result = d->tag.access(WavAPEIndex, true)->setProperties(properties); + return result; } WavPack::Properties *WavPack::File::audioProperties() const From 6e3391a846655d898280c60aa52f6b27164412c9 Mon Sep 17 00:00:00 2001 From: Michael Helmling Date: Mon, 10 Dec 2012 21:22:11 +0100 Subject: [PATCH 4/8] Add a test to show a problem with properties() and duplication. --- tests/data/id3v1_neq_v2.mp3 | Bin 0 -> 7825 bytes tests/test_id3v2.cpp | 17 +++++++++++++++++ 2 files changed, 17 insertions(+) create mode 100644 tests/data/id3v1_neq_v2.mp3 diff --git a/tests/data/id3v1_neq_v2.mp3 b/tests/data/id3v1_neq_v2.mp3 new file mode 100644 index 0000000000000000000000000000000000000000..984cb1d46aff7b3027c661e67816022e575cb9ce GIT binary patch literal 7825 zcmeHsXIK=$)8{Pg!U9W>BuU9g&PWg>hb89>vasYJARwa1k~0Fr5>#?dl9vo3QF2gn z5Cq8}NrJeC|Mx!M``mrId#>i0>FTcT`c+rg(>*fSGnt=TPO&AhE*trBg}*6ilgj@343^HqyolX;^~^L6uE%t;4nt0?AI zQx7J(#jeG!`A?gB)mhN`ltDSmQMfl z_NDiw|HHXO9UsWK|Fo=CWiwkq7X6#Jdp4!T>x-L0eG38)KCD<<&o}q6_yE%Lxja%u3pU)YagU~ zRDf)31i|q+fH-Gh=K|J$4X63TKO9C9=3AY>{{lyLsd>rRr4Jrv43B@x~C380SW-|br2S7v1Nz7rfCIkr%i%$ zYZu%H=8vsgy;)3!5ARJ9xf!tf)D544qZgIF&@Vf4)u;NFMgF*m?US^xVJl!vd*{+<5 zC8iy+xy4=S&(d3-qfF@rYz-uQ>w`R%nrjYaD7iQr^!lSz;Ral_e>iE(t^n`=7|M|Y zpm-=QFarPrtwNYWZ~dkS6j0^>;HDQn8yupiRThmiTZfMi{*y%Iv2%liSqumAEJeGcb_ zGyqT|uelO7?#LIS*%6hwp7*`mg}3b1v#5(=a~MD|Hm>1%>8wB@AlCyZu|#KSq1k2> z1{B~?y&+`N{c4Ipiu?@XEteC`9T3_ok_)N*DDY3&DGf6_w*)AmyCcC< zN|2EHaP{i|+PpIR2)a$Oyj)en;AFx(nzi#x+i%qUFK}vcOCbGn&Jlwc=QKHy#0ga} z5qYcick*HdMNL}KaGK!NRZUJjTPD1iF1;ROr;$tHI=z}43n^T@gy6vII*B578^gtc z7U_gdyTwFHaYIdS=9fLl;uK*6gOPUuIXO2!;_YMci(bfp8D3xbEj}MG=@dDU1o_U( z*uUxMMmlF-@paVUlAO*VvH@_JJ>dXC-5^er8=s|1$g1*W5v0Ze(aRXD~Y zImms;`KJdMs0C5kg+<%i=g=$)Zz|5_WZ@PWvAg-+X#ft(exd7B))L1=mfuymPLL|b z$6v*aN{!X(@VMpw%zfHQn#OPP*Z$lu>mv4=uCbalr3T(YIR@(>4&}_@B;^hF58u@g z_{dex$H=9!#2gyuGg;Nu_TwyKxDycop4Y6`H6Ew9Bo*?J>?%RNywC|RTR$%gX|S5) zY&NVLiZ+^EO0klk9WRxn}zmSDU99>fvZ{7$Qg?R!ldLhlf|X#I#kDUpAsvi& zS9G?iXL(I%Zv-iPC{V{JGfYJhF95e%7v)63)iwS_+%^#o3j9K4YL{25EMj~9g?(s$ za^*cE&Fa}-7i`?65hQ-?*Fr4MHK~#?bNOCH4%-}yA-_Z4@WYW>K&vKzqPTQ{$MZ;xDfW$jM_ zyffZ$9CDN>%Fhq{>|Hm3Se2x9;lXg-yCW;X>8!P3X<2=ZoE!_-PchjpZt-UzN$^ zhUKr>L@>a|3s7?5QLi~(J~|OOJ`;%9ALF_v+)hq)$1tdcN`6*H+|lmQYhn#kqckVa z`XLo57Y@h~r{qG;jZ-E0TlI{owI|nl4|UQ*vYg$t`V|yO4A|u*LbX|Xyd(wEi1F2x z{Ab=$<>KW@L_kc~lsvjrZf1PH{2XaC3%89H2^WgKjB4x}7r$HEw%2H-81>Bk(4FBy z{P4_h#(-E02+Sf`b#=o?-_Z~UdwP5;lj;;)>(hv<`?ToA<(A_zM+RF91wU75A9v_KGNAdHVzAMyhEoC`xj7S_b^ljgl; z#?Aa%X7*nmmWgk?&;0BU(L8s24#*F77l;%oIXFHSE-RN9kv2=;Rd^^o*$qmq6sL#7 zHhRBzl1PzW_dq4Ut%+2;n#gpcQtSC)oJ{KyJmbRKbE^hfI!b|puo@W~@G?rjfd^zF zLR$@%1Pm&w=Xdz-mo-Ed-oy}3EZA`*!n@2pbz`qm8zT^E%L^UE70m0#F;lK-@T)1V za{0;FBxOlYr`3{ovY91ICE02;-}1l;>PRmlAp^c~b>SLjw3&O2Zgs1OFuZ?If zjFL{+>L8-~7`NMFN3P<-TkPH62IXUAvX&V0lf?u<`hJWSLwIr}Gp&tRc zCu*t+Gjo}t%?`9_AfBlYrmV1gfOCyBA&y*|rcma~ z!Rb*uv5~H59>%EM7;XR+mu_ISNgh&yM&ARtfW zNRwKhtpb7=E|qUg3KWwx`wW=fJjPX&kdb6h)D>I8%tuPL6t;X8gBZWoFENC`iJ_4~ zVWt?=uJ)#h(Z&co zjQw78?a)1PyF%rs8=qbflPYpU4?Oga3kjsmt!fp4kiAyy8EMPkCzBEcN1qCWxr*{8 z)U?Ie{GE?msC9)a9uha<(KsiNQ7Uu==!L&cE>tPyPOt8GC%Q04zqk3aIsXa&*O>F} z=)d=sdvn!4-8TsI%r>dBPW4rO>=0z&!h<(*PkE~<2UFng1M?+_vMJV4ofa1Vk9xr$LZtBEJ^!ah4Be+gD9Gj zV(m{*m^gJ8Uf(d9^%A<%oPzJ+Xxp@k?7rK?W=$lV^w?T@^7!k=y$7mkJq?(3;mEh} z9Xd1qhZI_a9*t^jRV7FArKvLodb@v8MXNd060K+cid$9GA**652Ox7E$(T>6%by&& zYA!6xP&hChZ43|yB^3cle0+dOe*-A?qTCZ>3vQg0ZTWRUXo@P5%ByzPZncsur&+fEu5HfP!`9nGxsm@vhMx(?ZbMhjM=va88PcI zp=Q1zT51OLsDaASN7C7Logg|)ZOYuQlFTzr+d7)eG3-qSLG=Kb>j8FGQ3aU~Rp8RZ zx6#$)Aavmmm=rk1)yz?t-o9f{qAn>Gs+P(Rf7esf5+qVcKrn1!Zcrkwso}L?aTiAu ztu4f9uyJO3O!$!VN>4=PWT*>yqzbC$b#81Iror zH{7~2d9m?X%V%phi8m?NfsoBsk;bQpOwhCbL=*)xtI7Wa8E(yZ8Kx;%zkN<4gD^T8 zXB$;+Cf~uu)$&J-N2rKY`0L;?&uYN+`G7M)@2= zk3qced;?MI*}nPyoJ`uh#Hr|D-*Ez3w75!ss#$}1_CEhj@6tPNR1``#+Wp~zR~INV zF)eBr4P&C*Hyqa0BSX*mB}wQ~HmcaFC9%bOdm+4wK6YUQK)cWc1~ zv7ZgJU%EOP|K?xuitqG1ZMei}sZzRm_w8$O*3Do3){A3oR)$yZ*e`UPPKSBCH1NhDpZC znXnc9Yi!7e(2&h#n_XN`?nGyDZCY19u5GMtf4=tAjvz+?_U@9;RNeDC;nIBeH_jpn z%B&TO98P(M)0wuhGj>8?Rj`c=|D1JNn;MBSZrKh>SftU3qkUi1osL{`FH9Gc3j5J}>_0E2I+Oqetoqde zrY)7=Jl~Gs=%*B8)~m3O-J!eZ;v8AThGQ{w79}Efv0sM~df&xYr(&&tUJUL=M0zLC z-HSw-dPH?S6E%NCJ&wmmrK+e;lSX&6S|P#cnO9?ZAo9Z%4c$v{AZJ*5>HeXL_k%TS zi>?7|Qi#s$`htZ77;ibW+f)Sqkj~-S<2l{!r}Yf>hHO$6 zSN*O=#FYVWvC4hV_v~?ta|`CtDyH*?YSizoAUSPA6No{ZS^YYf`x;dSQg!+muf|f& z6is~7>4`J}azs5JcziBg>oEb2E(bW$@-=5X1%=GUpso}yFEshcTqQa= zDT2>hv1C3W3mj1@^qM<)a#PsrcUzDd*4K8tOBW#Y*l$GDfjrCJ%ixQH+Z-JDh?(aJ zo7^_*$(T1q0$`M37}G=vf|ofEg#A#>M+Fopjr&P@MwOub3VbE#~) zZy|S(92-(?D~0}C4AO=o;wjYXpvA|U#>60 zj(dWyIFvj|KRdx}pP1z2nTj;BB@khU>!s-xEIWs6m*R8>P8Xee>e)eQo|3I^v2mC= zM2tki=`~bADwbpEsS5Yt9*rQf;W@$FS8~K&Oy)d@x5BqSU?hd}s&vU3(-%^Kt@KhV89B~zl zCTdKA)u?6+8>RC%kC|rFAdm8kv))%MvCW!$=@%4)4v#TPTKAgOoT zQZqB`UjI_SUMZ3q;Y!IP630{|>f{&Sus_wPl7Od@seteFtS-El8`w8L{ z^N>iZz4P}fwOU}(adeLO;aZH$$A$AlXjoUT4Z~ zSz4#;z~SS~%LhbqIimKLmepG}{4^b6Y;+=0e9L>^wS%8ldI3)9vc4 z9F}!R#lBgW-9PL7AR6Mvq-1-du}RGc3Xut9R_Ruje7f?A?X+s`)x*&`SeYqQYB5?J zXPj4JD44!Kxioc(H$1~E8ve9um2+&qtfLa-#vB)-UA^aR*kyb0Ryrf_!S-}ucejDJ z^22%01g2W^2ex-S*h1t#9VxUOnHVI09pvI1a5Z(R2qMgWmY~6%{5Z2(Tb1n-;8MUN z9OeQ`uAAKAhz*0zI=4PQm}8KySfYLS`f!kURBP%s_?=wEC!b; zq``)s`6?_AB3dE()t(hRg1_yH(ZbI0}g1(m`KgHK2VEv*Pu>31eJ5IZ!WndFsp ze~Iv5NpSM1sAUN7gGC8W9;=m+I;xdce~&M^CtUG+e~OAB17A0vlYqXO`D9atBAFfs z`z)W)QZ z&0+W2a|v4Bsf8503p#_Wa_ZNXgqdGgtugTt=z3P*-meGsKF$N{c$Ko0Si&9!x zU9dPAxr_9*rE(%9#~-;vp1A198sh(GE1g@7m5^D5q~0q1ElU%jj%Muy2L3JW)nL%x zULC{DtB>?SJpHa0XsK~_S`O?@@DFJa2{ZE--SQqwOd4xWAh{r8_##!0QxVX{4ire+ z6_c&AnbNClPZl#*vXMf2K{P7TMt2`}=MV;j6ObL-)*!$y7}~it9(~iyqJSZXzq<7- z#p$@%Q7u{5?6-MM<@Rd`RoC$B?0$_9({TG;`_mijJJA%uXl~4n6Ib3{b;)f^+OmVj zV|1hHw#0-AwzD} z)CIehkpmebWW`>LY4InnhQKGQEArjhYd&*~5`y6K=lyl8&bC0cB{|Ax^*8m7}}n%A@%D@yi>=$op)R>3koX-YrZ0 zW@5s-QfD)ou}!3@JcUx4j-v9ILdelpzGRI(HQ!U5byTgUo+=Vef03IwN;hUF&`1v+ z_0eOqq6!|OY@^F}RB+`o%&-gQ-Qv?!@!Z$Ra_1*}c7jLarWz6{rA_g?_-DvvCL3Mz zTAXX1Yt&xC(^QIvY6HB33=vRzE9Yu$*t}5&8oQrc*c(=Fx#*8$tKL z{1b+9*utHA2_0%@6gh55%7U(LkR<{q(YwUMQH8s2cqk0zRd2Hi|Hp)Iv-NfVPv-bv KGWma>u>S+ug!-cZ literal 0 HcmV?d00001 diff --git a/tests/test_id3v2.cpp b/tests/test_id3v2.cpp index 48faf306..f2a420a8 100644 --- a/tests/test_id3v2.cpp +++ b/tests/test_id3v2.cpp @@ -75,6 +75,7 @@ class TestID3v2 : public CppUnit::TestFixture CPPUNIT_TEST(testW000); CPPUNIT_TEST(testPropertyInterface); CPPUNIT_TEST(testPropertyInterface2); + CPPUNIT_TEST(testBothID3Versions); CPPUNIT_TEST(testDeleteFrame); CPPUNIT_TEST(testSaveAndStripID3v1ShouldNotAddFrameFromID3v1ToId3v2); CPPUNIT_TEST_SUITE_END(); @@ -696,6 +697,22 @@ public: CPPUNIT_ASSERT_EQUAL(frame6, ID3v2::UniqueFileIdentifierFrame::findByOwner(&tag, "http://musicbrainz.org")); } +void testBothID3Versions() + { + ScopedFileCopy copy("id3v1_neq_v2", ".mp3"); + string newname = copy.fileName(); + MPEG::File f(newname.c_str()); + PropertyMap dict = f.properties(); + CPPUNIT_ASSERT(!dict.contains("ALBUM")); + CPPUNIT_ASSERT(dict.contains("ARTIST")); + + f.save(); + MPEG::File f2(newname.c_str()); + PropertyMap dict2 = f.properties(); + CPPUNIT_ASSERT(!dict2.contains("ALBUM")); + CPPUNIT_ASSERT(dict2.contains("ARTIST")); + } + void testDeleteFrame() { ScopedFileCopy copy("rare_frames", ".mp3"); From b14e6a35705eb5e79ace0324a234c782e1572a1f Mon Sep 17 00:00:00 2001 From: Michael Helmling Date: Thu, 20 Dec 2012 17:28:50 +0100 Subject: [PATCH 5/8] Update for pull request #89: Change how setProperties() updates tags. For file types supporting more than one tag format, setProperties() now always creates the most modern one. Deprecated tags are stripped. --- taglib/ape/apefile.cpp | 7 ++----- taglib/ape/apefile.h | 5 +++-- taglib/flac/flacfile.cpp | 9 +++------ taglib/flac/flacfile.h | 5 +++-- taglib/mpc/mpcfile.cpp | 7 ++----- taglib/mpc/mpcfile.h | 4 ++-- taglib/mpeg/mpegfile.cpp | 12 +++++------- taglib/mpeg/mpegfile.h | 13 +++++++++---- taglib/toolkit/tfile.h | 8 ++++++-- taglib/trueaudio/trueaudiofile.cpp | 7 ++----- taglib/trueaudio/trueaudiofile.h | 4 ++-- taglib/wavpack/wavpackfile.cpp | 7 ++----- taglib/wavpack/wavpackfile.h | 4 ++-- 13 files changed, 43 insertions(+), 49 deletions(-) diff --git a/taglib/ape/apefile.cpp b/taglib/ape/apefile.cpp index fae4ab03..7cc1c54d 100644 --- a/taglib/ape/apefile.cpp +++ b/taglib/ape/apefile.cpp @@ -131,12 +131,9 @@ void APE::File::removeUnsupportedProperties(const StringList &properties) PropertyMap APE::File::setProperties(const PropertyMap &properties) { - PropertyMap result; if(d->hasID3v1) - result = d->tag.access(ApeID3v1Index, false)->setProperties(properties); - if(d->hasAPE || !d->hasID3v1) - result = d->tag.access(ApeAPEIndex, true)->setProperties(properties); - return result; + strip(ID3v1); + return d->tag.access(ApeAPEIndex, true)->setProperties(properties); } APE::Properties *APE::File::audioProperties() const diff --git a/taglib/ape/apefile.h b/taglib/ape/apefile.h index 7be0f21d..df335ec6 100644 --- a/taglib/ape/apefile.h +++ b/taglib/ape/apefile.h @@ -128,8 +128,9 @@ namespace TagLib { /*! * Implements the unified property interface -- import function. - * As for the export, only one tag is taken into account. If the file - * has no tag at all, APE will be created. + * Creates an APEv2 tag if necessary. A pontentially existing ID3v1 + * tag is considered deprecated and will be removed, invalidating all + * pointers to that tag. */ PropertyMap setProperties(const PropertyMap &); diff --git a/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp index 26d79974..93f8bd7b 100644 --- a/taglib/flac/flacfile.cpp +++ b/taglib/flac/flacfile.cpp @@ -168,14 +168,11 @@ void FLAC::File::removeUnsupportedProperties(const StringList &unsupported) PropertyMap FLAC::File::setProperties(const PropertyMap &properties) { - PropertyMap result; if(d->hasID3v1) - result = d->tag.access(FlacID3v1Index, false)->setProperties(properties); + d->tag.access(FlacID3v1Index, false)->setProperties(properties); if(d->hasID3v2) - result = d->tag.access(FlacID3v2Index, false)->setProperties(properties); - if(d->hasXiphComment || !(d->hasID3v1 || d->hasID3v2)) - result = d->tag.access(FlacXiphIndex, true)->setProperties(properties); - return result; + d->tag.access(FlacID3v2Index, false)->setProperties(properties); + return d->tag.access(FlacXiphIndex, true)->setProperties(properties); } FLAC::Properties *FLAC::File::audioProperties() const diff --git a/taglib/flac/flacfile.h b/taglib/flac/flacfile.h index 716d4478..76f2e2d3 100644 --- a/taglib/flac/flacfile.h +++ b/taglib/flac/flacfile.h @@ -133,8 +133,9 @@ namespace TagLib { /*! * Implements the unified property interface -- import function. - * As with the export, only one tag is taken into account. If the file - * has no tag at all, a XiphComment will be created. + * This always creates a Xiph comment, if none exists. The return value + * relates to the Xiph comment only. + * Potential ID3v1 and ID3v2 tags will also be updated. */ PropertyMap setProperties(const PropertyMap &); diff --git a/taglib/mpc/mpcfile.cpp b/taglib/mpc/mpcfile.cpp index 6d6b3f6b..c6cb3a41 100644 --- a/taglib/mpc/mpcfile.cpp +++ b/taglib/mpc/mpcfile.cpp @@ -135,12 +135,9 @@ void MPC::File::removeUnsupportedProperties(const StringList &properties) PropertyMap MPC::File::setProperties(const PropertyMap &properties) { - PropertyMap result; if(d->hasID3v1) - result = d->tag.access(MPCID3v1Index, false)->setProperties(properties); - if(d->hasAPE || !d->hasID3v1) - result = d->tag.access(MPCAPEIndex, true)->setProperties(properties); - return result; + strip(ID3v1); + return d->tag.access(MPCAPEIndex, true)->setProperties(properties); } MPC::Properties *MPC::File::audioProperties() const diff --git a/taglib/mpc/mpcfile.h b/taglib/mpc/mpcfile.h index 167b768e..b484be70 100644 --- a/taglib/mpc/mpcfile.h +++ b/taglib/mpc/mpcfile.h @@ -124,8 +124,8 @@ namespace TagLib { /*! * Implements the unified property interface -- import function. - * As with the export, only one tag is taken into account. If the file - * has no tag at all, an APE tag will be created. + * Affects only the APEv2 tag which will be created if necessary. + * If an ID3v1 tag exists, it will be stripped from the file. */ PropertyMap setProperties(const PropertyMap &); diff --git a/taglib/mpeg/mpegfile.cpp b/taglib/mpeg/mpegfile.cpp index 90754007..fd7223c7 100644 --- a/taglib/mpeg/mpegfile.cpp +++ b/taglib/mpeg/mpegfile.cpp @@ -159,14 +159,12 @@ void MPEG::File::removeUnsupportedProperties(const StringList &properties) PropertyMap MPEG::File::setProperties(const PropertyMap &properties) { - PropertyMap result; - if(d->hasID3v1) - result = d->tag.access(ID3v1Index, false)->setProperties(properties); if(d->hasAPE) - result = d->tag.access(APEIndex, false)->setProperties(properties); - if(d->hasID3v2 || !(d->hasID3v1 || d->hasAPE)) - result = d->tag.access(ID3v2Index, true)->setProperties(properties); - return result; + strip(APE, true); + if(d->hasID3v1) + // update ID3v1 tag if it exists, but ignore the return value + d->tag.access(ID3v1Index, false)->setProperties(properties); + return d->tag.access(ID3v2Index, true)->setProperties(properties); } MPEG::Properties *MPEG::File::audioProperties() const diff --git a/taglib/mpeg/mpegfile.h b/taglib/mpeg/mpegfile.h index f7b98364..08415d55 100644 --- a/taglib/mpeg/mpegfile.h +++ b/taglib/mpeg/mpegfile.h @@ -133,7 +133,7 @@ namespace TagLib { virtual Tag *tag() const; /*! - * Implements the unified property interface -- export function. + * Implements the reading part of the unified property interface. * If the file contains more than one tag, only the * first one (in the order ID3v2, APE, ID3v1) will be converted to the * PropertyMap. @@ -143,9 +143,14 @@ namespace TagLib { void removeUnsupportedProperties(const StringList &properties); /*! - * Implements the unified tag dictionary interface -- import function. - * As with the export, only one tag is taken into account. If the file - * has no tag at all, ID3v2 will be created. + * Implements the writing part of the unified tag dictionary interface. + * In order to avoid problems with deprecated tag formats, this method + * always creates an ID3v2 tag if necessary, and removes potential APEv2 + * tags (also invalidating all pointers to the APE tag) which are + * considered bad practice in MP3 files. + * If an ID3v1 tag exists, it will be updated as well, within the + * limitations of that format. + * The returned PropertyMap refers to the ID3v2 tag only. */ PropertyMap setProperties(const PropertyMap &); diff --git a/taglib/toolkit/tfile.h b/taglib/toolkit/tfile.h index 7e6f2b93..4b231d06 100644 --- a/taglib/toolkit/tfile.h +++ b/taglib/toolkit/tfile.h @@ -80,8 +80,8 @@ namespace TagLib { /*! * Exports the tags of the file as dictionary mapping (human readable) tag - * names (Strings) to StringLists of tag values. Calls the according specialization - * in the File subclasses. + * names (uppercase Strings) to StringLists of tag values. Calls the according + * specialization in the File subclasses. * For each metadata object of the file that could not be parsed into the PropertyMap * format, the returend map's unsupportedData() list will contain one entry identifying * that object (e.g. the frame type for ID3v2 tags). Use removeUnsupportedProperties() @@ -105,6 +105,10 @@ namespace TagLib { * If some value(s) could not be written imported to the specific metadata format, * the returned PropertyMap will contain those value(s). Otherwise it will be empty, * indicating that no problems occured. + * With file types that support several tag formats (for instance, MP3 files can have + * ID3v1, ID3v2, and APEv2 tags), this function will create the most appropriate one + * and may remove deprecated ones (mostly ID3v1). See the documentation of the + * subclass implementations for detailed descriptions. * BIC: will become pure virtual in the future */ PropertyMap setProperties(const PropertyMap &properties); diff --git a/taglib/trueaudio/trueaudiofile.cpp b/taglib/trueaudio/trueaudiofile.cpp index 6af72aaa..85e5a21b 100644 --- a/taglib/trueaudio/trueaudiofile.cpp +++ b/taglib/trueaudio/trueaudiofile.cpp @@ -141,12 +141,9 @@ PropertyMap TrueAudio::File::properties() const PropertyMap TrueAudio::File::setProperties(const PropertyMap &properties) { - PropertyMap result; if(d->hasID3v1) - result = d->tag.access(TrueAudioID3v1Index, false)->setProperties(properties); - if(d->hasID3v2 || !d->hasID3v1) - result =d->tag.access(TrueAudioID3v2Index, true)->setProperties(properties); - return result; + d->tag.access(TrueAudioID3v1Index, false)->setProperties(properties); + return d->tag.access(TrueAudioID3v2Index, true)->setProperties(properties); } TrueAudio::Properties *TrueAudio::File::audioProperties() const diff --git a/taglib/trueaudio/trueaudiofile.h b/taglib/trueaudio/trueaudiofile.h index e3e1fe62..1b8a9353 100644 --- a/taglib/trueaudio/trueaudiofile.h +++ b/taglib/trueaudio/trueaudiofile.h @@ -139,8 +139,8 @@ namespace TagLib { /*! * Implements the unified property interface -- import function. - * As with the export, only one tag is taken into account. If the file - * has no tag at all, ID3v2 will be created. + * Creates in ID3v2 tag if necessary. If an ID3v1 tag exists, it will + * be updated as well, within the limitations of ID3v1. */ PropertyMap setProperties(const PropertyMap &); diff --git a/taglib/wavpack/wavpackfile.cpp b/taglib/wavpack/wavpackfile.cpp index 6cc86527..67be997c 100644 --- a/taglib/wavpack/wavpackfile.cpp +++ b/taglib/wavpack/wavpackfile.cpp @@ -119,12 +119,9 @@ PropertyMap WavPack::File::properties() const PropertyMap WavPack::File::setProperties(const PropertyMap &properties) { - PropertyMap result; if(d->hasID3v1) - result = d->tag.access(WavID3v1Index, false)->setProperties(properties); - if(d->hasAPE || !d->hasID3v1) - result = d->tag.access(WavAPEIndex, true)->setProperties(properties); - return result; + strip(ID3v1); + return d->tag.access(WavAPEIndex, true)->setProperties(properties); } WavPack::Properties *WavPack::File::audioProperties() const diff --git a/taglib/wavpack/wavpackfile.h b/taglib/wavpack/wavpackfile.h index 5bbbc65a..3e0c36c4 100644 --- a/taglib/wavpack/wavpackfile.h +++ b/taglib/wavpack/wavpackfile.h @@ -118,8 +118,8 @@ namespace TagLib { /*! * Implements the unified property interface -- import function. - * As for the export, only one tag is taken into account. If the file - * has no tag at all, APE will be created. + * Creates an APE tag if it does not exists and calls setProperties() on + * that. Any existing ID3v1 tag will be removed. */ PropertyMap setProperties(const PropertyMap&); From a095c468b2643f5a7c645d78fef91784092c30c9 Mon Sep 17 00:00:00 2001 From: Michael Helmling Date: Thu, 20 Dec 2012 17:30:19 +0100 Subject: [PATCH 6/8] Revert "Add a test to show a problem with properties() and duplication." This reverts commit 6e3391a846655d898280c60aa52f6b27164412c9. The "problem" demonstrated in there won't be fixed due to lack of significance. --- tests/data/id3v1_neq_v2.mp3 | Bin 7825 -> 0 bytes tests/test_id3v2.cpp | 17 ----------------- 2 files changed, 17 deletions(-) delete mode 100644 tests/data/id3v1_neq_v2.mp3 diff --git a/tests/data/id3v1_neq_v2.mp3 b/tests/data/id3v1_neq_v2.mp3 deleted file mode 100644 index 984cb1d46aff7b3027c661e67816022e575cb9ce..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 7825 zcmeHsXIK=$)8{Pg!U9W>BuU9g&PWg>hb89>vasYJARwa1k~0Fr5>#?dl9vo3QF2gn z5Cq8}NrJeC|Mx!M``mrId#>i0>FTcT`c+rg(>*fSGnt=TPO&AhE*trBg}*6ilgj@343^HqyolX;^~^L6uE%t;4nt0?AI zQx7J(#jeG!`A?gB)mhN`ltDSmQMfl z_NDiw|HHXO9UsWK|Fo=CWiwkq7X6#Jdp4!T>x-L0eG38)KCD<<&o}q6_yE%Lxja%u3pU)YagU~ zRDf)31i|q+fH-Gh=K|J$4X63TKO9C9=3AY>{{lyLsd>rRr4Jrv43B@x~C380SW-|br2S7v1Nz7rfCIkr%i%$ zYZu%H=8vsgy;)3!5ARJ9xf!tf)D544qZgIF&@Vf4)u;NFMgF*m?US^xVJl!vd*{+<5 zC8iy+xy4=S&(d3-qfF@rYz-uQ>w`R%nrjYaD7iQr^!lSz;Ral_e>iE(t^n`=7|M|Y zpm-=QFarPrtwNYWZ~dkS6j0^>;HDQn8yupiRThmiTZfMi{*y%Iv2%liSqumAEJeGcb_ zGyqT|uelO7?#LIS*%6hwp7*`mg}3b1v#5(=a~MD|Hm>1%>8wB@AlCyZu|#KSq1k2> z1{B~?y&+`N{c4Ipiu?@XEteC`9T3_ok_)N*DDY3&DGf6_w*)AmyCcC< zN|2EHaP{i|+PpIR2)a$Oyj)en;AFx(nzi#x+i%qUFK}vcOCbGn&Jlwc=QKHy#0ga} z5qYcick*HdMNL}KaGK!NRZUJjTPD1iF1;ROr;$tHI=z}43n^T@gy6vII*B578^gtc z7U_gdyTwFHaYIdS=9fLl;uK*6gOPUuIXO2!;_YMci(bfp8D3xbEj}MG=@dDU1o_U( z*uUxMMmlF-@paVUlAO*VvH@_JJ>dXC-5^er8=s|1$g1*W5v0Ze(aRXD~Y zImms;`KJdMs0C5kg+<%i=g=$)Zz|5_WZ@PWvAg-+X#ft(exd7B))L1=mfuymPLL|b z$6v*aN{!X(@VMpw%zfHQn#OPP*Z$lu>mv4=uCbalr3T(YIR@(>4&}_@B;^hF58u@g z_{dex$H=9!#2gyuGg;Nu_TwyKxDycop4Y6`H6Ew9Bo*?J>?%RNywC|RTR$%gX|S5) zY&NVLiZ+^EO0klk9WRxn}zmSDU99>fvZ{7$Qg?R!ldLhlf|X#I#kDUpAsvi& zS9G?iXL(I%Zv-iPC{V{JGfYJhF95e%7v)63)iwS_+%^#o3j9K4YL{25EMj~9g?(s$ za^*cE&Fa}-7i`?65hQ-?*Fr4MHK~#?bNOCH4%-}yA-_Z4@WYW>K&vKzqPTQ{$MZ;xDfW$jM_ zyffZ$9CDN>%Fhq{>|Hm3Se2x9;lXg-yCW;X>8!P3X<2=ZoE!_-PchjpZt-UzN$^ zhUKr>L@>a|3s7?5QLi~(J~|OOJ`;%9ALF_v+)hq)$1tdcN`6*H+|lmQYhn#kqckVa z`XLo57Y@h~r{qG;jZ-E0TlI{owI|nl4|UQ*vYg$t`V|yO4A|u*LbX|Xyd(wEi1F2x z{Ab=$<>KW@L_kc~lsvjrZf1PH{2XaC3%89H2^WgKjB4x}7r$HEw%2H-81>Bk(4FBy z{P4_h#(-E02+Sf`b#=o?-_Z~UdwP5;lj;;)>(hv<`?ToA<(A_zM+RF91wU75A9v_KGNAdHVzAMyhEoC`xj7S_b^ljgl; z#?Aa%X7*nmmWgk?&;0BU(L8s24#*F77l;%oIXFHSE-RN9kv2=;Rd^^o*$qmq6sL#7 zHhRBzl1PzW_dq4Ut%+2;n#gpcQtSC)oJ{KyJmbRKbE^hfI!b|puo@W~@G?rjfd^zF zLR$@%1Pm&w=Xdz-mo-Ed-oy}3EZA`*!n@2pbz`qm8zT^E%L^UE70m0#F;lK-@T)1V za{0;FBxOlYr`3{ovY91ICE02;-}1l;>PRmlAp^c~b>SLjw3&O2Zgs1OFuZ?If zjFL{+>L8-~7`NMFN3P<-TkPH62IXUAvX&V0lf?u<`hJWSLwIr}Gp&tRc zCu*t+Gjo}t%?`9_AfBlYrmV1gfOCyBA&y*|rcma~ z!Rb*uv5~H59>%EM7;XR+mu_ISNgh&yM&ARtfW zNRwKhtpb7=E|qUg3KWwx`wW=fJjPX&kdb6h)D>I8%tuPL6t;X8gBZWoFENC`iJ_4~ zVWt?=uJ)#h(Z&co zjQw78?a)1PyF%rs8=qbflPYpU4?Oga3kjsmt!fp4kiAyy8EMPkCzBEcN1qCWxr*{8 z)U?Ie{GE?msC9)a9uha<(KsiNQ7Uu==!L&cE>tPyPOt8GC%Q04zqk3aIsXa&*O>F} z=)d=sdvn!4-8TsI%r>dBPW4rO>=0z&!h<(*PkE~<2UFng1M?+_vMJV4ofa1Vk9xr$LZtBEJ^!ah4Be+gD9Gj zV(m{*m^gJ8Uf(d9^%A<%oPzJ+Xxp@k?7rK?W=$lV^w?T@^7!k=y$7mkJq?(3;mEh} z9Xd1qhZI_a9*t^jRV7FArKvLodb@v8MXNd060K+cid$9GA**652Ox7E$(T>6%by&& zYA!6xP&hChZ43|yB^3cle0+dOe*-A?qTCZ>3vQg0ZTWRUXo@P5%ByzPZncsur&+fEu5HfP!`9nGxsm@vhMx(?ZbMhjM=va88PcI zp=Q1zT51OLsDaASN7C7Logg|)ZOYuQlFTzr+d7)eG3-qSLG=Kb>j8FGQ3aU~Rp8RZ zx6#$)Aavmmm=rk1)yz?t-o9f{qAn>Gs+P(Rf7esf5+qVcKrn1!Zcrkwso}L?aTiAu ztu4f9uyJO3O!$!VN>4=PWT*>yqzbC$b#81Iror zH{7~2d9m?X%V%phi8m?NfsoBsk;bQpOwhCbL=*)xtI7Wa8E(yZ8Kx;%zkN<4gD^T8 zXB$;+Cf~uu)$&J-N2rKY`0L;?&uYN+`G7M)@2= zk3qced;?MI*}nPyoJ`uh#Hr|D-*Ez3w75!ss#$}1_CEhj@6tPNR1``#+Wp~zR~INV zF)eBr4P&C*Hyqa0BSX*mB}wQ~HmcaFC9%bOdm+4wK6YUQK)cWc1~ zv7ZgJU%EOP|K?xuitqG1ZMei}sZzRm_w8$O*3Do3){A3oR)$yZ*e`UPPKSBCH1NhDpZC znXnc9Yi!7e(2&h#n_XN`?nGyDZCY19u5GMtf4=tAjvz+?_U@9;RNeDC;nIBeH_jpn z%B&TO98P(M)0wuhGj>8?Rj`c=|D1JNn;MBSZrKh>SftU3qkUi1osL{`FH9Gc3j5J}>_0E2I+Oqetoqde zrY)7=Jl~Gs=%*B8)~m3O-J!eZ;v8AThGQ{w79}Efv0sM~df&xYr(&&tUJUL=M0zLC z-HSw-dPH?S6E%NCJ&wmmrK+e;lSX&6S|P#cnO9?ZAo9Z%4c$v{AZJ*5>HeXL_k%TS zi>?7|Qi#s$`htZ77;ibW+f)Sqkj~-S<2l{!r}Yf>hHO$6 zSN*O=#FYVWvC4hV_v~?ta|`CtDyH*?YSizoAUSPA6No{ZS^YYf`x;dSQg!+muf|f& z6is~7>4`J}azs5JcziBg>oEb2E(bW$@-=5X1%=GUpso}yFEshcTqQa= zDT2>hv1C3W3mj1@^qM<)a#PsrcUzDd*4K8tOBW#Y*l$GDfjrCJ%ixQH+Z-JDh?(aJ zo7^_*$(T1q0$`M37}G=vf|ofEg#A#>M+Fopjr&P@MwOub3VbE#~) zZy|S(92-(?D~0}C4AO=o;wjYXpvA|U#>60 zj(dWyIFvj|KRdx}pP1z2nTj;BB@khU>!s-xEIWs6m*R8>P8Xee>e)eQo|3I^v2mC= zM2tki=`~bADwbpEsS5Yt9*rQf;W@$FS8~K&Oy)d@x5BqSU?hd}s&vU3(-%^Kt@KhV89B~zl zCTdKA)u?6+8>RC%kC|rFAdm8kv))%MvCW!$=@%4)4v#TPTKAgOoT zQZqB`UjI_SUMZ3q;Y!IP630{|>f{&Sus_wPl7Od@seteFtS-El8`w8L{ z^N>iZz4P}fwOU}(adeLO;aZH$$A$AlXjoUT4Z~ zSz4#;z~SS~%LhbqIimKLmepG}{4^b6Y;+=0e9L>^wS%8ldI3)9vc4 z9F}!R#lBgW-9PL7AR6Mvq-1-du}RGc3Xut9R_Ruje7f?A?X+s`)x*&`SeYqQYB5?J zXPj4JD44!Kxioc(H$1~E8ve9um2+&qtfLa-#vB)-UA^aR*kyb0Ryrf_!S-}ucejDJ z^22%01g2W^2ex-S*h1t#9VxUOnHVI09pvI1a5Z(R2qMgWmY~6%{5Z2(Tb1n-;8MUN z9OeQ`uAAKAhz*0zI=4PQm}8KySfYLS`f!kURBP%s_?=wEC!b; zq``)s`6?_AB3dE()t(hRg1_yH(ZbI0}g1(m`KgHK2VEv*Pu>31eJ5IZ!WndFsp ze~Iv5NpSM1sAUN7gGC8W9;=m+I;xdce~&M^CtUG+e~OAB17A0vlYqXO`D9atBAFfs z`z)W)QZ z&0+W2a|v4Bsf8503p#_Wa_ZNXgqdGgtugTt=z3P*-meGsKF$N{c$Ko0Si&9!x zU9dPAxr_9*rE(%9#~-;vp1A198sh(GE1g@7m5^D5q~0q1ElU%jj%Muy2L3JW)nL%x zULC{DtB>?SJpHa0XsK~_S`O?@@DFJa2{ZE--SQqwOd4xWAh{r8_##!0QxVX{4ire+ z6_c&AnbNClPZl#*vXMf2K{P7TMt2`}=MV;j6ObL-)*!$y7}~it9(~iyqJSZXzq<7- z#p$@%Q7u{5?6-MM<@Rd`RoC$B?0$_9({TG;`_mijJJA%uXl~4n6Ib3{b;)f^+OmVj zV|1hHw#0-AwzD} z)CIehkpmebWW`>LY4InnhQKGQEArjhYd&*~5`y6K=lyl8&bC0cB{|Ax^*8m7}}n%A@%D@yi>=$op)R>3koX-YrZ0 zW@5s-QfD)ou}!3@JcUx4j-v9ILdelpzGRI(HQ!U5byTgUo+=Vef03IwN;hUF&`1v+ z_0eOqq6!|OY@^F}RB+`o%&-gQ-Qv?!@!Z$Ra_1*}c7jLarWz6{rA_g?_-DvvCL3Mz zTAXX1Yt&xC(^QIvY6HB33=vRzE9Yu$*t}5&8oQrc*c(=Fx#*8$tKL z{1b+9*utHA2_0%@6gh55%7U(LkR<{q(YwUMQH8s2cqk0zRd2Hi|Hp)Iv-NfVPv-bv KGWma>u>S+ug!-cZ diff --git a/tests/test_id3v2.cpp b/tests/test_id3v2.cpp index f2a420a8..48faf306 100644 --- a/tests/test_id3v2.cpp +++ b/tests/test_id3v2.cpp @@ -75,7 +75,6 @@ class TestID3v2 : public CppUnit::TestFixture CPPUNIT_TEST(testW000); CPPUNIT_TEST(testPropertyInterface); CPPUNIT_TEST(testPropertyInterface2); - CPPUNIT_TEST(testBothID3Versions); CPPUNIT_TEST(testDeleteFrame); CPPUNIT_TEST(testSaveAndStripID3v1ShouldNotAddFrameFromID3v1ToId3v2); CPPUNIT_TEST_SUITE_END(); @@ -697,22 +696,6 @@ public: CPPUNIT_ASSERT_EQUAL(frame6, ID3v2::UniqueFileIdentifierFrame::findByOwner(&tag, "http://musicbrainz.org")); } -void testBothID3Versions() - { - ScopedFileCopy copy("id3v1_neq_v2", ".mp3"); - string newname = copy.fileName(); - MPEG::File f(newname.c_str()); - PropertyMap dict = f.properties(); - CPPUNIT_ASSERT(!dict.contains("ALBUM")); - CPPUNIT_ASSERT(dict.contains("ARTIST")); - - f.save(); - MPEG::File f2(newname.c_str()); - PropertyMap dict2 = f.properties(); - CPPUNIT_ASSERT(!dict2.contains("ALBUM")); - CPPUNIT_ASSERT(dict2.contains("ARTIST")); - } - void testDeleteFrame() { ScopedFileCopy copy("rare_frames", ".mp3"); From f1d723077f2ee346182982d89febee0494a5b89e Mon Sep 17 00:00:00 2001 From: Michael Helmling Date: Wed, 26 Dec 2012 22:46:37 +0100 Subject: [PATCH 7/8] Consistently handle invalid and deprecated tags in setProperties() This commit reverts the use of strip() in setProperties() because the latter function should not change the file before save() is called. Instead, the following policy is now consistently applied for file formats with multiple tag types: - the recommended tag type is created, if it does not exist - deprecated tags are updated, if they exist, but not created - illegal tag types are ignored by setProperties(), but used in properties() if no others exist. The only tag types considered "illegal" so far are APEv2 in MPEG and ID3 in FLAC. --- taglib/ape/apefile.cpp | 2 +- taglib/ape/apefile.h | 3 +-- taglib/flac/flacfile.cpp | 4 ---- taglib/flac/flacfile.h | 3 ++- taglib/mpc/mpcfile.cpp | 2 +- taglib/mpc/mpcfile.h | 2 +- taglib/mpeg/mpegfile.cpp | 2 -- taglib/mpeg/mpegfile.h | 4 +--- taglib/wavpack/wavpackfile.cpp | 2 +- taglib/wavpack/wavpackfile.h | 2 +- 10 files changed, 9 insertions(+), 17 deletions(-) diff --git a/taglib/ape/apefile.cpp b/taglib/ape/apefile.cpp index 7cc1c54d..415eb17e 100644 --- a/taglib/ape/apefile.cpp +++ b/taglib/ape/apefile.cpp @@ -132,7 +132,7 @@ void APE::File::removeUnsupportedProperties(const StringList &properties) PropertyMap APE::File::setProperties(const PropertyMap &properties) { if(d->hasID3v1) - strip(ID3v1); + d->tag.access(ApeID3v1Index, false)->setProperties(properties); return d->tag.access(ApeAPEIndex, true)->setProperties(properties); } diff --git a/taglib/ape/apefile.h b/taglib/ape/apefile.h index df335ec6..95c60742 100644 --- a/taglib/ape/apefile.h +++ b/taglib/ape/apefile.h @@ -129,8 +129,7 @@ namespace TagLib { /*! * Implements the unified property interface -- import function. * Creates an APEv2 tag if necessary. A pontentially existing ID3v1 - * tag is considered deprecated and will be removed, invalidating all - * pointers to that tag. + * tag will be updated as well. */ PropertyMap setProperties(const PropertyMap &); diff --git a/taglib/flac/flacfile.cpp b/taglib/flac/flacfile.cpp index 93f8bd7b..972fe728 100644 --- a/taglib/flac/flacfile.cpp +++ b/taglib/flac/flacfile.cpp @@ -168,10 +168,6 @@ void FLAC::File::removeUnsupportedProperties(const StringList &unsupported) PropertyMap FLAC::File::setProperties(const PropertyMap &properties) { - if(d->hasID3v1) - d->tag.access(FlacID3v1Index, false)->setProperties(properties); - if(d->hasID3v2) - d->tag.access(FlacID3v2Index, false)->setProperties(properties); return d->tag.access(FlacXiphIndex, true)->setProperties(properties); } diff --git a/taglib/flac/flacfile.h b/taglib/flac/flacfile.h index 76f2e2d3..7423d8f1 100644 --- a/taglib/flac/flacfile.h +++ b/taglib/flac/flacfile.h @@ -135,7 +135,8 @@ namespace TagLib { * Implements the unified property interface -- import function. * This always creates a Xiph comment, if none exists. The return value * relates to the Xiph comment only. - * Potential ID3v1 and ID3v2 tags will also be updated. + * Ignores any changes to ID3v1 or ID3v2 comments since they are not allowed + * in the FLAC specification. */ PropertyMap setProperties(const PropertyMap &); diff --git a/taglib/mpc/mpcfile.cpp b/taglib/mpc/mpcfile.cpp index c6cb3a41..ee992ebc 100644 --- a/taglib/mpc/mpcfile.cpp +++ b/taglib/mpc/mpcfile.cpp @@ -136,7 +136,7 @@ void MPC::File::removeUnsupportedProperties(const StringList &properties) PropertyMap MPC::File::setProperties(const PropertyMap &properties) { if(d->hasID3v1) - strip(ID3v1); + d->tag.access(MPCID3v1Index, false)->setProperties(properties); return d->tag.access(MPCAPEIndex, true)->setProperties(properties); } diff --git a/taglib/mpc/mpcfile.h b/taglib/mpc/mpcfile.h index b484be70..59617074 100644 --- a/taglib/mpc/mpcfile.h +++ b/taglib/mpc/mpcfile.h @@ -125,7 +125,7 @@ namespace TagLib { /*! * Implements the unified property interface -- import function. * Affects only the APEv2 tag which will be created if necessary. - * If an ID3v1 tag exists, it will be stripped from the file. + * If an ID3v1 tag exists, it will be updated as well. */ PropertyMap setProperties(const PropertyMap &); diff --git a/taglib/mpeg/mpegfile.cpp b/taglib/mpeg/mpegfile.cpp index fd7223c7..c73da414 100644 --- a/taglib/mpeg/mpegfile.cpp +++ b/taglib/mpeg/mpegfile.cpp @@ -159,8 +159,6 @@ void MPEG::File::removeUnsupportedProperties(const StringList &properties) PropertyMap MPEG::File::setProperties(const PropertyMap &properties) { - if(d->hasAPE) - strip(APE, true); if(d->hasID3v1) // update ID3v1 tag if it exists, but ignore the return value d->tag.access(ID3v1Index, false)->setProperties(properties); diff --git a/taglib/mpeg/mpegfile.h b/taglib/mpeg/mpegfile.h index 08415d55..1d99898c 100644 --- a/taglib/mpeg/mpegfile.h +++ b/taglib/mpeg/mpegfile.h @@ -145,9 +145,7 @@ namespace TagLib { /*! * Implements the writing part of the unified tag dictionary interface. * In order to avoid problems with deprecated tag formats, this method - * always creates an ID3v2 tag if necessary, and removes potential APEv2 - * tags (also invalidating all pointers to the APE tag) which are - * considered bad practice in MP3 files. + * always creates an ID3v2 tag if necessary. * If an ID3v1 tag exists, it will be updated as well, within the * limitations of that format. * The returned PropertyMap refers to the ID3v2 tag only. diff --git a/taglib/wavpack/wavpackfile.cpp b/taglib/wavpack/wavpackfile.cpp index 67be997c..78df182d 100644 --- a/taglib/wavpack/wavpackfile.cpp +++ b/taglib/wavpack/wavpackfile.cpp @@ -120,7 +120,7 @@ PropertyMap WavPack::File::properties() const PropertyMap WavPack::File::setProperties(const PropertyMap &properties) { if(d->hasID3v1) - strip(ID3v1); + d->tag.access(WavID3v1Index, false)->setProperties(properties); return d->tag.access(WavAPEIndex, true)->setProperties(properties); } diff --git a/taglib/wavpack/wavpackfile.h b/taglib/wavpack/wavpackfile.h index 3e0c36c4..918edca8 100644 --- a/taglib/wavpack/wavpackfile.h +++ b/taglib/wavpack/wavpackfile.h @@ -119,7 +119,7 @@ namespace TagLib { /*! * Implements the unified property interface -- import function. * Creates an APE tag if it does not exists and calls setProperties() on - * that. Any existing ID3v1 tag will be removed. + * that. Any existing ID3v1 tag will be updated as well. */ PropertyMap setProperties(const PropertyMap&); From 8329d6ac1a3e09efa784fde047826b76db82bd62 Mon Sep 17 00:00:00 2001 From: Michael Helmling Date: Thu, 27 Dec 2012 11:38:01 +0100 Subject: [PATCH 8/8] Update documentation of the property map interface in TagLib::File. --- taglib/toolkit/tfile.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/taglib/toolkit/tfile.h b/taglib/toolkit/tfile.h index 4b231d06..30e53f93 100644 --- a/taglib/toolkit/tfile.h +++ b/taglib/toolkit/tfile.h @@ -86,6 +86,8 @@ namespace TagLib { * format, the returend map's unsupportedData() list will contain one entry identifying * that object (e.g. the frame type for ID3v2 tags). Use removeUnsupportedProperties() * to remove (a subset of) them. + * For files that contain more than one tag (e.g. an MP3 with both an ID3v2 and an ID3v2 + * tag) only the most "modern" one will be exported (ID3v2 in this case). * BIC: Will be made virtual in future releases. */ PropertyMap properties() const; @@ -107,11 +109,13 @@ namespace TagLib { * indicating that no problems occured. * With file types that support several tag formats (for instance, MP3 files can have * ID3v1, ID3v2, and APEv2 tags), this function will create the most appropriate one - * and may remove deprecated ones (mostly ID3v1). See the documentation of the - * subclass implementations for detailed descriptions. + * (ID3v2 for MP3 files). Older formats will be updated as well, if they exist, but won't + * be taken into account for the return value of this function. + * See the documentation of the subclass implementations for detailed descriptions. * BIC: will become pure virtual in the future */ PropertyMap setProperties(const PropertyMap &properties); + /*! * Returns a pointer to this file's audio properties. This should be * reimplemented in the concrete subclasses. If no audio properties were