Fix saving APE files.

This fixes the issue reported at #624.

diff --git a/taglib/ape/apefile.cpp b/taglib/ape/apefile.cpp
index 241711457f5020b0f051012b6f4a2261735cc355..17d976688727365fd08107438f4001cda78f1692 100644 (file)
--- a/taglib/ape/apefile.cpp
+++ b/taglib/ape/apefile.cpp
@@ -61,10 +61,7 @@ public:
     ID3v2Header(0),
     ID3v2Location(-1),
     ID3v2Size(0),
-    properties(0),
-    hasAPE(false),
-    hasID3v1(false),
-    hasID3v2(false) {}
+    properties(0) {}
 
   ~FilePrivate()
   {
@@ -84,13 +81,6 @@ public:
   TagUnion tag;
 
   Properties *properties;
-
-  // These indicate whether the file *on disk* has these tags, not if
-  // this data structure does.  This is used in computing offsets.
-
-  bool hasAPE;
-  bool hasID3v1;
-  bool hasID3v2;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -155,64 +145,67 @@ bool APE::File::save()
 
   // Update ID3v1 tag
 
-  if(ID3v1Tag()) {
-    if(d->hasID3v1) {
+  if(ID3v1Tag() && !ID3v1Tag()->isEmpty()) {
+
+    // ID3v1 tag is not empty. Update the old one or create a new one.
+
+    if(d->ID3v1Location >= 0) {
       seek(d->ID3v1Location);
-      writeBlock(ID3v1Tag()->render());
     }
     else {
       seek(0, End);
       d->ID3v1Location = tell();
-      writeBlock(ID3v1Tag()->render());
-      d->hasID3v1 = true;
     }
+
+    writeBlock(ID3v1Tag()->render());
   }
   else {
-    if(d->hasID3v1) {
-      removeBlock(d->ID3v1Location, 128);
-      d->hasID3v1 = false;
-      if(d->hasAPE) {
-        if(d->APELocation > d->ID3v1Location)
-          d->APELocation -= 128;
-      }
+
+    // ID3v1 tag is empty. Remove the old one.
+
+    if(d->ID3v1Location >= 0) {
+      truncate(d->ID3v1Location);
+      d->ID3v1Location = -1;
     }
   }
 
   // Update APE tag
 
-  if(APETag()) {
-    if(d->hasAPE)
-      insert(APETag()->render(), d->APELocation, d->APESize);
-    else {
-      if(d->hasID3v1)  {
-        insert(APETag()->render(), d->ID3v1Location, 0);
-        d->APESize = APETag()->footer()->completeTagSize();
-        d->hasAPE = true;
+  if(APETag() && !APETag()->isEmpty()) {
+
+    // APE tag is not empty. Update the old one or create a new one.
+
+    if(d->APELocation < 0) {
+      if(d->ID3v1Location >= 0)
         d->APELocation = d->ID3v1Location;
-        d->ID3v1Location += d->APESize;
-      }
-      else {
-        seek(0, End);
-        d->APELocation = tell();
-        writeBlock(APETag()->render());
-        d->APESize = APETag()->footer()->completeTagSize();
-        d->hasAPE = true;
-      }
+      else
+        d->APELocation = length();
     }
+
+    const ByteVector data = APETag()->render();
+    insert(data, d->APELocation, d->APESize);
+
+    if(d->ID3v1Location >= 0)
+      d->ID3v1Location += (data.size() - d->APESize);
+
+    d->APESize = data.size();
   }
   else {
-    if(d->hasAPE) {
+
+    // APE tag is empty. Remove the old one.
+
+    if(d->APELocation >= 0) {
       removeBlock(d->APELocation, d->APESize);
-      d->hasAPE = false;
-      if(d->hasID3v1) {
-        if(d->ID3v1Location > d->APELocation) {
-          d->ID3v1Location -= d->APESize;
-        }
-      }
+
+      if(d->ID3v1Location >= 0)
+        d->ID3v1Location -= d->APESize;
+
+      d->APELocation = -1;
+      d->APESize = 0;
     }
   }
 
-   return true;
+  return true;
 }
 
 ID3v1::Tag *APE::File::ID3v1Tag(bool create)
@@ -227,27 +220,24 @@ APE::Tag *APE::File::APETag(bool create)
 
 void APE::File::strip(int tags)
 {
-  if(tags & ID3v1) {
+  if(tags & ID3v1)
     d->tag.set(ApeID3v1Index, 0);
-    APETag(true);
-  }
 
-  if(tags & APE) {
+  if(tags & APE)
     d->tag.set(ApeAPEIndex, 0);
 
-    if(!ID3v1Tag())
-      APETag(true);
-  }
+  if(!ID3v1Tag())
+    APETag(true);
 }
 
 bool APE::File::hasAPETag() const
 {
-  return d->hasAPE;
+  return (d->APELocation >= 0);
 }
 
 bool APE::File::hasID3v1Tag() const
 {
-  return d->hasID3v1;
+  return (d->ID3v1Location >= 0);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -264,17 +254,14 @@ void APE::File::read(bool readProperties)
     seek(d->ID3v2Location);
     d->ID3v2Header = new ID3v2::Header(readBlock(ID3v2::Header::size()));
     d->ID3v2Size = d->ID3v2Header->completeTagSize();
-    d->hasID3v2 = true;
   }
 
   // Look for an ID3v1 tag
 
   d->ID3v1Location = Utils::findID3v1(this);
 
-  if(d->ID3v1Location >= 0) {
+  if(d->ID3v1Location >= 0)
     d->tag.set(ApeID3v1Index, new ID3v1::Tag(this, d->ID3v1Location));
-    d->hasID3v1 = true;
-  }
 
   // Look for an APE tag
 
@@ -284,10 +271,9 @@ void APE::File::read(bool readProperties)
     d->tag.set(ApeAPEIndex, new APE::Tag(this, d->APELocation));
     d->APESize = APETag()->footer()->completeTagSize();
     d->APELocation = d->APELocation + APE::Footer::size() - d->APESize;
-    d->hasAPE = true;
   }
 
-  if(!d->hasID3v1)
+  if(d->ID3v1Location < 0)
     APETag(true);
 
   // Look for APE audio properties
@@ -296,14 +282,14 @@ void APE::File::read(bool readProperties)
 
     long streamLength;
 
-    if(d->hasAPE)
+    if(d->APELocation >= 0)
       streamLength = d->APELocation;
-    else if(d->hasID3v1)
+    else if(d->ID3v1Location >= 0)
       streamLength = d->ID3v1Location;
     else
       streamLength = length();
 
-    if(d->hasID3v2) {
+    if(d->ID3v2Location >= 0) {
       seek(d->ID3v2Location + d->ID3v2Size);
       streamLength -= (d->ID3v2Location + d->ID3v2Size);
     }

diff --git a/taglib/ape/apefile.h b/taglib/ape/apefile.h
index e638d8653d0f494d6be4cd736170fa1586b13687..cfb19ff739db254b7de7346f2a915df99327abbf 100644 (file)
--- a/taglib/ape/apefile.h
+++ b/taglib/ape/apefile.h
@@ -146,9 +146,6 @@ namespace TagLib {
        *
        * \note According to the official Monkey's Audio SDK, an APE file
        * can only have either ID3V1 or APE tags, so a parameter is used here.
-       *
-       * \warning In the current implementation, it's dangerous to call save()
-       * repeatedly.  At worst it will corrupt the file.
        */
       virtual bool save();
 

diff --git a/tests/test_ape.cpp b/tests/test_ape.cpp
index 64869c4c5b6f1261b2e09dc47ab1c6f314d32dc1..12ca2db819b458cc78e4de56b596a85119dc45c3 100644 (file)
--- a/tests/test_ape.cpp
+++ b/tests/test_ape.cpp
@@ -23,6 +23,7 @@ class TestAPE : public CppUnit::TestFixture
   CPPUNIT_TEST(testFuzzedFile1);
   CPPUNIT_TEST(testFuzzedFile2);
   CPPUNIT_TEST(testStripAndProperties);
+  CPPUNIT_TEST(testRepeatedSave);
   CPPUNIT_TEST_SUITE_END();
 
 public:
@@ -134,6 +135,32 @@ public:
     }
   }
 
+  void testRepeatedSave()
+  {
+    ScopedFileCopy copy("mac-399", ".ape");
+
+    {
+      APE::File f(copy.fileName().c_str());
+      CPPUNIT_ASSERT(!f.hasAPETag());
+      CPPUNIT_ASSERT(!f.hasID3v1Tag());
+
+      f.APETag(true)->setTitle("01234 56789 ABCDE FGHIJ");
+      f.save();
+
+      f.APETag()->setTitle("0");
+      f.save();
+
+      f.ID3v1Tag(true)->setTitle("01234 56789 ABCDE FGHIJ");
+      f.APETag()->setTitle("01234 56789 ABCDE FGHIJ 01234 56789 ABCDE FGHIJ 01234 56789");
+      f.save();
+    }
+    {
+      APE::File f(copy.fileName().c_str());
+      CPPUNIT_ASSERT(f.hasAPETag());
+      CPPUNIT_ASSERT(f.hasID3v1Tag());
+    }
+  }
+
 };
 
 CPPUNIT_TEST_SUITE_REGISTRATION(TestAPE);