S3M+IT: fix reading, IT: untested support for message writing

IT: reading was off starting with global volume because of wrong read size.
S3M+IT: correctly read the number of used patterns.
IT: fixed reading of message tag and implemented writing of message tag
(not tested yet).

I also added S3M+IT test files. TODO: Unit tests using them.

9 files changed, 127 insertions, 22 deletions

diff --git a/taglib/it/itfile.cpp b/taglib/it/itfile.cpp
index d8040cdb..09ff077d 100644
--- a/taglib/it/itfile.cpp
+++ b/taglib/it/itfile.cpp
@@ -123,6 +123,56 @@ bool IT::File::save()
       writeString(String::null, 26);
   }
 
+  // write rest as message:
+  StringList messageLines;
+  for(uint i = instrumentCount + sampleCount; i < lines.size(); ++ i)
+    messageLines.append(lines[i]);
+  ByteVector message = messageLines.toString("\r").data(String::Latin1);
+  ushort special = 0;
+  ushort messageLength = 0;
+  ulong  messageOffset = 0;
+
+  seek(46);
+  if(!readU16L(special))
+    return false;
+
+  long fileSize = this->length();
+  if(special & 0x1)
+  {
+    seek(54);
+    if(!readU16L(messageLength) || !readU32L(messageOffset))
+      return false;
+
+    if(messageLength == 0)
+      messageOffset = fileSize;
+  }
+  else
+  {
+    messageOffset = fileSize;
+    seek(46);
+    writeU16L(special | 0x1);
+  }
+
+  if((messageOffset + messageLength) >= fileSize)
+  {
+    // append new message
+    seek(54);
+    writeU16L(message.size());
+    writeU32L(messageOffset);
+    seek(messageOffset);
+    writeBlock(message);
+    truncate(messageOffset + message.size());
+  }
+  else
+  {
+    // Only overwrite existing message.
+    // I'd need to parse (understand!) the whole file for more.
+    // Although I could just move the message to the end of file
+    // and let the existing one be, but that would waste space.
+    message.resize(messageLength, 0);
+    seek(messageOffset);
+    writeBlock(message);
+  }
   return true;
 }
 
@@ -141,7 +191,6 @@ void IT::File::read(bool)
   READ_U16L_AS(instrumentCount);
   READ_U16L_AS(sampleCount);
   
-  d->properties.setTableLength(length);
   d->properties.setInstrumentCount(instrumentCount);
   d->properties.setSampleCount(sampleCount);
   READ_U16L(d->properties.setPatternCount);
@@ -150,31 +199,30 @@ void IT::File::read(bool)
   READ_U16L(d->properties.setFlags);
   READ_U16L_AS(special);
   d->properties.setSpecial(special);
-  READ_U16L(d->properties.setGlobalVolume);
-  READ_U16L(d->properties.setMixVolume);
+  READ_BYTE(d->properties.setGlobalVolume);
+  READ_BYTE(d->properties.setMixVolume);
   READ_BYTE(d->properties.setBpmSpeed);
   READ_BYTE(d->properties.setTempo);
   READ_BYTE(d->properties.setPanningSeparation);
   READ_BYTE(d->properties.setPitchWheelDepth);
 
-  /*
-   * While the message would be a sorta comment tag, I don't
-   * see any IT files in the wild that use this or set the
-   * offset/length to a correct value.
-   *
-   * In all files I found where the message bit was set the
-   * offset was either 0 or a ridiculous high value and the
-   * length wasn't much better.
-   *
+  // IT supports some kind of comment tag. Still, the
+  // sample/instrument names are abused as comments so
+  // I just add all together.
+  String message;
   if(special & 0x1)
   {
     READ_U16L_AS(messageLength);
     READ_U32L_AS(messageOffset);
     seek(messageOffset);
-    READ_STRING_AS(message, messageLength);
-    debug("Message: \""+message+"\"");
+    ByteVector messageBytes = readBlock(messageLength);
+    READ_ASSERT(messageBytes.size() == messageLength);
+    int index = messageBytes.find((char) 0);
+    if(index > -1)
+      messageBytes.resize(index, 0);
+    messageBytes.replace('\r', '\n');
+    message = messageBytes;
   }
-  */
 
   seek(64);
 
@@ -192,6 +240,16 @@ void IT::File::read(bool)
     if(pannings[i] < 128 && volumes[i] > 0) ++ channels;
   }
   d->properties.setChannels(channels);
+  
+  // real length might be shorter because of skips and terminator
+  ushort realLength = 0;
+  for(ushort i = 0; i < length; ++ i)
+  {
+    READ_BYTE_AS(order);
+    if(order == 255) break;
+    if(order != 254) ++ realLength;
+  }
+  d->properties.setTableLength(realLength);
 
   StringList comment;
   // Note: I found files that have nil characters somewhere
@@ -249,6 +307,11 @@ void IT::File::read(bool)
     comment.append(sampleName);
   }
 
-  d->tag.setComment(comment.toString("\n"));
+  if(comment.size() > 0 && message.size() > 0)
+    d->tag.setComment(comment.toString("\n") + "\n" + message);
+  else if(comment.size() > 0)
+    d->tag.setComment(comment.toString("\n"));
+  else
+    d->tag.setComment(message);
   d->tag.setTrackerName("Impulse Tracker");
 }
diff --git a/taglib/mod/modfilebase.cpp b/taglib/mod/modfilebase.cpp
index 66df3857..e074dac8 100644
--- a/taglib/mod/modfilebase.cpp
+++ b/taglib/mod/modfilebase.cpp
@@ -55,6 +55,32 @@ bool Mod::FileBase::readString(String &s, ulong size)
   return true;
 }
 
+void Mod::FileBase::writeByte(uchar byte)
+{
+  ByteVector data(1, byte);
+  writeBlock(data);
+}
+
+void Mod::FileBase::writeU16L(ushort number)
+{
+  writeBlock(ByteVector::fromShort(number, false));
+}
+
+void Mod::FileBase::writeU32L(ulong number)
+{
+  writeBlock(ByteVector::fromUInt(number, false));
+}
+
+void Mod::FileBase::writeU16B(ushort number)
+{
+  writeBlock(ByteVector::fromShort(number, true));
+}
+
+void Mod::FileBase::writeU32B(ulong number)
+{
+  writeBlock(ByteVector::fromUInt(number, true));
+}
+
 bool Mod::FileBase::readByte(uchar &byte)
 {
   ByteVector data(readBlock(1));
diff --git a/taglib/mod/modfilebase.h b/taglib/mod/modfilebase.h
index 8ac24b10..d03af493 100644
--- a/taglib/mod/modfilebase.h
+++ b/taglib/mod/modfilebase.h
@@ -39,6 +39,12 @@ namespace TagLib {
       FileBase(IOStream *stream);
 
       void writeString(const String &s, ulong size, char padding = 0);
+      void writeByte(uchar byte);
+      void writeU16L(ushort number);
+      void writeU32L(ulong number);
+      void writeU16B(ushort number);
+      void writeU32B(ulong number);
+
       bool readString(String &s, ulong size);
       bool readByte(uchar &byte);
       bool readU16L(ushort &number);
diff --git a/taglib/s3m/s3mfile.cpp b/taglib/s3m/s3mfile.cpp
index 0752f15d..c786d0fc 100644
--- a/taglib/s3m/s3mfile.cpp
+++ b/taglib/s3m/s3mfile.cpp
@@ -143,7 +143,6 @@ void S3M::File::read(bool)
   READ_U16L_AS(length);
   READ_U16L_AS(sampleCount);
 
-  d->properties.setTableLength(length);
   d->properties.setSampleCount(sampleCount);
 
   READ_U16L(d->properties.setPatternCount);
@@ -177,12 +176,23 @@ void S3M::File::read(bool)
     if(setting != 0xff) ++ channels;
   }
   d->properties.setChannels(channels);
+  
+  seek(96);
+  ushort realLength = 0;
+  for(ushort i = 0; i < length; ++ i)
+  {
+	  READ_BYTE_AS(order);
+	  if(order == 255) break;
+	  if(order != 254) ++ realLength;
+  }
+  d->properties.setTableLength(realLength);
 
   seek(channels, Current);
 
-  // Note: The S3M spec mentions instruments, but I could
-  //       not figure out where these can be found. They are
-  //       similar to samples, though (SCRI instead of SCRS).
+  // Note: The S3M spec mentions samples and instruments, but in
+  //       the header there are only pointers to instruments.
+  //       However, there I never found instruments (SCRI) but
+  //       instead samples (SCRS).
   StringList comment;
   for(ushort i = 0; i < sampleCount; ++ i)
   {
@@ -209,7 +219,7 @@ void S3M::File::read(bool)
 
     READ_STRING_AS(sampleName, 28);
     // The next 4 bytes should be "SCRS", but I've found
-    // otherwise ok files with 4 nils instead.
+    // files that are otherwise ok with 4 nils instead.
     // READ_ASSERT(readBlock(4) == "SCRS");
 
     comment.append(sampleName);
diff --git a/taglib/xm/xmproperties.h b/taglib/xm/xmproperties.h
index 717775c6..fb8397aa 100644
--- a/taglib/xm/xmproperties.h
+++ b/taglib/xm/xmproperties.h
@@ -50,9 +50,9 @@ namespace TagLib {
       ushort bpmSpeed()        const;
 
     protected:
-      void setTableLength(ushort tableLength);
       void setChannels(int channels);
 
+      void setTableLength(ushort tableLength);
       void setVersion(ushort version);
       void setRestartPosition(ushort restartPosition);
       void setPatternCount(ushort patternCount);
diff --git a/tests/data/changed.it b/tests/data/changed.it
new file mode 100644
index 00000000..1e66c916
--- /dev/null
+++ b/tests/data/changed.it
diff --git a/tests/data/changed.s3m b/tests/data/changed.s3m
new file mode 100644
index 00000000..37bd49cd
--- /dev/null
+++ b/tests/data/changed.s3m
diff --git a/tests/data/test.it b/tests/data/test.it
new file mode 100644
index 00000000..9334a73a
--- /dev/null
+++ b/tests/data/test.it
diff --git a/tests/data/test.s3m b/tests/data/test.s3m
new file mode 100644
index 00000000..ee3b6d7a
--- /dev/null
+++ b/tests/data/test.s3m