Improved Common::Serializer in several ways:

* Added support versioned serialization
* Added a convenience API for handling 'magic IDs' transparently
* Exposed the err()/clearErr() methods of the underlying streams
* Added a basic unit test for versioned loading (more should be added, in particular for saving)
* Removed the syncString(char *, uint16) alias for syncBytes(byte *buf, uint32 size)

svn-id: r40723

4 files changed, 241 insertions, 61 deletions

diff --git a/common/serializer.h b/common/serializer.h
index 803807814a..a44883683b 100644
--- a/common/serializer.h
+++ b/common/serializer.h
@@ -34,7 +34,9 @@ namespace Common {
 
 #define SYNC_AS(SUFFIX,TYPE,SIZE) \
 	template <typename T> \
-	void syncAs ## SUFFIX(T &val) { \
+	void syncAs ## SUFFIX(T &val, Version minVersion = 0, Version maxVersion = kLastVersion) { \
+		if (_version < minVersion || _version > maxVersion) \
+			return;	\
 		if (_loadStream) \
 			val = static_cast<T>(_loadStream->read ## SUFFIX()); \
 		else { \
@@ -45,31 +47,130 @@ namespace Common {
 	}
 
 
-// TODO: Write comment for this
-// TODO: Inspired by the SCUMM engine -- move to common/ code and use in more engines?
+/**
+ * This class allows syncing / serializing data (primarily game savestates)
+ * between memory and Read/WriteStreams.
+ * It optionally supports versioning the serialized data (client code must
+ * use the syncVersion() method for this). This makes it possible to support
+ * multiple versions of a savegame format with a single codepath
+ *
+ * This class was heavily inspired by the save/load code in the SCUMM engine.
+ *
+ * @todo Maybe rename this to Synchronizer?
+ *
+ * @todo One feature the SCUMM code has but that is missing here: Support for
+ *       syncing arrays of a given type and *fixed* size; and also support
+ *       for when the array size changed between versions. Also, support for
+ *       2D-arrays.
+ *
+ * @todo Proper error handling!
+ */
 class Serializer {
 public:
+	typedef uint32 Version;
+	static const Version kLastVersion = 0xFFFFFFFF;
+
+protected:
+	Common::SeekableReadStream *_loadStream;
+	Common::WriteStream *_saveStream;
+
+	uint _bytesSynced;
+
+	Version _version;
+
+public:
 	Serializer(Common::SeekableReadStream *in, Common::WriteStream *out)
-		: _loadStream(in), _saveStream(out), _bytesSynced(0) {
+		: _loadStream(in), _saveStream(out), _bytesSynced(0), _version(0) {
 		assert(in || out);
 	}
+	virtual ~Serializer() {}
+
+	inline bool isSaving() { return (_saveStream != 0); }
+	inline bool isLoading() { return (_loadStream != 0); }
+
+	/**
+	 * Returns true if an I/O failure occurred.
+	 * This flag is never cleared automatically. In order to clear it,
+	 * client code has to call clearErr() explicitly.
+	 */
+	bool err() const {
+		if (_saveStream)
+			return _saveStream->err();
+		else
+			return _loadStream->err();
+	}
+
+	/**
+	 * Reset the I/O error status as returned by err().
+	 */
+	void clearErr() {
+		if (_saveStream)
+			_saveStream->clearErr();
+		else
+			_loadStream->clearErr();
+	}
+
+	/**
+	 * Sync the "version" of the savegame we are loading/creating.
+	 * @param currentVersion	current format version, used when writing a new file
+	 * @return true if the version of the savestate is not too new.
+	 */
+	bool syncVersion(Version currentVersion) {
+		_version = currentVersion;
+		syncAsUint32LE(_version);
+		return _version <= currentVersion;
+	}
+
+	/**
+	 * Return the version of the savestate being serialized. Useful if the engine
+	 * needs to perform additional adjustments when loading old savestates.
+	 */
+	Version getVersion() const { return _version; }
+
+	/**
+	 * Sync a 'magic id' of up to 256 bytes, and return whether it matched.
+	 * When saving, this will simply write out the magic id and return true.
+	 * When loading, this will read the specified number of bytes, compare it
+	 * to the given magic id and return true on a match, false otherwise.
+	 *
+	 * A typical magic id is a FOURCC like 'MAGI'.
+	 *
+	 * @param magic		magic id as a byte sequence
+	 * @param size		length of the magic id in bytes
+	 * @return true if the magic id matched, false otherwise
+	 *
+	 * @todo Should this have minVersion/maxVersion params, too?
+	 */
+	bool syncMagic(const char *magic, byte size) {
+		char buf[256];
+		bool match;
+		if (isSaving()) {
+			_saveStream->write(magic, size);
+			match = true;
+		} else {
+			_loadStream->read(buf, size);
+			match = (0 == memcmp(buf, magic, size));
+		}
+		_bytesSynced += size;
+		return match;
+	}
 
-	bool isSaving() { return (_saveStream != 0); }
-	bool isLoading() { return (_loadStream != 0); }
 
 	/**
 	 * Return the total number of bytes synced so far.
 	 */
 	uint bytesSynced() const { return _bytesSynced; }
 
-
 	/**
 	 * Skip a number of bytes in the data stream.
 	 * This is useful to skip obsolete fields in old savestates.
 	 */
-	void skip(uint32 size) {
+	void skip(uint32 size, Version minVersion = 0, Version maxVersion = kLastVersion) {
+		if (_version < minVersion || _version > maxVersion)
+			return;	// Ignore anything which is not supposed to be present in this save game version
+
 		_bytesSynced += size;
-		if (_loadStream)
+		if (isLoading())
 			_loadStream->skip(size);
 		else {
 			while (size--)
@@ -80,8 +181,11 @@ public:
 	/**
 	 * Sync a block of arbitrary fixed-length data.
 	 */
-	void syncBytes(byte *buf, uint32 size) {
-		if (_loadStream)
+	void syncBytes(byte *buf, uint32 size, Version minVersion = 0, Version maxVersion = kLastVersion) {
+		if (_version < minVersion || _version > maxVersion)
+			return;	// Ignore anything which is not supposed to be present in this save game version
+
+		if (isLoading())
 			_loadStream->read(buf, size);
 		else
 			_saveStream->write(buf, size);
@@ -90,9 +194,13 @@ public:
 
 	/**
 	 * Sync a C-string, by treating it as a zero-terminated byte sequence.
+	 * @todo Replace this method with a special Syncer class for Common::String
 	 */
-	void syncString(Common::String &str) {
-		if (_loadStream) {
+	void syncString(Common::String &str, Version minVersion = 0, Version maxVersion = kLastVersion) {
+		if (_version < minVersion || _version > maxVersion)
+			return;	// Ignore anything which is not supposed to be present in this save game version
+
+		if (isLoading()) {
 			char c;
 			str.clear();
 			while ((c = _loadStream->readByte())) {
@@ -107,13 +215,6 @@ public:
 		}
 	}
 
-	/**
-	 * Sync a fixed length C-string
-	 */
-	void syncString(char *buf, uint16 size) {
-		syncBytes((byte *)buf, size);
-	}
-
 	SYNC_AS(Byte, byte, 1)
 
 	SYNC_AS(Uint16LE, uint16, 2)
@@ -125,45 +226,13 @@ public:
 	SYNC_AS(Uint32BE, uint32, 4)
 	SYNC_AS(Sint32LE, int32, 4)
 	SYNC_AS(Sint32BE, int32, 4)
-
-protected:
-	Common::SeekableReadStream *_loadStream;
-	Common::WriteStream *_saveStream;
-
-	uint _bytesSynced;
 };
 
 #undef SYNC_AS
 
-// TODO: Make a subclass "VersionedSerializer", which makes it easy to support
-//       multiple versions of a savegame format (again inspired by SCUMM).
-/*
-class VersionedSerializer : public Serializer {
-public:
-	// "version" is the version of the savegame we are loading/creating
-	VersionedSerializer(Common::SeekableReadStream *in, Common::OutSaveFile *out, int version)
-		: Serializer(in, out), _version(version) {
-		assert(in || out);
-	}
-
-	void syncBytes(byte *buf, uint16 size, int minVersion = 0, int maxVersion = INF) {
-		if (_version < minVersion || _version > maxVersion)
-			return;	// Do nothing if too old or too new
-		if (_loadStream) {
-			_loadStream->read(buf, size);
-		} else {
-			_saveStream->write(buf, size);
-		}
-	}
-	...
-
-};
-
-*/
 
 // Mixin class / interface
-// TODO Maybe call it ISerializable or SerializableMixin ?
-// TODO: Taken from SCUMM engine -- move to common/ code?
+// TODO: Maybe rename this to Syncable ?
 class Serializable {
 public:
 	virtual ~Serializable() {}
diff --git a/engines/cruise/saveload.cpp b/engines/cruise/saveload.cpp
index dde51dd382..b9edb2decd 100644
--- a/engines/cruise/saveload.cpp
+++ b/engines/cruise/saveload.cpp
@@ -148,8 +148,8 @@ static void syncBasicInfo(Common::Serializer &s) {
 static void syncBackgroundTable(Common::Serializer &s) {
 	// restore backgroundTable
 	for (int i = 0; i < 8; i++) {
-		s.syncString(backgroundTable[i].name, 9);
-		s.syncString(backgroundTable[i].extention, 6);
+		s.syncBytes((byte *)backgroundTable[i].name, 9);
+		s.syncBytes((byte *)backgroundTable[i].extention, 6);
 	}
 }
 
@@ -189,7 +189,7 @@ static void syncFilesDatabase(Common::Serializer &s) {
 		}
 
 		s.syncAsSint16LE(fe.subData.index);
-		s.syncString(fe.subData.name, 13);
+		s.syncBytes((byte *)fe.subData.name, 13);
 		s.syncAsByte(dummyVal);
 
 		s.syncAsSint16LE(fe.subData.transparency);
@@ -213,7 +213,7 @@ static void syncPreloadData(Common::Serializer &s) {
 	for (int i = 0; i < 64; i++) {
 		preloadStruct &pe = preloadData[i];
 
-		s.syncString(pe.name, 15);
+		s.syncBytes((byte *)pe.name, 15);
 		s.syncAsByte(dummyByte);
 		s.syncAsUint32LE(pe.size);
 		s.syncAsUint32LE(pe.sourceSize);
@@ -231,7 +231,7 @@ static void syncOverlays1(Common::Serializer &s) {
 	for (int i = 0; i < numOfLoadedOverlay; i++) {
 		overlayStruct &oe = overlayTable[i];
 
-		s.syncString(oe.overlayName, 13);
+		s.syncBytes((byte *)oe.overlayName, 13);
 		s.syncAsByte(dummyByte);
 		s.syncAsUint32LE(dummyLong);
 		s.syncAsUint16LE(oe.alreadyLoaded);
@@ -464,7 +464,7 @@ static void syncIncrust(Common::Serializer &s) {
 		s.syncAsSint16LE(t->saveSize);
 		s.syncAsSint16LE(t->savedX);
 		s.syncAsSint16LE(t->savedY);
-		s.syncString(t->name, 13);
+		s.syncBytes((byte *)t->name, 13);
 		s.syncAsByte(dummyByte);
 		s.syncAsSint16LE(t->spriteId);
 		s.syncAsUint16LE(dummyWord);
@@ -597,7 +597,7 @@ static void DoSync(Common::Serializer &s) {
 	syncPalette(s, newPal);
 	syncPalette(s, workpal);
 
-	s.syncString(currentCtpName, 40);
+	s.syncBytes((byte *)currentCtpName, 40);
 
 	syncBackgroundTable(s);
 	syncPalScreen(s);
diff --git a/engines/cruise/sound.cpp b/engines/cruise/sound.cpp
index a68323cc43..114e37679a 100644
--- a/engines/cruise/sound.cpp
+++ b/engines/cruise/sound.cpp
@@ -70,7 +70,7 @@ void MusicPlayer::resume() {
 
 void MusicPlayer::doSync(Common::Serializer &s) {
 	// synchronise current music name, if any, state, and position
-	s.syncString(_musicName, 33);
+	s.syncBytes((byte *)_musicName, 33);
 	uint16 v = (uint16)songLoaded();
 	s.syncAsSint16LE(v);
 	s.syncAsSint16LE(_songPlayed);
diff --git a/test/common/serializer.h b/test/common/serializer.h
new file mode 100644
index 0000000000..7f79d88de0
--- /dev/null
+++ b/test/common/serializer.h
@@ -0,0 +1,111 @@
+#include <cxxtest/TestSuite.h>
+
+#include "common/serializer.h"
+#include "common/stream.h"
+
+class SerializerTestSuite : public CxxTest::TestSuite {
+	Common::SeekableReadStream *_inStreamV1;
+	Common::SeekableReadStream *_inStreamV2;
+public:
+	void setUp() {
+		// Our pseudo data format is as follows:
+		// * magic id - string "MAGI"
+		// * Version - uint32, LE
+		// * uint32, LE (available in v2 onward)
+		// * uint16, BE (available in v1 onward)
+		// * sint16, LE (available only in v1)
+		// * byte       (always available)
+		static const byte contents_v1[] = {
+			'M', 'A', 'G', 'I',		// magic id
+			0x01, 0x00, 0x00, 0x00,	// Version
+			0x06, 0x07, 			// uint16, BE (available in v1 onward)
+			0xfe, 0xff, 			// sint16, LE (available only in v1)
+			0x0a					// byte       (always available)
+		};
+		static const byte contents_v2[] = {
+			'M', 'A', 'G', 'I',		// magic id
+			0x02, 0x00, 0x00, 0x00,	// Version
+			0x02, 0x03, 0x04, 0x05,	// uint32, LE (available in v2 onward)
+			0x06, 0x07, 			// uint16, BE (available in v1 onward)
+			0x0a					// byte       (always available)
+		};
+
+		_inStreamV1 = new Common::MemoryReadStream(contents_v1, sizeof(contents_v1));
+		_inStreamV2 = new Common::MemoryReadStream(contents_v2, sizeof(contents_v2));
+	}
+
+    void tearDown() {
+    	delete _inStreamV1;
+    	delete _inStreamV2;
+    }
+
+	// A method which reads a v1 file
+	void readVersioned_v1(Common::SeekableReadStream *stream, int version) {
+		Common::Serializer  ser(stream, 0);
+
+		TS_ASSERT(ser.syncMagic("MAGI", 4));
+
+		TS_ASSERT(ser.syncVersion(1));
+		TS_ASSERT_EQUALS(ser.getVersion(), version);
+
+		uint32 tmp;
+
+		ser.syncAsUint16BE(tmp, Common::Serializer::Version(1));
+		TS_ASSERT_EQUALS(tmp, 0x0607);
+
+		ser.syncAsSint16LE(tmp, Common::Serializer::Version(1));
+		TS_ASSERT_EQUALS(tmp, -2);
+
+		ser.syncAsByte(tmp);
+		TS_ASSERT_EQUALS(tmp, 0x0a);
+	}
+
+	// A method which reads a v2 file
+	void readVersioned_v2(Common::SeekableReadStream *stream, int version) {
+		Common::Serializer  ser(stream, 0);
+
+		TS_ASSERT(ser.syncMagic("MAGI", 4));
+
+		TS_ASSERT(ser.syncVersion(2));
+		TS_ASSERT_EQUALS(ser.getVersion(), version);
+
+		uint32 tmp;
+
+		// Read a value only available starting with v2.
+		// Thus if we load an old save, it must be
+		// manually set. To simplify that, no sync method should
+		// modify the value passed to it if nothing was read!
+		tmp = 0x12345678;
+		ser.syncAsUint32LE(tmp, Common::Serializer::Version(2));
+		if (ser.getVersion() < 2) {
+			TS_ASSERT_EQUALS(tmp, 0x12345678);
+		} else {
+			TS_ASSERT_EQUALS(tmp, 0x05040302);
+		}
+
+		ser.syncAsUint16BE(tmp, Common::Serializer::Version(1));
+		TS_ASSERT_EQUALS(tmp, 0x0607);
+
+		// Skip over obsolete data
+		ser.skip(2, Common::Serializer::Version(1), Common::Serializer::Version(1));
+
+		ser.syncAsByte(tmp);
+		TS_ASSERT_EQUALS(tmp, 0x0a);
+	}
+
+	void test_read_v1_as_v1() {
+		readVersioned_v1(_inStreamV1, 1);
+	}
+
+	// There is no test_read_v2_as_v1() because a v1 parser cannot possibly
+	// read v2 data correctly. It should instead error out if it
+	// detects a version newer than its current version.
+
+	void test_read_v2_as_v1() {
+		readVersioned_v2(_inStreamV1, 1);
+	}
+
+	void test_read_v2_as_v2() {
+		readVersioned_v2(_inStreamV2, 2);
+	}
+};