Merge fixes from lp:~scouring/scour/transform, some sourced from lp:~ecmanaut/scour/transform.

scour.py

@@ -2335,123 +2335,164 @@ def optimizeTransform(transform):
 	
 	The transformation list is modified in-place.
 	"""
+	# FIXME: reordering these would optimize even more cases:
+	#	 first: Fold consecutive runs of the same transformation
+	#	 extra:	Attempt to cast between types to create sameness:
+	#		"matrix(0 1 -1 0 0 0) rotate(180) scale(-1)" all
+	#		are rotations (90, 180, 180) -- thus "rotate(90)"
+	#	second: Simplify transforms where numbers are optional.
+	#	 third: Attempt to simplify any single remaining matrix()
+	#
 	# if there's only one transformation and it's a matrix,
 	# try to make it a shorter non-matrix transformation
+	# NOTE: as matrix(a b c d e f) in SVG means the matrix:
+	# |¯  a  c  e  ¯|   make constants   |¯  A1  A2  A3  ¯|
+	# |   b  d  f   |  translating them  |   B1  B2  B3   |
+	# |_  0  0  1  _|  to more readable  |_  0    0   1  _|
 	if len(transform) == 1 and transform[0][0] == 'matrix':
+		matrix = A1, B1, A2, B2, A3, B3 = transform[0][1]
 		# |¯  1  0  0  ¯|
 		# |   0  1  0   |  Identity matrix (no transformation)
 		# |_  0  0  1  _|
-		if transform[0][1] == [1, 0, 0, 0, 1, 0]:
+		if matrix == [1, 0, 0, 1, 0, 0]:
 			del transform[0]
 		# |¯  1  0  X  ¯|
 		# |   0  1  Y   |  Translation by (X, Y).
 		# |_  0  0  1  _|
-		if (transform[0][1][0] == 1 and
+		elif (A1 ==  1 and A2 ==  0
-		    transform[0][1][1] == 0 and
+		 and  B1 ==  0 and B2 ==  1):
-		    transform[0][1][3] == 0 and
+			transform[0] = ('translate', [A3, B3])
-		    transform[0][1][4] == 1):
-			transform[0] = ('translate', [
-				transform[0][1][2], transform[0][1][5]
-			])
 		# |¯  X  0  0  ¯|
 		# |   0  Y  0   |  Scaling by (X, Y).
 		# |_  0  0  1  _|
-		elif (transform[0][1][1] == 0 and
+		elif (             A2 ==  0 and A3 ==  0
-		      transform[0][1][2] == 0 and
+		 and  B1 ==  0 and              B3 ==  0):
-		      transform[0][1][3] == 0 and
+			transform[0] = ('scale', [A1, B2])
-		      transform[0][1][5] == 0):
-			transform[0] = ('scale', [
-				transform[0][1][0], transform[0][1][4]
-			])
 		# |¯  cos(A) -sin(A)    0    ¯|  Rotation by angle A,
 		# |   sin(A)  cos(A)    0     |  clockwise, about the origin.
-		# |_    0       0       1    _|  A is in degrees.
+		# |_    0       0       1    _|  A is in degrees, [-180...180].
-		elif (transform[0][1][0] ==  transform[0][1][4]                and
+		elif (A1 == B2 and -1 <= A1 <= 1 and A3 == 0
-		      transform[0][1][1] == -transform[0][1][3]                and
+		 and -B1 == A2 and -1 <= B1 <= 1 and B3 == 0
-		      transform[0][1][0] >=  0                                 and
+		 # as cos² A + sin² A == 1 and as decimal trig is approximate:
-		      transform[0][1][0] <=  1                                 and
+		 # FIXME: the "epsilon" term here should really be some function
-		      transform[0][1][3] ==  sqrt(1 - transform[0][1][0] ** 2) and
+		 #        of the precision of the (sin|cos)_A terms, not 1e-15:
-		      transform[0][1][2] ==  0                                 and
+		 and  abs((B1 ** 2) + (A1 ** 2) - 1) < Decimal("1e-15")):
-		      transform[0][1][5] ==  0):
+			sin_A, cos_A = B1, A1
-			transform[0] = ('rotate', [
+			# while asin(A) and acos(A) both only have an 180° range
-				# What allows us to get the angle from the matrix
+			# the sign of sin(A) and cos(A) varies across quadrants,
-				# is the inverse sine of sin(A), which is the 4th
+			# letting us hone in on the angle the matrix represents:
-				# matrix component, or the inverse cosine of cos(A),
+			# -- => < -90 | -+ => -90..0 | ++ => 0..90 | +- => >= 90
-				# which is the 1st matrix component. I chose sin(A).
+			#
-				# math.asin returns radians, so convert.
+			# http://en.wikipedia.org/wiki/File:Sine_cosine_plot.svg
-				# SVG demands degrees.
+			# shows asin has the correct angle the middle quadrants:
-				Decimal(math.degrees(math.asin(transform[0][1][4])))
+			A = Decimal(str(math.degrees(math.asin(float(sin_A)))))
-			])
+			if cos_A < 0: # otherwise needs adjusting from the edges
+				if sin_A < 0:
+					A = -180 - A
+				else:
+					A =  180 - A
+			transform[0] = ('rotate', [A])
 	
 	# Simplify transformations where numbers are optional.
-	for singleTransform in transform:
+	for type, args in transform:
-		if singleTransform[0] == 'translate':
+		if type == 'translate':
 			# Only the X coordinate is required for translations.
 			# If the Y coordinate is unspecified, it's 0.
-			if (len(singleTransform[1]) == 2 and
+			if len(args) == 2 and args[1] == 0:
-			    singleTransform[1][1] == 0):
+				del args[1]
-				del singleTransform[1][1]
+		elif type == 'rotate':
-		elif singleTransform[0] == 'rotate':
 			# Only the angle is required for rotations.
 			# If the coordinates are unspecified, it's the origin (0, 0).
-			if (len(singleTransform[1]) == 3 and
+			if len(args) == 3 and args[1] == args[2] == 0:
-			    singleTransform[1][1] == 0 and
+				del args[1:]
-			    singleTransform[1][2] == 0):
+		elif type == 'scale':
-				del singleTransform[1][1:]
-		elif singleTransform[0] == 'scale':
 			# Only the X scaling factor is required.
 			# If the Y factor is unspecified, it's the same as X.
-			if (len(singleTransform[1]) == 2 and
+			if len(args) == 2 and args[0] == args[1]:
-			    singleTransform[1][0] == singleTransform[1][1]):
+				del args[1]
-				del singleTransform[1][1]
 
 	# Attempt to coalesce runs of the same transformation.
 	# Translations followed immediately by other translations,
 	# rotations followed immediately by other rotations,
 	# scaling followed immediately by other scaling,
 	# are safe to add.
-	# A matrix followed immediately by another matrix
+	# Identity skewX/skewY are safe to remove, but how do they accrete?
+	# |¯    1     0    0    ¯|
+	# |   tan(A)  1    0     |  skews X coordinates by angle A
+	# |_    0     0    1    _|
+	#
+	# |¯    1  tan(A)  0    ¯|
+	# |     0     1    0     |  skews Y coordinates by angle A
+	# |_    0     0    1    _|
+	#
+	# FIXME: A matrix followed immediately by another matrix
 	#	 would be safe to multiply together, too.
 	i = 1
 	while i < len(transform):
-		if transform[i][0] == transform[i - 1][0] == 'translate':
+		currType, currArgs = transform[i]
-			transform[i - 1][1][0] += transform[i][1][0] # x
+		prevType, prevArgs = transform[i - 1]
+		if currType == prevType == 'translate':
+			prevArgs[0] += currArgs[0] # x
 			# for y, only add if the second translation has an explicit y
-			if len(transform[i][1]) == 2:
+			if len(currArgs) == 2:
-				if len(transform[i - 1][1]) == 2:
+				if len(prevArgs) == 2:
-					transform[i - 1][1][1] += transform[i][1][1] # y
+					prevArgs[1] += currArgs[1] # y
-				elif len(transform[i - 1][1]) == 1:
+				elif len(prevArgs) == 1:
-					transform[i - 1][1].append(transform[i][1][1]) # y
+					prevArgs.append(currArgs[1]) # y
 			del transform[i]
-			if transform[i - 1][1][0] == transform[i - 1][1][1] == 0:
+			if prevArgs[0] == prevArgs[1] == 0:
 				# Identity translation!
 				i -= 1
 				del transform[i]
-		elif (transform[i][0] == transform[i - 1][0] == 'rotate'
+		elif (currType == prevType == 'rotate'
-		      and len(transform[i - 1][1]) == len(transform[i][1]) == 1):
+		      and len(prevArgs) == len(currArgs) == 1):
 			# Only coalesce if both rotations are from the origin.
-			transform[i - 1][1][0] += transform[i][1][0] # angle
+			prevArgs[0] += currArgs[0] # angle
+			# Put the new angle in the range ]-360, 360[.
+			# The modulo operator yields results with the sign of the
+			# divisor, so for negative dividends, we preserve the sign
+			# of the angle.
+			if prevArgs[0] < 0: prevArgs[0] = prevArgs[0] % -360
+			else:               prevArgs[0] = prevArgs[0] %  360
+
 			del transform[i]
-		elif transform[i][0] == transform[i - 1][0] == 'scale':
+		elif currType == prevType == 'scale':
-			transform[i - 1][1][0] *= transform[i][1][0] # x
+			prevArgs[0] *= currArgs[0] # x
 			# handle an implicit y
-			if len(transform[i - 1][1]) == 2 and len(transform[i][1]) == 2:
+			if len(prevArgs) == 2 and len(currArgs) == 2:
 				# y1 * y2
-				transform[i - 1][1][1] *= transform[i][1][1]
+				prevArgs[1] *= currArgs[1]
-			elif len(transform[i - 1][1]) == 1 and len(transform[i][1]) == 2:
+			elif len(prevArgs) == 1 and len(currArgs) == 2:
 				# create y2 = uniformscalefactor1 * y2
-				transform[i - 1][1].append(transform[i - 1][1][0] * transform[i][1][1])
+				prevArgs.append(prevArgs[0] * currArgs[1])
-			elif len(transform[i - 1][1]) == 2 and len(transform[i][1]) == 1:
+			elif len(prevArgs) == 2 and len(currArgs) == 1:
 				# y1 * uniformscalefactor2
-				transform[i - 1][1][1] *= transform[i][1][0]
+				prevArgs[1] *= currArgs[0]
 			del transform[i]
-			if transform[i - 1][1][0] == transform[i - 1][1][1] == 1:
+			if prevArgs[0] == prevArgs[1] == 1:
 				# Identity scale!
 				i -= 1
 				del transform[i]
 		else:
 			i += 1
 
+	# Some fixups are needed for single-element transformation lists, since
+	# the loop above was to coalesce elements with their predecessors in the
+	# list, and thus it required 2 elements.
+	i = 0
+	while i < len(transform):
+		currType, currArgs = transform[i]
+		if ((currType == 'skewX' or currType == 'skewY')
+		  and len(currArgs) == 1 and currArgs[0] == 0):
+			# Identity skew!
+			del transform[i]
+		elif ((currType == 'rotate')
+		  and len(currArgs) == 1 and currArgs[0] == 0):
+			# Identity rotation!
+			del transform[i]
+		else:
+			i += 1
+
 def optimizeTransforms(element, options) :
 	"""
 	Attempts to optimise transform specifications on the given node and its children.
@@ -2470,7 +2511,10 @@ def optimizeTransforms(element, options) :
 			newVal = serializeTransform(transform)
 			
 			if len(newVal) < len(val):
+				if len(newVal):
 					element.setAttribute(transformAttr, newVal)
+				else:
+					element.removeAttribute(transformAttr)
 				num += len(val) - len(newVal)
 	
 	for child in element.childNodes:

testscour.py

@@ -1217,6 +1217,96 @@ class RemoveDefsWithWhitespace(unittest.TestCase):
 		self.assertEquals(doc.getElementsByTagName('defs').length, 0,
 			'Kept defs, although it contains only whitespace or is <defs/>')
 
+class TransformIdentityMatrix(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-matrix-is-identity.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), '',
+			'Transform containing identity matrix not removed')
+
+class TransformRotate135(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-matrix-is-rotate-135.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), 'rotate(135)',
+			'Rotation matrix not converted to rotate(135)')
+
+class TransformRotate45(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-matrix-is-rotate-45.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), 'rotate(45)',
+			'Rotation matrix not converted to rotate(45)')
+
+class TransformRotate90(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-matrix-is-rotate-90.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), 'rotate(90)',
+			'Rotation matrix not converted to rotate(90)')
+
+class TransformRotateCCW135(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-matrix-is-rotate-neg-135.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), 'rotate(-135)',
+			'Counter-clockwise rotation matrix not converted to rotate(-135)')
+
+class TransformRotateCCW45(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-matrix-is-rotate-neg-45.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), 'rotate(-45)',
+			'Counter-clockwise rotation matrix not converted to rotate(-45)')
+
+class TransformRotateCCW90(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-matrix-is-rotate-neg-90.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), 'rotate(-90)',
+			'Counter-clockwise rotation matrix not converted to rotate(-90)')
+
+class TransformScale2by3(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-matrix-is-scale-2-3.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), 'scale(2 3)',
+			'Scaling matrix not converted to scale(2 3)')
+
+class TransformScaleMinus1(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-matrix-is-scale-neg-1.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), 'scale(-1)',
+			'Scaling matrix not converted to scale(-1)')
+
+class TransformTranslate(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-matrix-is-translate.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), 'translate(2 3)',
+			'Translation matrix not converted to translate(2 3)')
+
+class TransformRotation3Args(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-rotate-fold-3args.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), 'rotate(90)',
+			'Optional zeroes in rotate(angle 0 0) not removed')
+
+class TransformIdentityRotation(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-rotate-is-identity.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), '',
+			'Transform containing identity rotation not removed')
+
+class TransformIdentitySkewX(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-skewX-is-identity.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), '',
+			'Transform containing identity X-axis skew not removed')
+
+class TransformIdentitySkewY(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-skewY-is-identity.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), '',
+			'Transform containing identity Y-axis skew not removed')
+
+class TransformIdentityTranslate(unittest.TestCase):
+	def runTest(self):
+		doc = scour.scourXmlFile('unittests/transform-translate-is-identity.svg')
+		self.assertEquals(doc.getElementsByTagName('line')[0].getAttribute('transform'), '',
+			'Transform containing identity translation not removed')
+
 class DuplicateGradientsUpdateStyle(unittest.TestCase):
 	def runTest(self):
 		doc = scour.scourXmlFile('unittests/duplicate-gradients-update-style.svg',
@@ -1233,7 +1323,6 @@ class DuplicateGradientsUpdateStyle(unittest.TestCase):
 # TODO; write a test for embedding rasters
 # TODO: write a test for --disable-embed-rasters
 # TODO: write tests for --keep-editor-data
-# TODO: write tests for scouring transformations
 
 if __name__ == '__main__':
 	testcss = __import__('testcss')

unittests/transform-matrix-is-identity.svg

@@ -0,0 +1,3 @@
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="matrix(1 0 0 1 0 0)"/>
+</svg>

unittests/transform-matrix-is-rotate-135.svg

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="-9 0 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="matrix(-0.70710678118654746 0.70710678118654757 -0.70710678118654757 -0.70710678118654746 0 0)"/>
+</svg>

unittests/transform-matrix-is-rotate-45.svg

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="-9 0 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="matrix(0.70710678118654757 0.70710678118654746 -0.70710678118654746 0.70710678118654757 0 0)"/>
+</svg>

unittests/transform-matrix-is-rotate-90.svg

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="-9 0 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="matrix(0 1 -1 0 0 0)"/>
+</svg>

unittests/transform-matrix-is-rotate-neg-135.svg

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="-9 0 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="matrix(-0.70710678118654746 -0.70710678118654757 0.70710678118654757 -0.70710678118654746 0 0)"/>
+</svg>

unittests/transform-matrix-is-rotate-neg-45.svg

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="-9 0 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="matrix(0.70710678118654757 -0.70710678118654746 0.70710678118654746 0.70710678118654757 0 0)"/>
+</svg>

unittests/transform-matrix-is-rotate-neg-90.svg

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="-9 0 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="matrix(0 -1 1 0 0 0)"/>
+</svg>

unittests/transform-matrix-is-scale-2-3.svg

@@ -0,0 +1,3 @@
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 18 27">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="matrix(2 0 0 3 0 0)"/>
+</svg>

unittests/transform-matrix-is-scale-neg-1.svg

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="-9 -9 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="matrix(-1 0 0 -1 0 0)"/>
+</svg>

unittests/transform-matrix-is-translate.svg

@@ -0,0 +1,3 @@
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="2 3 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="matrix(1 0 0 1 2 3)"/>
+</svg>

unittests/transform-rotate-fold-3args.svg

@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="-9 0 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="rotate(90 0 0.0)"/>
+ <!--  optional zero trailing args to transform type rotate get removed -->
+</svg>

unittests/transform-rotate-is-identity.svg

@@ -0,0 +1,3 @@
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 9 9">
+ <line transform="rotate(-300) rotate(-60)" stroke="red" y1="9" x1="9"/>
+</svg>

unittests/transform-skewX-is-identity.svg

@@ -0,0 +1,4 @@
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="skewX(0)"/>
+ <!--  skewX(0) is the identity transform, which can safely be removed -->
+</svg>

unittests/transform-skewY-is-identity.svg

@@ -0,0 +1,4 @@
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="skewY(0)"/>
+ <!--  skewY(0) is the identity transform, which can safely be removed -->
+</svg>

unittests/transform-translate-is-identity.svg

@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="-9 0 9 9">
+ <line stroke="rgba(255,0,0,0.5)" y1="9" x1="9" transform="translate(0, 20) translate(10) translate(-10 -20.0)"/>
+ <line stroke="rgba(0,0,255,0.5)" y1="9" x1="9"/>
+</svg>