results of morning-after code review, added some doc strings, etc.


git-svn-id: svn://svn.code.sf.net/p/fonttools/code/trunk@210 4cde692c-a291-49d1-8350-778aa11640f8
diff --git a/Lib/fontTools/ttLib/tables/otBase.py b/Lib/fontTools/ttLib/tables/otBase.py
index 85ffcd7..05033fa 100644
--- a/Lib/fontTools/ttLib/tables/otBase.py
+++ b/Lib/fontTools/ttLib/tables/otBase.py
@@ -6,6 +6,10 @@
 
 class BaseTTXConverter(DefaultTable):
 	
+	"""Generic base class for TTX table converters. Functions as an adapter
+	between the TTX (ttLib actually) table model and the model we use for
+	OpenType tables, which is neccesarily subtly different."""
+	
 	def decompile(self, data, font):
 		import otTables
 		reader = OTTableReader(data, self.tableTag)
@@ -31,6 +35,8 @@
 
 class OTTableReader:
 	
+	"""Helper class to retrieve data from an OpenType table."""
+	
 	def __init__(self, data, tableType, offset=0, valueFormat=None, cachingStats=None):
 		self.data = data
 		self.offset = offset
@@ -102,6 +108,8 @@
 
 class OTTableWriter:
 	
+	"""Helper class to gather and assemble data for OpenType tables."""
+	
 	def __init__(self, tableType, valueFormat=None):
 		self.items = []
 		self.tableType = tableType
@@ -169,6 +177,7 @@
 
 
 class CountReference:
+	"""A reference to a Count value, not a count of a reference."""
 	def __init__(self, table, name):
 		self.table = table
 		self.name = name
@@ -176,14 +185,42 @@
 		return packUShort(self.table[self.name])
 
 
-def packUShort(offset):
-	assert 0 <= offset < 0x10000
-	return struct.pack(">H", offset)
+def packUShort(value):
+	assert 0 <= value < 0x10000
+	return struct.pack(">H", value)
 
 
 
+class TableStack:
+	"""A stack of table dicts, working as a stack of namespaces so we can
+	retrieve values from (and store values to) tables higher up the stack."""
+	def __init__(self):
+		self.stack = []
+	def push(self, table):
+		self.stack.insert(0, table)
+	def pop(self):
+		self.stack.pop(0)
+	def getTop(self):
+		return self.stack[0]
+	def getValue(self, name):
+		return self.__findTable(name)[name]
+	def storeValue(self, name, value):
+		table = self.__findTable(name)
+		if table[name] is None:
+			table[name] = value
+		else:
+			assert table[name] == value, (table[name], value)
+	def __findTable(self, name):
+		for table in self.stack:
+			if table.has_key(name):
+				return table
+		raise KeyError, name
+
+
 class BaseTable:
 	
+	"""Generic base class for all OpenType (sub)tables."""
+	
 	def getConverters(self):
 		return self.converters
 	
@@ -220,7 +257,7 @@
 			value = table.get(conv.name)
 			if conv.repeat:
 				if value is None:
-					value = []  # XXXXXX
+					value = []
 				tableStack.storeValue(conv.repeat, len(value) - conv.repeatOffset)
 				for item in value:
 					conv.write(writer, font, tableStack, item)
@@ -249,7 +286,7 @@
 		if attrs is None:
 			attrs = []
 		if hasattr(self, "Format"):
-			attrs = attrs + [("Format", str(self.Format))]
+			attrs = attrs + [("Format", self.Format)]
 		xmlWriter.begintag(tableName, attrs)
 		xmlWriter.newline()
 		self.toXML2(xmlWriter, font)
@@ -262,30 +299,30 @@
 		# do it ourselves. I think I'm getting schizophrenic...
 		for conv in self.getConverters():
 			value = getattr(self, conv.name)
-			if not conv.repeat:
-				conv.xmlWrite(xmlWriter, font, value, conv.name, [])
-			else:
+			if conv.repeat:
 				for i in range(len(value)):
 					item = value[i]
-					conv.xmlWrite(xmlWriter, font, item, conv.name, [("index", i)])
+					conv.xmlWrite(xmlWriter, font, item, conv.name,
+							[("index", i)])
+			else:
+				conv.xmlWrite(xmlWriter, font, value, conv.name, [])
 	
 	def fromXML(self, (name, attrs, content), font):
 		try:
 			conv = self.getConverterByName(name)
 		except KeyError:
-			print self, name, attrs, content
+##			print self, name, attrs, content
 			raise    # XXX on KeyError, raise nice error
 		value = conv.xmlRead(attrs, content, font)
-		name = conv.name
 		if conv.repeat:
 			try:
-				seq = getattr(self, name)
+				seq = getattr(self, conv.name)
 			except AttributeError:
 				seq = []
-				setattr(self, name, seq)
+				setattr(self, conv.name, seq)
 			seq.append(value)
 		else:
-			setattr(self, name, value)
+			setattr(self, conv.name, value)
 	
 	def __cmp__(self, other):
 		# this is only for debugging, so it's ok to barf
@@ -300,6 +337,9 @@
 
 class FormatSwitchingBaseTable(BaseTable):
 	
+	"""Small specialization of BaseTable, for tables that have multiple
+	formats, eg. CoverageFormat1 vs. CoverageFormat2."""
+	
 	def getConverters(self):
 		return self.converters[self.Format]
 	
@@ -316,6 +356,14 @@
 		BaseTable.compile(self, writer, font, tableStack)
 
 
+#
+# Support for ValueRecords
+#
+# This data type is so different from all other OpenType data types that
+# it requires quite a bit of code for itself. It even has special support
+# in OTTableReader and OTTableWriter...
+#
+
 valueRecordFormat = [
 #	Mask	 Name            isDevice  signed
 	(0x0001, "XPlacement",   0,        1),
@@ -348,6 +396,8 @@
 
 class ValueRecordFactory:
 	
+	"""Given a format code, this object convert ValueRecords."""
+	
 	def setFormat(self, valueFormat):
 		format = []
 		for mask, name, isDevice, signed in valueRecordFormat:
@@ -453,27 +503,3 @@
 		else:
 			return rv
 
-
-class TableStack:
-	def __init__(self):
-		self.stack = []
-	def push(self, table):
-		self.stack.insert(0, table)
-	def pop(self):
-		self.stack.pop(0)
-	def getTop(self):
-		return self.stack[0]
-	def getValue(self, name):
-		return self.__findTable(name)[name]
-	def storeValue(self, name, value):
-		table = self.__findTable(name)
-		if table[name] is None:
-			table[name] = value
-		else:
-			assert table[name] == value, (table[name], value)
-	def __findTable(self, name):
-		for table in self.stack:
-			if table.has_key(name):
-				return table
-		raise KeyError, name
-
diff --git a/Lib/fontTools/ttLib/tables/otConverters.py b/Lib/fontTools/ttLib/tables/otConverters.py
index ab18b10..27dc298 100644
--- a/Lib/fontTools/ttLib/tables/otConverters.py
+++ b/Lib/fontTools/ttLib/tables/otConverters.py
@@ -2,7 +2,10 @@
 from fontTools.misc.textTools import safeEval
 
 
-def buildConverterList(tableSpec, tableNamespace):
+def buildConverters(tableSpec, tableNamespace):
+	"""Given a table spec from otData.py, build a converter object for each
+	field of the table. This is called for each table in otData.py, and
+	the results are assigned to the corresponding class in otTables.py."""
 	converters = []
 	convertersByName = {}
 	for tp, name, repeat, repeatOffset, descr in tableSpec:
@@ -35,6 +38,9 @@
 
 class BaseConverter:
 	
+	"""Base class for converter objects. Apart from the constructor, this
+	is an abstract class."""
+	
 	def __init__(self, name, repeat, repeatOffset, tableClass):
 		self.name = name
 		self.repeat = repeat
@@ -43,15 +49,19 @@
 		self.isCount = name.endswith("Count")
 	
 	def read(self, reader, font, tableStack):
+		"""Read a value from the reader."""
 		raise NotImplementedError, self
 	
 	def write(self, writer, font, tableStack, value):
-		raise NotImplementedError, self
-	
-	def xmlWrite(self, xmlWriter, font, value, name, attrs):
+		"""Write a value to the writer."""
 		raise NotImplementedError, self
 	
 	def xmlRead(self, attrs, content, font):
+		"""Read a value from XML."""
+		raise NotImplementedError, self
+	
+	def xmlWrite(self, xmlWriter, font, value, name, attrs):
+		"""Write a value to XML."""
 		raise NotImplementedError, self
 
 
@@ -240,6 +250,7 @@
 			writer.writeUShort(tmp)
 	
 	def xmlWrite(self, xmlWriter, font, value, name, attrs):
+		# XXX this could do with a nicer format
 		xmlWriter.simpletag(name, attrs + [("value", value)])
 		xmlWriter.newline()
 	
diff --git a/Lib/fontTools/ttLib/tables/otTables.py b/Lib/fontTools/ttLib/tables/otTables.py
index 24a5440..5438ae8 100644
--- a/Lib/fontTools/ttLib/tables/otTables.py
+++ b/Lib/fontTools/ttLib/tables/otTables.py
@@ -1,8 +1,8 @@
 """fontTools.ttLib.tables.otTables -- A collection of classes representing the various
 OpenType subtables.
 
-Most are constructed upon import from data in otData.py. Most smartness is contained
-in otBase.BaseTable.
+Most are constructed upon import from data in otData.py, all are populated with
+converter objects from otConverters.py.
 """
 
 from otBase import BaseTable, FormatSwitchingBaseTable
@@ -15,28 +15,35 @@
 	"""Dummy class; this table isn't defined, but is used, and is always NULL."""
 
 
-_equivalents = [
-	('MarkArray', ("Mark1Array",)),
-	('LangSys', ('DefaultLangSys',)),
-	('Coverage', ('MarkCoverage', 'BaseCoverage', 'LigatureCoverage', 'Mark1Coverage',
+#
+# For each subtable format there is a class. However, we don't really distinguish
+# between "field name" and "format name": often these are the same. Yet there's
+# a whole bunch of fields with different names. The following dict is a mapping
+# from "format name" to "field name". _buildClasses() uses this to create a
+# subclass for each alternate field name.
+#
+_equivalents = {
+	'MarkArray': ("Mark1Array",),
+	'LangSys': ('DefaultLangSys',),
+	'Coverage': ('MarkCoverage', 'BaseCoverage', 'LigatureCoverage', 'Mark1Coverage',
 			'Mark2Coverage', 'BacktrackCoverage', 'InputCoverage',
-			'LookaheadCoverage')),
-	('ClassDef', ('ClassDef1', 'ClassDef2', 'BacktrackClassDef', 'InputClassDef',
-			'LookaheadClassDef', 'GlyphClassDef', 'MarkAttachClassDef')),
-	('Anchor', ('EntryAnchor', 'ExitAnchor', 'BaseAnchor', 'LigatureAnchor',
-			'Mark2Anchor', 'MarkAnchor')),
-	('Device', ('XPlaDevice', 'YPlaDevice', 'XAdvDevice', 'YAdvDevice',
-			'XDeviceTable', 'YDeviceTable', 'DeviceTable')),
-	('Axis', ('HorizAxis', 'VertAxis',)),
-	('MinMax', ('DefaultMinMax',)),
-	('BaseCoord', ('MinCoord', 'MaxCoord',)),
-	('JstfLangSys', ('DefJstfLangSys',)),
-	('JstfGSUBModList', ('ShrinkageEnableGSUB', 'ShrinkageDisableGSUB', 'ExtensionEnableGSUB',
-			'ExtensionDisableGSUB',)),
-	('JstfGPOSModList', ('ShrinkageEnableGPOS', 'ShrinkageDisableGPOS', 'ExtensionEnableGPOS',
-			'ExtensionDisableGPOS',)),
-	('JstfMax', ('ShrinkageJstfMax', 'ExtensionJstfMax',)),
-]
+			'LookaheadCoverage'),
+	'ClassDef': ('ClassDef1', 'ClassDef2', 'BacktrackClassDef', 'InputClassDef',
+			'LookaheadClassDef', 'GlyphClassDef', 'MarkAttachClassDef'),
+	'Anchor': ('EntryAnchor', 'ExitAnchor', 'BaseAnchor', 'LigatureAnchor',
+			'Mark2Anchor', 'MarkAnchor'),
+	'Device': ('XPlaDevice', 'YPlaDevice', 'XAdvDevice', 'YAdvDevice',
+			'XDeviceTable', 'YDeviceTable', 'DeviceTable'),
+	'Axis': ('HorizAxis', 'VertAxis',),
+	'MinMax': ('DefaultMinMax',),
+	'BaseCoord': ('MinCoord', 'MaxCoord',),
+	'JstfLangSys': ('DefJstfLangSys',),
+	'JstfGSUBModList': ('ShrinkageEnableGSUB', 'ShrinkageDisableGSUB', 'ExtensionEnableGSUB',
+			'ExtensionDisableGSUB',),
+	'JstfGPOSModList': ('ShrinkageEnableGPOS', 'ShrinkageDisableGPOS', 'ExtensionEnableGPOS',
+			'ExtensionDisableGPOS',),
+	'JstfMax': ('ShrinkageJstfMax', 'ExtensionJstfMax',),
+}
 
 
 def _buildClasses():
@@ -55,10 +62,11 @@
 			name = m.group(1)
 			baseClass = FormatSwitchingBaseTable
 		if not namespace.has_key(name):
+			# the class doesn't exist yet, so the base implementation is used.
 			cls = new.classobj(name, (baseClass,), {})
 			namespace[name] = cls
 	
-	for base, alts in _equivalents:
+	for base, alts in _equivalents.items():
 		base = namespace[base]
 		for alt in alts:
 			namespace[alt] = new.classobj(alt, (base,), {})
@@ -87,9 +95,12 @@
 		},
 	}
 	lookupTypes['JSTF'] = lookupTypes['GPOS']  # JSTF contains GPOS
+	for lookupEnum in lookupTypes.values():
+		for enum, cls in lookupEnum.items():
+			cls.LookupType = enum
 	
 	# add converters to classes
-	from otConverters import buildConverterList
+	from otConverters import buildConverters
 	for name, table in otData:
 		m = formatPat.match(name)
 		if m:
@@ -100,12 +111,12 @@
 			if not hasattr(cls, "converters"):
 				cls.converters = {}
 				cls.convertersByName = {}
-			converters, convertersByName = buildConverterList(table[1:], namespace)
+			converters, convertersByName = buildConverters(table[1:], namespace)
 			cls.converters[format] = converters
 			cls.convertersByName[format] = convertersByName
 		else:
 			cls = namespace[name]
-			cls.converters, cls.convertersByName = buildConverterList(table, namespace)
+			cls.converters, cls.convertersByName = buildConverters(table, namespace)
 
 
 _buildClasses()